Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remove tls cipher suite settings #127

Merged
merged 4 commits into from
Jan 10, 2024
Merged

remove tls cipher suite settings #127

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b4c042b
remove tls version settings
maipbui Nov 17, 2023
24b3bff
add MinVersion
maipbui Nov 17, 2023
bd92571
Merge remote-tracking branch 'upstream/master' into tls_settings
maipbui Jan 9, 2024
cdb3275
add PreferServerCipherSuites
maipbui Jan 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions rest/main/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,6 @@ func main() {
ClientCAs: prepareCACertificates(),
MinVersion: tls.VersionTLS12,
PreferServerCipherSuites: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will upgrade golang 1.15 and keep disable PreferServerCipherSuites settings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussion, keep Go 1.13 and retain PreferServerCipherSuites. only remove CipherSuites.

CipherSuites: getPreferredCipherSuites(),
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the requirements, but the changes have nothing to do with it. REST server already supports TLS1.2 and TLS1.3 as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point, will update the PR.

}

// Prepare HTTPS server
Expand Down Expand Up @@ -199,17 +198,6 @@ func getTLSClientAuthType() tls.ClientAuthType {
}
}

func getPreferredCipherSuites() []uint16 {
return []uint16{
tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,
tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,
tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
}
}

// findAManagementIP returns a valid IPv4 address of eth0.
// Empty string is returned if no address could be resolved.
func findAManagementIP() string {
Expand Down
Loading