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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (17)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 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
Suggested reviewers: 🚥 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 runReviewFindingsMedium
Labels: PR is a security fix (EC-1993, Red Team findings F7-1/F7-2) that modifies CLI command implementations Previous run (2)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 |
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>
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:
|
| @@ -435,6 +457,9 @@ func parseEffectiveTime(choosenTime string) (*time.Time, error) { | |||
| if when, err := time.Parse(time.RFC3339, choosenTime); err == nil { | |||
There was a problem hiding this comment.
[high] breaking-cli
Default behavior of --effective-time changed in backward-incompatible way. Past dates now rejected unless --allow-past-effective-time is passed. Downstream CI/CD pipelines or scripts using --effective-time with past dates will break after CLI upgrade.
Suggested fix: Document as breaking change in release notes and consider bumping the minor/major version. At minimum, release notes must call out the breaking behavior.
| // that case separately. | ||
| func ParseEffectiveTime(choosenTime string, allowPast bool) (time.Time, error) { | ||
| t, err := parseEffectiveTime(choosenTime, allowPast) | ||
| if err != nil { |
There was a problem hiding this comment.
[medium] scope-creep
New exported ParseEffectiveTime() has the same name as existing vsa.ParseEffectiveTime() in internal/validate/vsa/validator.go with different signature and behavior, creating a confusing dual-function situation.
Suggested fix: Document the relationship or file follow-up to unify the two functions.
| type: string | ||
| description: Run policy checks with the provided time. | ||
| default: "now" | ||
| - name: ALLOW_PAST_EFFECTIVE_TIME |
There was a problem hiding this comment.
[medium] breaking-config
New parameter added to existing 0.1 version rather than creating 0.2. Combined with behavioral change, existing pipelines using EFFECTIVE_TIME with past dates will break when they pick up the new task bundle.
Suggested fix: Either introduce as 0.2 version, or set default to 'true' initially for backward compatibility.
| // allowPast is false, times more than 5 minutes in the past are rejected. | ||
| // The "attestation" sentinel returns a zero time.Time — callers should handle | ||
| // that case separately. | ||
| func ParseEffectiveTime(choosenTime string, allowPast bool) (time.Time, error) { |
There was a problem hiding this comment.
[low] naming-convention
Parameter name choosenTime perpetuates existing misspelling in new exported ParseEffectiveTime function. Opportunity to use correct spelling chosenTime.
| @@ -74,6 +76,7 @@ Examples: | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
[low] flag-declaration-style
Flag description for --allow-past-effective-time in compare.go omits 'By default, dates more than 5 minutes in the past are rejected' which is present in image.go and input.go.
| @@ -633,6 +639,7 @@ type imageData struct { | |||
| certificateOIDCIssuer string | |||
There was a problem hiding this comment.
[low] struct-field-ordering
New allowPastEffectiveTime field breaks alphabetical ordering convention in imageData struct.
| // For now, use current time as default for attestation time | ||
| effectiveTimeValue, err := policy.ParseEffectiveTime(effectiveTime, allowPastEffectiveTime) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
[low] api-contract
The 'attestation' sentinel is accepted by --effective-time but not documented in the compare command's help text. Pre-existing condition.
| @@ -84,20 +87,12 @@ Examples: | |||
|
|
|||
| func runCompare(cmd *cobra.Command, args []string) error { | |||
|
|
|||
There was a problem hiding this comment.
[low] scope
Compare command refactoring replaces inline time parsing with ParseEffectiveTime. Behavioral change beyond stated fix scope — previously compare accepted any past time silently.
fullsend-ai-review
Bot
removed
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 5:46 PM UTC · Completed 6:08 PM UTC |
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: