From c813f58319f4ee90d26054a491db07eb53760dff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 13 Mar 2017 17:15:30 +0100 Subject: [PATCH 1/5] Add paranoidUnmarshalJSONObjectExactFields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a combination of json.Unmarshal + validateExactMapKeys except using the paranoidUnmarshalJSONObject parser (refusing duplicate and unknown fields), + unmarshaling into typed values instead of a map[string]interface{}, i.e. making int64Field/mapField/stringField unnecessary. This does not add any user yet. Signed-off-by: Miloslav Trmač --- signature/json.go | 22 ++++++++++++++++++++++ signature/json_test.go | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/signature/json.go b/signature/json.go index 1cb3c44727..29293c7eff 100644 --- a/signature/json.go +++ b/signature/json.go @@ -122,3 +122,25 @@ func paranoidUnmarshalJSONObject(data []byte, fieldResolver func(string) interfa } return nil } + +// paranoidUnmarshalJSONObject 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 { + seenKeys := map[string]struct{}{} + if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} { + if valuePtr, ok := exactFields[key]; ok { + seenKeys[key] = struct{}{} + return valuePtr + } + return nil + }); err != nil { + return err + } + for key := range exactFields { + if _, ok := seenKeys[key]; !ok { + return jsonFormatError(fmt.Sprintf(`Key "%s" missing in a JSON object`, key)) + } + } + return nil +} diff --git a/signature/json_test.go b/signature/json_test.go index fe63a1cd1e..e76fcda419 100644 --- a/signature/json_test.go +++ b/signature/json_test.go @@ -180,3 +180,45 @@ func TestParanoidUnmarshalJSONObject(t *testing.T) { assert.Error(t, err, input) } } + +func TestParanoidUnmarshalJSONObjectExactFields(t *testing.T) { + var stringValue string + var float64Value float64 + var rawValue json.RawMessage + var unmarshallCalled implementsUnmarshalJSON + exactFields := map[string]interface{}{ + "string": &stringValue, + "float64": &float64Value, + "raw": &rawValue, + "unmarshaller": &unmarshallCalled, + } + + // Empty object + 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) + require.NoError(t, err) + assert.Equal(t, "a", stringValue) + assert.Equal(t, 3.5, float64Value) + assert.Equal(t, json.RawMessage(`{"a":"b"}`), rawValue) + assert.Equal(t, implementsUnmarshalJSON(true), unmarshallCalled) + + // Various kinds of invalid input + for _, input := range []string{ + ``, // Empty input + `&`, // Entirely invalid JSON + `1`, // Not an object + `{&}`, // Invalid key JSON + `{1:1}`, // Key not a string + `{"string": "a", "string": "a", "float64": 3.5, "raw": {"a":"b"}, "unmarshaller": true}`, // Duplicate key + `{"string": "a", "float64": 3.5, "raw": {"a":"b"}, "unmarshaller": true, "thisisunknown", 1}`, // Unknown key + `{"string": &, "float64": 3.5, "raw": {"a":"b"}, "unmarshaller": true}`, // Invalid value JSON + `{"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) + assert.Error(t, err, input) + } +} From 89af51fcc7e4cf3cde0ccfb1abc06ff66a33e651 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 13 Mar 2017 17:18:13 +0100 Subject: [PATCH 2/5] Use paranoidUnmarshalJSONObjectExactFields in untrustedSignature.StrictUnmarshalJSON MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- signature/signature.go | 91 +++++++++++++++++-------------------- signature/signature_test.go | 1 + 2 files changed, 42 insertions(+), 50 deletions(-) diff --git a/signature/signature.go b/signature/signature.go index 30238e6115..79174c845d 100644 --- a/signature/signature.go +++ b/signature/signature.go @@ -120,78 +120,69 @@ func (s *untrustedSignature) 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 *untrustedSignature) strictUnmarshalJSON(data []byte) error { - var untyped interface{} - if err := json.Unmarshal(data, &untyped); err != nil { - return err - } - o, ok := untyped.(map[string]interface{}) - if !ok { - return InvalidSignatureError{msg: "Invalid signature format"} - } - if err := validateExactMapKeys(o, "critical", "optional"); err != nil { + var critical, optional json.RawMessage + if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ + "critical": &critical, + "optional": &optional, + }); err != nil { return err } - c, err := mapField(o, "critical") - if err != nil { - return err - } - if err := validateExactMapKeys(c, "type", "image", "identity"); err != nil { - return err - } - - optional, err := mapField(o, "optional") - if err != nil { + 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 _, ok := optional["creator"]; ok { - creatorID, err := stringField(optional, "creator") - if err != nil { - return err - } + if gotCreatorID { s.UntrustedCreatorID = &creatorID } - if _, ok := optional["timestamp"]; ok { - timestamp, err := int64Field(optional, "timestamp") - if err != nil { - return err + if gotTimestamp { + intTimestamp := int64(timestamp) + if float64(intTimestamp) != timestamp { + return InvalidSignatureError{msg: "Field optional.timestamp is not is not an integer"} } - s.UntrustedTimestamp = ×tamp + s.UntrustedTimestamp = &intTimestamp } - t, err := stringField(c, "type") - if err != nil { + 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 != signatureType { return InvalidSignatureError{msg: fmt.Sprintf("Unrecognized signature type %s", t)} } - image, err := mapField(c, "image") - if err != nil { - return err - } - if err := validateExactMapKeys(image, "docker-manifest-digest"); err != nil { - return err - } - digestString, err := stringField(image, "docker-manifest-digest") - if err != nil { + var digestString string + if err := paranoidUnmarshalJSONObjectExactFields(image, map[string]interface{}{ + "docker-manifest-digest": &digestString, + }); err != nil { return err } s.UntrustedDockerManifestDigest = digest.Digest(digestString) - identity, err := mapField(c, "identity") - if err != nil { - return err - } - if err := validateExactMapKeys(identity, "docker-reference"); err != nil { - return err - } - reference, err := stringField(identity, "docker-reference") - if err != nil { + if err := paranoidUnmarshalJSONObjectExactFields(identity, map[string]interface{}{ + "docker-reference": &s.UntrustedDockerReference, + }); err != nil { return err } - s.UntrustedDockerReference = reference return nil } diff --git a/signature/signature_test.go b/signature/signature_test.go index 3ddcffe574..1afbfd6de5 100644 --- a/signature/signature_test.go +++ b/signature/signature_test.go @@ -153,6 +153,7 @@ func TestUnmarshalJSON(t *testing.T) { 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 { err = tryUnmarshalModifiedSignature(t, &s, validJSON, fn) From 95908ee9a71e2bec60af7a9dca54a524c19b6fba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 13 Mar 2017 17:19:04 +0100 Subject: [PATCH 3/5] Remove no longer used JSON helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit i.e. validateExactMapKeys, int64Field, mapField, stringField. untrustedSignature.strictUnmarshalJSON no longer uses them. The test scenarios are covered by tests of untrustedSignature.strictUnmarshalJSON already (except for testing the full range of int64 timestamps). Signed-off-by: Miloslav Trmač --- signature/json.go | 58 ---------------------------- signature/json_test.go | 87 ------------------------------------------ 2 files changed, 145 deletions(-) diff --git a/signature/json.go b/signature/json.go index 29293c7eff..9e592863da 100644 --- a/signature/json.go +++ b/signature/json.go @@ -14,64 +14,6 @@ func (err jsonFormatError) Error() string { return string(err) } -// validateExactMapKeys returns an error if the keys of m are not exactly expectedKeys, which must be pairwise distinct -func validateExactMapKeys(m map[string]interface{}, expectedKeys ...string) error { - if len(m) != len(expectedKeys) { - return jsonFormatError("Unexpected keys in a JSON object") - } - - for _, k := range expectedKeys { - if _, ok := m[k]; !ok { - return jsonFormatError(fmt.Sprintf("Key %s missing in a JSON object", k)) - } - } - // Assuming expectedKeys are pairwise distinct, we know m contains len(expectedKeys) different values in expectedKeys. - 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] - if !ok { - return nil, jsonFormatError(fmt.Sprintf("Field %s missing", fieldName)) - } - v, ok := untyped.(map[string]interface{}) - if !ok { - return nil, jsonFormatError(fmt.Sprintf("Field %s is not a JSON object", fieldName)) - } - return v, nil -} - -// stringField returns a member fieldName of m, if it is a string, or an error. -func stringField(m map[string]interface{}, fieldName string) (string, error) { - untyped, ok := m[fieldName] - if !ok { - return "", jsonFormatError(fmt.Sprintf("Field %s missing", fieldName)) - } - v, ok := untyped.(string) - if !ok { - return "", jsonFormatError(fmt.Sprintf("Field %s is not a string", fieldName)) - } - return v, nil -} - // 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. diff --git a/signature/json_test.go b/signature/json_test.go index e76fcda419..83fe292989 100644 --- a/signature/json_test.go +++ b/signature/json_test.go @@ -2,8 +2,6 @@ package signature import ( "encoding/json" - "fmt" - "math" "testing" "github.com/stretchr/testify/assert" @@ -24,91 +22,6 @@ func x(m mSI, fields ...string) mSI { return m } -func TestValidateExactMapKeys(t *testing.T) { - // Empty map and keys - err := validateExactMapKeys(mSI{}) - assert.NoError(t, err) - - // Success - err = validateExactMapKeys(mSI{"a": nil, "b": 1}, "b", "a") - assert.NoError(t, err) - - // Extra map keys - err = validateExactMapKeys(mSI{"a": nil, "b": 1}, "a") - assert.Error(t, err) - - // Extra expected keys - err = validateExactMapKeys(mSI{"a": 1}, "b", "a") - assert.Error(t, err) - - // Unexpected key values - err = validateExactMapKeys(mSI{"a": 1}, "b") - 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") - assert.Error(t, err) - - // Field has a wrong type - _, err = mapField(mSI{"a": 1}, "a") - assert.Error(t, err) - - // Success - // FIXME? We can't use mSI as the type of child, that type apparently can't be converted to the raw map type. - child := map[string]interface{}{"b": mSI{}} - m, err := mapField(mSI{"a": child, "b": nil}, "a") - require.NoError(t, err) - assert.Equal(t, child, m) -} - -func TestStringField(t *testing.T) { - // Field not found - _, err := stringField(mSI{"a": "x"}, "b") - assert.Error(t, err) - - // Field has a wrong type - _, err = stringField(mSI{"a": 1}, "a") - assert.Error(t, err) - - // Success - s, err := stringField(mSI{"a": "x", "b": nil}, "a") - require.NoError(t, err) - assert.Equal(t, "x", s) -} - // implementsUnmarshalJSON is a minimalistic type used to detect that // paranoidUnmarshalJSONObject uses the json.Unmarshaler interface of resolved // pointers. From b6cea84639dc10f09d8ff5c0e73cbd4d66c44805 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 13 Mar 2017 18:14:59 +0100 Subject: [PATCH 4/5] Fix TestPolicyUnmarshalJSON MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A test was referring to the old "specific" name for the "transports" member. Signed-off-by: Miloslav Trmač --- signature/policy_config_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/signature/policy_config_test.go b/signature/policy_config_test.go index 12cb832ab7..c1d930ba22 100644 --- a/signature/policy_config_test.go +++ b/signature/policy_config_test.go @@ -297,8 +297,8 @@ func TestPolicyUnmarshalJSON(t *testing.T) { // Various allowed modifications to the policy allowedModificationFns := []func(mSI){ - // Delete the map of specific policies - func(v mSI) { delete(v, "specific") }, + // Delete the map of transport-specific scopes + func(v mSI) { delete(v, "transports") }, // Use an empty map of transport-specific scopes func(v mSI) { v["transports"] = map[string]PolicyTransportScopes{} }, } From 3994e560ea3a65dd6d28cbfaea74e8f9fe3020ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 13 Mar 2017 18:26:09 +0100 Subject: [PATCH 5/5] Use paranoidUnmarshalJSONObjectExactFields in policy.json parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In most of the cases, the original code did not explicitly require that a (string) field is present, but empty strings were later rejected (because the "type" field was required to be equal to a constant, or because parsing a docker/reference would fail). Either way, there were already tests verifying that missing fields are rejected; now the code is just a bit more explicit about it. Signed-off-by: Miloslav Trmač --- signature/policy_config.go | 84 +++++++++----------------------------- 1 file changed, 19 insertions(+), 65 deletions(-) diff --git a/signature/policy_config.go b/signature/policy_config.go index e4c93ed528..bc6c5e9a7d 100644 --- a/signature/policy_config.go +++ b/signature/policy_config.go @@ -255,13 +255,8 @@ var _ json.Unmarshaler = (*prInsecureAcceptAnything)(nil) func (pr *prInsecureAcceptAnything) UnmarshalJSON(data []byte) error { *pr = prInsecureAcceptAnything{} var tmp prInsecureAcceptAnything - if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} { - switch key { - case "type": - return &tmp.Type - default: - return nil - } + if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ + "type": &tmp.Type, }); err != nil { return err } @@ -290,13 +285,8 @@ var _ json.Unmarshaler = (*prReject)(nil) func (pr *prReject) UnmarshalJSON(data []byte) error { *pr = prReject{} var tmp prReject - if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} { - switch key { - case "type": - return &tmp.Type - default: - return nil - } + if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ + "type": &tmp.Type, }); err != nil { return err } @@ -465,15 +455,9 @@ func (pr *prSignedBaseLayer) UnmarshalJSON(data []byte) error { *pr = prSignedBaseLayer{} var tmp prSignedBaseLayer var baseLayerIdentity json.RawMessage - if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} { - switch key { - case "type": - return &tmp.Type - case "baseLayerIdentity": - return &baseLayerIdentity - default: - return nil - } + if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ + "type": &tmp.Type, + "baseLayerIdentity": &baseLayerIdentity, }); err != nil { return err } @@ -481,9 +465,6 @@ func (pr *prSignedBaseLayer) UnmarshalJSON(data []byte) error { if tmp.Type != prTypeSignedBaseLayer { return InvalidPolicyFormatError(fmt.Sprintf("Unexpected policy requirement type \"%s\"", tmp.Type)) } - if baseLayerIdentity == nil { - return InvalidPolicyFormatError(fmt.Sprintf("baseLayerIdentity not specified")) - } bli, err := newPolicyReferenceMatchFromJSON(baseLayerIdentity) if err != nil { return err @@ -541,13 +522,8 @@ var _ json.Unmarshaler = (*prmMatchExact)(nil) func (prm *prmMatchExact) UnmarshalJSON(data []byte) error { *prm = prmMatchExact{} var tmp prmMatchExact - if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} { - switch key { - case "type": - return &tmp.Type - default: - return nil - } + if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ + "type": &tmp.Type, }); err != nil { return err } @@ -576,13 +552,8 @@ var _ json.Unmarshaler = (*prmMatchRepoDigestOrExact)(nil) func (prm *prmMatchRepoDigestOrExact) UnmarshalJSON(data []byte) error { *prm = prmMatchRepoDigestOrExact{} var tmp prmMatchRepoDigestOrExact - if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} { - switch key { - case "type": - return &tmp.Type - default: - return nil - } + if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ + "type": &tmp.Type, }); err != nil { return err } @@ -611,13 +582,8 @@ var _ json.Unmarshaler = (*prmMatchRepository)(nil) func (prm *prmMatchRepository) UnmarshalJSON(data []byte) error { *prm = prmMatchRepository{} var tmp prmMatchRepository - if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} { - switch key { - case "type": - return &tmp.Type - default: - return nil - } + if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ + "type": &tmp.Type, }); err != nil { return err } @@ -656,15 +622,9 @@ var _ json.Unmarshaler = (*prmExactReference)(nil) func (prm *prmExactReference) UnmarshalJSON(data []byte) error { *prm = prmExactReference{} var tmp prmExactReference - if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} { - switch key { - case "type": - return &tmp.Type - case "dockerReference": - return &tmp.DockerReference - default: - return nil - } + if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ + "type": &tmp.Type, + "dockerReference": &tmp.DockerReference, }); err != nil { return err } @@ -704,15 +664,9 @@ var _ json.Unmarshaler = (*prmExactRepository)(nil) func (prm *prmExactRepository) UnmarshalJSON(data []byte) error { *prm = prmExactRepository{} var tmp prmExactRepository - if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} { - switch key { - case "type": - return &tmp.Type - case "dockerRepository": - return &tmp.DockerRepository - default: - return nil - } + if err := paranoidUnmarshalJSONObjectExactFields(data, map[string]interface{}{ + "type": &tmp.Type, + "dockerRepository": &tmp.DockerRepository, }); err != nil { return err }