-
Notifications
You must be signed in to change notification settings - Fork 42
Implement support for version-based trusted_task_rules #1609
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
|
These are currently standalone functions, actual integration depends on: #1595 |
policy/lib/tekton/trusted.rego
Outdated
| _is_valid_semver(version) | ||
| parsed_version := _semver_parse(version) | ||
|
|
||
| # Version must match at least one constraint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to understand this comment. Should it say "must match every constraint", since we have an every on the next line?
Or do we mean there must be at least one rule.versions, in which case should we do count(rule.versions) > 0 to make that explicit? Or does every behave that way implicitly, and the comment is to make that more clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, it should say "must match every constraint"
simonbaird
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming we don't have a good reason to re-implement semver compare. Would be interesting to see if the same tests pass.
policy/lib/tekton/trusted.rego
Outdated
| # Compare two parsed semantic versions | ||
| # Takes two parsed version objects from _semver_parse | ||
| # Returns -1 if a < b, 0 if a == b, 1 if a > b | ||
| _semver_compare(a, b) := -1 if { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hold up, I think rego has a builtin for this! https://www.openpolicyagent.org/docs/policy-reference/builtins/semver#builtin-semver-semvercompare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh...I looked for that and didn't find anything 🤦♂️
policy/lib/tekton/trusted.rego
Outdated
| # Extract version/tag from task reference | ||
| # For OCI bundles, this is the tagged_ref (e.g., "0.4" from "oci://registry.io/task:0.4") | ||
| # For git references, there's no version tag, so this function will return false | ||
| "tagged_ref" in object.keys(ref) | ||
| version := ref.tagged_ref |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs to be refactored. See: #1595 (review)
Add semver version constraint checking for Tekton task rules, enabling allow/deny decisions based on task version ranges. Tasks can now be restricted to specific versions using constraints like >=2, <3, >3.1.0, <=v4.2. The implementation includes a complete semver parser supporting major.minor.patch formats (with or without 'v' prefix) and comparison operators (>=, >, <=, <). Version constraints apply only to OCI bundle references with tagged versions. Co-authored-by: Claude Code <[email protected]> Ref: https://issues.redhat.com/browse/EC-1541
f5bd693 to
71564b1
Compare
|
The schema has |
|
Also, when I test it I get some errors. Cursor says it's from the |
I can't reproduce the error (i even added an acceptance test for that, see my last commit), can you clarify which tests are you running? |
I'm just running the cli ec validate image \
--image quay.io/redhat-user-workloads/rhtap-contract-tenant/golden-container/golden-container@sha256:185f6c39e5544479863024565bb7e63c6f2f0547c3ab4ddf99ac9b5755075cc9 \
--policy ./policy.yaml \
-k pub.key \
--ignore-rekor \
--output text \
--strict=false |
The version extracted from the task ref can't be relied upon, since it is easily counterfeitable. Instead, as per ADR decision (see attached link), we are going to extract the version annotation from inside the task manifest, and use that to evaluate the version constraints in the trusted task rules. Ref: https://github.com/konflux-ci/architecture/blob/main/ADR/0054-task-versioning.md#mark-each-meaningful-change-with-a-version-update
This allows for a more exact validation, instead of relying on the 'additionalProperties' attribute
The acceptance test cases cover the case of both trusted tasks and untrusted tasks. The version constraints in the trusted_task_rules are difficult to test at the moment, since they test the version annotation inside the manifest, and NOT the version in the oci ref of a task. Since no version annotations are currently present in any task manifest, the constraints will always evaluate to false (for allow rules) or to true (for deny rules). Co-authored-by: Claude Code <[email protected]>
joejstuart
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works great. The only issue is the performance. A normal Conforma run takes around 20 seconds for one image. With this it takes 60. I think we can merge this, but I'll create a story to handle the performance issue.
Add semver version constraint checking for Tekton task rules, enabling allow/deny decisions based on task version ranges. Tasks can now be restricted to specific versions using constraints like
The implementation includes a complete semver parser supporting major.minor.patch formats (with or without 'v' prefix) and comparison operators (>=, >, <=, <). Version constraints apply only to OCI bundle references with tagged versions.
Co-authored-by: Claude Code [email protected]
Ref: https://issues.redhat.com/browse/EC-1541