Make it possible to enable NIST P521 curve to verify a public key.

[kommendorkapten]

👋 from the Sigstore project.

This PR enables optional verification of NIST P521 keys. Default key policy is not changed, which means that default only P256 and P384 is supported, so no changes are made to the current behaviour.

We are currently using goodkeys to verify public keys (see here), but we don't use it to verify EC keys due to the lack of P521 support, which we do allow in our public key policy. By getting this change merged upstream, we could use goodkeys to verify all EC keys as well.

[pgporada]

For other reviewers, if we accept this we'll need to make a change to our CP/CPS per https://letsencrypt.org/documents/isrg-cp-cps-v5.3/#dv-ssl-subscriber-certificate before deploying a version of boulder with this change.

Spitballing here, we could make a configuration stanza for all boulder components that import goodkey which would prevent a CP/CPS change, something like

...
"goodkey": {
  "ecdsaAllowedCurves": [256, 384, 521],
  "rsaAllowedModuli": [2048, 4096]
},
...

[aarongable]

if we accept this we'll need to make a change to our CP/CPS

I don't think this is the case -- this PR does not change the KeyPolicy returned by NewKeyPolicy(). So Boulder's behavior will remain the same, continuing to block P521 keys (and the newly-added test confirms this).

But, that does mean that anyone who wants to actually use this new capability would need to either a) hand-construct their KeyPolicy object (which would result in not having a blocked-keys list, and always doing 0 rounds of fermat factorization), or b) manually change the value of KeyPolicy.AllowECDSANISTP521 after creating the object, like

kp, err := goodkey.NewKeyPolicy(c, bkc)
if err != nil { ... }
kp.AllowECDSANISTP521 = true

which, imo, isn't great.

In fact, the only reason these "Allow..." fields are public/exported at all is so that various other Boulder unittests can set them.

To me, this seems like an anti-pattern. Instead, I think that there should be a separate NewCustomKeyPolicy function which takes a list of RSA key sizes and ECDSA curves to accept, so users like Sigstore could simply say

kp, err := goodkey.NewCustomKeyPolicy(..., elliptic.P521)

or something vaguely like that.

All that said, I don't think that that needs to happen in this PR. (But if you want to do it here, @kommendorkapten, I won't stop you!) I think this PR is fine as-is, and that we should follow it up with a refactoring that cleans up this code smell.

[aarongable]

In fact, I've gone ahead and done that follow-up refactoring here: https://github.com/letsencrypt/boulder/pull/7543

[aarongable]

@kommendorkapten if you merge main, it should fix the GitHub Actions CI failures seen here.

[kommendorkapten]

Thanks for all the feedback, getting this merged would be great ❤️

I'm not authorized to merge this PR, so anyone with that permission would be welcome to do so.

[aarongable]

I'm not authorized to merge this PR, so anyone with that permission would be welcome to do so.

Yep, we require all CI checks to be passing and two approvals from team-members after the latest commit. Looks like the merge of main was clean, so GitHub didn't dismiss our previous approvals, so one of us will be able to merge as soon as CI passes again.

Thanks for the contribution!

[kommendorkapten]

Thanks y'all 🙌