fix(policy): reject unknown endpoint security modes - #3187
Conversation
267b665 to
c1c3bdd
Compare
There was a problem hiding this comment.
Great work on this!! @2000krysztof , the centralization in l7_validate.rs is a clean design and the test coverage there is thorough.
One question: I noticed that sandbox policies are persisted as binary protobuf blobs (encode_to_vec / decode in policy_store.rs). Changing NetworkEndpoint.tls, .enforcement, and .access from string (wire type 2) to enum (wire type 0) means that existing stored blobs with non-empty values for those fields will have those fields silently dropped to 0 (Unspecified) when decoded by the new code, since prost skips fields with a wire type mismatch rather than erroring.
The most sensitive case seems to be tls: skip, which would silently become tls: Unspecified (auto-detect) on upgrade. How is this currently handled for existing deployments?
Good catch I hadn’t called out the persisted wire-format impact. One relevant detail is that this is pre-0.1 work intended to stabilize the contract for the first release, so my assumption was that compatibility with existing development databases isn’t guaranteed. I also checked Prost’s behavior: it returns UnexpectedWireType here rather than silently defaulting the field, so it would fail closed instead of weakening the policy. If we do want to support upgrades from current development deployments, I’m happy to add a migration path. I’m just not sure the additional legacy support is worthwhile before 0.1. What do you think? |
c1c3bdd to
4d2a055
Compare
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @2000krysztof. I checked your explanation of the persisted protobuf wire-format concern: Prost does fail closed with UnexpectedWireType, but that still makes existing non-empty policy records unreadable after an in-place gateway upgrade. One blocking compatibility finding remains.
Action required: preserve readable upgrades for persisted policies, or obtain an explicit maintainer waiver of that compatibility requirement.
Blocking findings:
GATOR-4d2a055a-01: changing established protobuf tags from strings to enums causes existing policy blobs to fail decoding after upgrade.
Carried findings:
- None
Gator metadata
- Validation: Implements the fail-closed security-policy contract in linked issue #3046.
- Docs: Fern policy-schema and provider-profile docs are updated.
- Checks: DCO is green; Branch Checks and Helm Lint are pending on the current head.
- E2E:
test:e2eis required for policy-enforcement behavior and will be dispatched after blocking review feedback is resolved or waived. - Head SHA:
4d2a055ad370c710b0e857a069b369c5d4bf1a54 - Base SHA:
8af79a7f4b68abf09299371f20987fde90667139 - Merge base SHA:
320d4ef79dd572c642133f175f12bafc20d89fd9 - Patch ID:
d0b11f729c98eb3e534292f5774206b0e9686e32 - Gator payload:
8 - Review mode:
initial - Previous reviewed SHA: none
- Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review
johntmyers
added
gator:in-review
|
Label |
|
/ok to test 4d2a055 |
johntmyers
added
gator:watch-pipeline
agreed that legacy support isn't required |
4d2a055 to
cc50f9e
Compare
|
/ok to test cc50f9e |
johntmyers
added
test:e2e
|
Label |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @2000krysztof and @johntmyers. I checked the maintainer-approved pre-0.1 compatibility disposition and reviewed the author-only delta from 4d2a055a to cc50f9ea; it only corrects two generated Go comments and introduces no new blocking findings.
Action required: once current-head Branch E2E run 34484956640 becomes rerunnable, a maintainer must use Re-run all jobs as requested by the E2E Label Help bot.
Blocking findings:
- No blocking code findings remain.
Carried findings:
GATOR-4d2a055a-01: waived by maintainer @johntmyers and its review thread is resolved.
Gator metadata
- Validation: Implements the fail-closed security-policy contract in linked issue #3046.
- Docs: Fern policy-schema and provider-profile docs are updated.
- Checks: DCO is green; current-head Branch Checks and Branch E2E are queued; Helm Lint has a current-head run.
- E2E:
test:e2eis applied; mirror is current; E2E Label Help requires rerunning current-head run34484956640, but GitHub does not allow the rerun while it remains queued. - Head SHA:
cc50f9ea6fec00d42f6d33274558fee8ce1d3619 - Base SHA:
a0814443f19c07102b19ff09d6ead3d3ba59f9c5 - Merge base SHA:
320d4ef79dd572c642133f175f12bafc20d89fd9 - Patch ID:
91a5b1bfa646a264b1b510d4ee3a00533d8797d8 - Gator payload:
8 - Review mode:
follow_up - Previous reviewed SHA:
4d2a055ad370c710b0e857a069b369c5d4bf1a54 - Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:blocked - Blocked reason:
test_dispatch_required
johntmyers
added
gator:blocked
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
I reviewed the author-only rebase delta from c78f132b to dd34d7c1 in critical-only mode. It only updates the expected public-RPC schema hash to account for upstream schema changes and introduces no new Critical defect. @johntmyers's pre-0.1 compatibility waiver remains honored, and the prior Gator thread remains resolved. Current-head Branch Checks and E2E are running.
Blocking findings:
- No blocking code findings remain.
Carried findings:
GATOR-4d2a055a-01: waived by maintainer @johntmyers; its review thread is resolved.
Gator metadata
- Validation: Implements the fail-closed security-policy contract in linked issue #3046.
- Docs: Fern policy-schema and provider-profile docs are updated.
- Checks: Current-head Branch Checks and E2E are running; DCO and Helm Lint are green.
- E2E:
test:e2eis applied;/ok to test dd34d7c1368f18ebfa995509432f517ff82546a4refreshed the mirror; Branch E2E run35250388321is active. - Head SHA:
dd34d7c1368f18ebfa995509432f517ff82546a4 - Base SHA:
f419b9c1dfc2267cfbbf7d3af848f5c5c2715a73 - Merge base SHA:
f419b9c1dfc2267cfbbf7d3af848f5c5c2715a73 - Patch ID:
995a54c2a7499435c54b515878787689b1894950 - Gator payload:
9 - Review mode:
critical_only - Previous reviewed SHA:
c78f132b0e6510ddaa764f16e2ff079df1bc48a2 - Review budget exhausted: yes
- Maintainer decision required: no
- Next state:
gator:watch-pipeline
|
/ok to test d316a0f |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @2000krysztof. I reviewed the author-only rebase delta from dd34d7c1 to d316a0f6 in critical-only mode. It only updates schema inventory counts and fingerprints for upstream schema additions and introduces no new Critical defect. @johntmyers's pre-0.1 compatibility waiver remains honored, and the prior Gator thread remains resolved. Current-head Branch Checks and E2E are running.
Blocking findings:
- No blocking code findings remain.
Carried findings:
GATOR-4d2a055a-01: waived by maintainer @johntmyers; its review thread is resolved.
Gator metadata
- Validation: Implements the fail-closed security-policy contract in linked issue #3046.
- Docs: Fern policy-schema and provider-profile docs are updated.
- Checks: Current-head Branch Checks and E2E are running; DCO and Helm Lint are green.
- E2E:
test:e2eis applied;/ok to test d316a0f64dced088127cc7afd2d8862cd7098c55refreshed the mirror; Branch E2E run35270180959is active. - Head SHA:
d316a0f64dced088127cc7afd2d8862cd7098c55 - Base SHA:
07d4ac5474f927170a7abd2c5edaa317733408a5 - Merge base SHA:
07d4ac5474f927170a7abd2c5edaa317733408a5 - Patch ID:
1b09398d746630c8361be7cbb7f5f2f54321f169 - Gator payload:
9 - Review mode:
critical_only - Previous reviewed SHA:
dd34d7c1368f18ebfa995509432f517ff82546a4 - Review budget exhausted: yes
- Maintainer decision required: no
- Next state:
gator:watch-pipeline
|
/ok to test 55a60ed |
BlockedGator recognizes current head Next action: a maintainer must approve and run Trivy Changes run 35283244868. Gator will resume pipeline monitoring after that workflow is queued. Gator metadata
|
|
/ok to test 7086a84 |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @2000krysztof. I reviewed the author-only delta from d316a0f6 to 7086a84b in critical-only mode. The enum fixture corrections, adaptation to the current L7 target shape, and schema snapshot updates introduce no new Critical defect; @johntmyers's pre-0.1 compatibility waiver remains honored, and the prior Gator thread remains resolved.
Action required: a maintainer must approve and run Trivy Changes run 35288133860. Gator will resume pipeline monitoring after that required workflow is queued.
Blocking findings:
- No blocking code findings remain.
Carried findings:
GATOR-4d2a055a-01: waived by maintainer @johntmyers; its review thread is resolved.
Gator metadata
- Validation: Implements the fail-closed security-policy contract in linked issue #3046.
- Docs: Fern policy-schema and provider-profile docs are updated.
- Checks: Current-head Branch Checks and E2E are running; DCO and Helm Lint are green; Trivy Changes awaits approval.
- E2E:
test:e2eis applied and/ok to test 7086a84bb630a49a105be5272319bea99bf11ca1refreshed the mirror; the current-head Branch E2E workflow is active, and no current-head rerun instruction has been posted by E2E Label Help. - Head SHA:
7086a84bb630a49a105be5272319bea99bf11ca1 - Base SHA:
04146692d9f5805a427c65aaf26252b5b22afadf - Merge base SHA:
04146692d9f5805a427c65aaf26252b5b22afadf - Patch ID:
52024e119c39a62f4016b572256041da339efb0e - Gator payload:
9 - Review mode:
critical_only - Previous reviewed SHA:
d316a0f64dced088127cc7afd2d8862cd7098c55 - Review budget exhausted: yes
- Maintainer decision required: no
- Next state:
gator:blocked - Blocked reason:
required_trivy_approval_needed
|
/ok to test 328f748 |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @2000krysztof. I reviewed the author-only delta from 7086a84b to 328f7484 in critical-only mode. The policy commits remain equivalent except for schema inventory counts and fingerprints updated for upstream schema additions, and no newly introduced Critical defect was found. @johntmyers's pre-0.1 compatibility waiver remains honored, and the prior Gator thread remains resolved.
Action required: a maintainer must approve and run Trivy Changes run 35293107107. Gator will resume pipeline monitoring after that required workflow is queued.
Blocking findings:
- No blocking code findings remain.
Carried findings:
GATOR-4d2a055a-01: waived by maintainer @johntmyers; its review thread is resolved.
Gator metadata
- Validation: Implements the fail-closed security-policy contract in linked issue #3046.
- Docs: Fern policy-schema and provider-profile docs are updated.
- Checks: Current-head Branch Checks and E2E are active; DCO and Helm Lint are green; Trivy Changes awaits approval.
- E2E:
test:e2eis applied and/ok to test 328f7484c1ecfb95a843e72436fab53930f524d8refreshed the mirror; the current-head Branch E2E workflow is active, and no current-head rerun instruction has been posted by E2E Label Help. - Head SHA:
328f7484c1ecfb95a843e72436fab53930f524d8 - Base SHA:
1d010f4187830b56535f35a36b203514634f27aa - Merge base SHA:
1d010f4187830b56535f35a36b203514634f27aa - Patch ID:
661a9f6b310329b2bd0e00dccd8425411e85ac51 - Gator payload:
9 - Review mode:
critical_only - Previous reviewed SHA:
7086a84bb630a49a105be5272319bea99bf11ca1 - Review budget exhausted: yes
- Maintainer decision required: no
- Next state:
gator:blocked - Blocked reason:
required_trivy_approval_needed
|
/ok to test dc43601 |
BlockedGator recognizes current head Next action: a maintainer must approve and run Trivy Changes run 35295983458. Gator will resume pipeline monitoring after that workflow is queued. Gator metadata
|
|
/ok to test 14ae0a8 |
BlockedGator recognizes current head Next action: a maintainer must approve and run Trivy Changes run 35305187084. Gator will resume pipeline monitoring after that workflow is queued. Gator metadata
|
|
/ok to test d6fa2ca |
BlockedGator recognizes current head Next action: a maintainer must approve and run Trivy Changes run 35307228661. Gator will resume pipeline monitoring after that workflow is queued. Gator metadata
|
|
/ok to test 37fa4a4 |
BlockedGator recognizes current head Next action: a maintainer must approve and run Trivy Changes run 35313329805. Gator will resume pipeline monitoring after that workflow is queued. Gator metadata
|
|
/ok to test 636920e |
BlockedGator recognizes current head Next action: a maintainer must approve and run Trivy Changes run 35345200873. Gator will resume pipeline monitoring after that required workflow is queued. Gator metadata
|
|
/ok to test 864e752 |
BlockedGator recognizes current head Next action: a maintainer must approve and run Trivy Changes run 35351051545. Gator will resume pipeline monitoring after that required workflow is queued. Gator metadata
|
|
/ok to test 0b07ae0 |
BlockedGator recognizes current head Next action: a maintainer must approve and run Trivy Changes run 35357380786. Gator will resume pipeline monitoring after that required workflow is queued. Gator metadata
|
Closes NVIDIA#3046 Validate TLS, enforcement, and access values across policy and provider profile ingress, and prevent runtime parsing from falling back to audit for unknown enforcement values. Signed-off-by: Krzysztof Malczuk <kmalczuk@redhat.com>
Replace the public TLS, enforcement, and access strings with protobuf enums and carry the typed values through policy composition, provider profiles, drivers, and runtime conversion. Preserve the documented YAML spellings, reject unknown and invalid numeric enum values consistently, and update generated Go bindings, SDK conversions, tests, and policy documentation. Signed-off-by: Krzysztof Malczuk <kmalczuk@redhat.com>
|
/ok to test cdcb1a3 |
Monitoring CompleteMonitoring is complete because this PR has merged. Final status: the policy fail-closed change was reviewed and merged from head I removed the active Gator metadata
|
3 participants
Summary
Make security-sensitive network policy values fail closed across all policy ingress paths. Replace the public TLS, enforcement, and access strings with typed protobuf enums so invalid values cannot silently weaken enforcement.
Related Issue
Closes #3046
Changes
Testing
mise run pre-commitpassesmise run testpassesChecklist