Notation library baseline #5
Conversation
|
LGTM as this is based on prototype 2 and JWS refactoring will bring in more changes. |
|
Hello! 👋 This PR seems to implement a fair bit of the registry protocol in Go, in addition to implementing the new not-yet-standard bits necessary to store signatures. I happen to have some experience writing and maintaining a fairly widely used registry protocol client in Go, which you might be interested in reusing: go-containerregistry; specifically the low-level HTTP transport in This package ends up being used by a number of tools, including buildpacks, kaniko, ko, Tekton, Knative, and probably a dozen others. This means it's pretty well battle-tested in real world scenarios, and with more users it should have any bugs or vulnerabilities discovered and fixed more quickly than new single-use clients. This comment shouldn't block this PR, but I think it'd be worth discussing reusing an existing client instead of writing and maintaining your own, and (call me biased) I think go-containerregistry would be a good one. 😄 |
|
@imjasonh that's great feedback and we should consider reusing an existing registry client. I believe a lot of changes are done into oras-go to support the new signature manifest format with local cache store similar to how helm is working and also has dependency on and quite heavily used. Another aspect is move of the entire graph of signatures. |
|
Tracking refactoring in #8 |
|
Tracking signature format update in #9 |
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
| case len(params.X5c) > 0: | ||
| return v.getVerificationKeyPairFromX5c(params.X5c) | ||
| case params.KeyID != "": | ||
| return v.getVerificationKeyPairFromKeyID(params.KeyID) |
There was a problem hiding this comment.
Instead of giving x5c priority over keyId, in my opinion it would be better to fail the verification if both signature and keyId are present in signature.
| func (v *verifier) getVerificationKeyPairFromKeyID(keyID string) (libtrust.PublicKey, *x509.Certificate, error) { | ||
| key, found := v.keys[keyID] | ||
| if !found { | ||
| return nil, nil, errors.New("key not found: " + keyID) | ||
| } | ||
| cert, found := v.certs[keyID] | ||
| if !found { | ||
| return nil, nil, errors.New("cert not found: " + keyID) | ||
| } | ||
| return key, cert, nil | ||
| } |
There was a problem hiding this comment.
For future updates: the keyid mechanism doesn't need to return X509Cert, keyId can even refer to a raw private key.
| if err != nil { | ||
| return err | ||
| } | ||
| if err := key.Verify(strings.NewReader(signed), params.Algorithm, sig); err != nil { |
There was a problem hiding this comment.
We should add validation for list of supported algorithms(to avoid algorithm substitution and signature bypass attacks).
This notation library is based on the
prototype-3of the notary v2 library, which is based onprototype-2.The underlying artifact specification and registry referrer API are alighed to oras-project/artifacts-spec.