fix(EC-1993): reject past --effective-time values by default#3424
fix(EC-1993): reject past --effective-time values by default#3424cuipinghuo wants to merge 1 commit into
Conversation
|
🤖 Finished Review · ✅ Success · Started 6:26 PM UTC · Completed 6:31 PM UTC |
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThe change adds ChangesPast effective-time allowance
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TektonTask
participant CLI
participant Policy
participant EffectiveTimeParser
TektonTask->>CLI: pass --allow-past-effective-time
CLI->>Policy: provide allowance and effective time
Policy->>EffectiveTimeParser: parse and validate timestamp
EffectiveTimeParser-->>Policy: accept or reject timestamp
Policy-->>CLI: return policy or parsing result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ReviewFindingsHigh
Medium
Low
Previous runReviewFindingsHigh
Medium
Low
Previous run (2)ReviewFindingsMedium
Labels: PR is a security fix (EC-1993, Red Team findings F7-1/F7-2) that modifies CLI command implementations Previous run (3)Review — commentPR: fix(EC-1993): reject past --effective-time values by default SummaryThis PR adds a security mitigation to prevent users from backdating The core logic in Findings1. Duplicate effective-time validation in
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
3c390e7 to
b09a732
Compare
|
🤖 Finished Review · ✅ Success · Started 1:22 PM UTC · Completed 1:43 PM UTC |
b09a732 to
68ae46f
Compare
|
🤖 Review · ❌ Terminated · Started 5:46 PM UTC · Ended 6:08 PM UTC |
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
removed
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 5:46 PM UTC · Completed 6:08 PM UTC |
68ae46f to
4c43579
Compare
|
🤖 Review · |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/policy/policy.go (1)
86-96: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStruct field alignment broken by rename (fails
gofmt).
chosenTime(renamed fromchoosenTime) is now misaligned relative to the other struct fields, which is why the formatter check fails at line 89.🔧 Proposed fix
type policy struct { ecc.EnterpriseContractPolicySpec checkOpts *cosign.CheckOpts - chosenTime string + chosenTime string effectiveTime *time.Time attestationTime *time.Time identity cosign.Identity ignoreRekor bool skipImageSigCheck bool skipAttSigCheck bool }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/policy/policy.go` around lines 86 - 96, Align the chosenTime field declaration in the policy struct with the surrounding fields and run gofmt on the file, preserving the corrected spelling from choosenTime.Source: Linters/SAST tools
🧹 Nitpick comments (1)
internal/policy/policy_test.go (1)
247-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover default-deny propagation through constructors. All historical-time constructor tests force allowance on. Parser tests alone cannot detect a wrapper that ignores or misroutes the flag.
internal/policy/policy_test.go#L247-L253: add a past-timeNewPolicycase withAllowPastEffectiveTime: falsethat expects rejection.internal/policy/policy_test.go#L1206-L1206: parameterize allowance and testNewInputPolicyrejects a past timestamp when false.internal/policy/policy_test.go#L1377-L1438: add aPreProcessPolicydefault-deny case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/policy/policy_test.go` around lines 247 - 253, Add default-deny constructor coverage in internal/policy/policy_test.go: in the NewPolicy case around lines 247-253, add AllowPastEffectiveTime: false and assert past-time creation is rejected; parameterize the NewInputPolicy test around line 1206 to verify rejection when allowance is false; and add a corresponding PreProcessPolicy default-deny case in lines 1377-1438, while preserving the existing allow=true cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/policy/policy_test.go`:
- Line 801: Restore gofmt formatting for the chosenTime field in the affected
test struct, aligning it with the surrounding fields, then run gofmt on the file
to ensure the formatter check passes.
In `@internal/policy/policy.go`:
- Around line 446-482: Update parseEffectiveTime so both time.Parse failures are
retained instead of shadowing the outer err variable. Use distinct parse-error
assignments or otherwise preserve the RFC3339 error before joining the
DateFormat error, and return the combined errors in the existing invalid policy
time message.
---
Outside diff comments:
In `@internal/policy/policy.go`:
- Around line 86-96: Align the chosenTime field declaration in the policy struct
with the surrounding fields and run gofmt on the file, preserving the corrected
spelling from choosenTime.
---
Nitpick comments:
In `@internal/policy/policy_test.go`:
- Around line 247-253: Add default-deny constructor coverage in
internal/policy/policy_test.go: in the NewPolicy case around lines 247-253, add
AllowPastEffectiveTime: false and assert past-time creation is rejected;
parameterize the NewInputPolicy test around line 1206 to verify rejection when
allowance is false; and add a corresponding PreProcessPolicy default-deny case
in lines 1377-1438, while preserving the existing allow=true cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c7f3ee75-86e1-46e5-8659-606bb809fa74
📒 Files selected for processing (17)
cmd/compare/compare.gocmd/validate/image.gocmd/validate/image_test.gocmd/validate/input.godocs/modules/ROOT/pages/ec_compare.adocdocs/modules/ROOT/pages/ec_validate_image.adocdocs/modules/ROOT/pages/ec_validate_input.adocdocs/modules/ROOT/pages/verify-conforma-konflux-ta.adocdocs/modules/ROOT/pages/verify-enterprise-contract.adocfeatures/task_validate_image.featurefeatures/validate_image.featureinternal/input/report_test.gointernal/input/validate_test.gointernal/policy/policy.gointernal/policy/policy_test.gotasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yamltasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml
🚧 Files skipped from review as they are similar to previous changes (15)
- tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml
- cmd/validate/image_test.go
- docs/modules/ROOT/pages/ec_validate_input.adoc
- tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml
- docs/modules/ROOT/pages/ec_compare.adoc
- internal/input/report_test.go
- cmd/validate/image.go
- features/task_validate_image.feature
- cmd/validate/input.go
- cmd/compare/compare.go
- docs/modules/ROOT/pages/verify-enterprise-contract.adoc
- docs/modules/ROOT/pages/ec_validate_image.adoc
- docs/modules/ROOT/pages/verify-conforma-konflux-ta.adoc
- features/validate_image.feature
- internal/input/validate_test.go
Backdating --effective-time suppresses time-gated security rules at both the CLI level (severity demotion) and the Rego level (when_ns propagation). Reject effective-time values more than 5 minutes in the past unless --allow-past-effective-time is explicitly set. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> What: - Add validation in parseEffectiveTime() to reject times >5min in the past - Add --allow-past-effective-time flag to validate image, validate input, and compare commands - Add ALLOW_PAST_EFFECTIVE_TIME param to Tekton tasks - Export ParseEffectiveTime() from policy package; compare command delegates to it - 5-minute grace period to accommodate clock skew Why: - Setting --effective-time to a past date demotes time-gated rule failures to warnings (CLI level) and shifts the time reference for all Rego rules via when_ns (Rego level), effectively bypassing security checks at the integration test gate where users control IntegrationTestScenario params Tickets: - https://redhat.atlassian.net/browse/EC-1993 - Ref: EC-1843 (Red Team findings F7-1 and F7-2) BREAKING CHANGE: --effective-time no longer accepts dates more than 5 minutes in the past. Users who relied on backdating must now also pass --allow-past-effective-time (CLI) or set ALLOW_PAST_EFFECTIVE_TIME=true (Tekton task param). The default value "now" and "attestation" are unaffected.
4c43579 to
bc38cc2
Compare
|
🤖 Finished Review · ✅ Success · Started 2:02 PM UTC · Completed 2:23 PM UTC |
| @@ -379,6 +383,7 @@ spec: | |||
| --show-successes=true | |||
There was a problem hiding this comment.
[high] breaking-cli
The Tekton tasks (both verify-enterprise-contract and verify-conforma-konflux-ta) are modified in-place at version 0.1 with a breaking behavioral change. Existing pipelines that set EFFECTIVE_TIME to a past date without also setting ALLOW_PAST_EFFECTIVE_TIME=true will start failing. Modifying a shipped task version in-place risks silent breakage for pinned consumers.
Suggested fix: Consider bumping the Tekton task to a new version (e.g. 0.2) so existing pipelines pinned to 0.1 are unaffected, or prominently document the migration path in release notes.
| --show-successes=true | ||
| --show-policy-docs-link=true | ||
| --effective-time="${EFFECTIVE_TIME}" | ||
| --allow-past-effective-time="${ALLOW_PAST_EFFECTIVE_TIME}" |
There was a problem hiding this comment.
[medium] fail-open
ALLOW_PAST_EFFECTIVE_TIME Tekton parameter is type:string with default 'false'. Tekton has no input validation, but Go's strconv.ParseBool rejects invalid strings with an error (fail-closed). Acceptable behavior.
| serverPort int | ||
| effectiveTime string | ||
| allowPastEffectiveTime bool | ||
| filePaths []string |
There was a problem hiding this comment.
[low] code-organization
allowPastEffectiveTime field placement differs between input.go (grouped with effectiveTime) and image.go (alphabetical). Inconsistent ordering convention.
| if err != nil { | ||
| return fmt.Errorf("invalid effective time format: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
[low] edge-case
When --effective-time=attestation is used with compare, ParseEffectiveTime returns a zero time.Time and the IsZero() check falls back to time.Now().UTC(). Pre-existing behavior preserved by this PR.
1 participant
Backdating --effective-time suppresses time-gated security rules at both
the CLI level (severity demotion) and the Rego level (when_ns
propagation). Reject effective-time values more than 5 minutes in the
past unless --allow-past-effective-time is explicitly set.
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
What:
Why:
and shifts the time reference for all Rego rules via when_ns (Rego level), effectively bypassing
security checks at the integration test gate where users control IntegrationTestScenario params
Tickets: