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

Allow admin command to block key from a CSR file #7770

Merged
merged 5 commits into from
Nov 4, 2024
Merged

Conversation

mcpherrinm
Copy link
Contributor

@mcpherrinm mcpherrinm commented Oct 25, 2024

One format we receive key compromise reports is as a CSR file. For example, from https://pwnedkeys.com/revokinator

This allows the admin command to block a key from a CSR directly, instead of needing to validate it manually and get the SPKI or key from it.

I've added a flag (default true) to check the signature on the CSR, in case we ever decide we want to block a key from a CSR with a bad signature for whatever reason.

@mcpherrinm mcpherrinm requested a review from a team as a code owner October 25, 2024 18:09
@mcpherrinm mcpherrinm changed the title Allow admin command to revoke from a CSR file Allow admin command to block key from a CSR file Oct 25, 2024
aarongable
aarongable previously approved these changes Oct 25, 2024
Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

nit: the test only covers one path through the new function: the signature is bad, but we're not checking it. It would be nice to have tests for a) the signature is good and we are checking it, and b) the signature is bad but we are checking it.

@aarongable aarongable requested review from a team and jprenken and removed request for a team October 25, 2024 19:34
@andygabby
Copy link
Member

andygabby commented Oct 25, 2024

I worry a bit that it could be a foot gun. A CSR isn't always proof of control of a private key and this could trigger bad-key-revoker to revoke certificates without proof of key compromise. Perhaps we should check the subject for a particular set of phrases and require the CSR to have a subject that matches.

Even better would be if the requestor used acme to revoke the certificate with proof of key compromise which already supports blocking of the key.

EDIT: In a side discussion I was reminded that an operator has to use admin block-key which is probably explicit enough to avoid someone not knowing what they are doing. This operational path is at the discretion of the operating CA to decide if it is appropriate.

@mcpherrinm
Copy link
Contributor Author

mcpherrinm commented Oct 25, 2024

b) the signature is bad but we are checking it

This case is tested already, I could add a "Signature is good" test too, and I can see value in that.

@jsha
Copy link
Contributor

jsha commented Oct 25, 2024

an operator has to use admin block-key which is probably explicit enough to avoid someone not knowing what they are doing.

It's important to have this sort of manual override, but I think it's also important to support human operators with as much automation as possible. I think this mode should have a second flag, -expected-cn. Since there's mainly one person who reports compromised keys using CSRs, and they always use the same CN, I think it's okay to default it to that value.

Basically I agree with @andygabby's original take: this should have an additional guardrail on by default.

Also, what do you think of audit-logging the CSR we relied on? And perhaps its parsed Subject for convenience?

cmd/admin/key.go Outdated Show resolved Hide resolved
jprenken
jprenken previously approved these changes Oct 26, 2024
@aarongable
Copy link
Contributor

(I believe we're waiting for this PR to grow a "check the subject is an expected value" flag, which @jsha indicated he would contribute.)

@mcpherrinm
Copy link
Contributor Author

Also audit logging of the CSR too. I have other things to do today but could get to this tomorrow.

@jsha jsha dismissed stale reviews from beautifulentropy and jprenken via af51f6e November 1, 2024 21:46
@aarongable aarongable merged commit 1fa6678 into main Nov 4, 2024
12 checks passed
@aarongable aarongable deleted the mattm-admin-csr branch November 4, 2024 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants