Make creator ID and timestamp explicit fields of untrustedSignature

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č <[email protected]>

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)
 }
 

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]

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")

signature/signature.go

@@ -37,18 +37,34 @@ 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:            &timestamp,
+	}
 }
 
 // Compile-time check that untrustedSignature implements json.Marshaler
 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 = &timestamp
+	}
 
 	t, err := stringField(c, "type")
 	if err != nil {

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:            &timestamp,
+			},
+			"{\"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)