From 954ed4bb848e37305f5b3610029c1224e4fc71e8 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] 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 b2705a6d03..b2cfd22fe7 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