demo: fix UnusedBit panic on signing error - #262
Conversation
| if sig, err := certSig.Bytes(); err == nil && len(sig) == 0 || sig[len(sig)-1]&1 != 0 { | ||
| certSig.SetError(errors.New("last bit in signature with not zero, unable to encode as unused")) | ||
| sig, err := certSig.Bytes() | ||
| if err == nil && (len(sig) == 0 || sig[len(sig)-1]&1 != 0) { |
There was a problem hiding this comment.
Any reason this changed the structure to pull the assignment out of the if?
| if r := recover(); r != nil { | ||
| t.Fatalf("CreateCertificate panicked instead of returning an error: %v", r) | ||
| } | ||
| }() |
There was a problem hiding this comment.
What's this for? Go's test framework will already catch panics.
| @@ -0,0 +1,85 @@ | |||
| package main | |||
There was a problem hiding this comment.
TBH I'm not sure how much it's worth bothering with a test here. It seems mostly scar tissue. At the least, let's put this in encode_test.go.
| if !ok { | ||
| t.Fatalf("could not make issuer trust anchor ID") | ||
| } | ||
| cosignerID, ok := TrustAnchorIDFromString("32473.2") |
There was a problem hiding this comment.
Same ID as the issuer is fine. We expect to have at least one cosigner that's the same name as the issuer anyway.
| 0x0d, 0x46, 0xfb, 0xdd, 0xa9, 0xa9, 0x1e, 0x9d, 0xdc, 0xba, 0x5a, 0x01, | ||
| 0xe7, 0xd6, 0x97, 0xa8, 0x0a, 0x18, 0xf9, 0xc3, 0xc4, 0xa3, 0x1e, 0x56, | ||
| 0xe2, 0x7c, 0x83, 0x48, 0xdb, 0x16, 0x1a, 0x1c, 0xf5, 0x1d, 0x7e, 0xf1, | ||
| 0x94, 0x2d, 0x4b, 0xcf, 0x72, 0x22, 0xc1, |
There was a problem hiding this comment.
I suspect this test does not actually need you to put anything here, but if you put this in encode_test.go, it can at least share a constant.
| SignatureAlgorithm: SignatureAlgorithmEd25519, | ||
| Signer: failingSigner{}, | ||
| SignerOpts: crypto.Hash(0), | ||
| } |
There was a problem hiding this comment.
Hmm, I am not thrilled about breaking the Cosigner abstraction here. It's something we'll have to remember to update every time we update that struct.
Instead of all this, I think you could get the same coverage by just passing an invalid subtree to CreateCertificate.
87afb57 to
a6888e8
Compare
|
Thanks, that makes sense. Reduced this to the minimal one-line fix and dropped the regression test. |
2 participants
Fixes an operator-precedence bug in CreateCertificate's UnusedBit handling.
&& binds tighter than ||, so sig[len(sig)-1] could be evaluated even when certSig.Bytes() returned an error. Add parentheses so the signature bytes are only inspected when Bytes() succeeds.
Tested:
go test ./...