Skip to content

fix(policy): reject unknown endpoint security modes - #3187

Open
2000krysztof wants to merge 2 commits into
NVIDIA:mainfrom
2000krysztof:fix/fail-closed-policy-enums
Open

fix(policy): reject unknown endpoint security modes#3187
2000krysztof wants to merge 2 commits into
NVIDIA:mainfrom
2000krysztof:fix/fail-closed-policy-enums

Conversation

@2000krysztof

@2000krysztof 2000krysztof commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

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

  • Added shared validation for endpoint TLS, enforcement, and access values.
  • Rejects unknown values with actionable, field-level errors.
  • Applies validation across sandbox policies, policy updates, provider profiles, and merge operations.
  • Prevents malformed enforcement values such as enforc from falling back to audit mode.
  • Added defensive runtime rejection for invalid endpoint modes.
  • Returns gRPC INVALID_ARGUMENT before invalid policies are persisted or activated.
  • Replaced public protobuf strings for TLS, enforcement, and access with typed enums.
  • Updated generated Go bindings and SDK conversions to use the new enum types.
  • Preserved the documented YAML spellings for compatibility.
  • Added regression coverage across policy, provider-profile, gateway, SDK, and runtime paths.
  • Documented accepted values and fail-closed behavior.

Testing

  • mise run pre-commit passes
  • mise run test passes
  • Unit tests added/updated
  • E2E tests added/updated (not applicable)
  • Manually verified sandbox creation and live policy updates using the Podman gateway
  • Confirmed malformed TLS, enforcement, and access values are rejected
  • Confirmed rejected updates do not alter the active policy

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

@copy-pr-bot

copy-pr-bot Bot commented Sep 4, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@johntmyers
johntmyers marked this pull request as draft September 4, 2026 18:41
@2000krysztof
2000krysztof marked this pull request as ready for review September 7, 2026 11:03
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>
@2000krysztof
2000krysztof force-pushed the fix/fail-closed-policy-enums branch from 267b665 to c1c3bdd Compare September 7, 2026 11:43

@gmenher gmenher 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.

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?

Comment thread proto/sandbox.proto
Comment thread crates/openshell-policy/src/l7_validate.rs
@2000krysztof

Copy link
Copy Markdown
Contributor Author

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?

@2000krysztof
2000krysztof force-pushed the fix/fail-closed-policy-enums branch from c1c3bdd to 4d2a055 Compare September 9, 2026 17:59

@johntmyers johntmyers left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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:e2e is 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

Comment thread proto/sandbox.proto
@johntmyers johntmyers added gator:in-review Gator is reviewing or awaiting PR review feedback test:e2e Requires end-to-end coverage labels Sep 9, 2026
@github-actions

Copy link
Copy Markdown

Label test:e2e applied, but pull-request/3187 does not exist yet. A maintainer needs to comment /ok to test 4d2a055ad370c710b0e857a069b369c5d4bf1a54 to mirror this PR. Once the mirror exists, re-apply the label or re-run Branch E2E Checks from the Actions tab.

@johntmyers

Copy link
Copy Markdown
Collaborator

/ok to test 4d2a055

@johntmyers johntmyers added gator:watch-pipeline Gator is monitoring PR CI/CD status gator:in-review Gator is reviewing or awaiting PR review feedback and removed gator:in-review Gator is reviewing or awaiting PR review feedback gator:watch-pipeline Gator is monitoring PR CI/CD status labels Sep 10, 2026
@johntmyers

Copy link
Copy Markdown
Collaborator

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?

agreed that legacy support isn't required

@2000krysztof
2000krysztof force-pushed the fix/fail-closed-policy-enums branch from 4d2a055 to cc50f9e Compare September 10, 2026 13:30
@johntmyers

Copy link
Copy Markdown
Collaborator

/ok to test cc50f9e

@johntmyers johntmyers added test:e2e Requires end-to-end coverage and removed test:e2e Requires end-to-end coverage labels Sep 10, 2026
@github-actions

Copy link
Copy Markdown

Label test:e2e applied for cc50f9e. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

@johntmyers johntmyers left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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:e2e is applied; mirror is current; E2E Label Help requires rerunning current-head run 34484956640, 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 johntmyers removed the gator:in-review Gator is reviewing or awaiting PR review feedback label Sep 10, 2026
@johntmyers johntmyers added gator:watch-pipeline Gator is monitoring PR CI/CD status gator:in-review Gator is reviewing or awaiting PR review feedback and removed gator:in-review Gator is reviewing or awaiting PR review feedback gator:watch-pipeline Gator is monitoring PR CI/CD status labels Sep 11, 2026
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>
@2000krysztof
2000krysztof force-pushed the fix/fail-closed-policy-enums branch from cc50f9e to 552b3c5 Compare September 11, 2026 11:07
@2000krysztof

Copy link
Copy Markdown
Contributor Author

I pushed a new commit to fix the failing E2E tests.

@johntmyers

Copy link
Copy Markdown
Collaborator

/ok to test 552b3c5

@johntmyers johntmyers left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

gator-agent

PR Review Status

Thanks @2000krysztof. I reviewed the E2E-only delta from cc50f9ea to 552b3c51; replacing the stale string assignments with generated protobuf enum constants addresses the reported test construction failure and introduces no new blocking findings. @johntmyers's pre-0.1 compatibility waiver remains honored.

Blocking findings:

  • No blocking code findings remain.

Carried findings:

  • GATOR-4d2a055a-01: waived by maintainer @johntmyers and its review thread remains 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, Helm Lint, and E2E gates are pending dispatch.
  • E2E: test:e2e is applied and /ok to test 552b3c51fa9daca2af43701f1258f4d9333588b8 was posted; the current-head mirror and required workflows are not yet confirmed queued.
  • Head SHA: 552b3c51fa9daca2af43701f1258f4d9333588b8
  • Base SHA: 9b4b63ec693ceda9f682c699c3329616e33d46d6
  • Merge base SHA: 320d4ef79dd572c642133f175f12bafc20d89fd9
  • Patch ID: 5a4dadb3378a7321bec3bb04c785aaacf2441fa4
  • Gator payload: 8
  • Review mode: follow_up
  • Previous reviewed SHA: cc50f9ea6fec00d42f6d33274558fee8ce1d3619
  • Review budget exhausted: no
  • Maintainer decision required: no
  • Next state: gator:in-review

@johntmyers johntmyers added gator:watch-pipeline Gator is monitoring PR CI/CD status gator:blocked Gator is blocked by process or repository gates gator:approval-needed Gator completed review; maintainer approval needed and removed gator:in-review Gator is reviewing or awaiting PR review feedback gator:watch-pipeline Gator is monitoring PR CI/CD status gator:blocked Gator is blocked by process or repository gates gator:approval-needed Gator completed review; maintainer approval needed labels Sep 11, 2026
@johntmyers
johntmyers added this pull request to the merge queue Sep 11, 2026
@johntmyers johntmyers added gator:merge-ready and removed gator:approval-needed Gator completed review; maintainer approval needed labels Sep 11, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gator:blocked Gator is blocked by process or repository gates test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(policy)!: make security-sensitive policy values fail closed

3 participants