Skip to content
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

pki: When a role sets key_type to any ignore key_bits value when signing a csr #16246

Merged
merged 4 commits into from
Jul 8, 2022

Conversation

stevendpclark
Copy link
Contributor

This addresses issue #16237.

When signing a CSR using /sign/<:role name> api method we validate that the CSR matches up with the role's key_type and key_bits values. If the key_type is set to any, these checks don't really make sense, and these checks were effectively bypassed when the key_bits value was set to current default value of 0.

The issue that was missed was the default value for created role's key_bits parameter was changed in 1.10 from 2048 to 0. So effectively the fix we previously made within PR#14875 addressed the issue, but only when the role was created in Vault 1.10 and higher.

Now we bypass the validation for the role's key_bits value when signing CSRs if the key_type is set to any. We still validate the key is at least 2048 for RSA backed CSRs as we did in 1.9.x and lower.

 - Bypass the validation for the role's key_bits value when signing CSRs
   if the key_type is set to any. We still validate the key is at least
   2048 for RSA backed CSRs as we did in 1.9.x and lower.
@stevendpclark stevendpclark added bug Used to indicate a potential bug secret/pki backport/1.10.x labels Jul 7, 2022
@stevendpclark stevendpclark requested a review from a team July 7, 2022 18:24
@stevendpclark stevendpclark force-pushed the stevendpclark/vault-6913-fix-pki-signing branch from b2f8bb4 to a4cfdcc Compare July 7, 2022 18:39
Copy link
Contributor

@kitography kitography left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confused?

return nil, errutil.InternalError{Err: fmt.Sprintf("unknown internal error updating default values: %v", err)}
}

// We're using the KeyBits field as a minimum value, and P-224 is safe
// and a previously allowed value. However, the above call defaults
// to P-256 as that's a saner default than P-224 (w.r.t. generation).
// So, override our fake Role value if it was previously zero.
if actualKeyType == "ec" && roleBitsWasZero {
if actualKeyType == "ec" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't quite look right? If a role's Keybits was 256, and we were using P-256 why would we change that without testing it was 0 (or any)? Also comment no longer looks right.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed all this code is a tad confusing. The api docs specify the following

signing existing CSRs, any can be specified to allow keys of either type and with any bit size (subject to >1024 bits for RSA keys).

So we are within the any block so we are resetting the minimum size of key_bits no matter what the role had as a value to the lowest values we feel comfortable supporting for EC keys like we do for RSA keys.

I'll tweak the comment a bit dropping the "if it was previously zero" that I missed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slack conversation:

Kit Haines
4:46 PM
I’m no longer sure if we are referring to “any” as keyBits or keyType
4:47
if we have set keyType “any” and keyBits “256" would we want to update that on the role?
4:47
or is that role not saved?

Steve Clark
4:47 PM
ha, yeah okay. When keyType == ‘any’ basically we just enforce mins for key bits no matter what the role says
4:48
That role that we update in the code you are seeing it thrown away
4:49
Alex had a different fix which might be clearer, I’m not sure. He was going to change the getRole function to always return a value of 0 for KeyBits if the role had a KeyType == ‘any’

Kit Haines
4:50 PM
^ I think I’d fine that clearer, but given we are throwing away the changes, I guess it doesn’t actually matter

Copy link
Contributor

@kitography kitography left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

return nil, errutil.InternalError{Err: fmt.Sprintf("unknown internal error updating default values: %v", err)}
}

// We're using the KeyBits field as a minimum value, and P-224 is safe
// and a previously allowed value. However, the above call defaults
// to P-256 as that's a saner default than P-224 (w.r.t. generation).
// So, override our fake Role value if it was previously zero.
if actualKeyType == "ec" && roleBitsWasZero {
if actualKeyType == "ec" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slack conversation:

Kit Haines
4:46 PM
I’m no longer sure if we are referring to “any” as keyBits or keyType
4:47
if we have set keyType “any” and keyBits “256" would we want to update that on the role?
4:47
or is that role not saved?

Steve Clark
4:47 PM
ha, yeah okay. When keyType == ‘any’ basically we just enforce mins for key bits no matter what the role says
4:48
That role that we update in the code you are seeing it thrown away
4:49
Alex had a different fix which might be clearer, I’m not sure. He was going to change the getRole function to always return a value of 0 for KeyBits if the role had a KeyType == ‘any’

Kit Haines
4:50 PM
^ I think I’d fine that clearer, but given we are throwing away the changes, I guess it doesn’t actually matter

changelog/16246.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@cipherboy cipherboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this makes sense to me now too. Thanks for picking this up @stevendpclark!

@stevendpclark stevendpclark merged commit a186651 into main Jul 8, 2022
stevendpclark added a commit that referenced this pull request Jul 8, 2022
…ing a csr (#16246)

* pki: When a role sets key_type to any ignore key_bits value when signing

 - Bypass the validation for the role's key_bits value when signing CSRs
   if the key_type is set to any. We still validate the key is at least
   2048 for RSA backed CSRs as we did in 1.9.x and lower.
@stevendpclark stevendpclark deleted the stevendpclark/vault-6913-fix-pki-signing branch July 8, 2022 17:37
stevendpclark added a commit that referenced this pull request Jul 8, 2022
…ing a csr (#16246) (#16260)

* pki: When a role sets key_type to any ignore key_bits value when signing

 - Bypass the validation for the role's key_bits value when signing CSRs
   if the key_type is set to any. We still validate the key is at least
   2048 for RSA backed CSRs as we did in 1.9.x and lower.

Co-authored-by: Steven Clark <steven.clark@hashicorp.com>
stevendpclark added a commit that referenced this pull request Jul 8, 2022
…ing a csr (#16246)

* pki: When a role sets key_type to any ignore key_bits value when signing

 - Bypass the validation for the role's key_bits value when signing CSRs
   if the key_type is set to any. We still validate the key is at least
   2048 for RSA backed CSRs as we did in 1.9.x and lower.
stevendpclark added a commit that referenced this pull request Jul 8, 2022
…ing a csr (#16246) (#16259)

* pki: When a role sets key_type to any ignore key_bits value when signing

 - Bypass the validation for the role's key_bits value when signing CSRs
   if the key_type is set to any. We still validate the key is at least
   2048 for RSA backed CSRs as we did in 1.9.x and lower.

Co-authored-by: Steven Clark <steven.clark@hashicorp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Used to indicate a potential bug secret/pki
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consul Connect fails to renew leaf certificates after 1.10 update
3 participants