ca: set the correct SigningKeyID after config update with Vault provider #11672
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dnephin
added
theme/certificates
dnephin
commented
Nov 26, 2021
|
|
||
| cert, err = connect.ParseCert(newRoot.LeafSigningCert()) | ||
| require.NoError(t, err) | ||
| require.Equal(t, connect.HexString(cert.SubjectKeyId), newRoot.SigningKeyID) |
There was a problem hiding this comment.
This is the line that failed before this fix.
dnephin
force-pushed
the
dnephin/ca-fix-storing-vault-intermediate
branch
from
d155347
to
b631479
Compare
dnephin
force-pushed
the
dnephin/ca-fix-signing-key-id-post-update
branch
from
398a805
to
0373c1b
Compare
dnephin
force-pushed
the
dnephin/ca-fix-signing-key-id-post-update
branch
from
0373c1b
to
bea9b46
Compare
dnephin
force-pushed
the
dnephin/ca-fix-storing-vault-intermediate
branch
from
b631479
to
28a8a64
Compare
Base automatically changed from
dnephin/ca-fix-storing-vault-intermediate
to
main
December 2, 2021 21:02
dnephin
force-pushed
the
dnephin/ca-fix-signing-key-id-post-update
branch
from
bea9b46
to
df8618d
Compare
The test added in this commit shows the problem. Previously the SigningKeyID was set to the RootCert not the local leaf signing cert. This same bug was fixed in two other places back in 2019, but this last one was missed. While fixing this bug I noticed I had the same few lines of code in 3 places, so I extracted a new function for them. There would be 4 places, but currently the InitializeCA flow sets this SigningKeyID in a different way, so I've left that alone for now.
dnephin
force-pushed
the
dnephin/ca-fix-signing-key-id-post-update
branch
from
df8618d
to
17a2d14
Compare
|
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/514121. |
dnephin
added a commit
that referenced
this pull request
Dec 2, 2021
…d-post-update ca: set the correct SigningKeyID after config update with Vault provider
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #11662
Branched from #11671
Two other instance of this same bug were fixed a year+ ago in #6513 and #7012, but this last one remained.
The test failed before the fix, and I extracted a new function for this since the logic is the same in all places, and the method name helps indicate the significance of this intermediate certificate.
We already have logic to fix the
SigningKeyIDon storedCARootwhen theCAManangeris initialized, so we don't need to add more logic for it.