From e1f9e44b6dbbb7cb6ce87697dbe19e65d00c914a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 6 Jul 2022 02:37:56 +0200 Subject: [PATCH 1/4] Rename signature/signature.go to signature/simple.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To make its relationship with "simple signing" clearer. Signed-off-by: Miloslav Trmač --- signature/{signature.go => simple.go} | 0 signature/{signature_test.go => simple_test.go} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename signature/{signature.go => simple.go} (100%) rename signature/{signature_test.go => simple_test.go} (100%) diff --git a/signature/signature.go b/signature/simple.go similarity index 100% rename from signature/signature.go rename to signature/simple.go diff --git a/signature/signature_test.go b/signature/simple_test.go similarity index 100% rename from signature/signature_test.go rename to signature/simple_test.go From e98b49289b1645fcc2acae0248d78b61ce3075dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 6 Jul 2022 03:33:34 +0200 Subject: [PATCH 2/4] Move InvalidSignatureError to signature/internal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... so that future code in singature/internal can use it. This also requires requires a package-public (but internal) constructor. Signed-off-by: Miloslav Trmač --- signature/docker.go | 11 ++++++----- signature/internal/errors.go | 15 +++++++++++++++ signature/internal/errors_test.go | 14 ++++++++++++++ signature/mechanism_gpgme.go | 5 +++-- signature/mechanism_openpgp.go | 7 ++++--- signature/simple.go | 19 +++++++------------ signature/simple_test.go | 7 ------- 7 files changed, 49 insertions(+), 29 deletions(-) create mode 100644 signature/internal/errors.go create mode 100644 signature/internal/errors_test.go diff --git a/signature/docker.go b/signature/docker.go index 8e9ce0dd2..b09502dfe 100644 --- a/signature/docker.go +++ b/signature/docker.go @@ -9,6 +9,7 @@ import ( "github.com/containers/image/v5/docker/reference" "github.com/containers/image/v5/manifest" + "github.com/containers/image/v5/signature/internal" "github.com/opencontainers/go-digest" ) @@ -56,18 +57,18 @@ func VerifyDockerManifestSignature(unverifiedSignature, unverifiedManifest []byt sig, err := verifyAndExtractSignature(mech, unverifiedSignature, signatureAcceptanceRules{ validateKeyIdentity: func(keyIdentity string) error { if keyIdentity != expectedKeyIdentity { - return InvalidSignatureError{msg: fmt.Sprintf("Signature by %s does not match expected fingerprint %s", keyIdentity, expectedKeyIdentity)} + return internal.NewInvalidSignatureError(fmt.Sprintf("Signature by %s does not match expected fingerprint %s", keyIdentity, expectedKeyIdentity)) } return nil }, validateSignedDockerReference: func(signedDockerReference string) error { signedRef, err := reference.ParseNormalizedNamed(signedDockerReference) if err != nil { - return InvalidSignatureError{msg: fmt.Sprintf("Invalid docker reference %s in signature", signedDockerReference)} + return internal.NewInvalidSignatureError(fmt.Sprintf("Invalid docker reference %s in signature", signedDockerReference)) } if signedRef.String() != expectedRef.String() { - return InvalidSignatureError{msg: fmt.Sprintf("Docker reference %s does not match %s", - signedDockerReference, expectedDockerReference)} + return internal.NewInvalidSignatureError(fmt.Sprintf("Docker reference %s does not match %s", + signedDockerReference, expectedDockerReference)) } return nil }, @@ -77,7 +78,7 @@ func VerifyDockerManifestSignature(unverifiedSignature, unverifiedManifest []byt return err } if !matches { - return InvalidSignatureError{msg: fmt.Sprintf("Signature for docker digest %q does not match", signedDockerManifestDigest)} + return internal.NewInvalidSignatureError(fmt.Sprintf("Signature for docker digest %q does not match", signedDockerManifestDigest)) } return nil }, diff --git a/signature/internal/errors.go b/signature/internal/errors.go new file mode 100644 index 000000000..7872f0f43 --- /dev/null +++ b/signature/internal/errors.go @@ -0,0 +1,15 @@ +package internal + +// InvalidSignatureError is returned when parsing an invalid signature. +// This is publicly visible as signature.InvalidSignatureError +type InvalidSignatureError struct { + msg string +} + +func (err InvalidSignatureError) Error() string { + return err.msg +} + +func NewInvalidSignatureError(msg string) InvalidSignatureError { + return InvalidSignatureError{msg: msg} +} diff --git a/signature/internal/errors_test.go b/signature/internal/errors_test.go new file mode 100644 index 000000000..ee243d30b --- /dev/null +++ b/signature/internal/errors_test.go @@ -0,0 +1,14 @@ +package internal + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestInvalidSignatureError(t *testing.T) { + // A stupid test just to keep code coverage + s := "test" + err := NewInvalidSignatureError(s) + assert.Equal(t, s, err.Error()) +} diff --git a/signature/mechanism_gpgme.go b/signature/mechanism_gpgme.go index 4c7968417..54060af34 100644 --- a/signature/mechanism_gpgme.go +++ b/signature/mechanism_gpgme.go @@ -9,6 +9,7 @@ import ( "fmt" "os" + "github.com/containers/image/v5/signature/internal" "github.com/proglottis/gpgme" ) @@ -181,13 +182,13 @@ func (m *gpgmeSigningMechanism) Verify(unverifiedSignature []byte) (contents []b return nil, "", err } if len(sigs) != 1 { - return nil, "", InvalidSignatureError{msg: fmt.Sprintf("Unexpected GPG signature count %d", len(sigs))} + return nil, "", internal.NewInvalidSignatureError(fmt.Sprintf("Unexpected GPG signature count %d", len(sigs))) } sig := sigs[0] // This is sig.Summary == gpgme.SigSumValid except for key trust, which we handle ourselves if sig.Status != nil || sig.Validity == gpgme.ValidityNever || sig.ValidityReason != nil || sig.WrongKeyUsage { // FIXME: Better error reporting eventually - return nil, "", InvalidSignatureError{msg: fmt.Sprintf("Invalid GPG signature: %#v", sig)} + return nil, "", internal.NewInvalidSignatureError(fmt.Sprintf("Invalid GPG signature: %#v", sig)) } return signedBuffer.Bytes(), sig.Fingerprint, nil } diff --git a/signature/mechanism_openpgp.go b/signature/mechanism_openpgp.go index 63cb7788b..8798ee2cd 100644 --- a/signature/mechanism_openpgp.go +++ b/signature/mechanism_openpgp.go @@ -13,6 +13,7 @@ import ( "strings" "time" + "github.com/containers/image/v5/signature/internal" "github.com/containers/storage/pkg/homedir" // This is a fallback code; the primary recommendation is to use the gpgme mechanism // implementation, which is out-of-process and more appropriate for handling long-term private key material @@ -144,19 +145,19 @@ func (m *openpgpSigningMechanism) Verify(unverifiedSignature []byte) (contents [ return nil, "", fmt.Errorf("signature error: %v", md.SignatureError) } if md.SignedBy == nil { - return nil, "", InvalidSignatureError{msg: fmt.Sprintf("Invalid GPG signature: %#v", md.Signature)} + return nil, "", internal.NewInvalidSignatureError(fmt.Sprintf("Invalid GPG signature: %#v", md.Signature)) } if md.Signature != nil { if md.Signature.SigLifetimeSecs != nil { expiry := md.Signature.CreationTime.Add(time.Duration(*md.Signature.SigLifetimeSecs) * time.Second) if time.Now().After(expiry) { - return nil, "", InvalidSignatureError{msg: fmt.Sprintf("Signature expired on %s", expiry)} + return nil, "", internal.NewInvalidSignatureError(fmt.Sprintf("Signature expired on %s", expiry)) } } } else if md.SignatureV3 == nil { // Coverage: If md.SignedBy != nil, the final md.UnverifiedBody.Read() either sets one of md.Signature or md.SignatureV3, // or sets md.SignatureError. - return nil, "", InvalidSignatureError{msg: "Unexpected openpgp.MessageDetails: neither Signature nor SignatureV3 is set"} + return nil, "", internal.NewInvalidSignatureError("Unexpected openpgp.MessageDetails: neither Signature nor SignatureV3 is set") } // Uppercase the fingerprint to be compatible with gpgme diff --git a/signature/simple.go b/signature/simple.go index 291c1c0ee..da54c9f45 100644 --- a/signature/simple.go +++ b/signature/simple.go @@ -10,6 +10,7 @@ import ( "fmt" "time" + "github.com/containers/image/v5/signature/internal" "github.com/containers/image/v5/version" digest "github.com/opencontainers/go-digest" ) @@ -19,13 +20,7 @@ const ( ) // InvalidSignatureError is returned when parsing an invalid signature. -type InvalidSignatureError struct { - msg string -} - -func (err InvalidSignatureError) Error() string { - return err.msg -} +type InvalidSignatureError = internal.InvalidSignatureError // Signature is a parsed content of a signature. // The only way to get this structure from a blob should be as a return value from a successful call to verifyAndExtractSignature below. @@ -112,7 +107,7 @@ func (s *untrustedSignature) UnmarshalJSON(data []byte) error { err := s.strictUnmarshalJSON(data) if err != nil { if formatErr, ok := err.(jsonFormatError); ok { - err = InvalidSignatureError{msg: formatErr.Error()} + err = internal.NewInvalidSignatureError(formatErr.Error()) } } return err @@ -153,7 +148,7 @@ func (s *untrustedSignature) strictUnmarshalJSON(data []byte) error { if gotTimestamp { intTimestamp := int64(timestamp) if float64(intTimestamp) != timestamp { - return InvalidSignatureError{msg: "Field optional.timestamp is not is not an integer"} + return internal.NewInvalidSignatureError("Field optional.timestamp is not is not an integer") } s.UntrustedTimestamp = &intTimestamp } @@ -168,7 +163,7 @@ func (s *untrustedSignature) strictUnmarshalJSON(data []byte) error { return err } if t != signatureType { - return InvalidSignatureError{msg: fmt.Sprintf("Unrecognized signature type %s", t)} + return internal.NewInvalidSignatureError(fmt.Sprintf("Unrecognized signature type %s", t)) } var digestString string @@ -231,7 +226,7 @@ func verifyAndExtractSignature(mech SigningMechanism, unverifiedSignature []byte var unmatchedSignature untrustedSignature if err := json.Unmarshal(signed, &unmatchedSignature); err != nil { - return nil, InvalidSignatureError{msg: err.Error()} + return nil, internal.NewInvalidSignatureError(err.Error()) } if err := rules.validateSignedDockerManifestDigest(unmatchedSignature.UntrustedDockerManifestDigest); err != nil { return nil, err @@ -269,7 +264,7 @@ func GetUntrustedSignatureInformationWithoutVerifying(untrustedSignatureBytes [] } var untrustedDecodedContents untrustedSignature if err := json.Unmarshal(untrustedContents, &untrustedDecodedContents); err != nil { - return nil, InvalidSignatureError{msg: err.Error()} + return nil, internal.NewInvalidSignatureError(err.Error()) } var timestamp *time.Time // = nil diff --git a/signature/simple_test.go b/signature/simple_test.go index 56045171c..a97fd6a63 100644 --- a/signature/simple_test.go +++ b/signature/simple_test.go @@ -15,13 +15,6 @@ import ( "github.com/xeipuuv/gojsonschema" ) -func TestInvalidSignatureError(t *testing.T) { - // A stupid test just to keep code coverage - s := "test" - err := InvalidSignatureError{msg: s} - assert.Equal(t, s, err.Error()) -} - func TestNewUntrustedSignature(t *testing.T) { timeBefore := time.Now() sig := newUntrustedSignature(TestImageManifestDigest, TestImageSignatureReference) From 103fb7148256844d45fb6fbf2be9741d48b1b12f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 6 Jul 2022 03:43:45 +0200 Subject: [PATCH 3/4] Move signature/json.go to signature/internal/json.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We are going to need it in signature/internal . Signed-off-by: Miloslav Trmač --- signature/{ => internal}/json.go | 36 +++++++++++++-------------- signature/{ => internal}/json_test.go | 30 ++++++---------------- signature/policy_config.go | 31 ++++++++++++----------- signature/policy_config_test.go | 14 +++++++++++ signature/simple.go | 16 ++++++------ 5 files changed, 64 insertions(+), 63 deletions(-) rename signature/{ => internal}/json.go (64%) rename signature/{ => internal}/json_test.go (77%) diff --git a/signature/json.go b/signature/internal/json.go similarity index 64% rename from signature/json.go rename to signature/internal/json.go index 9e592863d..0f39fe0ad 100644 --- a/signature/json.go +++ b/signature/internal/json.go @@ -1,4 +1,4 @@ -package signature +package internal import ( "bytes" @@ -7,34 +7,34 @@ import ( "io" ) -// jsonFormatError is returned when JSON does not match expected format. -type jsonFormatError string +// JSONFormatError is returned when JSON does not match expected format. +type JSONFormatError string -func (err jsonFormatError) Error() string { +func (err JSONFormatError) Error() string { return string(err) } -// paranoidUnmarshalJSONObject unmarshals data as a JSON object, but failing on the slightest unexpected aspect +// ParanoidUnmarshalJSONObject unmarshals data as a JSON object, but failing on the slightest unexpected aspect // (including duplicated keys, unrecognized keys, and non-matching types). Uses fieldResolver to // determine the destination for a field value, which should return a pointer to the destination if valid, or nil if the key is rejected. // // The fieldResolver approach is useful for decoding the Policy.Transports map; using it for structs is a bit lazy, // we could use reflection to automate this. Later? -func paranoidUnmarshalJSONObject(data []byte, fieldResolver func(string) interface{}) error { +func ParanoidUnmarshalJSONObject(data []byte, fieldResolver func(string) interface{}) error { seenKeys := map[string]struct{}{} dec := json.NewDecoder(bytes.NewReader(data)) t, err := dec.Token() if err != nil { - return jsonFormatError(err.Error()) + return JSONFormatError(err.Error()) } if t != json.Delim('{') { - return jsonFormatError(fmt.Sprintf("JSON object expected, got \"%s\"", t)) + return JSONFormatError(fmt.Sprintf("JSON object expected, got \"%s\"", t)) } for { t, err := dec.Token() if err != nil { - return jsonFormatError(err.Error()) + return JSONFormatError(err.Error()) } if t == json.Delim('}') { break @@ -43,34 +43,34 @@ func paranoidUnmarshalJSONObject(data []byte, fieldResolver func(string) interfa key, ok := t.(string) if !ok { // Coverage: This should never happen, dec.Token() rejects non-string-literals in this state. - return jsonFormatError(fmt.Sprintf("Key string literal expected, got \"%s\"", t)) + return JSONFormatError(fmt.Sprintf("Key string literal expected, got \"%s\"", t)) } if _, ok := seenKeys[key]; ok { - return jsonFormatError(fmt.Sprintf("Duplicate key \"%s\"", key)) + return JSONFormatError(fmt.Sprintf("Duplicate key \"%s\"", key)) } seenKeys[key] = struct{}{} valuePtr := fieldResolver(key) if valuePtr == nil { - return jsonFormatError(fmt.Sprintf("Unknown key \"%s\"", key)) + return JSONFormatError(fmt.Sprintf("Unknown key \"%s\"", key)) } // This works like json.Unmarshal, in particular it allows us to implement UnmarshalJSON to implement strict parsing of the field value. if err := dec.Decode(valuePtr); err != nil { - return jsonFormatError(err.Error()) + return JSONFormatError(err.Error()) } } if _, err := dec.Token(); err != io.EOF { - return jsonFormatError("Unexpected data after JSON object") + return JSONFormatError("Unexpected data after JSON object") } return nil } -// paranoidUnmarshalJSONObject unmarshals data as a JSON object, but failing on the slightest unexpected aspect +// ParanoidUnmarshalJSONObjectExactFields unmarshals data as a JSON object, but failing on the slightest unexpected aspect // (including duplicated keys, unrecognized keys, and non-matching types). Each of the fields in exactFields // must be present exactly once, and none other fields are accepted. -func paranoidUnmarshalJSONObjectExactFields(data []byte, exactFields map[string]interface{}) error { +func ParanoidUnmarshalJSONObjectExactFields(data []byte, exactFields map[string]interface{}) error { seenKeys := map[string]struct{}{} - if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} { + if err := ParanoidUnmarshalJSONObject(data, func(key string) interface{} { if valuePtr, ok := exactFields[key]; ok { seenKeys[key] = struct{}{} return valuePtr @@ -81,7 +81,7 @@ func paranoidUnmarshalJSONObjectExactFields(data []byte, exactFields map[string] } for key := range exactFields { if _, ok := seenKeys[key]; !ok { - return jsonFormatError(fmt.Sprintf(`Key "%s" missing in a JSON object`, key)) + return JSONFormatError(fmt.Sprintf(`Key "%s" missing in a JSON object`, key)) } } return nil diff --git a/signature/json_test.go b/signature/internal/json_test.go similarity index 77% rename from signature/json_test.go rename to signature/internal/json_test.go index f62c1994a..bb408333f 100644 --- a/signature/json_test.go +++ b/signature/internal/json_test.go @@ -1,4 +1,4 @@ -package signature +package internal import ( "encoding/json" @@ -8,20 +8,6 @@ import ( "github.com/stretchr/testify/require" ) -type mSI map[string]interface{} // To minimize typing the long name - -// A short-hand way to get a JSON object field value or panic. No error handling done, we know -// what we are working with, a panic in a test is good enough, and fitting test cases on a single line -// is a priority. -func x(m mSI, fields ...string) mSI { - for _, field := range fields { - // Not .(mSI) because type assertion of an unnamed type to a named type always fails (the types - // are not "identical"), but the assignment is fine because they are "assignable". - m = m[field].(map[string]interface{}) - } - return m -} - // implementsUnmarshalJSON is a minimalistic type used to detect that // paranoidUnmarshalJSONObject uses the json.Unmarshaler interface of resolved // pointers. @@ -58,20 +44,20 @@ func TestParanoidUnmarshalJSONObject(t *testing.T) { // Empty object ts = testStruct{} - err := paranoidUnmarshalJSONObject([]byte(`{}`), tsResolver) + err := ParanoidUnmarshalJSONObject([]byte(`{}`), tsResolver) require.NoError(t, err) assert.Equal(t, testStruct{}, ts) // Success ts = testStruct{} - err = paranoidUnmarshalJSONObject([]byte(`{"a":"x", "b":2}`), tsResolver) + err = ParanoidUnmarshalJSONObject([]byte(`{"a":"x", "b":2}`), tsResolver) require.NoError(t, err) assert.Equal(t, testStruct{A: "x", B: 2}, ts) // json.Unmarshaler is used for decoding values ts = testStruct{} unmarshalJSONCalled = implementsUnmarshalJSON(false) - err = paranoidUnmarshalJSONObject([]byte(`{"implementsUnmarshalJSON":true}`), tsResolver) + err = ParanoidUnmarshalJSONObject([]byte(`{"implementsUnmarshalJSON":true}`), tsResolver) require.NoError(t, err) assert.Equal(t, unmarshalJSONCalled, implementsUnmarshalJSON(true)) @@ -89,7 +75,7 @@ func TestParanoidUnmarshalJSONObject(t *testing.T) { `{"a":"value"}{}`, // Extra data after object } { ts = testStruct{} - err := paranoidUnmarshalJSONObject([]byte(input), tsResolver) + err := ParanoidUnmarshalJSONObject([]byte(input), tsResolver) assert.Error(t, err, input) } } @@ -107,11 +93,11 @@ func TestParanoidUnmarshalJSONObjectExactFields(t *testing.T) { } // Empty object - err := paranoidUnmarshalJSONObjectExactFields([]byte(`{}`), map[string]interface{}{}) + err := ParanoidUnmarshalJSONObjectExactFields([]byte(`{}`), map[string]interface{}{}) require.NoError(t, err) // Success - err = paranoidUnmarshalJSONObjectExactFields([]byte(`{"string": "a", "float64": 3.5, "raw": {"a":"b"}, "unmarshaller": true}`), exactFields) + err = ParanoidUnmarshalJSONObjectExactFields([]byte(`{"string": "a", "float64": 3.5, "raw": {"a":"b"}, "unmarshaller": true}`), exactFields) require.NoError(t, err) assert.Equal(t, "a", stringValue) assert.Equal(t, 3.5, float64Value) @@ -131,7 +117,7 @@ func TestParanoidUnmarshalJSONObjectExactFields(t *testing.T) { `{"string": 1, "float64": 3.5, "raw": {"a":"b"}, "unmarshaller": true}`, // Type mismatch `{"string": "a", "float64": 3.5, "raw": {"a":"b"}, "unmarshaller": true}{}`, // Extra data after object } { - err := paranoidUnmarshalJSONObjectExactFields([]byte(input), exactFields) + err := ParanoidUnmarshalJSONObjectExactFields([]byte(input), exactFields) assert.Error(t, err, input) } } diff --git a/signature/policy_config.go b/signature/policy_config.go index 00b94128d..51d4da275 100644 --- a/signature/policy_config.go +++ b/signature/policy_config.go @@ -22,6 +22,7 @@ import ( "regexp" "github.com/containers/image/v5/docker/reference" + "github.com/containers/image/v5/signature/internal" "github.com/containers/image/v5/transports" "github.com/containers/image/v5/types" "github.com/containers/storage/pkg/homedir" @@ -104,7 +105,7 @@ var _ json.Unmarshaler = (*Policy)(nil) func (p *Policy) UnmarshalJSON(data []byte) error { *p = Policy{} transports := policyTransportsMap{} - if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} { + if err := internal.ParanoidUnmarshalJSONObject(data, func(key string) interface{} { switch key { case "default": return &p.Default @@ -135,10 +136,10 @@ func (m *policyTransportsMap) UnmarshalJSON(data []byte) error { // We can't unmarshal directly into map values because it is not possible to take an address of a map value. // So, use a temporary map of pointers-to-slices and convert. tmpMap := map[string]*PolicyTransportScopes{} - if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} { + if err := internal.ParanoidUnmarshalJSONObject(data, func(key string) interface{} { // transport can be nil transport := transports.Get(key) - // paranoidUnmarshalJSONObject detects key duplication for us, check just to be safe. + // internal.ParanoidUnmarshalJSONObject detects key duplication for us, check just to be safe. if _, ok := tmpMap[key]; ok { return nil } @@ -181,8 +182,8 @@ func (m *policyTransportScopesWithTransport) UnmarshalJSON(data []byte) error { // We can't unmarshal directly into map values because it is not possible to take an address of a map value. // So, use a temporary map of pointers-to-slices and convert. tmpMap := map[string]*PolicyRequirements{} - if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} { - // paranoidUnmarshalJSONObject detects key duplication for us, check just to be safe. + if err := internal.ParanoidUnmarshalJSONObject(data, func(key string) interface{} { + // internal.ParanoidUnmarshalJSONObject detects key duplication for us, check just to be safe. if _, ok := tmpMap[key]; ok { return nil } @@ -269,7 +270,7 @@ var _ json.Unmarshaler = (*prInsecureAcceptAnything)(nil) func (pr *prInsecureAcceptAnything) UnmarshalJSON(data []byte) error { *pr = prInsecureAcceptAnything{} var tmp prInsecureAcceptAnything - if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ + if err := internal.ParanoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ "type": &tmp.Type, }); err != nil { return err @@ -299,7 +300,7 @@ var _ json.Unmarshaler = (*prReject)(nil) func (pr *prReject) UnmarshalJSON(data []byte) error { *pr = prReject{} var tmp prReject - if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ + if err := internal.ParanoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ "type": &tmp.Type, }); err != nil { return err @@ -361,7 +362,7 @@ func (pr *prSignedBy) UnmarshalJSON(data []byte) error { var tmp prSignedBy var gotKeyPath, gotKeyData = false, false var signedIdentity json.RawMessage - if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} { + if err := internal.ParanoidUnmarshalJSONObject(data, func(key string) interface{} { switch key { case "type": return &tmp.Type @@ -469,7 +470,7 @@ func (pr *prSignedBaseLayer) UnmarshalJSON(data []byte) error { *pr = prSignedBaseLayer{} var tmp prSignedBaseLayer var baseLayerIdentity json.RawMessage - if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ + if err := internal.ParanoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ "type": &tmp.Type, "baseLayerIdentity": &baseLayerIdentity, }); err != nil { @@ -538,7 +539,7 @@ var _ json.Unmarshaler = (*prmMatchExact)(nil) func (prm *prmMatchExact) UnmarshalJSON(data []byte) error { *prm = prmMatchExact{} var tmp prmMatchExact - if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ + if err := internal.ParanoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ "type": &tmp.Type, }); err != nil { return err @@ -568,7 +569,7 @@ var _ json.Unmarshaler = (*prmMatchRepoDigestOrExact)(nil) func (prm *prmMatchRepoDigestOrExact) UnmarshalJSON(data []byte) error { *prm = prmMatchRepoDigestOrExact{} var tmp prmMatchRepoDigestOrExact - if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ + if err := internal.ParanoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ "type": &tmp.Type, }); err != nil { return err @@ -598,7 +599,7 @@ var _ json.Unmarshaler = (*prmMatchRepository)(nil) func (prm *prmMatchRepository) UnmarshalJSON(data []byte) error { *prm = prmMatchRepository{} var tmp prmMatchRepository - if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ + if err := internal.ParanoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ "type": &tmp.Type, }); err != nil { return err @@ -638,7 +639,7 @@ var _ json.Unmarshaler = (*prmExactReference)(nil) func (prm *prmExactReference) UnmarshalJSON(data []byte) error { *prm = prmExactReference{} var tmp prmExactReference - if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ + if err := internal.ParanoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ "type": &tmp.Type, "dockerReference": &tmp.DockerReference, }); err != nil { @@ -680,7 +681,7 @@ var _ json.Unmarshaler = (*prmExactRepository)(nil) func (prm *prmExactRepository) UnmarshalJSON(data []byte) error { *prm = prmExactRepository{} var tmp prmExactRepository - if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ + if err := internal.ParanoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ "type": &tmp.Type, "dockerRepository": &tmp.DockerRepository, }); err != nil { @@ -752,7 +753,7 @@ var _ json.Unmarshaler = (*prmRemapIdentity)(nil) func (prm *prmRemapIdentity) UnmarshalJSON(data []byte) error { *prm = prmRemapIdentity{} var tmp prmRemapIdentity - if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ + if err := internal.ParanoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ "type": &tmp.Type, "prefix": &tmp.Prefix, "signedPrefix": &tmp.SignedPrefix, diff --git a/signature/policy_config_test.go b/signature/policy_config_test.go index 449e8093c..7f82d4962 100644 --- a/signature/policy_config_test.go +++ b/signature/policy_config_test.go @@ -18,6 +18,20 @@ import ( "github.com/stretchr/testify/require" ) +type mSI map[string]interface{} // To minimize typing the long name + +// A short-hand way to get a JSON object field value or panic. No error handling done, we know +// what we are working with, a panic in a test is good enough, and fitting test cases on a single line +// is a priority. +func x(m mSI, fields ...string) mSI { + for _, field := range fields { + // Not .(mSI) because type assertion of an unnamed type to a named type always fails (the types + // are not "identical"), but the assignment is fine because they are "assignable". + m = m[field].(map[string]interface{}) + } + return m +} + // policyFixtureContents is a data structure equal to the contents of "fixtures/policy.json" var policyFixtureContents = &Policy{ Default: PolicyRequirements{NewPRReject()}, diff --git a/signature/simple.go b/signature/simple.go index da54c9f45..1ca571e5a 100644 --- a/signature/simple.go +++ b/signature/simple.go @@ -106,18 +106,18 @@ var _ json.Unmarshaler = (*untrustedSignature)(nil) func (s *untrustedSignature) UnmarshalJSON(data []byte) error { err := s.strictUnmarshalJSON(data) if err != nil { - if formatErr, ok := err.(jsonFormatError); ok { + if formatErr, ok := err.(internal.JSONFormatError); ok { err = internal.NewInvalidSignatureError(formatErr.Error()) } } return err } -// strictUnmarshalJSON is UnmarshalJSON, except that it may return the internal jsonFormatError error type. -// Splitting it into a separate function allows us to do the jsonFormatError → InvalidSignatureError in a single place, the caller. +// strictUnmarshalJSON is UnmarshalJSON, except that it may return the internal.JSONFormatError error type. +// Splitting it into a separate function allows us to do the internal.JSONFormatError → InvalidSignatureError in a single place, the caller. func (s *untrustedSignature) strictUnmarshalJSON(data []byte) error { var critical, optional json.RawMessage - if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ + if err := internal.ParanoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ "critical": &critical, "optional": &optional, }); err != nil { @@ -127,7 +127,7 @@ func (s *untrustedSignature) strictUnmarshalJSON(data []byte) error { var creatorID string var timestamp float64 var gotCreatorID, gotTimestamp = false, false - if err := paranoidUnmarshalJSONObject(optional, func(key string) interface{} { + if err := internal.ParanoidUnmarshalJSONObject(optional, func(key string) interface{} { switch key { case "creator": gotCreatorID = true @@ -155,7 +155,7 @@ func (s *untrustedSignature) strictUnmarshalJSON(data []byte) error { var t string var image, identity json.RawMessage - if err := paranoidUnmarshalJSONObjectExactFields(critical, map[string]interface{}{ + if err := internal.ParanoidUnmarshalJSONObjectExactFields(critical, map[string]interface{}{ "type": &t, "image": &image, "identity": &identity, @@ -167,14 +167,14 @@ func (s *untrustedSignature) strictUnmarshalJSON(data []byte) error { } var digestString string - if err := paranoidUnmarshalJSONObjectExactFields(image, map[string]interface{}{ + if err := internal.ParanoidUnmarshalJSONObjectExactFields(image, map[string]interface{}{ "docker-manifest-digest": &digestString, }); err != nil { return err } s.UntrustedDockerManifestDigest = digest.Digest(digestString) - return paranoidUnmarshalJSONObjectExactFields(identity, map[string]interface{}{ + return internal.ParanoidUnmarshalJSONObjectExactFields(identity, map[string]interface{}{ "docker-reference": &s.UntrustedDockerReference, }) } From ceacd853eba412ea19eda34cf740af682a89188f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 6 Jul 2022 03:07:20 +0200 Subject: [PATCH 4/4] Add signature.untrustedCosignPayload, with JSON marshaling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The code and tests are basically copy&pasted from simple signing. Signed-off-by: Miloslav Trmač --- signature/internal/cosign_payload.go | 160 +++++++++++++++++ signature/internal/cosign_payload_test.go | 199 ++++++++++++++++++++++ signature/internal/fixtures_info_test.go | 10 ++ 3 files changed, 369 insertions(+) create mode 100644 signature/internal/cosign_payload.go create mode 100644 signature/internal/cosign_payload_test.go create mode 100644 signature/internal/fixtures_info_test.go diff --git a/signature/internal/cosign_payload.go b/signature/internal/cosign_payload.go new file mode 100644 index 000000000..9b0679886 --- /dev/null +++ b/signature/internal/cosign_payload.go @@ -0,0 +1,160 @@ +package internal + +import ( + "encoding/json" + "errors" + "fmt" + "time" + + "github.com/containers/image/v5/version" + digest "github.com/opencontainers/go-digest" +) + +const ( + cosignSignatureType = "cosign container image signature" +) + +// UntrustedCosignPayload is a parsed content of a Cosign signature payload (not the full signature) +type UntrustedCosignPayload struct { + UntrustedDockerManifestDigest digest.Digest + UntrustedDockerReference string // FIXME: more precise type? + UntrustedCreatorID *string + // This is intentionally an int64; the native JSON float64 type would allow to represent _some_ sub-second precision, + // but not nearly enough (with current timestamp values, a single unit in the last place is on the order of hundreds of nanoseconds). + // So, this is explicitly an int64, and we reject fractional values. If we did need more precise timestamps eventually, + // we would add another field, UntrustedTimestampNS int64. + UntrustedTimestamp *int64 +} + +// NewUntrustedCosignPayload returns an untrustedCosignPayload object with +// the specified primary contents and appropriate metadata. +func NewUntrustedCosignPayload(dockerManifestDigest digest.Digest, dockerReference string) UntrustedCosignPayload { + // FIXME: Move this comment to the caller. + // AARGH. sigstore/cosign mostly ignores dockerReference for actual policy decisions. + // That’s bad enough, BUT they also: + // - Record the repo (but NOT THE TAG) in the value; without the tag we can’t detect version rollbacks. + // - parse dockerReference @ dockerManifestDigest and expect that to be valid. + // - It seems (FIXME: TEST THIS) that putting a repo:tag in would pass the current implementation. + // And signing digest references is _possible_ but probably rare (because signing typically happens on push, when + // the digest reference is not known in advance). + // SO: We put the full value in, which is not interoperable for signed digest references right now, + // and TODO: Talk sigstore/cosign to relax that. + // + // Use intermediate variables for these values so that we can take their addresses. + // Golang guarantees that they will have a new address on every execution. + creatorID := "containers/image " + version.Version + timestamp := time.Now().Unix() + return UntrustedCosignPayload{ + UntrustedDockerManifestDigest: dockerManifestDigest, + UntrustedDockerReference: dockerReference, + UntrustedCreatorID: &creatorID, + UntrustedTimestamp: ×tamp, + } +} + +// Compile-time check that untrustedCosignPayload implements json.Marshaler +var _ json.Marshaler = (*UntrustedCosignPayload)(nil) + +// MarshalJSON implements the json.Marshaler interface. +func (s UntrustedCosignPayload) MarshalJSON() ([]byte, error) { + if s.UntrustedDockerManifestDigest == "" || s.UntrustedDockerReference == "" { + return nil, errors.New("Unexpected empty signature content") + } + critical := map[string]interface{}{ + "type": cosignSignatureType, + "image": map[string]string{"docker-manifest-digest": s.UntrustedDockerManifestDigest.String()}, + "identity": map[string]string{"docker-reference": s.UntrustedDockerReference}, + } + optional := map[string]interface{}{} + if s.UntrustedCreatorID != nil { + optional["creator"] = *s.UntrustedCreatorID + } + if s.UntrustedTimestamp != nil { + optional["timestamp"] = *s.UntrustedTimestamp + } + signature := map[string]interface{}{ + "critical": critical, + "optional": optional, + } + return json.Marshal(signature) +} + +// Compile-time check that untrustedCosignPayload implements json.Unmarshaler +var _ json.Unmarshaler = (*UntrustedCosignPayload)(nil) + +// UnmarshalJSON implements the json.Unmarshaler interface +func (s *UntrustedCosignPayload) UnmarshalJSON(data []byte) error { + err := s.strictUnmarshalJSON(data) + if err != nil { + if formatErr, ok := err.(JSONFormatError); ok { + err = NewInvalidSignatureError(formatErr.Error()) + } + } + return err +} + +// strictUnmarshalJSON is UnmarshalJSON, except that it may return the internal JSONFormatError error type. +// Splitting it into a separate function allows us to do the JSONFormatError → InvalidSignatureError in a single place, the caller. +func (s *UntrustedCosignPayload) strictUnmarshalJSON(data []byte) error { + var critical, optional json.RawMessage + if err := ParanoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ + "critical": &critical, + "optional": &optional, + }); err != nil { + return err + } + + var creatorID string + var timestamp float64 + var gotCreatorID, gotTimestamp = false, false + if err := ParanoidUnmarshalJSONObject(optional, func(key string) interface{} { + switch key { + case "creator": + gotCreatorID = true + return &creatorID + case "timestamp": + gotTimestamp = true + return ×tamp + default: + var ignore interface{} + return &ignore + } + }); err != nil { + return err + } + if gotCreatorID { + s.UntrustedCreatorID = &creatorID + } + if gotTimestamp { + intTimestamp := int64(timestamp) + if float64(intTimestamp) != timestamp { + return NewInvalidSignatureError("Field optional.timestamp is not is not an integer") + } + s.UntrustedTimestamp = &intTimestamp + } + + var t string + var image, identity json.RawMessage + if err := ParanoidUnmarshalJSONObjectExactFields(critical, map[string]interface{}{ + "type": &t, + "image": &image, + "identity": &identity, + }); err != nil { + return err + } + if t != cosignSignatureType { + return NewInvalidSignatureError(fmt.Sprintf("Unrecognized signature type %s", t)) + } + + var digestString string + if err := ParanoidUnmarshalJSONObjectExactFields(image, map[string]interface{}{ + "docker-manifest-digest": &digestString, + }); err != nil { + return err + } + s.UntrustedDockerManifestDigest = digest.Digest(digestString) + + return ParanoidUnmarshalJSONObjectExactFields(identity, map[string]interface{}{ + "docker-reference": &s.UntrustedDockerReference, + }) +} diff --git a/signature/internal/cosign_payload_test.go b/signature/internal/cosign_payload_test.go new file mode 100644 index 000000000..792b1aac3 --- /dev/null +++ b/signature/internal/cosign_payload_test.go @@ -0,0 +1,199 @@ +package internal + +import ( + "encoding/json" + "testing" + "time" + + "github.com/containers/image/v5/version" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type mSI map[string]interface{} // To minimize typing the long name + +// A short-hand way to get a JSON object field value or panic. No error handling done, we know +// what we are working with, a panic in a test is good enough, and fitting test cases on a single line +// is a priority. +func x(m mSI, fields ...string) mSI { + for _, field := range fields { + // Not .(mSI) because type assertion of an unnamed type to a named type always fails (the types + // are not "identical"), but the assignment is fine because they are "assignable". + m = m[field].(map[string]interface{}) + } + return m +} + +func TestNewUntrustedCosignPayload(t *testing.T) { + timeBefore := time.Now() + sig := NewUntrustedCosignPayload(TestImageManifestDigest, TestImageSignatureReference) + assert.Equal(t, TestImageManifestDigest, sig.UntrustedDockerManifestDigest) + assert.Equal(t, TestImageSignatureReference, sig.UntrustedDockerReference) + require.NotNil(t, sig.UntrustedCreatorID) + assert.Equal(t, "containers/image "+version.Version, *sig.UntrustedCreatorID) + require.NotNil(t, sig.UntrustedTimestamp) + timeAfter := time.Now() + assert.True(t, timeBefore.Unix() <= *sig.UntrustedTimestamp) + assert.True(t, *sig.UntrustedTimestamp <= timeAfter.Unix()) +} + +func TestUntrustedCosignPayloadMarshalJSON(t *testing.T) { + // Empty string values + s := NewUntrustedCosignPayload("", "_") + _, err := s.MarshalJSON() + assert.Error(t, err) + s = NewUntrustedCosignPayload("_", "") + _, err = s.MarshalJSON() + assert.Error(t, err) + + // Success + // Use intermediate variables for these values so that we can take their addresses. + creatorID := "CREATOR" + timestamp := int64(1484683104) + for _, c := range []struct { + input UntrustedCosignPayload + expected string + }{ + { + UntrustedCosignPayload{ + UntrustedDockerManifestDigest: "digest!@#", + UntrustedDockerReference: "reference#@!", + UntrustedCreatorID: &creatorID, + UntrustedTimestamp: ×tamp, + }, + "{\"critical\":{\"identity\":{\"docker-reference\":\"reference#@!\"},\"image\":{\"docker-manifest-digest\":\"digest!@#\"},\"type\":\"cosign container image signature\"},\"optional\":{\"creator\":\"CREATOR\",\"timestamp\":1484683104}}", + }, + { + UntrustedCosignPayload{ + UntrustedDockerManifestDigest: "digest!@#", + UntrustedDockerReference: "reference#@!", + }, + "{\"critical\":{\"identity\":{\"docker-reference\":\"reference#@!\"},\"image\":{\"docker-manifest-digest\":\"digest!@#\"},\"type\":\"cosign container image signature\"},\"optional\":{}}", + }, + } { + marshaled, err := c.input.MarshalJSON() + require.NoError(t, err) + assert.Equal(t, []byte(c.expected), marshaled) + + // Also call MarshalJSON through the JSON package. + marshaled, err = json.Marshal(c.input) + assert.NoError(t, err) + assert.Equal(t, []byte(c.expected), marshaled) + } +} + +// Return the result of modifying validJSON with fn +func modifiedUntrustedCosignPayloadJSON(t *testing.T, validJSON []byte, modifyFn func(mSI)) []byte { + var tmp mSI + err := json.Unmarshal(validJSON, &tmp) + require.NoError(t, err) + + modifyFn(tmp) + + modifiedJSON, err := json.Marshal(tmp) + require.NoError(t, err) + return modifiedJSON +} + +// Verify that input can be unmarshaled as an untrustedCosignPayload. +func successfullyUnmarshalUntrustedCosignPayload(t *testing.T, input []byte) UntrustedCosignPayload { + var s UntrustedCosignPayload + err := json.Unmarshal(input, &s) + require.NoError(t, err, string(input)) + + return s +} + +// Verify that input can't be unmarshaled as an untrustedCosignPayload. +func assertUnmarshalUntrustedCosignPayloadFails(t *testing.T, input []byte) { + var s UntrustedCosignPayload + err := json.Unmarshal(input, &s) + assert.Error(t, err, string(input)) +} + +func TestUntrustedCosignPayloadUnmarshalJSON(t *testing.T) { + // Invalid input. Note that json.Unmarshal is guaranteed to validate input before calling our + // UnmarshalJSON implementation; so test that first, then test our error handling for completeness. + assertUnmarshalUntrustedCosignPayloadFails(t, []byte("&")) + var s UntrustedCosignPayload + err := s.UnmarshalJSON([]byte("&")) + assert.Error(t, err) + + // Not an object + assertUnmarshalUntrustedCosignPayloadFails(t, []byte("1")) + + // Start with a valid JSON. + validSig := NewUntrustedCosignPayload("digest!@#", "reference#@!") + validJSON, err := validSig.MarshalJSON() + require.NoError(t, err) + + // Success + s = successfullyUnmarshalUntrustedCosignPayload(t, validJSON) + assert.Equal(t, validSig, s) + + // Various ways to corrupt the JSON + breakFns := []func(mSI){ + // A top-level field is missing + func(v mSI) { delete(v, "critical") }, + func(v mSI) { delete(v, "optional") }, + // Extra top-level sub-object + func(v mSI) { v["unexpected"] = 1 }, + // "critical" not an object + func(v mSI) { v["critical"] = 1 }, + // "optional" not an object + func(v mSI) { v["optional"] = 1 }, + // A field of "critical" is missing + func(v mSI) { delete(x(v, "critical"), "type") }, + func(v mSI) { delete(x(v, "critical"), "image") }, + func(v mSI) { delete(x(v, "critical"), "identity") }, + // Extra field of "critical" + func(v mSI) { x(v, "critical")["unexpected"] = 1 }, + // Invalid "type" + func(v mSI) { x(v, "critical")["type"] = 1 }, + func(v mSI) { x(v, "critical")["type"] = "unexpected" }, + // Invalid "image" object + func(v mSI) { x(v, "critical")["image"] = 1 }, + func(v mSI) { delete(x(v, "critical", "image"), "docker-manifest-digest") }, + func(v mSI) { x(v, "critical", "image")["unexpected"] = 1 }, + // Invalid "docker-manifest-digest" + func(v mSI) { x(v, "critical", "image")["docker-manifest-digest"] = 1 }, + // Invalid "identity" object + func(v mSI) { x(v, "critical")["identity"] = 1 }, + func(v mSI) { delete(x(v, "critical", "identity"), "docker-reference") }, + func(v mSI) { x(v, "critical", "identity")["unexpected"] = 1 }, + // Invalid "docker-reference" + func(v mSI) { x(v, "critical", "identity")["docker-reference"] = 1 }, + // Invalid "creator" + func(v mSI) { x(v, "optional")["creator"] = 1 }, + // Invalid "timestamp" + func(v mSI) { x(v, "optional")["timestamp"] = "unexpected" }, + func(v mSI) { x(v, "optional")["timestamp"] = 0.5 }, // Fractional input + } + for _, fn := range breakFns { + testJSON := modifiedUntrustedCosignPayloadJSON(t, validJSON, fn) + assertUnmarshalUntrustedCosignPayloadFails(t, testJSON) + } + + // Modifications to unrecognized fields in "optional" are allowed and ignored + allowedModificationFns := []func(mSI){ + // Add an optional field + func(v mSI) { x(v, "optional")["unexpected"] = 1 }, + } + for _, fn := range allowedModificationFns { + testJSON := modifiedUntrustedCosignPayloadJSON(t, validJSON, fn) + s := successfullyUnmarshalUntrustedCosignPayload(t, testJSON) + assert.Equal(t, validSig, s) + } + + // Optional fields can be missing + validSig = UntrustedCosignPayload{ + UntrustedDockerManifestDigest: "digest!@#", + UntrustedDockerReference: "reference#@!", + UntrustedCreatorID: nil, + UntrustedTimestamp: nil, + } + validJSON, err = validSig.MarshalJSON() + require.NoError(t, err) + s = successfullyUnmarshalUntrustedCosignPayload(t, validJSON) + assert.Equal(t, validSig, s) +} diff --git a/signature/internal/fixtures_info_test.go b/signature/internal/fixtures_info_test.go new file mode 100644 index 000000000..41fce493f --- /dev/null +++ b/signature/internal/fixtures_info_test.go @@ -0,0 +1,10 @@ +package internal + +import "github.com/opencontainers/go-digest" + +const ( + // TestImageManifestDigest is the Docker manifest digest of "image.manifest.json" + TestImageManifestDigest = digest.Digest("sha256:20bf21ed457b390829cdbeec8795a7bea1626991fda603e0d01b4e7f60427e55") + // TestImageSignatureReference is the Docker image reference signed in "image.signature" + TestImageSignatureReference = "testing/manifest" +)