get-keypairs: Tolerate key set items without certificates #14370
Conversation
k8s-ci-robot
added
cncf-cla: yes
| } | ||
| sort.Strings(alternateNames) | ||
|
|
||
| if includeDistrusted || (item.DistrustTimestamp == nil && item.Certificate != nil) { |
There was a problem hiding this comment.
It's arguably strange to overload the meaning of "distrusted" here. We could update the documentation to mention this additional consideration, or introduce a separate flag to include these items lacking a certificate. I'm not sure that it's worth it.
There was a problem hiding this comment.
I think ToCertificateBytes() and ToPublicKeys() in fi.Keyset need to also have Certificate != nil checks in addition to their current DistrustTimestamp checks, otherwise they could also trigger nil dereferences.
Given the above were done, it would be safe to claim that keypairs without certificates are inherently distrusted.
Similarly, VFSCAStore.StoreKeyset should verify that Certificate != nil for the primary id. promote_keypair.go should refuse to promote a keypair without a certificate.
There was a problem hiding this comment.
John, I noticed that (*fi.Keyset).ToCertificateBytes filters items that lack a certificate. It could filter them a little bit earlier, but it looks they already won't make it into the composed CA bundle. Likewise, (*fi.Keyset).ToPublicKeys is guarded in the same way.
There was a problem hiding this comment.
Okay, so it seems promote_keypair.go is the only remaining issue.
There was a problem hiding this comment.
I did augment (*fi.VFSCAStore).StoreKeyset. I'm now working on the keypair promotion.
There was a problem hiding this comment.
Please take another look, John. Thank you for the advice so far.
hakman
requested review from
justinsb and
johngmyers
and removed request for
rdrgmnzs
johngmyers
requested review from
johngmyers
and removed request for
johngmyers
Allow the "kops get keypairs" command to consume key sets with old key pair items that lack an associated X.509 certificate. When the command is invoked without the "--distrusted" flag set to true, omit these old items as if they're distrusted. Conversely, when the command is invoked with the "--distrusted" flag set to true, include these items, but omit their details that would be contingent on the nonexistent certificate. In order to supply only information that is known to be true, treat the following fields in the output as newly optional: - issuer - notAfter - notBefore - subject With no certificate present, it's not possible to present concrete values for those fields.
Forbid the "kops promote keypair" command from promoting a key pair item that lacks an associated X.509 certificate. Along with that prohibition, refuse to store a key set in a VFS whose primary key pair lacks a certificate. This allows us to continue storing such key pairs, but we will never allow them to serve as the primary key pair within the containing key set.
seh
force-pushed
the
ignore-keypairs-without-certificates
branch
from
fec6c44 to
95f9889
Compare
| for id, item := range keyset.Items { | ||
| if item.PrivateKey != nil && item.DistrustTimestamp == nil { | ||
| if item.PrivateKey != nil && item.DistrustTimestamp == nil && item.Certificate != nil { |
There was a problem hiding this comment.
I could order the certificate check after the private key check, since those two things seem to go together more closely than the distrusting timestamp.
| @@ -162,17 +162,17 @@ func promoteKeypair(out io.Writer, name string, keypairID string, keyStore fi.CA | |||
| } | |||
|
|
|||
| if keypairID == "" { | |||
| highestTrustedId := big.NewInt(0) | |||
| highestCandidateId := big.NewInt(0) | |||
There was a problem hiding this comment.
Since we're searching for a private key that's neither distrusted nor lacking a certificate, I figured I'd change the name here to be more general.
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: johngmyers 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
|
Can we also back-port this to the version 1.25 release line? That's the version of kOps we're using for the time being. |
…-upstream-release-1.25 Automated cherry pick of #14370: get-keypairs: Tolerate items without certificates
Allow the kops get keypairs command to consume key sets with old key pair items that lack an associated X.509 certificate. When the command is invoked without the
--distrustedflag set to true, omit these old items as if they're distrusted. Conversely, when the command is invoked with the--distrustedflag set to true, include these items, but omit their details that would be contingent on the nonexistent certificate.In order to supply only information that is known to be true, treat the following fields in the output as newly optional:
With no certificate present, it's not possible to present concrete values for those fields.
Fixes #14174.