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

Add proof possession revocation for PKI secrets engine #16566

Merged
merged 7 commits into from
Aug 16, 2022

Conversation

cipherboy
Copy link
Contributor

@cipherboy cipherboy commented Aug 3, 2022

This PR adds support to the PKI secrets engine for revocation of certs without requiring privileged access to the /revoke endpoint. In particular, we use the private key as evidence of permission to revoke the certificate: either the private key was leaked, therefore suggesting the certificate should be revoked anyways, or someone with access to the private key material (and thus, likely the original issuer) suggests the certificate should be revoked and could publish the private key publicly (therefore, triggering case one).

This is done by hitting a new /revoke-with-key endpoint with the private_key field (taking a PEM-encoded private key). This endpoint otherwise has semantics similar to /revoke (allowing it to either revoke by BYOC or stored cert serial number), but is less privileged.


@cipherboy cipherboy added this to the 1.12.0-rc1 milestone Aug 3, 2022
@cipherboy cipherboy requested review from kitography, stevendpclark and a team August 3, 2022 20:31
@cipherboy cipherboy force-pushed the cipherboy-add-proof-possession-revocation branch from 2bdf39e to 3b9bcdd Compare August 3, 2022 20:43
Revocation by proof of possession ensures that we have a private key
matching the (provided or stored) certificate. This allows callers to
revoke certificate they own (as proven by holding the corresponding
private key), without having an admin create innumerable ACLs around
the serial_number parameter for every issuance/user.

We base this on Go TLS stack's verification of certificate<->key
matching, but extend it where applicable to ensure curves match, the
private key is indeed valid, and has the same structure as the
corresponding public key from the certificate.

This endpoint currently is authenticated, allowing operators to disable
the endpoint if it isn't desirable to use, via ACL policies.

Signed-off-by: Alexander Scheel <[email protected]>
Signed-off-by: Alexander Scheel <[email protected]>
After some discussion, given the potential for DoS (via submitting a lot
of keys/certs to validate, including invalid pairs), it seems best to
leave this as an authenticated endpoint. Presently in Vault, there's no
way to have an authenticated-but-unauthorized path (i.e., one which
bypasses ACL controls), so it is recommended (but not enforced) to make
this endpoint generally available by permissive ACL policies.

Signed-off-by: Alexander Scheel <[email protected]>
Signed-off-by: Alexander Scheel <[email protected]>
@cipherboy cipherboy force-pushed the cipherboy-add-proof-possession-revocation branch from 3b9bcdd to 9b7782a Compare August 15, 2022 22:54
@cipherboy cipherboy marked this pull request as ready for review August 15, 2022 22:55
@cipherboy cipherboy requested a review from taoism4504 as a code owner August 15, 2022 22:55
@cipherboy
Copy link
Contributor Author

@kitography / @stevendpclark -- this is ready for review now \o/

Copy link
Contributor

@stevendpclark stevendpclark left a comment

Choose a reason for hiding this comment

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

👍 one small nit of an additional test request.

builtin/logical/pki/crl_test.go Show resolved Hide resolved
@cipherboy cipherboy force-pushed the cipherboy-add-proof-possession-revocation branch from c59b21c to 5076d1d Compare August 16, 2022 17:47
@cipherboy cipherboy enabled auto-merge (squash) August 16, 2022 17:52
@cipherboy cipherboy merged commit 8de51d4 into main Aug 16, 2022
@cipherboy cipherboy deleted the cipherboy-add-proof-possession-revocation branch December 1, 2022 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants