-
Notifications
You must be signed in to change notification settings - Fork 380
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
Add keyPaths
, keyDatas
to prSigstoreSigned
#2524
Conversation
keyPaths
, keyDatas
, and same for Rekor, in prSigstoreSigned
keyPaths
, keyDatas
to prSigstoreSigned
A highlight for reviewers: |
… The data is in PEM format, one key per blob. While we do need |
Support for multiple Rekor public keys has been split to #2526. |
Now tested by manually writing WRT the option of accepting a single |
return nil, nil | ||
data = d | ||
} | ||
if src.data != nil { |
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.
This is a change in behaviour, Previous behavior ignored src.data when src.path is set? Is this intentional?
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.
AFAICS previously this would have matched
case data != nil && path != "":
return nil, fmt.Errorf(`Internal inconsistency: both "%sPath" and "%sData" specified`, prefix, prefix)
and it would have been rejected. Now we also reject that, but at the very end, based on sources > 1
.
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.
ok
LGTM |
@mtrmac look like you need to update/rebase this one. |
signature/policy_eval_signedby.go
Outdated
} | ||
if keySources != 1 { | ||
if data == nil { | ||
return sarRejected, nil, errors.New(`Internal inconsistency: not exactly one of "keyPath", "keyPaths" and "keyData" specified`) |
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.
Should "keyDatas" be added here too?
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.
No; for simple signing, we don’t have a keyDatas
option (e.g. a few lines above, the datas:
field is not set).
[The keyDatas
option is not necessary for simple signing, because the input format is a GPG keyring, which already supports multiple keys. (That’s structurally ~similar to the possibility to support multiple PEM-formatted keys in a single blob, discussed above.)]
One minor nit, otherwise LGTM. @mheon? |
LGTM on my end |
Rebased. |
signature/policy_eval_signedby.go
Outdated
keySources++ | ||
data = [][]byte{pr.KeyData} | ||
data, err := loadBytesFromConfigSources(configBytesSources{ | ||
inconsistencyErrorMessage: `Internal inconsistency: not exactly one of "keyPath", "keyPaths" and "keyData" specified`, |
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.
nit: since you use this exact error message twice, maybe place it into a const
?
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.
Why not, fixed.
signature/policy_eval_sigstore.go
Outdated
if publicKeyPEM != nil { | ||
pk, err := cryptoutils.UnmarshalPEMToPublicKey(publicKeyPEM) | ||
if publicKeyPEMs != nil { | ||
if len(publicKeyPEMs) != 1 { // Coverage: We only provide single-element sources to loadBytesFromConfigSources, and at most one is allowed. |
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.
nit: I guess the comment is telling something like "Should never happen", but my tiny brains fail to read it this way.
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.
“Coverage:” is, in this subpackage, a shorthand for “this line is not covered by unit tests because …”.
Added an explicit “this should never happen” to the start of the explanation, hopefully that helps.
@@ -165,24 +165,37 @@ type SigstorePayloadAcceptanceRules struct { | |||
ValidateSignedDockerManifestDigest func(digest.Digest) error | |||
} | |||
|
|||
// verifySigstorePayloadBlobSignature verifies verifies unverifiedSignature of unverifiedPayload was correctly created |
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.
s/ verifies verifies / verifies /
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.
Fixed, thanks!
// | ||
// This is an internal implementation detail of VerifySigstorePayload and should have no other callers. | ||
// It is INSUFFICIENT alone to consider the signature acceptable. | ||
func verifySigstorePayloadBlobSignature(publicKey crypto.PublicKey, unverifiedPayload []byte, unverifiedSignature []byte) error { |
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.
nit:
func verifySigstorePayloadBlobSignature(publicKey crypto.PublicKey, unverifiedPayload []byte, unverifiedSignature []byte) error { | |
func verifySigstorePayloadBlobSignature(publicKey crypto.PublicKey, unverifiedPayload, unverifiedSignature []byte) error { | |
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.
Fixed.
return err | ||
func verifySigstorePayloadBlobSignature(publicKeys []crypto.PublicKey, unverifiedPayload []byte, unverifiedSignature []byte) error { | ||
if len(publicKeys) == 0 { | ||
return fmt.Errorf("Need at least one public key to verify the sigstore payload, but got 0") |
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.
nit: s/fmt.Errorf/errors.New/
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.
Fixed.
signature/policy_types.go
Outdated
@@ -111,11 +111,14 @@ type prSignedBaseLayer struct { | |||
type prSigstoreSigned struct { | |||
prCommon | |||
|
|||
// KeyPath is a pathname to a local file containing the trusted key. Exactly one of KeyPath, KeyData, Fulcio must be specified. | |||
// KeyPath is a pathname to a local file containing the trusted key. Exactly one of KeyPath, KeyPaths, KeyData, KeyDatas or Fulcio must be specified. |
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.
In the code above it says "and fulcio" and here it says "or".
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.
I’m not sure there is a difference in meaning, but being consistent is nice. Fixed.
signature/policy_types.go
Outdated
KeyData []byte `json:"keyData,omitempty"` | ||
// FIXME: Multiple public keys? | ||
// KeyDatas is a set of trusted keys, base64-encoded. Exactly one of KeyPath, KeyPaths, KeyData, KeyDatas or Fulcio must be specified. | ||
KeyDatas [][]byte `json:"keyDatas,omitempty"` | ||
|
||
// Fulcio specifies which Fulcio-generated certificates are accepted. Exactly one of KeyPath, KeyData, Fulcio must be specified. |
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.
This comment needs to be amended, too.
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.
Fixed, thanks!
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Use a struct as an input, so that the parameters are named and we minimize risk of inconsistencies, and make it easier to add more sources. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Extend loadBytesFromConfigSources to return multiple values, and to support reading the from files; then share the code. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
because we will want to support multiple public keys, and that's easier to do in a separate function. Should not change behavior except for order of error checks. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
The new fields `KeyPaths` and `KeyDatas` is taken directly from `/etc/containers/policy.json` and allows users to provide multiple signature keys to be used to verify images. Only one of the keys has to verify, thereby this mechanism allows us to have support seamless key rotation on a registry. This fixes containers#2319 Signed-off-by: Dan Čermák <dcermak@suse.com> Co-authored-by: Danish Prakash <danish.prakash@suse.com> Signed-off-by: Miloslav Trmač <mitr@redhat.com>
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.
LGTM
@TomSweeneyRedHat I don’t know of any outstanding issues. Care to hit the merge button, then? |
Merged! On to #2526 then. |
/lgtm |
This is #2456 + various cleanups on top.
Huge thanks to @dcermak and @danishprakash for doing almost all the work in #2456.
This is a few preparatory commits + landing the whole feature at once; see https://github.com/mtrmac/image/tree/sigstore-multi-individual-commits for a (tiny bit older) version with individual commits, and showing the relationship to original #2456.
Currently not tested end-to-end in practice.