-
Notifications
You must be signed in to change notification settings - Fork 29
feat: added PKCS#7 envelope implementation #298
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
d9fabe7
99e0fab
7dc8aca
cf8313d
59d15e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,229 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Copyright The Notary Project Authors. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // you may not use this file except in compliance with the License. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // You may obtain a copy of the License at | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Unless required by applicable law or agreed to in writing, software | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // See the License for the specific language governing permissions and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // limitations under the License. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Package pkcs7 provides PKCS#7/CMS signature envelope for dm-verity signing. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Supports both local keys and remote signers (Azure Key Vault, plugins) via | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // the signerAdapter pattern. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| package pkcs7 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "crypto" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "crypto/x509" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "encoding/asn1" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "fmt" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "io" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "github.com/notaryproject/notation-core-go/signature" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| gopkcs7 "go.mozilla.org/pkcs7" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. jws and cose have compile-time assertions... this doesn't.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assertion has been added to end of file since this envelope is not wrapped in base.Envelope |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // MediaTypeEnvelope is the PKCS#7 signature envelope media type. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const MediaTypeEnvelope = "application/pkcs7-signature" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func init() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err := signature.RegisterEnvelopeType(MediaTypeEnvelope, NewEnvelope, ParseEnvelope); err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| panic(err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+40
to
+44
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will leave the init() as is for now to be consistent with JWS/COSE since I'm not sure if having a single consumer justifies deviating from the pattern. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type envelope struct { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raw []byte | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| p7 *gopkcs7.PKCS7 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| certs []*x509.Certificate | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sigBytes []byte | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // NewEnvelope creates a new PKCS#7 envelope. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Note: Unlike JWS/COSE, PKCS#7 for dm-verity does NOT use base.Envelope wrapper | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // because dm-verity signatures must not have signing-time (kernel requirement). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func NewEnvelope() signature.Envelope { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return &envelope{} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. jws and cose return &base.Envelope{Envelope: &envelope{}} which has critical validations. returning a bare envelope bypasses all of these checks.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This initial PR is scoped to a narrow profile of detached PKCS#7, no signed attributes, rsa-2048, and sha-256, validated end-to-end. The long-term plan would be to broaden this profile after the initial change lands. Broadening the profile would likely mean also coordinating changes to Given the scope described above, this envelope intentionally does not wrap To ensure validation is still present, we use |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+59
to
+61
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sharing from comment above, this initial PR is scoped to a narrow profile of detached PKCS#7 and gated behind |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // ParseEnvelope parses PKCS#7 DER bytes into an envelope. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func ParseEnvelope(envelopeBytes []byte) (signature.Envelope, error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| p7, err := gopkcs7.Parse(envelopeBytes) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, &signature.InvalidSignatureError{Msg: err.Error()} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var sigBytes []byte | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if len(p7.Signers) > 0 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sigBytes = p7.Signers[0].EncryptedDigest | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return &envelope{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raw: envelopeBytes, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| p7: p7, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| certs: p7.Certificates, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sigBytes: sigBytes, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
dallasd1 marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Sign implements signature.Envelope interface. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Uses signerAdapter pattern to support both local and remote signers (AKV, plugins). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
The current code declares SHA-256 in the envelope but accepts any signature the upstream signer produces. See top-level Finding 3 for the full chain. Two ways to fix the body: (a) restrict
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adopted option (a) with |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func (e *envelope) Sign(req *signature.SignRequest) ([]byte, error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Get key spec to determine algorithm | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| keySpec, err := req.Signer.KeySpec() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, &signature.InvalidSignRequestError{Msg: err.Error()} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Sign the content using the underlying signer (works with AKV, local keys, etc.) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sig, certs, err := req.Signer.Sign(req.Payload.Content) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, &signature.InvalidSignatureError{Msg: fmt.Sprintf("signing failed: %v", err)} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if len(certs) == 0 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, &signature.InvalidSignatureError{Msg: "no certificates returned from signer"} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Create a crypto.Signer adapter that returns our pre-computed signature | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| adapter := &signerAdapter{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sig: sig, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| certs: certs, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
dallasd1 marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Build PKCS#7 SignedData using mozilla library | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sd, err := gopkcs7.NewSignedData(req.Payload.Content) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, &signature.InvalidSignatureError{Msg: err.Error()} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Set digest algorithm to SHA-256 (kernel dm-verity requirement) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sd.SetDigestAlgorithm(gopkcs7.OIDDigestAlgorithmSHA256) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. notary spec has algo/hash pairings:
but this hardcodes SHA-256 for all. Tests only cover RSA-2048 so this bug is never exposed.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The hardcoded SHA-256 has been removed and validation API The tests now cover some rejected RSA and ECDSA types
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed by narrowing to RSA-2048 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Set encryption algorithm based on key type | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| encryptionOID, err := getEncryptionOID(keySpec) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, &signature.UnsupportedSigningKeyError{Msg: err.Error()} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sd.SetEncryptionAlgorithm(encryptionOID) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Sign without authenticated attributes (kernel dm-verity requirement) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err := sd.SignWithoutAttr(certs[0], adapter, gopkcs7.SignerInfoConfig{}); err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will add comment in a new commit bundled with the pending tspclient change |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, &signature.InvalidSignatureError{Msg: err.Error()} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Add intermediate/CA certificates to the chain | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for i := 1; i < len(certs); i++ { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sd.AddCertificate(certs[i]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Create detached signature (content not embedded) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sd.Detach() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| encoded, err := sd.Finish() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, &signature.InvalidSignatureError{Msg: err.Error()} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Parse the result to populate envelope fields | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| p7, _ := gopkcs7.Parse(encoded) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't swallow an error on parsing... if caller sees success, subsequent calls to Verify() or Content() will be incorrect.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Error will now be surfaced on failure |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| e.raw = encoded | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| e.p7 = p7 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| e.certs = certs | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if p7 != nil && len(p7.Signers) > 0 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| e.sigBytes = p7.Signers[0].EncryptedDigest | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
"Always check errors before processing any return values." We just produced the bytes ourselves; if Parse fails on our own output, something is very wrong and we should fail fast.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Errors are now checked |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return encoded, nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // getEncryptionOID returns the encryption algorithm OID for the key spec. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func getEncryptionOID(keySpec signature.KeySpec) (asn1.ObjectIdentifier, error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| switch keySpec.Type { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| case signature.KeyTypeRSA: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return gopkcs7.OIDEncryptionAlgorithmRSA, nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| case signature.KeyTypeEC: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| switch keySpec.Size { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| case 256: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return gopkcs7.OIDEncryptionAlgorithmECDSAP256, nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| case 384: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return gopkcs7.OIDEncryptionAlgorithmECDSAP384, nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| case 521: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return gopkcs7.OIDEncryptionAlgorithmECDSAP521, nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| default: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, fmt.Errorf("unsupported EC key size: %d", keySpec.Size) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| default: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, fmt.Errorf("unsupported key type: %v", keySpec.Type) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
dallasd1 marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Two issues with the current code: (1) the EC branches are misleading because the OIDs returned ( |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // signerAdapter wraps a pre-computed signature to satisfy crypto.Signer interface. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // This enables remote signers (Azure Key Vault, plugins) to work with the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // mozilla pkcs7 library which expects crypto.Signer. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type signerAdapter struct { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sig []byte // pre-computed signature from actual signer | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| certs []*x509.Certificate // certificate chain | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
The current implementation silently violates |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Public returns the public key from the leaf certificate. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func (a *signerAdapter) Public() crypto.PublicKey { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if len(a.certs) > 0 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return a.certs[0].PublicKey | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Sign returns the pre-computed signature. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // The digest parameter is ignored since signing already happened via req.Signer.Sign(). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func (a *signerAdapter) Sign(rand io.Reader, digest []byte, opts crypto.SignerOpts) ([]byte, error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return a.sig, nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
dallasd1 marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Verify implements signature.Envelope interface. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func (e *envelope) Verify() (*signature.EnvelopeContent, error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if e.raw == nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, &signature.SignatureEnvelopeNotFoundError{} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
dallasd1 marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // For detached signatures, the kernel does dm-verity verification | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return e.Content() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this method calls Content() which only parses structure... no cert chain validation or expiration check. cose and jws implementations each validate, but this doesn't. should call e.p7.Verify()
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The initial proposal highlights that verification for this envelope happens in the host kernel via a trusted keyring, so This envelope is an addition to the existing JWS/COSE digest signatures rather than a replacement, so users still run the existing JWS/COSE Alternatively, we could extend notation-go's verifier to not only pull the manifest signatures but also these new layer signatures. Then we could add a new API to verify this detached signature against the payload and validate the certificate chain. Since this would be non-trivial and the new dm-verity signing is gated behind the |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+247
to
+252
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
A |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Content implements signature.Envelope interface. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func (e *envelope) Content() (*signature.EnvelopeContent, error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if e.raw == nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, &signature.SignatureEnvelopeNotFoundError{} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| alg, err := extractAlgorithm(e.certs) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, &signature.InvalidSignatureError{Msg: err.Error()} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return &signature.EnvelopeContent{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| SignerInfo: signature.SignerInfo{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| SignatureAlgorithm: alg, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CertificateChain: e.certs, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Signature: e.sigBytes, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Payload: signature.Payload{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ContentType: MediaTypeEnvelope, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
dallasd1 marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // extractAlgorithm derives the signature algorithm from the leaf certificate. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func extractAlgorithm(certs []*x509.Certificate) (signature.Algorithm, error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if len(certs) == 0 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return 0, fmt.Errorf("no certificates available to determine algorithm") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| keySpec, err := signature.ExtractKeySpec(certs[0]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return 0, err | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return keySpec.SignatureAlgorithm(), nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will file the issue for the new enums linking the changes in each repo independently of this PR |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing to the in-tree CMS library @shizhMSFT. Agreed that dropping
go.mozilla.org/pkcs7is the right call here.I see two reasonable paths and would like guidance before pushing:
(a) Copy
tspclient-go/internal/cmsintonotation-core-go/internal/cms. This would be fast to land but introduces duplication and the two copies could drift.tspclient-go/internal/cmshas been stable for ~a year so that risk seems to be low.(b) Lift
tspclient-go/internal/cmsto a publictspclient-go/cmspackage and have notation-core-go import it. This seems to be cleaner long-term but requires a new PR and a new tspclient-go release.I'm happy to push (a) with a tracking issue, or switch to (b) if you'd prefer. Please let me know which direction you'd like to take.