From 1a54eb5692fbd3eeea0c859e05935f4ba1dc42ff Mon Sep 17 00:00:00 2001 From: Luiz Carvalho Date: Mon, 28 Mar 2022 12:06:25 -0400 Subject: [PATCH 1/2] Fix sanity checks during policy processing Previously, the code was comparing a predicate URI against a payload type. This is never expected to match. Modify the code to extract the actual predicate type from within the payload so the verification can be done successfully. Signed-off-by: Luiz Carvalho --- cmd/cosign/cli/verify/verify_attestation.go | 38 +++++++++------------ 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/cmd/cosign/cli/verify/verify_attestation.go b/cmd/cosign/cli/verify/verify_attestation.go index 63c8df468a2..f2413f15b4a 100644 --- a/cmd/cosign/cli/verify/verify_attestation.go +++ b/cmd/cosign/cli/verify/verify_attestation.go @@ -195,22 +195,6 @@ func (c *VerifyAttestationCommand) Exec(ctx context.Context, images []string) (e return errors.Wrap(err, "unmarshal payload data") } - predicateURI, ok := options.PredicateTypeMap[c.PredicateType] - if !ok { - return fmt.Errorf("invalid predicate type: %s", c.PredicateType) - } - - // sanity checks - if val, ok := payloadData["payloadType"]; ok { - // we need to check only given type from the cli flag - // so we are skipping other types - if predicateURI != val { - continue - } - } else { - return fmt.Errorf("could not find 'payloadType' in payload data") - } - var decodedPayload []byte if val, ok := payloadData["payload"]; ok { decodedPayload, err = base64.StdEncoding.DecodeString(val.(string)) @@ -221,14 +205,24 @@ func (c *VerifyAttestationCommand) Exec(ctx context.Context, images []string) (e return fmt.Errorf("could not find 'payload' in payload data") } + predicateURI, ok := options.PredicateTypeMap[c.PredicateType] + if !ok { + return fmt.Errorf("invalid predicate type: %s", c.PredicateType) + } + + // Only apply the policy against the requested predicate type + var statement in_toto.Statement + if err := json.Unmarshal(decodedPayload, &statement); err != nil { + return fmt.Errorf("unmarshal in-toto statement: %w", err) + } + if statement.PredicateType != predicateURI { + continue + } + var payload []byte switch c.PredicateType { case options.PredicateCustom: - var cosignStatement in_toto.Statement - if err := json.Unmarshal(decodedPayload, &cosignStatement); err != nil { - return fmt.Errorf("unmarshal CosignStatement: %w", err) - } - payload, err = json.Marshal(cosignStatement) + payload, err = json.Marshal(statement) if err != nil { return fmt.Errorf("error when generating CosignStatement: %w", err) } @@ -259,6 +253,8 @@ func (c *VerifyAttestationCommand) Exec(ctx context.Context, images []string) (e if err != nil { return fmt.Errorf("error when generating SPDXStatement: %w", err) } + default: + return fmt.Errorf("unsupported predicate type: %s", c.PredicateType) } if len(cuePolicies) > 0 { From 90009e9f217300bd0a9288731230dd183adf9009 Mon Sep 17 00:00:00 2001 From: Luiz Carvalho Date: Mon, 28 Mar 2022 12:08:50 -0400 Subject: [PATCH 2/2] Fix rego policy verification This commit addresses multiple issues when applying a rego policy against the payload of an attestation. 1. If `data.signature.deny` evaluated to `true`, the policy verification would pass. This is obviously unexpected. The code now looks for `data.signature.allow` instead, and expects it to be `true`. 2. If a query result returned an undefined results, the policy verification would pass. The code now explicitly checks for this condition and ensure that if `ResultSet.IsAllowed()` returns `false`, the policy verification also fails. 3. Improve error messages to assist user in defining correct variable name and type. 4. Add unit tests to validate behavior and prevent breaking changes in the future. Signed-off-by: Luiz Carvalho --- pkg/cosign/rego/rego.go | 23 +++++--- pkg/cosign/rego/rego_test.go | 100 +++++++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 8 deletions(-) create mode 100644 pkg/cosign/rego/rego_test.go diff --git a/pkg/cosign/rego/rego.go b/pkg/cosign/rego/rego.go index e6fc00237c8..f3def4dcc76 100644 --- a/pkg/cosign/rego/rego.go +++ b/pkg/cosign/rego/rego.go @@ -24,11 +24,16 @@ import ( "github.com/open-policy-agent/opa/rego" ) +// The query below should meet the following requirements: +// * Provides no Bindings. Do not use a query that sets a variable, e.g. x := data.signature.allow +// * Queries for a single value. +const QUERY = "data.signature.allow" + func ValidateJSON(jsonBody []byte, entrypoints []string) []error { ctx := context.Background() r := rego.New( - rego.Query("data.signature.deny"), // hardcoded, ? data.cosign.allow→ + rego.Query(QUERY), rego.Load(entrypoints, nil)) query, err := r.PrepareForEval(ctx) @@ -48,6 +53,8 @@ func ValidateJSON(jsonBody []byte, entrypoints []string) []error { return []error{err} } + // Ensure the resultset contains a single result where the Expression contains a single value + // which is true and there are no Bindings. if rs.Allowed() { return nil } @@ -55,14 +62,14 @@ func ValidateJSON(jsonBody []byte, entrypoints []string) []error { var errs []error for _, result := range rs { for _, expression := range result.Expressions { - for _, v := range expression.Value.([]interface{}) { - if s, ok := v.(string); ok { - errs = append(errs, fmt.Errorf(s)) - } else { - errs = append(errs, fmt.Errorf("%s", v)) - } - } + errs = append(errs, fmt.Errorf("expression value, %v, is not true", expression)) } } + + // When rs.Allowed() is not true and len(rs) is 0, the result is undefined. This is a policy + // check failure. + if len(errs) == 0 { + errs = append(errs, fmt.Errorf("result is undefined for query '%s'", QUERY)) + } return errs } diff --git a/pkg/cosign/rego/rego_test.go b/pkg/cosign/rego/rego_test.go new file mode 100644 index 00000000000..875249594a1 --- /dev/null +++ b/pkg/cosign/rego/rego_test.go @@ -0,0 +1,100 @@ +// +// Copyright 2022 The Sigstore Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package rego + +import ( + "fmt" + "os" + "testing" +) + +const simpleJSONBody = `{ + "_type": "https://in-toto.io/Statement/v0.1", + "predicateType": "https://slsa.dev/provenance/v0.2" +}` + +func TestValidationJSON(t *testing.T) { + cases := []struct { + name string + jsonBody string + policy string + pass bool + errors []string + }{ + { + name: "passing policy", + jsonBody: simpleJSONBody, + policy: ` + package signature + + allow { + input.predicateType == "https://slsa.dev/provenance/v0.2" + } + `, + pass: true, + }, + { + name: "undefined result due to no matching rules", + jsonBody: simpleJSONBody, + policy: ` + package signature + + allow { + input.predicateType == "https://slsa.dev/provenance/v99.9" + } + `, + pass: false, + errors: []string{"result is undefined for query 'data.signature.allow'"}, + }, + { + name: "policy query evaluates to false", + jsonBody: simpleJSONBody, + policy: ` + package signature + + default allow = false + `, + pass: false, + errors: []string{"expression value, false, is not true"}, + }, + } + + for _, tt := range cases { + t.Run(tt.name, func(t *testing.T) { + // Do not use t.TempDir() because rego has issues loading policy files + // from an absolute path on Windows. Use a relative path instead. See: + // https://github.com/open-policy-agent/opa/issues/4521 + policyFileName := "tmp-policy.rego" + if err := os.WriteFile(policyFileName, []byte(tt.policy), 0644); err != nil { + t.Fatal(err) + } + defer os.Remove(policyFileName) + + if errs := ValidateJSON([]byte(tt.jsonBody), []string{policyFileName}); (errs == nil) != tt.pass { + t.Fatalf("Unexpected result: %v", errs) + } else if errs != nil { + if len(errs) != len(tt.errors) { + t.Fatalf("Expected %d errors, got %d errors: %v", len(tt.errors), len(errs), errs) + } + for i, err := range errs { + if fmt.Sprintf("%s", err) != tt.errors[i] { + t.Errorf("Expected error %q, got %q", tt.errors[i], err) + } + } + } + }) + } +}