remove tls cipher suite settings #127
Conversation
Signed-off-by: Mai Bui <maibui@microsoft.com>
Signed-off-by: Mai Bui <maibui@microsoft.com>
|
@anand-kumar-subramanian could you help review this PR or direct to appropriate reviewer? |
|
cc: @sachinholla Can you please point me to the relevant ADO for TLS1.3 requirements? I cannot seem to locate it using a web search. Does this mean that TLS1.2 clients will not be able to connect? With TLS1.2, there are weak ciphers that need to be rejected, hence, it appears that the original restrictions were put in place. |
ADO: https://msazure.visualstudio.com/One/_workitems/edit/24943795/ |
| @@ -105,8 +105,6 @@ func main() { | |||
| Certificates: prepareServerCertificate(), | |||
| ClientCAs: prepareCACertificates(), | |||
| MinVersion: tls.VersionTLS12, | |||
| PreferServerCipherSuites: true, | |||
There was a problem hiding this comment.
Are you planning to upgrade golang version too? If not it is better to retain the PreferServerCipherSuites setting. Without this, the golang 1.15 tls library selects client advertised cipher suite. Hence there is a chance of using an unsafe cipher if the clients are not configured properly.
There was a problem hiding this comment.
I will upgrade golang 1.15 and keep disable PreferServerCipherSuites settings
There was a problem hiding this comment.
After discussion, keep Go 1.13 and retain PreferServerCipherSuites. only remove CipherSuites.
| @@ -105,8 +105,6 @@ func main() { | |||
| Certificates: prepareServerCertificate(), | |||
| ClientCAs: prepareCACertificates(), | |||
| MinVersion: tls.VersionTLS12, | |||
| PreferServerCipherSuites: true, | |||
| CipherSuites: getPreferredCipherSuites(), | |||
There was a problem hiding this comment.
Did you see any issue using TLS1.3 clients? Golang uses this CipherSuites setting only for TLS1.0-1.2 communications. It is ignored when TLS1.3 is selected -- server automatically picks the best cipher. Current configuration forces stronger ciphers when TLS1.2 is selected.
If you want to force TLS1.3 only, please change MinVersion configuration to tls.VersionTLS13 at line 107.
There was a problem hiding this comment.
If CipherSuites is not specified, then a safe default cipher suites list is used. I think using default list is better? Cipher suites from this getPreferredCipherSuites may not be safe over time.
There was a problem hiding this comment.
The ciphers returned by getPreferredCipherSuites() are recommended by security team; but I agree it was almost 3 years back. I am okay to remove it and let golang library decide the best values (assuming we upgrade the golang version to latest).
But, my point was that it should not have affected TLS1.3 communication. Golang TLS library ignores the CipherSuites values when the client selects TLS1.3 (API doc). So, the TLS1.3 communication should have worked without these changes too. Did you see any failures with the old code?
There was a problem hiding this comment.
I agree that it should not have impacted TLS1.3 communication. The requirement is must support TLS 1.3 by default, if not possible, fall back to TLS 1.2. We remove getPreferredCipherSuites ciphers to ensure default ciphers remain secure in case TLS clients/ servers negotiates TLS 1.2.
There was a problem hiding this comment.
I understand the requirements, but the changes have nothing to do with it. REST server already supports TLS1.2 and TLS1.3 as expected.
There was a problem hiding this comment.
I agree server already supports TLS 1.2 and TLS 1.3. I'm confused why "the changes have nothing to do with it".
For TLS 1.3, server automatically picks the best cipher.
For TLS 1.2, the changes in this PR (and upgrade Golang to version 1.15) will pick default cipher suites when TLS 1.2 is selected, correct? I'm wondering why not use same practice for both TLS 1.2 and TLS 1.3?
There was a problem hiding this comment.
hi @maipbui, I am okay to remove the CipherSuites preference. But we need to keep PreferServerCipherSuites for now (until we upgrade to go1.17 or later). Without it the server picks client's CipherSuite order and can end up using a weak cipher today.
There was a problem hiding this comment.
I see your point, will update the PR.
Signed-off-by: Mai Bui <maibui@microsoft.com>
maipbui
requested review from
sachinholla and
qiluo-msft
and removed request for
sachinholla
MSFT ADO: 24943795
To make TLS 1.3 compliant in sonic, it is not recommended to set cipher suite objects in Tls.Config struct. Refer to ADO for TLS 1.3 requirements.
Verified by building new sonic image, compare new syslog with old one.