Add the PAM access rule evaluation engine and governing-rule resolver#7992
Add the PAM access rule evaluation engine and governing-rule resolver#7992Hinton wants to merge 10 commits into
Conversation
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the PAM access-rule evaluation engine and governing-rule resolver: the Code Review DetailsNo new findings. The two pre-existing review threads cover the notable behaviors:
The |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## pam/access-rule-api #7992 +/- ##
=======================================================
+ Coverage 62.14% 62.20% +0.05%
=======================================================
Files 2301 2308 +7
Lines 100219 100343 +124
Branches 9050 9068 +18
=======================================================
+ Hits 62286 62422 +136
+ Misses 35751 35740 -11
+ Partials 2182 2181 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| public bool Contains(DayOfWeek day, TimeOnly time) | ||
| { | ||
| // AccessWeekday values align with System.DayOfWeek (Sunday = 0), so a direct cast compares correctly. | ||
| if (Days.All(d => (DayOfWeek)d != day)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| return TimeOnly.TryParseExact(From, "HH:mm", CultureInfo.InvariantCulture, DateTimeStyles.None, out var from) | ||
| && TimeOnly.TryParseExact(To, "HH:mm", CultureInfo.InvariantCulture, DateTimeStyles.None, out var to) | ||
| && time >= from && time <= to; | ||
| } |
There was a problem hiding this comment.
❓ QUESTION: Are overnight windows (from later than to) a supported configuration?
Details
Contains matches with time >= from && time <= to, so a window like { "from": "20:00", "to": "04:00" } matches nothing — a night-shift window silently admits no one. The validator (AccessRuleValidator.VisitTimeOfDay) only regex-checks HH:mm and does not reject from > to, so such a rule is accepted at write time but can never auto-approve at evaluation time.
This is fail-closed (safe direction), so it is not a security concern. But if overnight windows are intended to be usable, this needs either split-at-midnight evaluation or a validator rule rejecting from >= to. If overnight windows are intentionally unsupported, rejecting them in the validator would surface the misconfiguration to the admin instead of silently denying.
There was a problem hiding this comment.
@abergs @patriksvensson I suspect we may need to do something here.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| foreach (var cidr in Cidrs) | ||
| { | ||
| if (IPNetwork.TryParse(cidr, out var network) && network.Contains(signals.IpAddress)) | ||
| { | ||
| return AccessEvaluation.Allow; | ||
| } | ||
| } |
| foreach (var cidr in Cidrs) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(cidr) || !IPNetwork.TryParse(cidr, out _)) | ||
| { | ||
| return AccessRuleValidationResult.Invalid($"Invalid CIDR: '{cidr}'."); | ||
| } | ||
| } |
44b46da to
c17c469
Compare
fcbbb6d to
504efaf
Compare
Extract the pure rule-evaluation core from the PAM POC: AccessRuleEngine evaluates a flat list of AccessConditions against request-time AccessSignals, combining results as deny > requires-approval > allow and failing closed on any condition, timezone, or IP it cannot interpret. The engine reads no state and issues no leases. Registered as a singleton (pure and stateless).
Resolve the access rule that governs a cipher for a caller: load the rules on every collection through which the caller reaches the cipher, pick the oldest (earliest creation date, ties broken on id) so selection is deterministic and structural, then evaluate that rule's conditions through the engine to report whether it requires human approval. Malformed stored conditions fail safe to human approval so access never silently auto-approves. Registered scoped; the resolver is the shared entry point the access-request and lease flows build on.
…onverter Close the coverage gaps in the PAM engine slice: - Engine: malformed-but-present CIDR and time-window bounds (fail closed), and matching on a later CIDR/window entry rather than only the first. - Resolver: a governing rule that no longer loads is skipped, both when it is the only candidate (no governance) and when a surviving newer rule takes over; and a time_of_day condition is parsed and evaluated through the resolver. - Port AccessWeekdayJsonConverterTests (source was already on the branch, untested), including the AccessWeekday/DayOfWeek numeric-alignment guard.
Move per-condition evaluation onto each AccessCondition via an abstract Evaluate(AccessSignals). The engine is now only the combiner: it folds the per-condition results (deny > approval > allow, empty allows, null entry fails closed) and no longer knows how any kind decides. Validate conditions through an exhaustive IAccessConditionVisitor rather than a type switch, so adding a kind fails to compile until validation handles it. UnsupportedCondition consequently only guards a null entry now — an unknown kind is rejected at the JSON layer and can't reach evaluation. Also document each condition's JSON wire format.
Move each condition's evaluation branches out of AccessRuleEngineTests into IpAllowlistConditionTests and TimeOfDayConditionTests, and test the combination semantics directly on AccessEvaluation.Combine. AccessRuleEngineTests keeps only the engine's orchestration (delegation, folding, signal forwarding, empty list, null-entry fail-closed), exercised with a stub condition. Also add direct TimeWindow.Contains coverage for the inclusive boundaries (time == from/to) and unparseable bounds, which the engine tests never hit.
AccessRule.Enabled was never checked, so a disabled rule still governed its collections. Because selection is oldest-wins and structural, a disabled but permissive rule (e.g. empty conditions, which auto-grant) could shadow a newer enabled restrictive rule and silently grant access the active rule would have gated. Drop rules with Enabled == false from the candidate set, alongside the existing deleted-rule skip, so a disabled rule stops governing entirely. Pin Enabled in the resolver test fixtures so a governing rule is deterministically enabled (AutoFixture's bool sequence could otherwise disable it), and add coverage for a disabled sole rule (ungoverned) and a disabled oldest rule (newer enabled rule governs).
Give each AccessCondition a Validate() alongside Evaluate() and remove the IAccessConditionVisitor. AccessRuleValidator now delegates per-condition checks to condition.Validate() and only enforces the document-level shape (array, size limit, null-entry guard). TimeWindow gains its own Validate() plus a shared TryParseTime, so Contains and validation use one HH:mm parse rather than a regex that could drift from evaluation. AccessRuleValidationResult moves into the conditions namespace so the condition models do not depend on the service layer.
Now that each condition validates itself, move the per-kind validation assertions to the condition tests: IpAllowlistConditionTests (empty/invalid CIDR), TimeOfDayConditionTests (missing/unknown tz, no windows), and TimeWindowTests (no days, malformed HH:mm bounds). Trim AccessRuleValidatorTests to the validator's remaining job — JSON parse/shape/size, null entry, and delegation — keeping the unknown-weekday-token case as a deserialization concern. Mirrors the earlier relocation of the evaluation tests onto the conditions.
504efaf to
f4a2254
Compare
c17c469 to
ccf6444
Compare
1 participant
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-40525
📔 Objective
Extract the PAM access-rule evaluation engine and the governing-rule resolver from the POC, and reshape the condition model so each condition kind owns its own behavior.
AccessRuleEngine(pure, singleton) — folds a flat list ofAccessConditions into a single decision, combining their results as deny > requires-approval > allow. It reads no state and issues no leases. Each condition now evaluates itself against request-timeAccessSignals; the engine only combines. An empty list allows; a null/uninterpretable entry fails closed.human_approval,ip_allowlist, andtime_of_dayeach implementEvaluate(AccessSignals)and fail closed on their own bad input (empty/unparseable CIDR, unknown/invalid timezone, malformedHH:mmbounds). An unknownkindcan't reach evaluation: it's rejected at JSON deserialization, and validation dispatches through an exhaustiveIAccessConditionVisitor(no type switch), so adding a kind fails to compile until it's handled.GoverningRuleResolver(scoped) — resolves the rule governing a cipher for a caller: load the rules on every collection through which the caller reaches the cipher, pick the oldest (earliest creation date, ties broken on id) so selection is deterministic and structural, then evaluate that rule's conditions through the engine to report whether it requires human approval. Malformed stored conditions fail safe to human approval; a rule that no longer loads (deleted after read) stops governing.The resolver is the shared entry point the upcoming access-request and lease slices both build on, which is why it's folded in here rather than split out.
The base branch already provides
Collection.AccessRuleId, the collection/collection-cipher repositories, and the condition models — this slice adds no persistence, schema, or package changes. It does refactor the existing conditions andAccessRuleValidator(a sharedAccessConditionJsonserializer, visitor-based validation replacing the type switch) and documents the condition enums, fields, and JSON wire formats.⏰ Reminders before review
🦮 Reviewer guidelines
Tests are organized to match where the behavior lives: conditions own their evaluation branches,
AccessEvaluationowns combination, and the engine owns only orchestration.Coverage (61 tests, all green):
Evaluate, folds viaCombine, forwards signals, empty-list allows, null-entry fail-closed (uses a stub condition).AccessEvaluation.Combine(7): precedence (deny > approval > allow) in both orders, empty allows, first-deny short-circuit preserving its reason.IpAllowlistCondition(6): in/out of range, unknown IP, empty list, malformed-but-present CIDR (fail closed), later-CIDR matching.TimeOfDayCondition(14): within/outside window, day-not-listed, configured-timezone (DST), unknown timezone, malformed bounds (fail closed), later-window matching; plusTimeWindow.Containsinclusive boundaries (time == from/to) and unparseable-bounds fail-closed.time_of_dayparsed/evaluated through the shared camelCase deserializer.AccessWeekdayJsonConverter(14): token (de)serialization, case-insensitivity, invalid-token-throws, and theAccessWeekday↔DayOfWeeknumeric-alignment guard thetime_of_daywindow match relies on.