Refactor keypair code in preparation for secret rotation #11219
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
area/nodeup
area/provider/aws
johngmyers
force-pushed
the
refactor-keypair
branch
from
849e519 to
a167732
Compare
|
/assign justinsb |
johngmyers
force-pushed
the
refactor-keypair
branch
from
a167732 to
7bbb9c6
Compare
nodeup/pkg/model/kube_apiserver.go
Outdated
| return fmt.Errorf("service-account keyset not found") | ||
| } | ||
|
|
||
| buf := new(bytes.Buffer) |
There was a problem hiding this comment.
Nit/tip: I think you can do var buf bytes.Buffer (though you then have to use &buf)
There was a problem hiding this comment.
I don't think bytes.Buffer.Write() handles a nil *Buffer.
| func (c *VFSCAStore) FindKeypair(id string) (*pki.Certificate, *pki.PrivateKey, bool, error) { | ||
| cert, legacyFormat, err := c.findCert(id) | ||
| func (c *VFSCAStore) FindKeyset(id string) (*Keyset, error) { | ||
| certs, err := c.loadKeyset(c.buildCertificatePoolPath(id)) |
There was a problem hiding this comment.
So technically we should be able to load just the certificates, even if we don't have permission to the keys. I had a look, and the reason we get away with this (I think) is that AWS/S3 is the only cloud where we lock down access so most nodes can't access the keys, and on AWS we default to using the kops-controller.
Not sure if we want to address this or just move everyone to kops-controller and work towards not writing the certificate-only forms. It wouldn't be too hard though I don't think - probably the easiest way is just to create FindKeysetCertificates and check each invocation of FindKeyset to see if it needs the certificates.
Also, I like the merge logic and we may want to keep it, but the way we avoid problems here is that we should always write the private keys first. From the private keys, we can get the public keys / certs (but not vice versa) so that's why we do it in that order. So it should suffice to just load the private keys, assuming we have permission.
There was a problem hiding this comment.
Things that just want the certificates should get them through userdata or equivalent, not VFS. That way when the certificates change the thing gets updated by rolling update.
In any case, everything that currently calls FindKeyset (or FindPrimaryKeypair) has or needs access to the private key.
There was a problem hiding this comment.
The merge logic mimics the behavior of the previous FindKeypair(). A future PR could remove the certificate-only bundle entirely once it ensures that all certificate-only consumers have been refactored to use userdata or equivalent.
|
Thanks @johngmyers - it is indeed a big refactor! The important things I think are to figure out the semantics of StoreKeyset (replace vs merge) and to figure out when we can just load the certificates. I'm going to target this to milestone 1.22 - let me know if you disagree /milestone v1.22 |
|
My intention was for |
|
Things that just want the certificates should get them through userdata or equivalent, not VFS. That way when the certificates change the thing gets updated by rolling update. |
|
/retest |
johngmyers
force-pushed
the
refactor-keypair
branch
from
c24e359 to
760bcb6
Compare
|
/retest |
|
Done responding to this round of review comments. |
|
@justinsb this PR should also be reviewed commit-by-commit. That goes for pretty much any of my PRs. I put in effort to make the commit stream reviewable, for example moving code verbatim in one commit before modifying it in a subsequent one. |
k8s-ci-robot
added
the
needs-rebase
johngmyers
force-pushed
the
refactor-keypair
branch
from
9f6e159 to
c3b2bef
Compare
k8s-ci-robot
removed
the
needs-rebase
johngmyers
force-pushed
the
refactor-keypair
branch
from
c3b2bef to
12465ac
Compare
|
Looks good, thanks @johngmyers. Holding so you can sequence with the other PR /approve /hold |
k8s-ci-robot
added
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
Prepare for rotating service-account signing keys, per #11204.
Drops support for downgrading to kops versions before 1.18. Doing so will cause all keypairs to revert to the state they were prior to upgrading kops.
Stopping short of landing any part of #11204 which actually stages or rotates any keys, pending resolution (including between @olemarkus and I) of the user interface for rotating keypairs.