Signing config cleanup #2420
Signing config cleanup #2420
Conversation
| iter := jwks.Keys(ctx) | ||
| for iter.Next(ctx) { | ||
| keyI := iter.Pair().Value | ||
| key, ok := keyI.(jwk.Key) | ||
| if !ok { | ||
| return nil, fmt.Errorf("Job %s keyset contains a non-key at index %d", keysetType, iter.Pair().Index) | ||
| } |
There was a problem hiding this comment.
it's deeply odd to me that the iterator for the jwk.Set type (which can only contain jwk.Keys) returns an int index and an any key, but ¯\_(ツ)_/¯
| func injectAlgorithm(key jwk.Key, cfg PipelineUploadConfig) error { | ||
| keyHasAlgorithm := key.Algorithm() != nil && key.Algorithm().String() != "" | ||
|
|
||
| var alg jwa.KeyAlgorithm | ||
| var from string // for error messages | ||
| switch { | ||
| case !keyHasAlgorithm && cfg.SigningAlgorithm == "": | ||
| return fmt.Errorf("signing algorithm is required if JWKS key doesn't have an alg parameter") | ||
|
|
||
| case keyHasAlgorithm && cfg.SigningAlgorithm != "": | ||
| return fmt.Errorf("signing algorithm is not allowed if JWKS key has an alg parameter") | ||
|
|
||
| case cfg.SigningAlgorithm != "": | ||
| alg = jwa.SignatureAlgorithm(cfg.SigningAlgorithm) | ||
| from = "from --signing-algorithm flag" | ||
|
|
||
| case keyHasAlgorithm: | ||
| alg = key.Algorithm() | ||
| from = fmt.Sprintf("from JWKS, key ID %s", cfg.SigningKeyID) | ||
| } | ||
|
|
||
| // The above switch is exhaustive, but just in case, check that the alg isn't invalid (ie. empty) | ||
| if _, ok := alg.(jwa.InvalidKeyAlgorithm); ok { | ||
| return fmt.Errorf("signing algorithm %s is not a valid signing algorithm", alg) | ||
| } | ||
|
|
||
| signingAlg, ok := alg.(jwa.SignatureAlgorithm) | ||
| if !ok { | ||
| return fmt.Errorf("algorithm %s (%s) cannot be used for signing/verification", alg, from) | ||
| } | ||
|
|
||
| err := key.Set(jwk.AlgorithmKey, signingAlg) | ||
| if err != nil { | ||
| return fmt.Errorf("setting signing algorithm on key: %v", err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
goodbye, ugly terrible function, you have served your purpose
internal/pipeline/sign.go
Outdated
| @@ -111,7 +111,7 @@ func (s *Signature) Verify(env map[string]string, sf SignedFielder, keySet jwk.S | |||
| } | |||
|
|
|||
| _, err = jws.Verify([]byte(s.Value), | |||
| jws.WithKeySet(keySet), | |||
| jws.WithKeySet(keySet, jws.WithRequireKid(false)), | |||
There was a problem hiding this comment.
without this change, all keys would have to have key ids. while the keygen utility we wrote will always output keys with key ids, we don't enforce that people use our tooling, and should let users not use key ids if they don't want to.
There was a problem hiding this comment.
hmm, one downside of this change is that it makes the error messaging for "you don't have the right keys for verification" a bit worse. When we required key IDs, the message was something like
WARNING: Job verification failed: invalid signature: key provider 0 failed: failed to find key with key ID "my-cool-key" in key set
which tells you a fair bit about the problem.
with jws.WithRequireKid(false), the error message is:
⚠️ WARNING: Job verification failed: invalid signature: could not verify message using any of the signatures or keys
which isn't too too much worse, but it's the same as if the job couldn't be verified (ie someone did crimes on the job).
i'm in two minds as to whether we should require/not require key IDs anyway, would love to hear some thoughts on this
There was a problem hiding this comment.
I'm happy to require kids because:
- You're going to need them if you want to rotate keys and maintain your sanity
- Our tool keygen requires them
So if we enforce that the key always has an ID for both signing and verification, users who use the 'blessed path' aren't going to fail.
One thing we could do is generate a key id if users can't be bothered to supply one. I should have thought of that when reviewing this: #2415
If we do that, then users don't have to think about it until they need it.
moskyb
force-pushed
the
signing-config-cleanup
branch
from
e13cd9d to
2e2fee9
Compare
|
i've just rebased away the commit that makes key IDs optional |
moskyb
force-pushed
the
signing-config-cleanup
branch
2 times, most recently
from
c8b5647 to
33ab6a4
Compare
…ature-behavior into a single flag
moskyb
force-pushed
the
signing-config-cleanup
branch
from
33ab6a4 to
cb72f8e
Compare
Closes: PDP-1764
This PR: Makes three changes to the way pipeline signing is configured:
--signing-algorithmoption, and the way that it would inject the algorithm into keys. As such, thealgkey is now mandatory in signing JWKst makesThis has been reverted, key IDs are now mandatory again--signing-key-idoptional. If one is provided, then the agent will use the signing key referred to by that ID, if it's present. If no signing key id is provided, the agent will use the first key in the JWKS. When verifyinh, the jwx library will just try every key in the verification key set--job-verification-no-signature-behaviorand--job-verification-invalid-signature-behaviorinto one combined flag,--job-verification-failure-behaviour. This is simpler and a lot less wordy.