Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix handling of policy in verify-attestation #1672

Merged
merged 2 commits into from
Mar 30, 2022

Conversation

lcarva
Copy link
Contributor

@lcarva lcarva commented Mar 28, 2022

Summary

The first commit fixes the sanity checks during policy processing. Previously, the code was comparing a predicate URI against a payload type. This is never expected to match.

The code was modifying to extract the actual predicate type from within the payload so the verification can be done successfully.

The second 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 result, the policy verification would pass. The code now explicitly checks for this
    condition and ensures that if ResultSet.Allowed() 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.

Release Note

* Modified rego policy evaluation in the `verify-attestation` sub-command to query for the variable `data.signature.allow`.
* Fixed rego policy evaluation to no longer pass when query result is undefined.
* Fixed rego policy evaluation to no longer pass when `data.signature.deny` is `true`.
* Fixed predicate type sanity checks during policy evaluation in the `verify-attestation` sub-command.

@codecov-commenter
Copy link

Codecov Report

Merging #1672 (eca3731) into main (bdef009) will increase coverage by 1.26%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##             main    #1672      +/-   ##
==========================================
+ Coverage   28.08%   29.34%   +1.26%     
==========================================
  Files         139      141       +2     
  Lines        8025     8372     +347     
==========================================
+ Hits         2254     2457     +203     
- Misses       5523     5646     +123     
- Partials      248      269      +21     
Impacted Files Coverage Δ
cmd/cosign/cli/verify/verify_attestation.go 0.00% <0.00%> (ø)
pkg/cosign/rego/rego.go 72.72% <100.00%> (ø)
pkg/cosign/kubernetes/webhook/validator.go 80.49% <0.00%> (-1.22%) ⬇️
pkg/cosign/tuf/client.go 62.34% <0.00%> (-0.95%) ⬇️
cmd/cosign/cli/verify/verify_blob.go 11.34% <0.00%> (-0.64%) ⬇️
pkg/cosign/common.go 0.00% <0.00%> (ø)
cmd/cosign/cli/sign.go 0.00% <0.00%> (ø)
cmd/cosign/cli/attest.go 0.00% <0.00%> (ø)
cmd/cosign/cli/verify.go 0.00% <0.00%> (ø)
cmd/cosign/cli/commands.go 0.00% <0.00%> (ø)
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bdef009...eca3731. Read the comment docs.

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 <[email protected]>
@lcarva lcarva force-pushed the handle-undefined-rego branch 2 times, most recently from 7969eaf to 89219e2 Compare March 28, 2022 20:59
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 <[email protected]>
@vaikas
Copy link
Contributor

vaikas commented Mar 30, 2022

Thanks @lcarva for doing this! I'm going to merge this so I can start adding the e2e tests in a followup.

@vaikas vaikas merged commit 3b597b9 into sigstore:main Mar 30, 2022
@github-actions github-actions bot added this to the v1.7.0 milestone Mar 30, 2022
mlieberman85 pushed a commit to mlieberman85/cosign that referenced this pull request May 6, 2022
* 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 <[email protected]>

* 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants