From 12514e700fec68c82934e092ceff050e39ddab07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 11 Jan 2017 19:33:17 +0100 Subject: [PATCH 1/4] Use a separate untrustedSignature instead of privateSignature embedding a Signature MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of a privateSignature containing a Signature, and using the privateSignature type to attach private implementatinos of json.Marshaler and json.Unmarshaler and other private methods, use a completely separate private untrustedSignature type. This allows us to use scarier Untrusted… names for the members, but the only real code change is that verifyAndExtractSignature now needs to do a member-by-member copy instead of copying the full Signature struct. Signed-off-by: Miloslav Trmač --- signature/docker.go | 8 +++--- signature/signature.go | 54 ++++++++++++++++++++++--------------- signature/signature_test.go | 39 +++++++++++++-------------- 3 files changed, 53 insertions(+), 48 deletions(-) diff --git a/signature/docker.go b/signature/docker.go index 9f8a40a077..3c87adf959 100644 --- a/signature/docker.go +++ b/signature/docker.go @@ -16,11 +16,9 @@ func SignDockerManifest(m []byte, dockerReference string, mech SigningMechanism, if err != nil { return nil, err } - sig := privateSignature{ - Signature{ - DockerManifestDigest: manifestDigest, - DockerReference: dockerReference, - }, + sig := untrustedSignature{ + UntrustedDockerManifestDigest: manifestDigest, + UntrustedDockerReference: dockerReference, } return sig.sign(mech, keyIdentity) } diff --git a/signature/signature.go b/signature/signature.go index 22588771fc..bac6451101 100644 --- a/signature/signature.go +++ b/signature/signature.go @@ -27,33 +27,35 @@ func (err InvalidSignatureError) Error() string { } // 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. type Signature struct { DockerManifestDigest digest.Digest DockerReference string // FIXME: more precise type? } -// Wrap signature to add to it some methods which we don't want to make public. -type privateSignature struct { - Signature +// untrustedSignature is a parsed content of a signature. +type untrustedSignature struct { + UntrustedDockerManifestDigest digest.Digest + UntrustedDockerReference string // FIXME: more precise type? } -// Compile-time check that privateSignature implements json.Marshaler -var _ json.Marshaler = (*privateSignature)(nil) +// Compile-time check that untrustedSignature implements json.Marshaler +var _ json.Marshaler = (*untrustedSignature)(nil) // MarshalJSON implements the json.Marshaler interface. -func (s privateSignature) MarshalJSON() ([]byte, error) { +func (s untrustedSignature) MarshalJSON() ([]byte, error) { return s.marshalJSONWithVariables(time.Now().UTC().Unix(), "atomic "+version.Version) } // Implementation of MarshalJSON, with a caller-chosen values of the variable items to help testing. -func (s privateSignature) marshalJSONWithVariables(timestamp int64, creatorID string) ([]byte, error) { - if s.DockerManifestDigest == "" || s.DockerReference == "" { +func (s untrustedSignature) marshalJSONWithVariables(timestamp int64, creatorID string) ([]byte, error) { + if s.UntrustedDockerManifestDigest == "" || s.UntrustedDockerReference == "" { return nil, errors.New("Unexpected empty signature content") } critical := map[string]interface{}{ "type": signatureType, - "image": map[string]string{"docker-manifest-digest": s.DockerManifestDigest.String()}, - "identity": map[string]string{"docker-reference": s.DockerReference}, + "image": map[string]string{"docker-manifest-digest": s.UntrustedDockerManifestDigest.String()}, + "identity": map[string]string{"docker-reference": s.UntrustedDockerReference}, } optional := map[string]interface{}{ "creator": creatorID, @@ -66,11 +68,11 @@ func (s privateSignature) marshalJSONWithVariables(timestamp int64, creatorID st return json.Marshal(signature) } -// Compile-time check that privateSignature implements json.Unmarshaler -var _ json.Unmarshaler = (*privateSignature)(nil) +// Compile-time check that untrustedSignature implements json.Unmarshaler +var _ json.Unmarshaler = (*untrustedSignature)(nil) // UnmarshalJSON implements the json.Unmarshaler interface -func (s *privateSignature) UnmarshalJSON(data []byte) error { +func (s *untrustedSignature) UnmarshalJSON(data []byte) error { err := s.strictUnmarshalJSON(data) if err != nil { if _, ok := err.(jsonFormatError); ok { @@ -82,7 +84,7 @@ func (s *privateSignature) UnmarshalJSON(data []byte) error { // 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 *privateSignature) strictUnmarshalJSON(data []byte) error { +func (s *untrustedSignature) strictUnmarshalJSON(data []byte) error { var untyped interface{} if err := json.Unmarshal(data, &untyped); err != nil { return err @@ -128,7 +130,7 @@ func (s *privateSignature) strictUnmarshalJSON(data []byte) error { if err != nil { return err } - s.DockerManifestDigest = digest.Digest(digestString) + s.UntrustedDockerManifestDigest = digest.Digest(digestString) identity, err := mapField(c, "identity") if err != nil { @@ -141,13 +143,18 @@ func (s *privateSignature) strictUnmarshalJSON(data []byte) error { if err != nil { return err } - s.DockerReference = reference + s.UntrustedDockerReference = reference return nil } // Sign formats the signature and returns a blob signed using mech and keyIdentity -func (s privateSignature) sign(mech SigningMechanism, keyIdentity string) ([]byte, error) { +// (If it seems surprising that this is a method on untrustedSignature, note that there +// isn’t a good reason to think that a key used by the user is trusted by any component +// of the system just because it is a private key — actually the presence of a private key +// on the system increases the likelihood of an a successful attack on that private key +// on that particular system.) +func (s untrustedSignature) sign(mech SigningMechanism, keyIdentity string) ([]byte, error) { json, err := json.Marshal(s) if err != nil { return nil, err @@ -178,16 +185,19 @@ func verifyAndExtractSignature(mech SigningMechanism, unverifiedSignature []byte return nil, err } - var unmatchedSignature privateSignature + var unmatchedSignature untrustedSignature if err := json.Unmarshal(signed, &unmatchedSignature); err != nil { return nil, InvalidSignatureError{msg: err.Error()} } - if err := rules.validateSignedDockerManifestDigest(unmatchedSignature.DockerManifestDigest); err != nil { + if err := rules.validateSignedDockerManifestDigest(unmatchedSignature.UntrustedDockerManifestDigest); err != nil { return nil, err } - if err := rules.validateSignedDockerReference(unmatchedSignature.DockerReference); err != nil { + if err := rules.validateSignedDockerReference(unmatchedSignature.UntrustedDockerReference); err != nil { return nil, err } - signature := unmatchedSignature.Signature // Policy OK. - return &signature, nil + // signatureAcceptanceRules have accepted this value. + return &Signature{ + DockerManifestDigest: unmatchedSignature.UntrustedDockerManifestDigest, + DockerReference: unmatchedSignature.UntrustedDockerReference, + }, nil } diff --git a/signature/signature_test.go b/signature/signature_test.go index 7142d466df..fe24991a54 100644 --- a/signature/signature_test.go +++ b/signature/signature_test.go @@ -20,15 +20,15 @@ func TestInvalidSignatureError(t *testing.T) { func TestMarshalJSON(t *testing.T) { // Empty string values - s := privateSignature{Signature{DockerManifestDigest: "", DockerReference: "_"}} + s := untrustedSignature{UntrustedDockerManifestDigest: "", UntrustedDockerReference: "_"} _, err := s.MarshalJSON() assert.Error(t, err) - s = privateSignature{Signature{DockerManifestDigest: "_", DockerReference: ""}} + s = untrustedSignature{UntrustedDockerManifestDigest: "_", UntrustedDockerReference: ""} _, err = s.MarshalJSON() assert.Error(t, err) // Success - s = privateSignature{Signature{DockerManifestDigest: "digest!@#", DockerReference: "reference#@!"}} + s = untrustedSignature{UntrustedDockerManifestDigest: "digest!@#", UntrustedDockerReference: "reference#@!"} marshaled, err := s.marshalJSONWithVariables(0, "CREATOR") require.NoError(t, err) assert.Equal(t, []byte("{\"critical\":{\"identity\":{\"docker-reference\":\"reference#@!\"},\"image\":{\"docker-manifest-digest\":\"digest!@#\"},\"type\":\"atomic container signature\"},\"optional\":{\"creator\":\"CREATOR\",\"timestamp\":0}}"), @@ -41,7 +41,7 @@ func TestMarshalJSON(t *testing.T) { } // Return the result of modifying validJSON with fn and unmarshaling it into *sig -func tryUnmarshalModifiedSignature(t *testing.T, sig *privateSignature, validJSON []byte, modifyFn func(mSI)) error { +func tryUnmarshalModifiedSignature(t *testing.T, sig *untrustedSignature, validJSON []byte, modifyFn func(mSI)) error { var tmp mSI err := json.Unmarshal(validJSON, &tmp) require.NoError(t, err) @@ -51,12 +51,12 @@ func tryUnmarshalModifiedSignature(t *testing.T, sig *privateSignature, validJSO testJSON, err := json.Marshal(tmp) require.NoError(t, err) - *sig = privateSignature{} + *sig = untrustedSignature{} return json.Unmarshal(testJSON, sig) } func TestUnmarshalJSON(t *testing.T) { - var s privateSignature + var s untrustedSignature // 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. err := json.Unmarshal([]byte("&"), &s) @@ -69,17 +69,15 @@ func TestUnmarshalJSON(t *testing.T) { assert.Error(t, err) // Start with a valid JSON. - validSig := privateSignature{ - Signature{ - DockerManifestDigest: "digest!@#", - DockerReference: "reference#@!", - }, + validSig := untrustedSignature{ + UntrustedDockerManifestDigest: "digest!@#", + UntrustedDockerReference: "reference#@!", } validJSON, err := validSig.MarshalJSON() require.NoError(t, err) // Success - s = privateSignature{} + s = untrustedSignature{} err = json.Unmarshal(validJSON, &s) require.NoError(t, err) assert.Equal(t, validSig, s) @@ -140,11 +138,9 @@ func TestSign(t *testing.T) { mech, err := newGPGSigningMechanismInDirectory(testGPGHomeDirectory) require.NoError(t, err) - sig := privateSignature{ - Signature{ - DockerManifestDigest: "digest!@#", - DockerReference: "reference#@!", - }, + sig := untrustedSignature{ + UntrustedDockerManifestDigest: "digest!@#", + UntrustedDockerReference: "reference#@!", } // Successful signing @@ -159,13 +155,13 @@ func TestSign(t *testing.T) { return nil }, validateSignedDockerReference: func(signedDockerReference string) error { - if signedDockerReference != sig.DockerReference { + if signedDockerReference != sig.UntrustedDockerReference { return errors.Errorf("Unexpected signedDockerReference") } return nil }, validateSignedDockerManifestDigest: func(signedDockerManifestDigest digest.Digest) error { - if signedDockerManifestDigest != sig.DockerManifestDigest { + if signedDockerManifestDigest != sig.UntrustedDockerManifestDigest { return errors.Errorf("Unexpected signedDockerManifestDigest") } return nil @@ -173,10 +169,11 @@ func TestSign(t *testing.T) { }) require.NoError(t, err) - assert.Equal(t, sig.Signature, *verified) + assert.Equal(t, sig.UntrustedDockerManifestDigest, verified.DockerManifestDigest) + assert.Equal(t, sig.UntrustedDockerReference, verified.DockerReference) // Error creating blob to sign - _, err = privateSignature{}.sign(mech, TestKeyFingerprint) + _, err = untrustedSignature{}.sign(mech, TestKeyFingerprint) assert.Error(t, err) // Error signing From 69464bdffc7901545be1a3b1eea84091fa17f643 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 17 Jan 2017 16:54:45 +0100 Subject: [PATCH 2/4] =?UTF-8?q?Don=E2=80=99t=20call=20.UTC()=20on=20a=20ti?= =?UTF-8?q?me.Time=20before=20.Unix()?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit .Unix() ignores the time location, so .UTC() does’t really do anything. Signed-off-by: Miloslav Trmač --- signature/signature.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/signature/signature.go b/signature/signature.go index bac6451101..fa0ce8fc39 100644 --- a/signature/signature.go +++ b/signature/signature.go @@ -44,7 +44,7 @@ var _ json.Marshaler = (*untrustedSignature)(nil) // MarshalJSON implements the json.Marshaler interface. func (s untrustedSignature) MarshalJSON() ([]byte, error) { - return s.marshalJSONWithVariables(time.Now().UTC().Unix(), "atomic "+version.Version) + return s.marshalJSONWithVariables(time.Now().Unix(), "atomic "+version.Version) } // Implementation of MarshalJSON, with a caller-chosen values of the variable items to help testing. From e7055300486142b9e3a77a2a3542439213113636 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 11 Jan 2017 19:45:50 +0100 Subject: [PATCH 3/4] Make creator ID and timestamp explicit fields of untrustedSignature MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of silently embedding values in untrustedSignature.MarshalJSON (and having to add a marshalJSONWithvariables to work around this), make the creator ID and timestamp explicit fields of untrustedSignature, and MarshalJSON a simple marshaller of existing data. The values are now filled by calling newUntrustedSignature. Now that the fields are explicit, we can also record them by untrustedSignature.UnmarshalJSON. This also explicitly defines the timestamp to be an integer, instead of allowing floating-point values, because the JSON float64 is not precise enough for nanosecond timstamps. For now, we reject fractional values, which will allow us to record the nanosecond part separately in the future if it became necessary. Signed-off-by: Miloslav Trmač --- signature/docker.go | 5 +- signature/json.go | 17 +++++++ signature/json_test.go | 33 +++++++++++++ signature/signature.go | 50 ++++++++++++++++---- signature/signature_test.go | 92 ++++++++++++++++++++++++++++--------- 5 files changed, 162 insertions(+), 35 deletions(-) diff --git a/signature/docker.go b/signature/docker.go index 3c87adf959..b2126be656 100644 --- a/signature/docker.go +++ b/signature/docker.go @@ -16,10 +16,7 @@ func SignDockerManifest(m []byte, dockerReference string, mech SigningMechanism, if err != nil { return nil, err } - sig := untrustedSignature{ - UntrustedDockerManifestDigest: manifestDigest, - UntrustedDockerReference: dockerReference, - } + sig := newUntrustedSignature(manifestDigest, dockerReference) return sig.sign(mech, keyIdentity) } diff --git a/signature/json.go b/signature/json.go index 60a0bb4b03..1cb3c44727 100644 --- a/signature/json.go +++ b/signature/json.go @@ -29,6 +29,23 @@ func validateExactMapKeys(m map[string]interface{}, expectedKeys ...string) erro return nil } +// int64Field returns a member fieldName of m, if it is an int64, or an error. +func int64Field(m map[string]interface{}, fieldName string) (int64, error) { + untyped, ok := m[fieldName] + if !ok { + return -1, jsonFormatError(fmt.Sprintf("Field %s missing", fieldName)) + } + f, ok := untyped.(float64) + if !ok { + return -1, jsonFormatError(fmt.Sprintf("Field %s is not a number", fieldName)) + } + v := int64(f) + if float64(v) != f { + return -1, jsonFormatError(fmt.Sprintf("Field %s is not an integer", fieldName)) + } + return v, nil +} + // mapField returns a member fieldName of m, if it is a JSON map, or an error. func mapField(m map[string]interface{}, fieldName string) (map[string]interface{}, error) { untyped, ok := m[fieldName] diff --git a/signature/json_test.go b/signature/json_test.go index 01781e982e..fe63a1cd1e 100644 --- a/signature/json_test.go +++ b/signature/json_test.go @@ -2,6 +2,8 @@ package signature import ( "encoding/json" + "fmt" + "math" "testing" "github.com/stretchr/testify/assert" @@ -44,6 +46,37 @@ func TestValidateExactMapKeys(t *testing.T) { assert.Error(t, err) } +func TestInt64Field(t *testing.T) { + // Field not found + _, err := int64Field(mSI{"a": "x"}, "b") + assert.Error(t, err) + + // Field has a wrong type + _, err = int64Field(mSI{"a": "string"}, "a") + assert.Error(t, err) + + for _, value := range []float64{ + 0.5, // Fractional input + math.Inf(1), // Infinity + math.NaN(), // NaN + } { + _, err = int64Field(mSI{"a": value}, "a") + assert.Error(t, err, fmt.Sprintf("%f", value)) + } + + // Success + // The float64 type has 53 bits of effective precision, so ±1FFFFFFFFFFFFF is the + // range of integer values which can all be represented exactly (beyond that, + // some are representable if they are divisible by a high enough power of 2, + // but most are not). + for _, value := range []int64{0, 1, -1, 0x1FFFFFFFFFFFFF, -0x1FFFFFFFFFFFFF} { + testName := fmt.Sprintf("%d", value) + v, err := int64Field(mSI{"a": float64(value), "b": nil}, "a") + require.NoError(t, err, testName) + assert.Equal(t, value, v, testName) + } +} + func TestMapField(t *testing.T) { // Field not found _, err := mapField(mSI{"a": mSI{}}, "b") diff --git a/signature/signature.go b/signature/signature.go index fa0ce8fc39..7e13314174 100644 --- a/signature/signature.go +++ b/signature/signature.go @@ -37,6 +37,27 @@ type Signature struct { type untrustedSignature 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 +} + +// newUntrustedSignature returns an untrustedSignature object with +// the specified primary contents and appropriate metadata. +func newUntrustedSignature(dockerManifestDigest digest.Digest, dockerReference string) untrustedSignature { + // 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 := "atomic " + version.Version + timestamp := time.Now().Unix() + return untrustedSignature{ + UntrustedDockerManifestDigest: dockerManifestDigest, + UntrustedDockerReference: dockerReference, + UntrustedCreatorID: &creatorID, + UntrustedTimestamp: ×tamp, + } } // Compile-time check that untrustedSignature implements json.Marshaler @@ -44,11 +65,6 @@ var _ json.Marshaler = (*untrustedSignature)(nil) // MarshalJSON implements the json.Marshaler interface. func (s untrustedSignature) MarshalJSON() ([]byte, error) { - return s.marshalJSONWithVariables(time.Now().Unix(), "atomic "+version.Version) -} - -// Implementation of MarshalJSON, with a caller-chosen values of the variable items to help testing. -func (s untrustedSignature) marshalJSONWithVariables(timestamp int64, creatorID string) ([]byte, error) { if s.UntrustedDockerManifestDigest == "" || s.UntrustedDockerReference == "" { return nil, errors.New("Unexpected empty signature content") } @@ -57,9 +73,12 @@ func (s untrustedSignature) marshalJSONWithVariables(timestamp int64, creatorID "image": map[string]string{"docker-manifest-digest": s.UntrustedDockerManifestDigest.String()}, "identity": map[string]string{"docker-reference": s.UntrustedDockerReference}, } - optional := map[string]interface{}{ - "creator": creatorID, - "timestamp": timestamp, + 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, @@ -109,7 +128,20 @@ func (s *untrustedSignature) strictUnmarshalJSON(data []byte) error { if err != nil { return err } - _ = optional // We don't use anything from here for now. + if _, ok := optional["creator"]; ok { + creatorID, err := stringField(optional, "creator") + if err != nil { + return err + } + s.UntrustedCreatorID = &creatorID + } + if _, ok := optional["timestamp"]; ok { + timestamp, err := int64Field(optional, "timestamp") + if err != nil { + return err + } + s.UntrustedTimestamp = ×tamp + } t, err := stringField(c, "type") if err != nil { diff --git a/signature/signature_test.go b/signature/signature_test.go index fe24991a54..b68a159141 100644 --- a/signature/signature_test.go +++ b/signature/signature_test.go @@ -4,7 +4,9 @@ import ( "encoding/json" "io/ioutil" "testing" + "time" + "github.com/containers/image/version" "github.com/opencontainers/go-digest" "github.com/pkg/errors" "github.com/stretchr/testify/assert" @@ -18,26 +20,62 @@ func TestInvalidSignatureError(t *testing.T) { assert.Equal(t, s, err.Error()) } +func TestNewUntrustedSignature(t *testing.T) { + timeBefore := time.Now() + sig := newUntrustedSignature(TestImageManifestDigest, TestImageSignatureReference) + assert.Equal(t, TestImageManifestDigest, sig.UntrustedDockerManifestDigest) + assert.Equal(t, TestImageSignatureReference, sig.UntrustedDockerReference) + require.NotNil(t, sig.UntrustedCreatorID) + assert.Equal(t, "atomic "+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 TestMarshalJSON(t *testing.T) { // Empty string values - s := untrustedSignature{UntrustedDockerManifestDigest: "", UntrustedDockerReference: "_"} + s := newUntrustedSignature("", "_") _, err := s.MarshalJSON() assert.Error(t, err) - s = untrustedSignature{UntrustedDockerManifestDigest: "_", UntrustedDockerReference: ""} + s = newUntrustedSignature("_", "") _, err = s.MarshalJSON() assert.Error(t, err) // Success - s = untrustedSignature{UntrustedDockerManifestDigest: "digest!@#", UntrustedDockerReference: "reference#@!"} - marshaled, err := s.marshalJSONWithVariables(0, "CREATOR") - require.NoError(t, err) - assert.Equal(t, []byte("{\"critical\":{\"identity\":{\"docker-reference\":\"reference#@!\"},\"image\":{\"docker-manifest-digest\":\"digest!@#\"},\"type\":\"atomic container signature\"},\"optional\":{\"creator\":\"CREATOR\",\"timestamp\":0}}"), - marshaled) + // Use intermediate variables for these values so that we can take their addresses. + creatorID := "CREATOR" + timestamp := int64(1484683104) + for _, c := range []struct { + input untrustedSignature + expected string + }{ + { + untrustedSignature{ + UntrustedDockerManifestDigest: "digest!@#", + UntrustedDockerReference: "reference#@!", + UntrustedCreatorID: &creatorID, + UntrustedTimestamp: ×tamp, + }, + "{\"critical\":{\"identity\":{\"docker-reference\":\"reference#@!\"},\"image\":{\"docker-manifest-digest\":\"digest!@#\"},\"type\":\"atomic container signature\"},\"optional\":{\"creator\":\"CREATOR\",\"timestamp\":1484683104}}", + }, + { + untrustedSignature{ + UntrustedDockerManifestDigest: "digest!@#", + UntrustedDockerReference: "reference#@!", + }, + "{\"critical\":{\"identity\":{\"docker-reference\":\"reference#@!\"},\"image\":{\"docker-manifest-digest\":\"digest!@#\"},\"type\":\"atomic container signature\"},\"optional\":{}}", + }, + } { + marshaled, err := c.input.MarshalJSON() + require.NoError(t, err) + assert.Equal(t, []byte(c.expected), marshaled) - // We can't test MarshalJSON directly because the timestamp will keep changing, so just test that - // it doesn't fail. And call it through the JSON package for a good measure. - _, err = json.Marshal(s) - assert.NoError(t, err) + // 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 and unmarshaling it into *sig @@ -69,10 +107,7 @@ func TestUnmarshalJSON(t *testing.T) { assert.Error(t, err) // Start with a valid JSON. - validSig := untrustedSignature{ - UntrustedDockerManifestDigest: "digest!@#", - UntrustedDockerReference: "reference#@!", - } + validSig := newUntrustedSignature("digest!@#", "reference#@!") validJSON, err := validSig.MarshalJSON() require.NoError(t, err) @@ -114,34 +149,47 @@ func TestUnmarshalJSON(t *testing.T) { 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" }, } for _, fn := range breakFns { err = tryUnmarshalModifiedSignature(t, &s, validJSON, fn) assert.Error(t, err) } - // Modifications to "optional" are allowed and ignored + // 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 }, - // Delete an optional field - func(v mSI) { delete(x(v, "optional"), "creator") }, } for _, fn := range allowedModificationFns { err = tryUnmarshalModifiedSignature(t, &s, validJSON, fn) require.NoError(t, err) assert.Equal(t, validSig, s) } + + // Optional fields can be missing + validSig = untrustedSignature{ + UntrustedDockerManifestDigest: "digest!@#", + UntrustedDockerReference: "reference#@!", + UntrustedCreatorID: nil, + UntrustedTimestamp: nil, + } + validJSON, err = validSig.MarshalJSON() + require.NoError(t, err) + s = untrustedSignature{} + err = json.Unmarshal(validJSON, &s) + require.NoError(t, err) + assert.Equal(t, validSig, s) } func TestSign(t *testing.T) { mech, err := newGPGSigningMechanismInDirectory(testGPGHomeDirectory) require.NoError(t, err) - sig := untrustedSignature{ - UntrustedDockerManifestDigest: "digest!@#", - UntrustedDockerReference: "reference#@!", - } + sig := newUntrustedSignature("digest!@#", "reference#@!") // Successful signing signature, err := sig.sign(mech, TestKeyFingerprint) From 596d3190a06e7de4816dfe3c6fe8dd6a25ccd42e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 11 Jan 2017 20:29:19 +0100 Subject: [PATCH 4/4] Add signature.GetUntrustedSignatureInformationWithoutVerifying MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add signature.GetUntrustedSignatureInformationWithoutVerifying, which returns the contents of a signature without doing any cryptographic verification. Expected uses are 1) debugging verification failures, and (CAREFULLY!) 2) listing which signatures are stored in a registry along with an image. It is STRONGLY recommended to do cryptographic verification in the latter case, and to clearly indicate untrusted signatures in the UI. Signed-off-by: Miloslav Trmač --- .../fixtures/no-optional-fields.signature | Bin 0 -> 383 bytes signature/fixtures_info_test.go | 2 + signature/mechanism.go | 38 ++++++++++++ signature/mechanism_test.go | 56 ++++++++++++++++++ signature/signature.go | 55 +++++++++++++++++ signature/signature_test.go | 39 ++++++++++++ 6 files changed, 190 insertions(+) create mode 100644 signature/fixtures/no-optional-fields.signature diff --git a/signature/fixtures/no-optional-fields.signature b/signature/fixtures/no-optional-fields.signature new file mode 100644 index 0000000000000000000000000000000000000000..482ae3acf0d80c22aa8b1a7b18e48e2b04a6ffa0 GIT binary patch literal 383 zcmV-_0f7Fa0h_?f%)rEWyXccd_m-R!jHeH%Cox3Sb@f*(B^PCuWF{x(C|Ol2Wu~O& zm1LGwg4ikf$=Rtzx<#pJsYR)I$*D?KN+qeqC7F5Y`nidDnQ1__Qmu|sW^Q77Dw2Ab zoNh{HI!K9?QgKG2k*S%LkwH?Lkzs0ziK%&#v8929k!5m9Qfg|lg}J3^qIptkqM@0Q znWd#+T1ujsfpMxqih*I0Nve68nSqIsd8(-?$g+~k0+2frOY(CwlNFNl^GXsk^HPfx ziZj#m5=%;pQbCIH3raHc^S~aet>x;N!@|JG#K6YN1oAEe7pDL$5KP}Q|CQxEdf?iT z>oJuvx30at8&fN@>6W#1n%~38%QyP#Cj0Gwyw4%JuSg;F_u*r=s>SCly6E}jRFg&1 zFWq%aXOwoQ<}TcQdH!|7wT=^6-&tGS@;)RI9^v5X_^{!T;H`fyix$@Xe1Ei|_|g>9 dKT&fXzc>`SoomwQP+E4bv`?bx;PvLU_W&nc%{2f3 literal 0 HcmV?d00001 diff --git a/signature/fixtures_info_test.go b/signature/fixtures_info_test.go index e9f21f7e35..a44a6b7364 100644 --- a/signature/fixtures_info_test.go +++ b/signature/fixtures_info_test.go @@ -9,4 +9,6 @@ const ( TestImageSignatureReference = "testing/manifest" // TestKeyFingerprint is the fingerprint of the private key in this directory. TestKeyFingerprint = "1D8230F6CDB6A06716E414C1DB72F2188BB46CC8" + // TestKeyShortID is the short ID of the private key in this directory. + TestKeyShortID = "DB72F2188BB46CC8" ) diff --git a/signature/mechanism.go b/signature/mechanism.go index 196ad92789..4b4393bcaf 100644 --- a/signature/mechanism.go +++ b/signature/mechanism.go @@ -4,9 +4,13 @@ package signature import ( "bytes" + "errors" "fmt" + "io/ioutil" + "strings" "github.com/mtrmac/gpgme" + "golang.org/x/crypto/openpgp" ) // SigningMechanism abstracts a way to sign binary blobs and verify their signatures. @@ -21,6 +25,12 @@ type SigningMechanism interface { Sign(input []byte, keyIdentity string) ([]byte, error) // Verify parses unverifiedSignature and returns the content and the signer's identity Verify(unverifiedSignature []byte) (contents []byte, keyIdentity string, err error) + // UntrustedSignatureContents returns UNTRUSTED contents of the signature WITHOUT ANY VERIFICATION, + // along with a short identifier of the key used for signing. + // WARNING: The short key identifier (which correponds to "Key ID" for OpenPGP keys) + // is NOT the same as a "key identity" used in other calls ot this interface, and + // the values may have no recognizable relationship if the public key is not available. + UntrustedSignatureContents(untrustedSignature []byte) (untrustedContents []byte, shortKeyIdentifier string, err error) } // A GPG/OpenPGP signing mechanism. @@ -119,3 +129,31 @@ func (m gpgSigningMechanism) Verify(unverifiedSignature []byte) (contents []byte } return signedBuffer.Bytes(), sig.Fingerprint, nil } + +// UntrustedSignatureContents returns UNTRUSTED contents of the signature WITHOUT ANY VERIFICATION, +// along with a short identifier of the key used for signing. +// WARNING: The short key identifier (which correponds to "Key ID" for OpenPGP keys) +// is NOT the same as a "key identity" used in other calls ot this interface, and +// the values may have no recognizable relationship if the public key is not available. +func (m gpgSigningMechanism) UntrustedSignatureContents(untrustedSignature []byte) (untrustedContents []byte, shortKeyIdentifier string, err error) { + // This uses the Golang-native OpenPGP implementation instead of gpgme because we are not doing any cryptography. + md, err := openpgp.ReadMessage(bytes.NewReader(untrustedSignature), openpgp.EntityList{}, nil, nil) + if err != nil { + return nil, "", err + } + if !md.IsSigned { + return nil, "", errors.New("The input is not a signature") + } + content, err := ioutil.ReadAll(md.UnverifiedBody) + if err != nil { + // Coverage: An error during reading the body can happen only if + // 1) the message is encrypted, which is not our case (and we don’t give ReadMessage the key + // to decrypt the contents anyway), or + // 2) the message is signed AND we give ReadMessage a correspnding public key, which we don’t. + return nil, "", err + } + + // Uppercase the key ID for minimal consistency with the gpgme-returned fingerprints + // (but note that key ID is a suffix of the fingerprint only for V4 keys, not V3)! + return content, strings.ToUpper(fmt.Sprintf("%016X", md.SignedByKeyId)), nil +} diff --git a/signature/mechanism_test.go b/signature/mechanism_test.go index e6cbc0a7b6..5aee945faa 100644 --- a/signature/mechanism_test.go +++ b/signature/mechanism_test.go @@ -147,3 +147,59 @@ func TestGPGSigningMechanismVerify(t *testing.T) { // The various GPG/GPGME failures cases are not obviously easy to reach. } + +func TestGPGSigningMechanismUntrustedSignatureContents(t *testing.T) { + mech, err := newGPGSigningMechanismInDirectory(testGPGHomeDirectory) + require.NoError(t, err) + + // A valid signature + signature, err := ioutil.ReadFile("./fixtures/invalid-blob.signature") + require.NoError(t, err) + content, shortKeyID, err := mech.UntrustedSignatureContents(signature) + require.NoError(t, err) + assert.Equal(t, []byte("This is not JSON\n"), content) + assert.Equal(t, TestKeyShortID, shortKeyID) + + // Completely invalid signature. + _, _, err = mech.UntrustedSignatureContents([]byte{}) + assert.Error(t, err) + + _, _, err = mech.UntrustedSignatureContents([]byte("invalid signature")) + assert.Error(t, err) + + // Literal packet, not a signature + signature, err = ioutil.ReadFile("./fixtures/unsigned-literal.signature") + require.NoError(t, err) + content, shortKeyID, err = mech.UntrustedSignatureContents(signature) + assert.Error(t, err) + + // Encrypted data, not a signature. + signature, err = ioutil.ReadFile("./fixtures/unsigned-encrypted.signature") + require.NoError(t, err) + content, shortKeyID, err = mech.UntrustedSignatureContents(signature) + assert.Error(t, err) + + // Expired signature + signature, err = ioutil.ReadFile("./fixtures/expired.signature") + require.NoError(t, err) + content, shortKeyID, err = mech.UntrustedSignatureContents(signature) + require.NoError(t, err) + assert.Equal(t, []byte("This signature is expired.\n"), content) + assert.Equal(t, TestKeyShortID, shortKeyID) + + // Corrupt signature + signature, err = ioutil.ReadFile("./fixtures/corrupt.signature") + require.NoError(t, err) + content, shortKeyID, err = mech.UntrustedSignatureContents(signature) + require.NoError(t, err) + assert.Equal(t, []byte(`{"critical":{"identity":{"docker-reference":"testing/manifest"},"image":{"docker-manifest-digest":"sha256:20bf21ed457b390829cdbeec8795a7bea1626991fda603e0d01b4e7f60427e55"},"type":"atomic container signature"},"optional":{"creator":"atomic ","timestamp":1458239713}}`), content) + assert.Equal(t, TestKeyShortID, shortKeyID) + + // Valid signature with an unknown key + signature, err = ioutil.ReadFile("./fixtures/unknown-key.signature") + require.NoError(t, err) + content, shortKeyID, err = mech.UntrustedSignatureContents(signature) + require.NoError(t, err) + assert.Equal(t, []byte(`{"critical":{"identity":{"docker-reference":"testing/manifest"},"image":{"docker-manifest-digest":"sha256:20bf21ed457b390829cdbeec8795a7bea1626991fda603e0d01b4e7f60427e55"},"type":"atomic container signature"},"optional":{"creator":"atomic 0.1.13-dev","timestamp":1464633474}}`), content) + assert.Equal(t, "E5476D1110D07803", shortKeyID) +} diff --git a/signature/signature.go b/signature/signature.go index 7e13314174..30238e6115 100644 --- a/signature/signature.go +++ b/signature/signature.go @@ -45,6 +45,22 @@ type untrustedSignature struct { UntrustedTimestamp *int64 } +// UntrustedSignatureInformation is information available in an untrusted signature. +// This may be useful when debugging signature verification failures, +// or when managing a set of signatures on a single image. +// +// WARNING: Do not use the contents of this for ANY security decisions, +// and be VERY CAREFUL about showing this information to humans in any way which suggest that these values “are probably” reliable. +// There is NO REASON to expect the values to be correct, or not intentionally misleading +// (including things like “✅ Verified by $authority”) +type UntrustedSignatureInformation struct { + UntrustedDockerManifestDigest digest.Digest + UntrustedDockerReference string // FIXME: more precise type? + UntrustedCreatorID *string + UntrustedTimestamp *time.Time + UntrustedShortKeyIdentifier string +} + // newUntrustedSignature returns an untrustedSignature object with // the specified primary contents and appropriate metadata. func newUntrustedSignature(dockerManifestDigest digest.Digest, dockerReference string) untrustedSignature { @@ -233,3 +249,42 @@ func verifyAndExtractSignature(mech SigningMechanism, unverifiedSignature []byte DockerReference: unmatchedSignature.UntrustedDockerReference, }, nil } + +// GetUntrustedSignatureInformationWithoutVerifying extracts information available in an untrusted signature, +// WITHOUT doing any cryptographic verification. +// This may be useful when debugging signature verification failures, +// or when managing a set of signatures on a single image. +// +// WARNING: Do not use the contents of this for ANY security decisions, +// and be VERY CAREFUL about showing this information to humans in any way which suggest that these values “are probably” reliable. +// There is NO REASON to expect the values to be correct, or not intentionally misleading +// (including things like “✅ Verified by $authority”) +func GetUntrustedSignatureInformationWithoutVerifying(untrustedSignatureBytes []byte) (*UntrustedSignatureInformation, error) { + // NOTE: This should eventualy do format autodetection. + mech, err := NewGPGSigningMechanism() + if err != nil { + return nil, err + } + + untrustedContents, shortKeyIdentifier, err := mech.UntrustedSignatureContents(untrustedSignatureBytes) + if err != nil { + return nil, err + } + var untrustedDecodedContents untrustedSignature + if err := json.Unmarshal(untrustedContents, &untrustedDecodedContents); err != nil { + return nil, InvalidSignatureError{msg: err.Error()} + } + + var timestamp *time.Time // = nil + if untrustedDecodedContents.UntrustedTimestamp != nil { + ts := time.Unix(*untrustedDecodedContents.UntrustedTimestamp, 0) + timestamp = &ts + } + return &UntrustedSignatureInformation{ + UntrustedDockerManifestDigest: untrustedDecodedContents.UntrustedDockerManifestDigest, + UntrustedDockerReference: untrustedDecodedContents.UntrustedDockerReference, + UntrustedCreatorID: untrustedDecodedContents.UntrustedCreatorID, + UntrustedTimestamp: timestamp, + UntrustedShortKeyIdentifier: shortKeyIdentifier, + }, nil +} diff --git a/signature/signature_test.go b/signature/signature_test.go index b68a159141..3ddcffe574 100644 --- a/signature/signature_test.go +++ b/signature/signature_test.go @@ -336,3 +336,42 @@ func TestVerifyAndExtractSignature(t *testing.T) { assert.Nil(t, sig) assert.Equal(t, signatureData, recorded) } + +func TestGetUntrustedSignatureInformationWithoutVerifying(t *testing.T) { + signature, err := ioutil.ReadFile("./fixtures/image.signature") + require.NoError(t, err) + // Successful parsing, all optional fields present + info, err := GetUntrustedSignatureInformationWithoutVerifying(signature) + require.NoError(t, err) + assert.Equal(t, TestImageSignatureReference, info.UntrustedDockerReference) + assert.Equal(t, TestImageManifestDigest, info.UntrustedDockerManifestDigest) + assert.NotNil(t, info.UntrustedCreatorID) + assert.Equal(t, "atomic ", *info.UntrustedCreatorID) + assert.NotNil(t, info.UntrustedTimestamp) + assert.Equal(t, time.Unix(1458239713, 0), *info.UntrustedTimestamp) + assert.Equal(t, TestKeyShortID, info.UntrustedShortKeyIdentifier) + // Successful parsing, no optional fields present + signature, err = ioutil.ReadFile("./fixtures/no-optional-fields.signature") + require.NoError(t, err) + // Successful parsing + info, err = GetUntrustedSignatureInformationWithoutVerifying(signature) + require.NoError(t, err) + assert.Equal(t, TestImageSignatureReference, info.UntrustedDockerReference) + assert.Equal(t, TestImageManifestDigest, info.UntrustedDockerManifestDigest) + assert.Nil(t, info.UntrustedCreatorID) + assert.Nil(t, info.UntrustedTimestamp) + assert.Equal(t, TestKeyShortID, info.UntrustedShortKeyIdentifier) + + // Completely invalid signature. + _, err = GetUntrustedSignatureInformationWithoutVerifying([]byte{}) + assert.Error(t, err) + + _, err = GetUntrustedSignatureInformationWithoutVerifying([]byte("invalid signature")) + assert.Error(t, err) + + // Valid signature of non-JSON + invalidBlobSignature, err := ioutil.ReadFile("./fixtures/invalid-blob.signature") + require.NoError(t, err) + _, err = GetUntrustedSignatureInformationWithoutVerifying(invalidBlobSignature) + assert.Error(t, err) +}