*: support TLS cipher suite whitelist #9801
Conversation
gyuho
force-pushed
the
cipher-suites
branch
2 times, most recently
from
1e1a4d8
to
4ffd31d
Compare
Codecov Report
@@ Coverage Diff @@
## master #9801 +/- ##
==========================================
+ Coverage 69.58% 69.63% +0.04%
==========================================
Files 376 377 +1
Lines 35229 35263 +34
==========================================
+ Hits 24514 24554 +40
+ Misses 8954 8950 -4
+ Partials 1761 1759 -2
Continue to review full report at Codecov.
|
|
/cc @liggitt |
embed/config.go
Outdated
| } | ||
| return updateCipherSuites(&cfg.ClientTLSInfo, cfg.ClientCipherSuites) | ||
| } | ||
| if cfg.ClientAutoTLS { |
There was a problem hiding this comment.
Since you're already modifying the logic here, could we simplify this method to just have if !cfg.ClientAutoTLS { return nil } at the top? It would remove this if on 743 and simplify the check above.
There was a problem hiding this comment.
Good point! Will update.
embed/config.go
Outdated
| } | ||
| return updateCipherSuites(&cfg.PeerTLSInfo, cfg.PeerCipherSuites) | ||
| } | ||
| if cfg.PeerAutoTLS { |
There was a problem hiding this comment.
Similar change here as above, just start with if !cfg.PeerAutoTLS { return nil }, and remove that check for the rest of the method.
embed/config.go
Outdated
| // ClientCipherSuites is a list of supported cipher suites between server and client. | ||
| // If empty, Go auto-populates it by default. | ||
| // Note that cipher suites are prioritized in the given order. | ||
| ClientCipherSuites []string `json:"client-cipher-suites"` |
There was a problem hiding this comment.
Why does this have a JSON tag?
There was a problem hiding this comment.
We have YAML configuration file format, which uses json tag to read flag values.
|
As @ericchiang suggested, will be switching to one shared flag both for server and client side. |
Signed-off-by: Gyuho Lee <[email protected]>
Signed-off-by: Gyuho Lee <[email protected]>
Signed-off-by: Gyuho Lee <[email protected]>
Signed-off-by: Gyuho Lee <[email protected]>
Signed-off-by: Gyuho Lee <[email protected]>
Signed-off-by: Gyuho Lee <[email protected]>
Signed-off-by: Gyuho Lee <[email protected]>
Signed-off-by: Gyuho Lee <[email protected]>
| if err != nil { | ||
| return err | ||
| } | ||
| return updateCipherSuites(&cfg.PeerTLSInfo, cfg.CipherSuites) |
There was a problem hiding this comment.
updateCipherSuites fails if called with non-empty ciphers twice, right?
in configurePeerListeners(), we call updateCipherSuites() and then call PeerSelfCert(), which also calls updateCipherSuites(). doesn't that mean we will fail if cfg.PeerAutoTLS and cfg.CipherSuites are both set?
There was a problem hiding this comment.
@liggitt transport.SelfCert initializes cfg.PeerTLSInfo.CipherSuites as empty, so this should be safe?
There was a problem hiding this comment.
@liggitt transport.SelfCert initializes cfg.PeerTLSInfo.CipherSuites as empty, so this should be safe?
ok, didn't trace it past these two methods, was just looking at the config inputs
Fail TLS handshake when client hello is requested with invalid cipher suites.
Succeeds if client requests with matching or empty cipher suites.
Fix #8320.