auth: Add v9 downgrade property tests - #68916
Conversation
adee0b8 to
86a5496
Compare
fbc7ec1 to
ff5c5bc
Compare
86a5496 to
a72ecc3
Compare
ff5c5bc to
cab44d4
Compare
44a058e to
d9a9250
Compare
d9a9250 to
9a1d26b
Compare
b6af66e to
5c48e12
Compare
e51e750 to
8f90cef
Compare
c3b5c78 to
e63463f
Compare
|
@codex review please |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
e63463f to
ddfa498
Compare
|
@codex review please |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
0358e9c to
d3680c6
Compare
|
These tests look good. The only gap i see is that the PR description states "A pure allow_all role and any client on v19 or later are returned unchanged.", but there is nothing here which actually asserts that, it only asserts downgrades behave correctly. |
@carrithers - thanks for the quick feedback. IMO, the gap you noticed was a PR description bug (fixed) rather than a test gap. Both unchanged paths are already covered by subtests of I think it is fine that those are ordinary table tests rather than property tests, since the discriminating input space is small. |
d3680c6 to
bf0a1db
Compare
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
d44c92c to
d3a062f
Compare
|
@codex review please |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
d3a062f to
4c9f1cc
Compare
|
@codex review please |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
4c9f1cc to
2569fd2
Compare
|
@codex review please |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Exercise `maybeDowngradeRoleVersionToV8` with property tests so the guarantee holds for arbitrary roles, not just the handful of hand-written roles the example test checks. The downgrade is a security control. A pre-v19 agent cannot enforce a v9 role's `app_resources` restriction, so the downgraded v8 copy must move the role's allow app selector to the deny side and never leave one of its own apps reachable. The allow-to-deny move branches on whether a deny selector already exists, which is where a fail-open bug would go unnoticed. Generate random allow and deny label selectors and candidate apps, then assert three properties. Every app the role allowed before the downgrade is denied after. Every app it already denied stays denied. The input role comes back unmodified, because the caller passes the cache's shared role. Add apps matching each selector, and the deny expression, to every generated set, because random apps match a keyed selector in under a tenth of draws and the assertion body is otherwise skipped. Draw globs as selector values, since `services.MatchLabels` matches them as regular expressions and a downgrade that dropped them would serve a copy denying nothing. Cover the AND-to-OR downgrade in a second property, which adds a label expression on each side. An allow rule needs both `app_labels` and `app_labels_expression` to match while a deny rule needs either, so moving them across separately denies more apps than the role allowed. Add an example test for that over-deny, where a wildcard label set moved to the deny side blocks apps outside the allow rule. Use `pgregory.net/rapid`, already a direct dependency of this branch.
2569fd2 to
8852d9a
Compare
|
@codex review please |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Add property tests for
MaybeDowngradeRoleVersionToV8, which serves av8copy of av9role to pre-v19 agents that cannot enforce itsapp_resourcesrestriction. The example testTestMaybeDowngradeRoleVersionToV8from #68736 checks a handful of hand-written roles. The first property covers arbitrary allow and deny label selectors. The second adds a label expression on each side.The two interesting cases, before and after the downgrade:
An
app_labels_expressionchanges in the same way, and is merged with||when the deny side already has one. Both sides'app_resourcesare cleared, the version is set tov8, and a downgrade reason is added to the metadata labels without disturbing the role's own labels.The properties assert:
Each property test adds apps matching the role's selectors to the randomly drawn ones, since random apps rarely match a keyed selector.