Add role parameter to restrict issuance of wildcard certificates #14238
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
This field allows the PKI role to control whether or not issuance of wildcard certificates are allowed. We default (both on migration and new role creation) to the less secure true value for backwards compatibility with existing Vault versions. Signed-off-by: Alexander Scheel <[email protected]>
Per comment, this variable name was confusing during the reproduction and subsequent fix of the earlier vulnerability and associated bug report. Because the common name isn't necessarily _sanitized_ in any way (and indeed must be considered in relation to other parts or the whole), but portions of the entire name are removed, reducedName appears to make the most sense. Signed-off-by: Alexander Scheel <[email protected]>
This commit adds the bulk of correctly validating wildcard certificate Common Names during issuance according to RFC 6125 Section 6.4.3 semantics. As part of this, support for RFC 2818-conforming wildcard certificates (wherein there are almost no restrictions on issuance) has been removed. Note that this flag does take precedence over AllowAnyName, giving a little more safety in wildcard issuance in this case. Signed-off-by: Alexander Scheel <[email protected]>
Test cases 19, 70+71, and 83+84 didn't conform with the RFC 6125, and so should've been rejected under strict conformance. For 70+71 and 83+84, we previously conditioned around the value of AllowSubdomains (allowing issuance when true), but they likely should've been rejected either way. Additionally, update the notes about globs matching wildcard certificates to notate this is indeed the case. Signed-off-by: Alexander Scheel <[email protected]>
This allows for regression tests to cover the new AllowWildcardCertificate conditional. We add additional test cases ensuring that wildcard issuance is properly forbidden in all relevant scenarios, while allowing the existing test cases to validate that wildcard status doesn't affect non-wildcard certificates. Signed-off-by: Alexander Scheel <[email protected]>
When using sign-verbatim, sign-intermediate, or getting certificate generation parameters, set AllowWildcardCertificates to mirror existing policies. Signed-off-by: Alexander Scheel <[email protected]>
This was referenced Feb 23, 2022
Signed-off-by: Alexander Scheel <[email protected]>
cipherboy
force-pushed
the
cipherboy-add-wildcard-restrictions
branch
from
48a18cd to
0a72404
Compare
schultz-is
approved these changes
Feb 23, 2022
cipherboy
added a commit
that referenced
this pull request
Feb 24, 2022
) * Add new AllowWildcardCertificate field to PKI role This field allows the PKI role to control whether or not issuance of wildcard certificates are allowed. We default (both on migration and new role creation) to the less secure true value for backwards compatibility with existing Vault versions. Signed-off-by: Alexander Scheel <[email protected]> * Refactor sanitizedName to reducedName Per comment, this variable name was confusing during the reproduction and subsequent fix of the earlier vulnerability and associated bug report. Because the common name isn't necessarily _sanitized_ in any way (and indeed must be considered in relation to other parts or the whole), but portions of the entire name are removed, reducedName appears to make the most sense. Signed-off-by: Alexander Scheel <[email protected]> * Enforce AllowWildcardCertificates during issuance This commit adds the bulk of correctly validating wildcard certificate Common Names during issuance according to RFC 6125 Section 6.4.3 semantics. As part of this, support for RFC 2818-conforming wildcard certificates (wherein there are almost no restrictions on issuance) has been removed. Note that this flag does take precedence over AllowAnyName, giving a little more safety in wildcard issuance in this case. Signed-off-by: Alexander Scheel <[email protected]> * Update test cases to conform with RFC 6125 Test cases 19, 70+71, and 83+84 didn't conform with the RFC 6125, and so should've been rejected under strict conformance. For 70+71 and 83+84, we previously conditioned around the value of AllowSubdomains (allowing issuance when true), but they likely should've been rejected either way. Additionally, update the notes about globs matching wildcard certificates to notate this is indeed the case. Signed-off-by: Alexander Scheel <[email protected]> * Check AllowWildcardCertifciates in issuance tests This allows for regression tests to cover the new AllowWildcardCertificate conditional. We add additional test cases ensuring that wildcard issuance is properly forbidden in all relevant scenarios, while allowing the existing test cases to validate that wildcard status doesn't affect non-wildcard certificates. Signed-off-by: Alexander Scheel <[email protected]> * Add Wildcard allowance during signing operations When using sign-verbatim, sign-intermediate, or getting certificate generation parameters, set AllowWildcardCertificates to mirror existing policies. Signed-off-by: Alexander Scheel <[email protected]> * Add changelog entry Signed-off-by: Alexander Scheel <[email protected]>
cipherboy
added a commit
that referenced
this pull request
Feb 24, 2022
) * Add new AllowWildcardCertificate field to PKI role This field allows the PKI role to control whether or not issuance of wildcard certificates are allowed. We default (both on migration and new role creation) to the less secure true value for backwards compatibility with existing Vault versions. Signed-off-by: Alexander Scheel <[email protected]> * Refactor sanitizedName to reducedName Per comment, this variable name was confusing during the reproduction and subsequent fix of the earlier vulnerability and associated bug report. Because the common name isn't necessarily _sanitized_ in any way (and indeed must be considered in relation to other parts or the whole), but portions of the entire name are removed, reducedName appears to make the most sense. Signed-off-by: Alexander Scheel <[email protected]> * Enforce AllowWildcardCertificates during issuance This commit adds the bulk of correctly validating wildcard certificate Common Names during issuance according to RFC 6125 Section 6.4.3 semantics. As part of this, support for RFC 2818-conforming wildcard certificates (wherein there are almost no restrictions on issuance) has been removed. Note that this flag does take precedence over AllowAnyName, giving a little more safety in wildcard issuance in this case. Signed-off-by: Alexander Scheel <[email protected]> * Update test cases to conform with RFC 6125 Test cases 19, 70+71, and 83+84 didn't conform with the RFC 6125, and so should've been rejected under strict conformance. For 70+71 and 83+84, we previously conditioned around the value of AllowSubdomains (allowing issuance when true), but they likely should've been rejected either way. Additionally, update the notes about globs matching wildcard certificates to notate this is indeed the case. Signed-off-by: Alexander Scheel <[email protected]> * Check AllowWildcardCertifciates in issuance tests This allows for regression tests to cover the new AllowWildcardCertificate conditional. We add additional test cases ensuring that wildcard issuance is properly forbidden in all relevant scenarios, while allowing the existing test cases to validate that wildcard status doesn't affect non-wildcard certificates. Signed-off-by: Alexander Scheel <[email protected]> * Add Wildcard allowance during signing operations When using sign-verbatim, sign-intermediate, or getting certificate generation parameters, set AllowWildcardCertificates to mirror existing policies. Signed-off-by: Alexander Scheel <[email protected]> * Add changelog entry Signed-off-by: Alexander Scheel <[email protected]>
cipherboy
added a commit
that referenced
this pull request
Feb 24, 2022
) * Add new AllowWildcardCertificate field to PKI role This field allows the PKI role to control whether or not issuance of wildcard certificates are allowed. We default (both on migration and new role creation) to the less secure true value for backwards compatibility with existing Vault versions. Signed-off-by: Alexander Scheel <[email protected]> * Refactor sanitizedName to reducedName Per comment, this variable name was confusing during the reproduction and subsequent fix of the earlier vulnerability and associated bug report. Because the common name isn't necessarily _sanitized_ in any way (and indeed must be considered in relation to other parts or the whole), but portions of the entire name are removed, reducedName appears to make the most sense. Signed-off-by: Alexander Scheel <[email protected]> * Enforce AllowWildcardCertificates during issuance This commit adds the bulk of correctly validating wildcard certificate Common Names during issuance according to RFC 6125 Section 6.4.3 semantics. As part of this, support for RFC 2818-conforming wildcard certificates (wherein there are almost no restrictions on issuance) has been removed. Note that this flag does take precedence over AllowAnyName, giving a little more safety in wildcard issuance in this case. Signed-off-by: Alexander Scheel <[email protected]> * Update test cases to conform with RFC 6125 Test cases 19, 70+71, and 83+84 didn't conform with the RFC 6125, and so should've been rejected under strict conformance. For 70+71 and 83+84, we previously conditioned around the value of AllowSubdomains (allowing issuance when true), but they likely should've been rejected either way. Additionally, update the notes about globs matching wildcard certificates to notate this is indeed the case. Signed-off-by: Alexander Scheel <[email protected]> * Check AllowWildcardCertifciates in issuance tests This allows for regression tests to cover the new AllowWildcardCertificate conditional. We add additional test cases ensuring that wildcard issuance is properly forbidden in all relevant scenarios, while allowing the existing test cases to validate that wildcard status doesn't affect non-wildcard certificates. Signed-off-by: Alexander Scheel <[email protected]> * Add Wildcard allowance during signing operations When using sign-verbatim, sign-intermediate, or getting certificate generation parameters, set AllowWildcardCertificates to mirror existing policies. Signed-off-by: Alexander Scheel <[email protected]> * Add changelog entry Signed-off-by: Alexander Scheel <[email protected]>
cipherboy
added a commit
that referenced
this pull request
Feb 24, 2022
…certificates + Clarify documentation around certificate issuance (#14251) * Add role parameter to restrict issuance of wildcard certificates (#14238) * Add new AllowWildcardCertificate field to PKI role This field allows the PKI role to control whether or not issuance of wildcard certificates are allowed. We default (both on migration and new role creation) to the less secure true value for backwards compatibility with existing Vault versions. Signed-off-by: Alexander Scheel <[email protected]> * Refactor sanitizedName to reducedName Per comment, this variable name was confusing during the reproduction and subsequent fix of the earlier vulnerability and associated bug report. Because the common name isn't necessarily _sanitized_ in any way (and indeed must be considered in relation to other parts or the whole), but portions of the entire name are removed, reducedName appears to make the most sense. Signed-off-by: Alexander Scheel <[email protected]> * Enforce AllowWildcardCertificates during issuance This commit adds the bulk of correctly validating wildcard certificate Common Names during issuance according to RFC 6125 Section 6.4.3 semantics. As part of this, support for RFC 2818-conforming wildcard certificates (wherein there are almost no restrictions on issuance) has been removed. Note that this flag does take precedence over AllowAnyName, giving a little more safety in wildcard issuance in this case. Signed-off-by: Alexander Scheel <[email protected]> * Update test cases to conform with RFC 6125 Test cases 19, 70+71, and 83+84 didn't conform with the RFC 6125, and so should've been rejected under strict conformance. For 70+71 and 83+84, we previously conditioned around the value of AllowSubdomains (allowing issuance when true), but they likely should've been rejected either way. Additionally, update the notes about globs matching wildcard certificates to notate this is indeed the case. Signed-off-by: Alexander Scheel <[email protected]> * Check AllowWildcardCertifciates in issuance tests This allows for regression tests to cover the new AllowWildcardCertificate conditional. We add additional test cases ensuring that wildcard issuance is properly forbidden in all relevant scenarios, while allowing the existing test cases to validate that wildcard status doesn't affect non-wildcard certificates. Signed-off-by: Alexander Scheel <[email protected]> * Add Wildcard allowance during signing operations When using sign-verbatim, sign-intermediate, or getting certificate generation parameters, set AllowWildcardCertificates to mirror existing policies. Signed-off-by: Alexander Scheel <[email protected]> * Add changelog entry Signed-off-by: Alexander Scheel <[email protected]> * Clarify documentation around certificate issuance (#14236) We note that: - allow_bare_domains, allow_glob_domains, and allow_subdomains are all independent, - enforce_hostnames and allow_wildcard_certificates take precedence over allow_any_name, - We limit to RFC 6125 wildcards. - Clarify that both allow_bare_domains and allow_glob_domains will permit wildcard issuance in certain scenarios. Co-authored-by: mickael-hc <[email protected]> Co-authored-by: Kit Haines <[email protected]> Signed-off-by: Alexander Scheel <[email protected]> Co-authored-by: mickael-hc <[email protected]> Co-authored-by: Kit Haines <[email protected]> Co-authored-by: mickael-hc <[email protected]> Co-authored-by: Kit Haines <[email protected]>
cipherboy
added a commit
that referenced
this pull request
Feb 24, 2022
…certificates + Clarify documentation around certificate issuance (#14252) * Add role parameter to restrict issuance of wildcard certificates (#14238) * Add new AllowWildcardCertificate field to PKI role This field allows the PKI role to control whether or not issuance of wildcard certificates are allowed. We default (both on migration and new role creation) to the less secure true value for backwards compatibility with existing Vault versions. Signed-off-by: Alexander Scheel <[email protected]> * Refactor sanitizedName to reducedName Per comment, this variable name was confusing during the reproduction and subsequent fix of the earlier vulnerability and associated bug report. Because the common name isn't necessarily _sanitized_ in any way (and indeed must be considered in relation to other parts or the whole), but portions of the entire name are removed, reducedName appears to make the most sense. Signed-off-by: Alexander Scheel <[email protected]> * Enforce AllowWildcardCertificates during issuance This commit adds the bulk of correctly validating wildcard certificate Common Names during issuance according to RFC 6125 Section 6.4.3 semantics. As part of this, support for RFC 2818-conforming wildcard certificates (wherein there are almost no restrictions on issuance) has been removed. Note that this flag does take precedence over AllowAnyName, giving a little more safety in wildcard issuance in this case. Signed-off-by: Alexander Scheel <[email protected]> * Update test cases to conform with RFC 6125 Test cases 19, 70+71, and 83+84 didn't conform with the RFC 6125, and so should've been rejected under strict conformance. For 70+71 and 83+84, we previously conditioned around the value of AllowSubdomains (allowing issuance when true), but they likely should've been rejected either way. Additionally, update the notes about globs matching wildcard certificates to notate this is indeed the case. Signed-off-by: Alexander Scheel <[email protected]> * Check AllowWildcardCertifciates in issuance tests This allows for regression tests to cover the new AllowWildcardCertificate conditional. We add additional test cases ensuring that wildcard issuance is properly forbidden in all relevant scenarios, while allowing the existing test cases to validate that wildcard status doesn't affect non-wildcard certificates. Signed-off-by: Alexander Scheel <[email protected]> * Add Wildcard allowance during signing operations When using sign-verbatim, sign-intermediate, or getting certificate generation parameters, set AllowWildcardCertificates to mirror existing policies. Signed-off-by: Alexander Scheel <[email protected]> * Add changelog entry Signed-off-by: Alexander Scheel <[email protected]> * Clarify documentation around certificate issuance (#14236) We note that: - allow_bare_domains, allow_glob_domains, and allow_subdomains are all independent, - enforce_hostnames and allow_wildcard_certificates take precedence over allow_any_name, - We limit to RFC 6125 wildcards. - Clarify that both allow_bare_domains and allow_glob_domains will permit wildcard issuance in certain scenarios. Co-authored-by: mickael-hc <[email protected]> Co-authored-by: Kit Haines <[email protected]> Signed-off-by: Alexander Scheel <[email protected]> Co-authored-by: mickael-hc <[email protected]> Co-authored-by: Kit Haines <[email protected]> Co-authored-by: mickael-hc <[email protected]> Co-authored-by: Kit Haines <[email protected]>
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.
This pull request allows Vault operators to restrict issuance of wildcard certificates via role configuration. We also fix several bugs in the parsing of wildcard certificates from a RFC 6125 perspective, permitting wildcard globs only in the left-most DNS label.
For compatibility with existing Vault releases, this field defaults to
trueand must explicitly be set tofalse(at role creation time or via an update) in order to take advantage of this change.Documented in: #14236
Resolves: #7592