Skip to content

Allow to choose TLS ciphersuites#110

Merged
summerwind merged 1 commit intosummerwind:masterfrom
vankoven:configurable_ciphersuites
Dec 27, 2019
Merged

Allow to choose TLS ciphersuites#110
summerwind merged 1 commit intosummerwind:masterfrom
vankoven:configurable_ciphersuites

Conversation

@vankoven
Copy link
Copy Markdown
Contributor

If server implementation provides only h2 (HTTP2 over TLS), then it might
be useful to control which ciphersuites are used to establish secure
connection to check the encrypted traffic in Wireshark or similar
utilities.

A new optional command-line option --ciphers <list> is added in this PR. Just the same like in curl. But unlike in curl, ciphersuite names doesn't mirror openssl ones, since in future Golang releases native golang names will be provided.

If server implementation provides only h2 (HTTP2 over TLS), then it might
be useful to control which ciphersuites are used to establish secure
connection to check the encrypted traffic in Wireshark or similar
utilities.
}

// Addr returns the string concatinated with hostname and port number.
// Addr returns the string concatenated with hostname and port number.
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Copy Markdown
Owner

@summerwind summerwind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I feel that the ciphers option is useful for testing.
I left the comment related insecure ciphers.

func CiphersuiteByName(name string) uint16 {
switch name {
case "TLS_RSA_WITH_RC4_128_SHA":
return tls.TLS_RSA_WITH_RC4_128_SHA
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RC4 and 3DES are not recommended to use now. Do you have any good reason to support them?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for review!

For some reason Golang didn't provide literal names for ciphersuites and it wasn't possible to get ciphersuite id by name. So I had to add a such convert function on my own. I've listed all TLS1.0-1.2 ciphersuites here without any security considerations just to keep the function more library-like. TLS 1.3 ciphersuites are not listed there since they are not configurable. That's why I've covered all ciphersuites listed in crtypto/tls module.

But I actually was surprised that master branch of Golang was updated after this PR was created. In upcoming 1.14 release they have added a CipherSuite() function, which provide the missing link between literal names and ids which can be used to get ciphersuite id by literal name (simillary to new cipherSuiteByID() function).

I can adapt PR to changes in upcoming 1.4 Golang release, but then I will need to bump go version in go.mod and the PR will have to be postponed until Golang 1.4 is released (it's in beta now).
Or I can leave the changes as is, then the code will work fine in both Go 1.2 and 1.4. Which variant do you prefer?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for describing!
It's OK to use your current code because Go 1.4 has not released yet.

Copy link
Copy Markdown
Owner

@summerwind summerwind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@summerwind summerwind merged commit 52c0dc1 into summerwind:master Dec 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants