Skip to content

feat(authzen): report data freshness and a decision id in evaluation responses - #224

Open
leifj wants to merge 8 commits into
mainfrom
feat/221-trust-data
Open

leifj wants to merge 8 commits into
mainfrom
feat/221-trust-data

Conversation

@leifj

@leifj leifj commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

Implements #221.

What

  • context.trust_data[] in POST /evaluation responses: {registry, as_of, next_update}, one entry per leaf registry that could have contributed to the decision. Composites contribute their children.
  • context.id: a unique id per decision (also on early denials), for audit correlation.
  • Optional registry.FreshnessReporter interface, implemented by vical, mdocrical, etsi and lote (earliest NextUpdate across loaded lists). Registries without it (static, whitelist, ...) are listed with no as_of/next_update: callers must read that as unknown.

Design notes

  • Attached once in RegistryManager.Evaluate, so all four strategies are covered without per-strategy plumbing. It is attached on a copy; a registry's own response is never mutated.
  • The reported set is every policy-applicable registry, not only those that answered. For a first-match allow that can only make the reported bound earlier, never later.
  • Denials carry trust_data too: a denial can flip when a list is replaced.
  • Unparseable source dates are unknown, never guessed.
  • Additive: existing clients ignore the new members. cache_hint is deliberately left out until trust_data has a consumer.

Tests

Manager tests cover all strategies, composite flattening, denials, id uniqueness, early denial, and the no-mutation guarantee (they fail with the hook disabled). Each reporting registry has its own test.

oidfed and walletcompat tests need network access and fail/time out in my sandbox; CI is the reference.

🤖 Generated with Claude Code

…responses

Add context.trust_data[] (registry, as_of, next_update), one entry per leaf
registry that could have contributed to the decision, so callers that cache
decisions can bound how long they keep them. Also set a unique context.id
per decision for audit correlation.

Registries opt in via the optional FreshnessReporter interface; vical,
mdocrical, etsi and lote implement it (earliest NextUpdate across loaded
lists). Registries without it are listed with no as_of/next_update, which
callers must read as unknown. Unparseable source dates are unknown, never
guessed.

Attached once in RegistryManager.Evaluate so every strategy is covered, on a
copy of the response so a registry's own response is never mutated.

Refs #221

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 26, 2026 12:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Freshness metadata can overstate safe cache lifetimes, and preserved registry IDs can break per-decision uniqueness.

4 open issues
What changed in this PR

Adds freshness metadata and decision IDs to AuthZEN evaluation responses, helping clients bound cached decisions and correlate them with audits.

Changes:

  • Adds context.trust_data and manager-level decision ID assignment.
  • Introduces optional freshness reporting for VICAL, RICAL, ETSI, and LoTE registries, including composite flattening.
  • Adds tests for reporting, strategy coverage, denials, ID uniqueness, and response copying.
File Description
pkg/​registry/​vical/​registry.go Reports VICAL fetch time and next update.
pkg/​registry/​vical/​freshness_test.go Tests VICAL freshness reporting.
pkg/​registry/​mdocrical/​registry.go Reports RICAL fetch time and next update.
pkg/​registry/​mdocrical/​freshness_test.go Tests RICAL freshness reporting.
pkg/​registry/​manager.go Attaches freshness metadata and decision IDs.
pkg/​registry/​lote/​registry.go Aggregates loaded LoTE freshness.
pkg/​registry/​lote/​freshness_test.go Tests LoTE freshness aggregation.
pkg/​registry/​freshness.go Defines reporting, parsing, and attachment helpers.
pkg/​registry/​freshness_test.go Tests manager integration and freshness helpers.
pkg/​registry/​etsi/​registry.go Aggregates loaded TSL freshness.
pkg/​registry/​etsi/​freshness_test.go Tests TSL freshness aggregation.
pkg/​authzen/​types.go Defines the additive trust-data response fields.

Review effort: Balanced

Comment thread pkg/registry/etsi/registry.go Resolved
Comment thread pkg/registry/lote/registry.go Resolved
Comment thread pkg/registry/manager.go Resolved
Comment thread pkg/registry/manager.go Resolved
- etsi/lote: next_update is unknown if any loaded list has no usable date,
  instead of another list's date (EarliestKnown replaces EarliestNonZero).
- manager: snapshot freshness before and after evaluation and report an
  entry only when they agree, so a registry reloading mid-evaluation cannot
  attach the newer list's next_update to a decision made from the older one.
- manager: always generate context.id, even when a registry's response
  already carries one.
- cmd/gt: the response-equivalence test ignores the per-decision id.

Refs #221

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Freshness reporting can advertise unsupported cache deadlines by omitting contributing sources and assuming UTC for timezone-free dates.

No open issues 🎉

4 previously missed issues

In code that hasn't changed since last review

Medium severity Report unknown freshness when PEM bundle contributes

pkg/​registry/​etsi/​registry.go:1070

When CertBundle is configured alongside TSL sources, load() merges both into r.certPool, and an unfiltered evaluation can allow solely because of a PEM-bundle certificate. This aggregation only considers r.tsls, so it reports a TSL's next_update even though the contributing bundle has no known update bound. Treat the bundle as an unknown-freshness contributor, or track which source actually authorized the decision, so callers apply their unknown-freshness cap.

Medium severity Reject timezone-free timestamps instead of assuming UTC

pkg/​registry/​freshness.go:34

The two timezone-free layouts silently assume UTC: 2026-10-26T00:00:00 becomes 2026-10-26T00:00:00Z, although the source supplied no offset. That invents a deadline and can report a later next_update than the actual source time, allowing consumers to cache too long. Require an explicit timezone unless the source format defines one; otherwise return unknown, and update the date-only test accordingly.

Medium severity Report unknown freshness for National Register fallback

pkg/​registry/​lote/​registry.go:401

When RegisterClient is configured, Evaluate can allow an entity absent from every LoTE using the National Register fallback. This method still reports the LoTEs' load time and next_update for that decision, although the register data has no freshness contract. Return unknown when fallback may contribute, or carry freshness from the actual register result instead of assigning it an unrelated LoTE deadline.

Medium severity Include parent and nested LoTL deadlines in aggregate

pkg/​registry/​lote/​registry.go:408

For LoTLSources, resolveLoTL retains only the discovered child LoTEs in r.lotes, so this deadline excludes the parent and any nested LoTL. A parent due tomorrow can remove a child's pointer while this reports the child's next_update next month. Retain and include every followed LoTL's NextUpdate in the aggregate, treating an undated parent as unknown too.

4 resolved since last review

Review effort: Balanced

- ParseSourceTime requires an explicit UTC offset; offset-free and
  date-only values are unknown instead of assumed UTC.
- etsi: a configured PEM bundle contributes trust anchors with no update
  bound, so next_update is unknown when one is configured.
- lote: the LoTLs followed to discover LoTEs (nested ones included) bound
  the registry, and an undated one makes it unknown; a configured National
  Register fallback also makes next_update unknown.
- vical/mdocrical share registry.NewFreshness (Sonar duplication gate).

Refs #221

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@leifj

leifj commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the four items from the latest review body (they were not inline threads), in 7455395:

  • PEM bundle (etsi): a configured CertBundle is an unknown-freshness contributor, so next_update is omitted when one is set.
  • Timezone-free timestamps: ParseSourceTime now requires an explicit offset; offset-free and date-only values are unknown. Test updated.
  • National Register fallback (lote): next_update is unknown when a RegisterClient is configured.
  • Parent/nested LoTL deadlines (lote): every followed LoTL's NextUpdate is recorded per refresh and included in the aggregate; an undated one makes it unknown. End-to-end test through refresh() covers parent-earlier, nested-earlier and undated-parent.

Also folded the duplicated vical/mdocrical reporter code into registry.NewFreshness for the Sonar duplication gate. Each new behaviour has a test that fails with the fix disabled.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Snapshot precision loss and incomplete failure tracking can produce misleading freshness bounds for cached decisions.

1 open issue
2 previously missed issues

In code that hasn't changed since last review

Medium severity Preserve nanosecond precision in AsOf freshness comparisons

pkg/​registry/​freshness.go:74

Formatting AsOf to whole seconds can hide reloads from the before/after comparison. If versions A → B → C load within one second and A and C share a NextUpdate, a decision made from B can receive C's later deadline instead of unknown freshness. Preserve nanoseconds in AsOf and add a regression test for this schedule.

Medium severity Mark expired cache fetch failures as unknown freshness

pkg/​registry/​manager.go:273

When a VICAL/RICAL cache expires and fetching fails, the registry returns a denial with no Go error while leaving the old cache intact. Both freshness snapshots then match, so the denial receives the old list's potentially future next_update, although it can flip as soon as fetching recovers. Propagate operational-failure status through evaluation and leave the affected freshness bounds unknown.

Review effort: Balanced

Comment thread pkg/registry/lote/registry.go Resolved
- lote: a LoTL pointer target that fails to load makes next_update unknown.
- vical/mdocrical: an expired cache reports nothing (CachedFreshness), since
  a failed refetch keeps the old list and denies.
- Compare freshness snapshots at full precision, not as rounded strings.
- mdocrical reporter written in the file's existing cache-read style.

Refs #221

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@leifj

leifj commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Also addressed, in af1629e, the two items from the latest review body:

  • Precision: before/after freshness snapshots are now compared at full time precision, not as second-rounded strings (test: a 1ms reload with an unchanged NextUpdate yields unknown).
  • Expired VICAL/RICAL cache: an expired cache now reports no freshness (registry.CachedFreshness), since a failed refetch keeps the old list and denies. Tests added for both registries.

Sonar duplication: the mdocrical reporter is now written in that file's existing cache-read style rather than as a copy of vical's.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Transient evaluation failures and negative cache lifetimes can attach misleading future freshness dates to denials.

1 open issue
1 previously missed issue

In code that hasn't changed since last review

Medium severity Avoid freshness dates for failed or circuit-skipped denials

pkg/​registry/​manager.go:273

The filtered strategies return a denial with a nil error when registries fail or are skipped by an open circuit, so this gate can attach a future next_update despite no trust-data evaluation succeeding. With unchanged freshness snapshots, that denial receives a date even though it can become an allow as soon as the circuit recovers, without a list update. Carry evaluation status into freshness attachment, leave dates unknown for failed or circuit-skipped entries on these denials, and cover an open circuit in a regression test.

1 resolved since last review

Review effort: Balanced

Comment thread pkg/registry/freshness.go Resolved
…TL edge

- A denial in which a registry errored, panicked, was skipped by an open
  circuit breaker or gave no decision reports that registry (and, for a
  composite, every leaf under it) with unknown freshness: the denial can
  become an allow without any list changing. Allows are unaffected.
- A zero or negative CacheTTL is always expired, matching getVical/getRical.
- vical/mdocrical share registry.CacheFreshness (generic), removing the
  duplicated reporter code.

Refs #221

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@leifj

leifj commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Review-body item (failed or circuit-skipped denials) addressed in 3eabba0: when a denial includes a registry that errored, panicked, was skipped by an open circuit, or gave no decision, that registry (every leaf, for a composite) is reported with no as_of/next_update. Allows are unaffected, since an allow stands on the registry that allowed it. Tests cover first_match, all and sequential, an open circuit, and an open composite; they fail with the change reverted. The vical/mdocrical reporter code now shares one generic helper (registry.CacheFreshness) for the Sonar duplication gate.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Circuit-skipped registries and failing composite children can still report misleading freshness deadlines for transient denials.

No open issues 🎉

2 previously missed issues

In code that hasn't changed since last review

Medium severity Report failures for open-circuit registries in filtered evaluation

pkg/​registry/​freshness.go:131

Under AllRegistries and BestMatch, evaluateAllFiltered skips an open-circuit registry without emitting a result, so this scan never marks it as failed. A denial can then advertise that registry's future next_update even though recovery can change the decision without any data update. Emit an explicit failure result for circuit skips in strategies.go:723–726, and extend the circuit-skip regression to both strategies.

Medium severity Propagate nested composite failures to prevent stale freshness

pkg/​registry/​freshness.go:135

A composite child error is stored under reason.details/error_count, while the composite returns decision:false with no outer error, so this check treats it as a genuine denial. An OR composite with a temporarily failing child can therefore report a future next_update even though recovery can change the decision without any data update. Preserve failure information through nested composites and use it here to report unknown freshness for the affected leaves or subtree, with a child-error regression test.

1 resolved since last review

Review effort: Balanced

…posites

- AllRegistries/BestMatch (filtered and unfiltered) now record a result for
  a registry skipped by an open circuit breaker, as first_match and
  sequential already did, instead of dropping it silently.
- A composite marks its reason "incomplete" when a child failed or itself
  reports an incomplete result, so the failure survives nesting.
- Freshness reporting treats an incomplete composite denial as failed, and
  reports unknown for every leaf under it.

Refs #221

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@leifj

leifj commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Both items from the latest review body addressed in c5bcda1:

  • Open circuit under all/best_match: the filtered and unfiltered all-registries paths now record a failure result for a registry skipped by an open circuit (as first_match and sequential already did), so the denial reports it with unknown freshness. Regression test covers both strategies.
  • Nested composite failures: a composite now marks its reason incomplete when a child failed or itself reports incomplete, so the failure survives nesting, and freshness reporting reports unknown for every leaf under such a composite. Tests cover a direct child failure, a nested one, and a healthy composite that keeps its bound.
    Each new test fails with its fix reverted.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Nil child responses in composites can leave misleading known freshness deadlines on incomplete denials.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Medium severity Treat nil child responses as incomplete in OR composites

pkg/​registry/​composite.go:192

A child returning (nil, nil) leaves incomplete false, so an OR composite can deny without a child decision while still reporting that child's future next_update. That denial can change as soon as the child responds, without its data changing, making the reported cache bound misleading. Treat nil child responses as incomplete and add a regression test for a freshness-reporting child that returns no response.

Review effort: Balanced

- A composite child that returns no response (nil, nil) makes the composite
  denial incomplete, so freshness is reported unknown for it.
- Circuit-skip results use a shared errCircuitOpen instead of repeating the
  block; the mdocrical reporter uses a method value, not a copy of vical's
  closure (Sonar duplication gate).

Refs #221

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@leifj

leifj commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed in 5a7cd8a: a composite child that returns no response (nil, nil) now makes the composite denial incomplete, so every leaf under it reports unknown freshness; regression test added (fails with the change reverted). Also removed the duplicated circuit-skip block I had introduced in strategies.go and reshaped the mdocrical reporter, for the Sonar duplication gate.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Freshness metadata affects cached authorization across concurrent refreshes and partial failures, requiring final human validation and CI confirmation.

No open issues 🎉

Review effort: Balanced

…of evaluate

Behaviour unchanged. Brings RegistryManager.evaluate's cognitive complexity
down (Sonar go:S3776) after the freshness hooks were added to it.

Refs #221

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No unresolved blocking issues remain, and regression coverage addresses the principal freshness, failure-handling and decision-ID edge cases.

No open issues 🎉

Review effort: Balanced

@leifj
leifj requested a review from Wicpar October 26, 2026 14:37
@leifj

leifj commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

@Wicpar could you review this when you have a moment? It implements #221 (freshness in evaluation responses). CI is green apart from the Go vulnerability check, which is a Go standard library issue that also fails on main, and Copilot's latest review recommends approval.

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.

2 participants