-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't allow crl-signing issuer usage without CRLSign KeyUsage #16865
Merged
cipherboy
merged 5 commits into
main
from
cipherboy-dont-allow-crl-usage-without-signing-bit
Aug 24, 2022
Merged
Don't allow crl-signing issuer usage without CRLSign KeyUsage #16865
cipherboy
merged 5 commits into
main
from
cipherboy-dont-allow-crl-usage-without-signing-bit
Aug 24, 2022
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
When Vault imports certificates without KU for CRLSign, we shouldn't provision CRLUsage on the backing issuer; otherwise, we'll attempt to build CRLs and Go will cause us to err out. This change makes it clear (at issuer configuration time) that we can't possibly support this operation and hopefully prevent users from running into the more cryptic Go error. Note that this does not apply for OCSP EKU: the EKU exists, per RFC 6960 Section 2.6 OCSP Signature Authority Delegation, to allow delegation of OCSP signing to a child certificate. This EKU is not necessary on the issuer itself, and generally assumes issuers are allowed to issue OCSP responses regardless of KU/EKU. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
cipherboy
force-pushed
the
cipherboy-dont-allow-crl-usage-without-signing-bit
branch
from
August 24, 2022 12:54
b985064
to
7faba7e
Compare
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
stevendpclark
approved these changes
Aug 24, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, one possible additional test assertion we could add but feel free to merge without it.
kitography
approved these changes
Aug 24, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me :)
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
cipherboy
deleted the
cipherboy-dont-allow-crl-usage-without-signing-bit
branch
December 1, 2022 14:58
maxb
added a commit
to maxb/vault
that referenced
this pull request
Apr 18, 2023
Fix PKI issuer upgrade logic when upgrading to 1.12 or later, to actually turn off the issuer crl-signing usage when it intended to. Fix minor typo in docs.
cipherboy
pushed a commit
that referenced
this pull request
Apr 18, 2023
stevendpclark
pushed a commit
that referenced
this pull request
Apr 18, 2023
stevendpclark
pushed a commit
that referenced
this pull request
Apr 18, 2023
stevendpclark
pushed a commit
that referenced
this pull request
Apr 18, 2023
* Minor follow-ups to #16865 Fix PKI issuer upgrade logic when upgrading to 1.12 or later, to actually turn off the issuer crl-signing usage when it intended to. Fix minor typo in docs. * changelog Co-authored-by: Max Bowsher <maxbowsher@gmail.com>
stevendpclark
pushed a commit
that referenced
this pull request
Apr 18, 2023
* Minor follow-ups to #16865 Fix PKI issuer upgrade logic when upgrading to 1.12 or later, to actually turn off the issuer crl-signing usage when it intended to. Fix minor typo in docs. * changelog Co-authored-by: Max Bowsher <maxbowsher@gmail.com>
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.
When Vault imports certificates without KU for CRLSign, we shouldn't
provision CRLUsage on the backing issuer; otherwise, we'll attempt to
build CRLs and Go will cause us to err out. This change makes it clear
(at issuer configuration time) that we can't possibly support this
operation and hopefully prevent users from running into the more cryptic
Go error.
Note that this does not apply for OCSP EKU: the EKU exists, per RFC 6960
Section 2.6 OCSP Signature Authority Delegation, to allow delegation of
OCSP signing to a child certificate. This EKU is not necessary on the
issuer itself, and generally assumes issuers are allowed to issue OCSP
responses regardless of KU/EKU.
n.b.: this mostly fails when the CRLSign usage isn't set on an externally created certificate (and subsequently imported into Vault). Vault itself doesn't allow modification of KU during root creation.