Support other key algorithms for Rekor v2 #520
Conversation
|
In the first iteration of this change, I had created a test struct implementing support for other signing algorithms. It seemed useful to just add support to EphemeralKeypair, but if there's any concerns, I can change this. |
|
Thanks Hayden! The code looks good, some minor comments only. |
haydentherapper
force-pushed
the
support-other-keys-rekorv2
branch
from
ecd4e1e to
9f828c3
Compare
pkg/sign/certificate.go
Outdated
| // Fulcio doesn't support verifying Ed25519ph signatures at the moment, | ||
| // so if we're signing with the prehash variant, we need to adapt the | ||
| // signer to sign with Ed25519 instead. | ||
| subjectSignature, _, err := keypair.SignData(context.WithValue(ctx, adaptEd25519Ctx{}, true), []byte(subject)) |
There was a problem hiding this comment.
We did something similar in Cosign - https://github.com/sigstore/cosign/blob/eed2a117b8e707d702f43e113bc3488a4e3af457/cmd/cosign/cli/sign/sign.go#L633
There was a problem hiding this comment.
Just so I am understanding this. If a client uses Ed25519ph when requesting a cert from fulcio, we silently fallback to Ed25519 but with the same key?
There was a problem hiding this comment.
Correct, with the same key. We only fallback to Ed25519 to sign the proof of possession, not the artifact signature (Rekor does properly verify ed25519ph).
The reason Fulcio doesn't support Ed25519ph is because crypto/x509 doesn't support the prehash variant. There's no public OID to represent the key algorithm. When I work on the next major release of Fulcio, I'll see what I can to hack this in.
There was a problem hiding this comment.
I appreciate the work being done here to try to work around a lack of support for ed25519ph in Fulcio, but I wonder if instead we should return an error for now instead.
What you have here works, but if / when Fulcio supports ed25519ph in the future, figuring out how to unwind this and migrate people off is going to be a bit tricky. Is there a particular need for this use-case right now?
There was a problem hiding this comment.
The use-case is a bit niche, to support hashedrekord entries with ed25519 signatures and with a Fulcio certificate (note that Rekor already supports verifying both ed25519 and -ph signatures).
Note that if you are signing an entry with a key and not requesting a Fulcio certificate, then this isn't needed. This would only be if you use Cosign with --issue-certificate or if you specify the signature algorithm (not yet supported, sigstore/cosign#3497).
I'm OK with an error for now.
There was a problem hiding this comment.
I've dropped support for ed25519ph with Fulcio. I also added e2e testing when signing with a self-managed key.
haydentherapper
force-pushed
the
support-other-keys-rekorv2
branch
2 times, most recently
from
34d0581 to
0e0b287
Compare
We hardcoded ECDSA-P256-SHA256 as the only supported algorithm. This uses the algorithm registry to load the correct signing algorithm to specify its type and digest in the request to Rekor v2. This also fixes an incompatibility with Ed25519 and hashedrekord with Rekor v2, which requires Ed25519ph where the digest is provided during verification. To test this, I've added support for other signing algorithms in EphemeralKeypair, which will also make the struct useable with Cosign when a signing algorithm is provided. Signed-off-by: Hayden <[email protected]>
haydentherapper
force-pushed
the
support-other-keys-rekorv2
branch
from
0e0b287 to
a9e0374
Compare
We hardcoded ECDSA-P256-SHA256 as the only supported algorithm. This uses the algorithm registry to load the correct signing algorithm to specify its type and digest in the request to Rekor v2. This also fixes an incompatibility with Ed25519 and hashedrekord with Rekor v2, which requires Ed25519ph where the digest is provided during verification.
To test this, I've added support for other signing algorithms in EphemeralKeypair, which will also make the struct useable with Cosign when a signing algorithm is provided.
Summary
Release Note
Documentation