Configure TLS Curve Preferences and Cipher Suites - #4987
akhilnittala wants to merge 6 commits into
Conversation
Signed-off-by: akhil nittala <nakhil@redhat.com>
akhilnittala
changed the title
|
Hey @nabokihms @sagikazarmark could you please help in the review process? We need the PR in upstream. Thanks for any input! |
sagikazarmark
left a comment
sagikazarmark
left a comment
There was a problem hiding this comment.
Thanks for the PR!
The approach looks good.
Blocking: SecP256r1MLKEM768/SecP384r1MLKEM1024 require Go 1.26.
Rebasing should resolve that.
I think it would also make sense to add X25519MLKEM768 (so users don't lose PQ key exchange when setting curve prefs).
|
Also: maybe add an example to the config file? |
added, thanks |
|
@sagikazarmark addressed all your comments, could you please re-review it. |
done, added @sagikazarmark |
sagikazarmark
left a comment
sagikazarmark
left a comment
There was a problem hiding this comment.
Sorry @akhilnittala but I think I found a few more issues. Can you please take a look?
| if len(c.Web.TLSCiphers) > 0 { | ||
| ciphers, err := parseCipherSuites(c.Web.TLSCiphers) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid TLS cipher suites: %w", err) | ||
| } | ||
| c.Web.tlsCipherIDs = ciphers | ||
| } | ||
|
|
||
| if len(c.Web.TLSCurvePreferences) > 0 { | ||
| curves, err := parseCurvePreferences(c.Web.TLSCurvePreferences) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid TLS curve preferences: %w", err) | ||
| } | ||
| c.Web.tlsCurveIDs = curves | ||
| } |
There was a problem hiding this comment.
Looking closer at this: c is a value receiver and Web is a value as well, so I think this will simply not set anything.
Can you please test it?
There was a problem hiding this comment.
I tested the changes, changes are working fine @sagikazarmark
There was a problem hiding this comment.
able to test openssl connection with tlsminversion, maxversion and negotoations are happening only with passed curve preferences and ciphersuites.
| MinVersion: uint16(tlsMinVersion), | ||
| MaxVersion: uint16(tlsMaxVersion), | ||
| CipherSuites: allowedTLSCiphers, | ||
| CipherSuites: cipherSuites, |
There was a problem hiding this comment.
I think this can potentially cause some issues. According to the docs, tls.Config.CipherSuites allows setting ciphers for TLS 1.0-1.2:
// CipherSuites is a list of enabled TLS 1.0–1.2 cipher suites. The order of
// the list is ignored. Note that TLS 1.3 ciphersuites are not configurable.
tls.CipherSuites on the other hand returns TLS 1.3 only ciphers, like TLS_AES_256_GCM_SHA384.
I suspect this would lead to handshake failures when TLS 1.2 is used.
We probably need to exclude ciphers that have tls.CipherSuite.SupportedVersions TLS 1.3 only (maybe even do a warning log so people know it isn't applied).
It should also be documented in the example config file.
There was a problem hiding this comment.
I tested the changes with tls1.2 and also tls 1.1 with openssl conenction for the dex issuer url, everything working fine including negotiations of curve preferences and ciphersuites, the documentation is pointing that for tls 1.3 ciphersuites are not configurable.
Signed-off-by: akhil nittala <nakhil@redhat.com>
2 participants
Overview
TLS Ciphersuites are hardcoded currently, to meet the platform side compliance ciphersuites needs to be configureable, so made changes for tls ciphersuites and curve preferences to be configurable for web.
What this PR does / why we need it
Configures the tls ciphersuites and curvepreferences based on the parameters in config.yaml
Fixes: #4913