Connect: Verify the leaf cert to determine if its ready. #4540
Conversation
connect/tls.go
Outdated
|
|
||
| if cfg.leaf.Leaf == nil { | ||
| if cert, err := x509.ParseCertificate(cfg.leaf.Certificate[0]); err == nil { | ||
| cfg.leaf.Leaf = cert |
There was a problem hiding this comment.
This is modifying state while only holding an RLock. Is that dangerous?
There was a problem hiding this comment.
Should we update the comment on ReadyWait to be strong than "which we assume are valid"? Happy to leave it as only triggering on the first ready state change rather than being a more elaborate watch mechanism.
|
I still need one more thing for this PR. Ignore reviewing for the time being. |
mkeeler
force-pushed
the
connect-tls-verify-ready
branch
3 times, most recently
from
69d3e0b to
7e39155
Compare
This improves the checking so that if a certificate were to expire or the roots changed then we will go into a non-ready state. This parses the x509 certificates from the TLS certificate when the leaf is set and whenever the leaf is set or the roots are set will perform x509 verification to ensure that the given roots can verify the leaf cert. Only when a valid roots/leaf cert pair are loaded will the notify function be called and the readyCh be closed. The Ready function will now also perform x509 verification of the certificate which will in addition to verifying the signatures also verify the cert has not expired. In this way repeated calls to Ready will return a non-Ready state when a certificate expires.
mkeeler
force-pushed
the
connect-tls-verify-ready
branch
from
7e39155 to
446025b
Compare
|
@banks I made a bunch of changes. The latest implementation only writes to the TLS certs when using a write lock (SetLeaf) or in the constructor newDynamicTLSConfig. I had experimented with the idea of needing to have x509 verification of the leaf cert succeed before closing the readyCh. The one case that doesn't work so well for is where the certificate is valid from a certain point in the future (either it was created that way or due to clock skew on the node where it is being used). We could spawn a go routine to periodically check for validity before closing the readyCh but then we would need more chans for communication with it in addition to the readyCh. I started implementing this but it was getting ugly and seemed unnecessary. So instead I left ReadyWait semantics the same. Once all the data is installed (whether valid or not) that chan will be unblocked. |
This improves the checking so that if a certificate were to expire or the roots changed then we will go into a non-ready state.
This also caches the parsed x509 certificate in the tls certificate so that future checks shouldn't need to parse anything but rather just validate against the roots and cert expiration.