localca: Rotation support part 1 - #971
Conversation
This commit fleshes out localca's rotation support by changing the interface so that signing operations should always go through a pool. All existing in-tree callers are converted over to this pattern. This ensures that signing can properly continue as a CA pool is rotated, without requiring any process restarts. The pool is periodically reloaded from disk in the course of signing operations, with the loaded data being cached for up to a minute. Some code for storing the CA state using PEM files was removed because it seemed to be dead. Follow-on changes will give localjwt a similar treatment, and add administrative commands for running rotations on pools.
cba5644 to
eba8eb4
Compare
| if err := p.refreshIfNecessary(); err != nil { | ||
| return nil, fmt.Errorf("while refreshing pool: %w", err) | ||
| } |
There was a problem hiding this comment.
If we fail to refresh the pool from disk, should we still allow certs to be created with the pool that's currently in memory?
| signingCert = selectedCA.IntermediateCertificates[0] | ||
| } | ||
|
|
||
| subjectCertDER, err := x509.CreateCertificate(rand.Reader, template, signingCert, subjectPublicKey, selectedCA.SigningKey) |
There was a problem hiding this comment.
Do we need to use the private key of the intermediate here, if the signing cert is an intermediate? Currently we're always passing in the private key of the CA IIUC (selectedCA.SigningKey).
There was a problem hiding this comment.
The private key of the CA is the private key of the intermediate, if intermediates are in use. I should make this clearer.
| // (Actor identity broker, egress gateway) to properly continue signing even as | ||
| // an administrator rotates one of the CA pools, without requiring any | ||
| // components to restart. | ||
| type RefreshingPool struct { |
There was a problem hiding this comment.
Should we add tests for RefreshingPool?
| // In substrate's default setup, the CA pool state is kept in a Kubernetes | ||
| // secret, and administered with admin CLI commands. | ||
| // | ||
| // If you are writing an online signing component, us a projected volume to put |
There was a problem hiding this comment.
[nit] "us a projected ..." -> "use a projected ..."
|
|
||
| pool := &localca.Pool{ | ||
| pool := &localca.ConcretePool{ | ||
| CAs: []*localca.CA{ca}, |
There was a problem hiding this comment.
Should we set this ca as ActiveForSigning to avoid hard reliance on the selectedCA := p.CAs[0] fallback in RefreshingPool.CreateCertificate?
There was a problem hiding this comment.
Oh, good point, we need to do this.
However, I was actually intending the fallback behavior to handle all the existing generated CAs that people have created for their dev installs of substrate, so it needs to remain.
There was a problem hiding this comment.
Sure, I saw your intent for that fallback was backwards compatibility. But I think we should avoid introducing new places where we rely on it as much as possible.
| // For backwards compatibility, pick the first CA if none is designated. | ||
| selectedCA := p.CAs[0] | ||
| for _, ca := range p.CAs { | ||
| if ca.ID == p.ActiveForSigning { |
There was a problem hiding this comment.
Falling back to p.CAs[0] if ActiveForSigning is actually set but no CAs match seems dangerous. Should we return an error in that case?
| // Pool's currently-active CAs. | ||
| CreateCertificate(template *x509.Certificate, subjectPublicKey crypto.PublicKey) ([][]byte, error) | ||
|
|
||
| // TrustAnchors returns the root certificates for all of the pool's CAs, including |
There was a problem hiding this comment.
the comment seems unfinished
| } | ||
|
|
||
| // RefreshingPool is a wrapper around Pool that periodically reloads the CA | ||
| // state from disk. This allows our various pieces that sign certificates |
There was a problem hiding this comment.
Remove the extra space between disk. and This
| if err := p.refreshIfNecessary(); err != nil { | ||
| return nil, fmt.Errorf("while refreshing pool: %w", err) | ||
| } | ||
| return p.pool.CreateCertificate(template, subjectPublicKey) |
There was a problem hiding this comment.
Does p.pool.CreateCertificate need to be protected by p.lock?
| if err := p.refreshIfNecessary(); err != nil { | ||
| return nil, fmt.Errorf("while refreshing pool: %w", err) | ||
| } | ||
| return p.pool.TrustAnchors() |
There was a problem hiding this comment.
Does p.pool.TrustAnchors need to be protected by p.lock?
There was a problem hiding this comment.
Let us add some test coverage for the new logic added in localca.go
3 participants
This commit fleshes out localca's rotation support by changing the interface so that signing operations should always go through a pool. All existing in-tree callers are converted over to this pattern. This ensures that signing can properly continue as a CA pool is rotated, without requiring any process restarts.
The pool is periodically reloaded from disk in the course of signing operations, with the loaded data being cached for up to a minute.
Some code for storing the CA state using PEM files was removed because it seemed to be dead.
Follow-on changes will give localjwt a similar treatment, and add administrative commands for running rotations on pools.