fix: make opa local setup more resilient#331
Open
mckornfield wants to merge 1 commit into
Open
Conversation
Contributor
|
🌿 Preview your docs: https://nvidia-preview-auth-opa-fix-mck.docs.buildwithfern.com/nemo-platform |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (17)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughWalkthroughAdds ChangesEmbedded PDP policy.wasm auto-build
Sequence Diagram(s)sequenceDiagram
participant Runner as platform_runner
participant Server as run_server / run_server_with_reload
participant Preflight as preflight_embedded_auth_policy_wasm
participant EnsureWasm as ensure_embedded_policy_wasm
participant BuildScript as build_policy_wasm.sh
participant AuthService as AuthService.on_startup
Runner->>Server: start
Server->>Preflight: preflight_embedded_auth_policy_wasm(auth_config)
Preflight->>EnsureWasm: ensure_embedded_policy_wasm(auto_build=...)
EnsureWasm->>BuildScript: subprocess.run(build_policy_wasm.sh) [if stale/missing]
BuildScript-->>EnsureWasm: policy.wasm written
EnsureWasm-->>Preflight: wasm_path
Preflight-->>Server: ok
Server->>AuthService: on_startup()
AuthService->>EnsureWasm: ensure_embedded_policy_wasm(auto_build=config.embedded_pdp_auto_build_wasm)
EnsureWasm-->>AuthService: wasm_path
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)services/core/auth/tests/test_policy_wasm.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.14][ERROR]: unable to find a config; path Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/core/auth/src/nmp/core/auth/app/embedded_pdp/engine.py (1)
137-143:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
embedded_pdp_auto_build_wasmis not honored inget_policy().Line 137 calls
ensure_embedded_policy_wasm()before reading config, so this path always behaves asauto_build=Trueeven when users setauth.embedded_pdp_auto_build_wasm: false.Suggested fix
- path = ensure_embedded_policy_wasm() - from nmp.common.config import get_service_config from nmp.core.auth.config import AuthServiceConfig cfg = get_service_config(AuthServiceConfig) + path = ensure_embedded_policy_wasm(auto_build=cfg.embedded_pdp_auto_build_wasm) _policy = OPAPolicy( str(path), fuel_limit=cfg.embedded_pdp_cpu_limit * 1_000_000,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/core/auth/src/nmp/core/auth/app/embedded_pdp/engine.py` around lines 137 - 143, The `get_policy()` method unconditionally calls `ensure_embedded_policy_wasm()` before reading the configuration, which means the `embedded_pdp_auto_build_wasm` setting is ignored. Move the `get_service_config(AuthServiceConfig)` call to execute before the `ensure_embedded_policy_wasm()` invocation, then conditionally call `ensure_embedded_policy_wasm()` only if the `cfg.embedded_pdp_auto_build_wasm` setting is True, so that users who set this config to false are respected.
🧹 Nitpick comments (1)
script/build_policy_wasm.sh (1)
194-194: 💤 Low valueUse
--to end option parsing before the glob.If a
.regofile ever starts with-, it could be misinterpreted as anopa buildoption. Adding--is trivial and eliminates the edge case.Suggested fix
-(cd "${POLICY_DIR}" && "${OPA}" build -t wasm ${ENTRYPOINTS} -o "${BUNDLE_TMP}/bundle.tar.gz" *.rego) +(cd "${POLICY_DIR}" && "${OPA}" build -t wasm ${ENTRYPOINTS} -o "${BUNDLE_TMP}/bundle.tar.gz" -- *.rego)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/build_policy_wasm.sh` at line 194, The opa build command in the POLICY_DIR directory does not use `--` to signal the end of option parsing before the glob pattern `*.rego`. If any .rego file name starts with a hyphen, it will be misinterpreted as an opa build option rather than a file argument. Add `--` immediately before the `*.rego` glob pattern in the opa build command to properly terminate option parsing and ensure all filenames are treated as arguments regardless of their starting character.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/auth/authorization/policy-engine.mdx`:
- Around line 38-44: The configuration example in the policy-engine.mdx file
shows `embedded_pdp_auto_build_wasm: false`, but this contradicts the section's
description of auto-build behavior and the actual code default of `true`. Update
the example value for `embedded_pdp_auto_build_wasm` from `false` to `true` to
align with the documented default behavior and code implementation.
In `@packages/nmp_platform_runner/src/nmp/platform_runner/server.py`:
- Around line 80-86: The try-except block around the import of
ensure_embedded_policy_wasm from nmp.core.auth.app.embedded_pdp.policy_wasm
currently catches all ImportError exceptions, including transitive import
failures inside policy_wasm that should not be remapped to a "package not
installed" message. Modify the exception handler to distinguish between a
missing nmp.core.auth.app.embedded_pdp.policy_wasm module and other import
errors: check if the caught ImportError's name attribute matches the module
being imported (or check the exception message for
nmp.core.auth.app.embedded_pdp.policy_wasm), and only convert that case to the
RuntimeError with the installation guidance; for all other import errors,
re-raise the original exception unchanged to preserve the actual debugging
information.
In `@services/core/auth/src/nmp/core/auth/app/embedded_pdp/policy_wasm.py`:
- Around line 131-139: The subprocess.run call can raise a TimeoutExpired
exception when the timeout is exceeded, but this exception is not being caught
and wrapped, causing it to bypass the policy-wasm error handling. Wrap the
subprocess.run call in a try-except block that catches subprocess.TimeoutExpired
and re-raises it as a PolicyWasmError with an appropriate error message,
ensuring all subprocess execution failures follow the same error path.
---
Outside diff comments:
In `@services/core/auth/src/nmp/core/auth/app/embedded_pdp/engine.py`:
- Around line 137-143: The `get_policy()` method unconditionally calls
`ensure_embedded_policy_wasm()` before reading the configuration, which means
the `embedded_pdp_auto_build_wasm` setting is ignored. Move the
`get_service_config(AuthServiceConfig)` call to execute before the
`ensure_embedded_policy_wasm()` invocation, then conditionally call
`ensure_embedded_policy_wasm()` only if the `cfg.embedded_pdp_auto_build_wasm`
setting is True, so that users who set this config to false are respected.
---
Nitpick comments:
In `@script/build_policy_wasm.sh`:
- Line 194: The opa build command in the POLICY_DIR directory does not use `--`
to signal the end of option parsing before the glob pattern `*.rego`. If any
.rego file name starts with a hyphen, it will be misinterpreted as an opa build
option rather than a file argument. Add `--` immediately before the `*.rego`
glob pattern in the opa build command to properly terminate option parsing and
ensure all filenames are treated as arguments regardless of their starting
character.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4fbe4c5a-fae8-492f-92fe-5949276dc906
📒 Files selected for processing (16)
.github/actions/build-policy-wasm/action.yamldocs/auth/authorization/policy-engine.mdxdocs/auth/deployment/configuration.mdxpackages/nmp_common/src/nmp/common/config/base.pypackages/nmp_platform_runner/src/nmp/platform_runner/run.pypackages/nmp_platform_runner/src/nmp/platform_runner/server.pypackages/nmp_platform_runner/tests/test_run_policy_wasm.pypackages/nmp_platform_runner/tests/test_server.pyscript/build_policy_wasm.shservices/core/auth/src/nmp/core/auth/app/embedded_pdp/__init__.pyservices/core/auth/src/nmp/core/auth/app/embedded_pdp/engine.pyservices/core/auth/src/nmp/core/auth/app/embedded_pdp/policy_wasm.pyservices/core/auth/src/nmp/core/auth/service.pyservices/core/auth/tests/test_config.pyservices/core/auth/tests/test_policy_wasm.pyservices/core/auth/tests/test_service_policy_wasm.py
💤 Files with no reviewable changes (1)
- .github/actions/build-policy-wasm/action.yaml
Comment on lines
38
to
44
| ```yaml | ||
| auth: | ||
| enabled: true | ||
| policy_decision_point_provider: "embedded" | ||
| policy_decision_point_base_url: "http://auth:8000" | ||
| embedded_pdp_auto_build_wasm: false | ||
| policy_data_refresh_interval: 30 # seconds |
There was a problem hiding this comment.
Config example contradicts the documented/default behavior.
This snippet sets embedded_pdp_auto_build_wasm: false, but the section describes auto-build behavior for source checkouts and the code default is true.
Suggested fix
- embedded_pdp_auto_build_wasm: false
+ embedded_pdp_auto_build_wasm: true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ```yaml | |
| auth: | |
| enabled: true | |
| policy_decision_point_provider: "embedded" | |
| policy_decision_point_base_url: "http://auth:8000" | |
| embedded_pdp_auto_build_wasm: false | |
| policy_data_refresh_interval: 30 # seconds |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/auth/authorization/policy-engine.mdx` around lines 38 - 44, The
configuration example in the policy-engine.mdx file shows
`embedded_pdp_auto_build_wasm: false`, but this contradicts the section's
description of auto-build behavior and the actual code default of `true`. Update
the example value for `embedded_pdp_auto_build_wasm` from `false` to `true` to
align with the documented default behavior and code implementation.
Comment on lines
+80
to
+86
| try: | ||
| from nmp.core.auth.app.embedded_pdp.policy_wasm import ensure_embedded_policy_wasm | ||
| except ImportError as exc: | ||
| raise RuntimeError( | ||
| "Auth is enabled with the embedded PDP, but the nmp-auth package is not installed. " | ||
| "Install nmp-auth or set auth.policy_decision_point_provider='opa'." | ||
| ) from exc |
There was a problem hiding this comment.
Narrow the import fallback to true missing-module cases.
Catching ImportError here also captures transitive import failures inside policy_wasm, then reports the wrong remediation (“package not installed”). Only remap missing nmp.core.auth.app.embedded_pdp.policy_wasm; re-raise other import errors unchanged.
Proposed fix
- try:
- from nmp.core.auth.app.embedded_pdp.policy_wasm import ensure_embedded_policy_wasm
- except ImportError as exc:
+ try:
+ from nmp.core.auth.app.embedded_pdp.policy_wasm import ensure_embedded_policy_wasm
+ except ModuleNotFoundError as exc:
+ if exc.name != "nmp.core.auth.app.embedded_pdp.policy_wasm":
+ raise
raise RuntimeError(
"Auth is enabled with the embedded PDP, but the nmp-auth package is not installed. "
"Install nmp-auth or set auth.policy_decision_point_provider='opa'."
) from exc🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/nmp_platform_runner/src/nmp/platform_runner/server.py` around lines
80 - 86, The try-except block around the import of ensure_embedded_policy_wasm
from nmp.core.auth.app.embedded_pdp.policy_wasm currently catches all
ImportError exceptions, including transitive import failures inside policy_wasm
that should not be remapped to a "package not installed" message. Modify the
exception handler to distinguish between a missing
nmp.core.auth.app.embedded_pdp.policy_wasm module and other import errors: check
if the caught ImportError's name attribute matches the module being imported (or
check the exception message for nmp.core.auth.app.embedded_pdp.policy_wasm), and
only convert that case to the RuntimeError with the installation guidance; for
all other import errors, re-raise the original exception unchanged to preserve
the actual debugging information.
Comment on lines
+131
to
+139
| result = subprocess.run( | ||
| [str(build_script)], | ||
| cwd=repo_root, | ||
| env=build_env, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=timeout_seconds, | ||
| check=False, | ||
| ) |
There was a problem hiding this comment.
Wrap subprocess timeouts as PolicyWasmError.
subprocess.run(..., timeout=...) can raise TimeoutExpired; currently that bypasses your policy-wasm error path and surfaces an unexpected exception type.
Suggested fix
- result = subprocess.run(
- [str(build_script)],
- cwd=repo_root,
- env=build_env,
- capture_output=True,
- text=True,
- timeout=timeout_seconds,
- check=False,
- )
+ try:
+ result = subprocess.run(
+ [str(build_script)],
+ cwd=repo_root,
+ env=build_env,
+ capture_output=True,
+ text=True,
+ timeout=timeout_seconds,
+ check=False,
+ )
+ except subprocess.TimeoutExpired as exc:
+ raise PolicyWasmError(
+ "Timed out while building embedded auth PDP policy.wasm.\n\n"
+ "Command:\n"
+ " script/build_policy_wasm.sh\n\n"
+ f"Timeout: {timeout_seconds}s" + _offline_build_hint(repo_root, wasm_path)
+ ) from exc🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/core/auth/src/nmp/core/auth/app/embedded_pdp/policy_wasm.py` around
lines 131 - 139, The subprocess.run call can raise a TimeoutExpired exception
when the timeout is exceeded, but this exception is not being caught and
wrapped, causing it to bypass the policy-wasm error handling. Wrap the
subprocess.run call in a try-except block that catches subprocess.TimeoutExpired
and re-raises it as a PolicyWasmError with an appropriate error message,
ensuring all subprocess execution failures follow the same error path.
Contributor
|
mckornfield
force-pushed
the
auth-opa-fix/mck
branch
from
6e55eb0 to
fd9030e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
services/core/auth/src/nmp/core/auth/app/embedded_pdp/policy_wasm.py (1)
131-139:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
TimeoutExpirednot wrapped asPolicyWasmError.
subprocess.run(..., timeout=...)raisesTimeoutExpiredon timeout, bypassing the PolicyWasmError path.Suggested fix
+ try: + result = subprocess.run( + [str(build_script)], + cwd=repo_root, + env=build_env, + capture_output=True, + text=True, + timeout=timeout_seconds, + check=False, + ) + except subprocess.TimeoutExpired as exc: + raise PolicyWasmError( + f"Build timed out after {timeout_seconds}s.\n\n" + "Command:\n" + " script/build_policy_wasm.sh" + _offline_build_hint(repo_root, wasm_path) + ) from exc - result = subprocess.run( - [str(build_script)], - cwd=repo_root, - env=build_env, - capture_output=True, - text=True, - timeout=timeout_seconds, - check=False, - )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/core/auth/src/nmp/core/auth/app/embedded_pdp/policy_wasm.py` around lines 131 - 139, The subprocess.run() call with timeout parameter can raise subprocess.TimeoutExpired exception directly, which is not caught and wrapped as PolicyWasmError like other errors. Wrap the subprocess.run() call in a try-except block to catch subprocess.TimeoutExpired and raise it as a PolicyWasmError instead, ensuring consistent error handling across all failure paths in this function.
🧹 Nitpick comments (1)
script/build_policy_wasm.sh (1)
182-210: 💤 Low valueConsider
--before glob to prevent option injection.If a
.regofile were named-e.rego, shell expansion could pass it as an option toopa build. Use-- *.regoto signal end-of-options.-(cd "${POLICY_DIR}" && "${OPA}" build -t wasm ${ENTRYPOINTS} -o "${BUNDLE_TMP}/bundle.tar.gz" *.rego) +(cd "${POLICY_DIR}" && "${OPA}" build -t wasm ${ENTRYPOINTS} -o "${BUNDLE_TMP}/bundle.tar.gz" -- *.rego)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/build_policy_wasm.sh` around lines 182 - 210, The glob pattern `*.rego` in the OPA build command invocation could expand to filenames starting with a hyphen (like `-e.rego`), which would be interpreted as command options rather than file arguments, creating a potential option injection vulnerability. Add `--` before the `*.rego` glob pattern in the OPA build command to explicitly signal the end of options and ensure all expanded filenames are treated as arguments, not options.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openapi/ga/openapi.yaml`:
- Around line 53-58: The entrypoint field in the OpenAPI specification currently
uses examples to document allowed values but lacks an enum constraint, which
allows generated clients to accept arbitrary strings. Add an enum property to
the entrypoint field definition that explicitly lists the supported values
(allow, has_permissions, has_role) to enforce type safety at the OpenAPI
contract level and ensure generated clients properly validate input.
In `@openapi/openapi.yaml`:
- Around line 52-59: The entrypoint parameter schema in the OpenAPI definition
currently uses type string without constraints, allowing any string value to be
accepted. Add an enum field to the entrypoint schema definition (around line
52-59) that restricts the valid values to only the three documented entrypoints:
allow, has_permissions, and has_role. This will ensure generated clients
properly validate the input before making requests, preventing unnecessary 400
errors.
In `@services/core/auth/src/nmp/core/auth/assets/static-authz.yaml`:
- Line 312: The permission auth.authz.create is referenced in both the Editor
role definition and the endpoint mapping but is missing from the permissions
registry section which violates the single source of truth principle stated in
the file. Add the auth.authz.create permission declaration to the permissions
registry (lines 9-265) to complete the definition, ensuring all permissions used
throughout the configuration are properly declared in the central registry
location.
---
Duplicate comments:
In `@services/core/auth/src/nmp/core/auth/app/embedded_pdp/policy_wasm.py`:
- Around line 131-139: The subprocess.run() call with timeout parameter can
raise subprocess.TimeoutExpired exception directly, which is not caught and
wrapped as PolicyWasmError like other errors. Wrap the subprocess.run() call in
a try-except block to catch subprocess.TimeoutExpired and raise it as a
PolicyWasmError instead, ensuring consistent error handling across all failure
paths in this function.
---
Nitpick comments:
In `@script/build_policy_wasm.sh`:
- Around line 182-210: The glob pattern `*.rego` in the OPA build command
invocation could expand to filenames starting with a hyphen (like `-e.rego`),
which would be interpreted as command options rather than file arguments,
creating a potential option injection vulnerability. Add `--` before the
`*.rego` glob pattern in the OPA build command to explicitly signal the end of
options and ensure all expanded filenames are treated as arguments, not options.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 33a6e7b0-0caf-403b-b01c-5b2bf8c45144
⛔ Files ignored due to path filters (1)
sdk/stainless.yamlis excluded by!sdk/**
📒 Files selected for processing (21)
.github/actions/build-policy-wasm/action.yamldocs/auth/authorization/policy-engine.mdxdocs/auth/deployment/configuration.mdxdocs/set-up/config-reference.mdxopenapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlpackages/nmp_common/src/nmp/common/config/base.pypackages/nmp_platform_runner/src/nmp/platform_runner/run.pypackages/nmp_platform_runner/src/nmp/platform_runner/server.pypackages/nmp_platform_runner/tests/test_run_policy_wasm.pypackages/nmp_platform_runner/tests/test_server.pyscript/build_policy_wasm.shservices/core/auth/src/nmp/core/auth/app/embedded_pdp/__init__.pyservices/core/auth/src/nmp/core/auth/app/embedded_pdp/engine.pyservices/core/auth/src/nmp/core/auth/app/embedded_pdp/policy_wasm.pyservices/core/auth/src/nmp/core/auth/assets/static-authz.yamlservices/core/auth/src/nmp/core/auth/service.pyservices/core/auth/tests/test_config.pyservices/core/auth/tests/test_policy_wasm.pyservices/core/auth/tests/test_service_policy_wasm.py
💤 Files with no reviewable changes (1)
- .github/actions/build-policy-wasm/action.yaml
✅ Files skipped from review due to trivial changes (2)
- docs/set-up/config-reference.mdx
- docs/auth/authorization/policy-engine.mdx
🚧 Files skipped from review as they are similar to previous changes (9)
- services/core/auth/src/nmp/core/auth/service.py
- packages/nmp_common/src/nmp/common/config/base.py
- packages/nmp_platform_runner/tests/test_server.py
- services/core/auth/src/nmp/core/auth/app/embedded_pdp/engine.py
- packages/nmp_platform_runner/src/nmp/platform_runner/run.py
- packages/nmp_platform_runner/tests/test_run_policy_wasm.py
- services/core/auth/tests/test_policy_wasm.py
- services/core/auth/tests/test_service_policy_wasm.py
- packages/nmp_platform_runner/src/nmp/platform_runner/server.py
Comment on lines
+53
to
+58
| type: string | ||
| description: Policy entrypoint to evaluate | ||
| examples: | ||
| - allow | ||
| - has_permissions | ||
| - has_role |
There was a problem hiding this comment.
Constrain entrypoint with enum.
examples documents allowed values, but generated clients still accept arbitrary strings. Add an enum so the OpenAPI contract matches supported entrypoints.
Suggested diff
schema:
type: string
+ enum:
+ - allow
+ - has_permissions
+ - has_role
description: Policy entrypoint to evaluate
- examples:
- - allow
- - has_permissions
- - has_role
title: Entrypoint📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| type: string | |
| description: Policy entrypoint to evaluate | |
| examples: | |
| - allow | |
| - has_permissions | |
| - has_role | |
| type: string | |
| enum: | |
| - allow | |
| - has_permissions | |
| - has_role | |
| description: Policy entrypoint to evaluate |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openapi/ga/openapi.yaml` around lines 53 - 58, The entrypoint field in the
OpenAPI specification currently uses examples to document allowed values but
lacks an enum constraint, which allows generated clients to accept arbitrary
strings. Add an enum property to the entrypoint field definition that explicitly
lists the supported values (allow, has_permissions, has_role) to enforce type
safety at the OpenAPI contract level and ensure generated clients properly
validate input.
Comment on lines
+52
to
+59
| schema: | ||
| type: string | ||
| description: Policy entrypoint to evaluate | ||
| examples: | ||
| - allow | ||
| - has_permissions | ||
| - has_role | ||
| title: Entrypoint |
There was a problem hiding this comment.
Constrain entrypoint with an enum.
The operation documents only three valid entrypoints, but the schema accepts any string, so generated clients won’t validate this finite contract before hitting a 400.
Proposed fix
schema:
type: string
+ enum:
+ - allow
+ - has_permissions
+ - has_role
description: Policy entrypoint to evaluate
- examples:
- - allow
- - has_permissions
- - has_role
title: Entrypoint📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| schema: | |
| type: string | |
| description: Policy entrypoint to evaluate | |
| examples: | |
| - allow | |
| - has_permissions | |
| - has_role | |
| title: Entrypoint | |
| schema: | |
| type: string | |
| enum: | |
| - allow | |
| - has_permissions | |
| - has_role | |
| description: Policy entrypoint to evaluate | |
| title: Entrypoint |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openapi/openapi.yaml` around lines 52 - 59, The entrypoint parameter schema
in the OpenAPI definition currently uses type string without constraints,
allowing any string value to be accepted. Add an enum field to the entrypoint
schema definition (around line 52-59) that restricts the valid values to only
the three documented entrypoints: allow, has_permissions, and has_role. This
will ensure generated clients properly validate the input before making
requests, preventing unnecessary 400 errors.
| description: "Read and write access to workspace resources" | ||
| includes: ["Viewer"] | ||
| permissions: | ||
| - auth.authz.create |
There was a problem hiding this comment.
Missing auth.authz.create permission definition.
Permission auth.authz.create is used in the Editor role (line 312) and endpoint mapping (line 402) but not declared in the permissions registry (lines 9-265). This violates the "single source of truth" principle stated at line 4.
Add the permission declaration:
Proposed fix
permissions:
+ auth:
+ authz:
+ create:
+ description: "Evaluate authorization policies"
entities:
create:
description: "Create entities"Also applies to: 399-405
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/core/auth/src/nmp/core/auth/assets/static-authz.yaml` at line 312,
The permission auth.authz.create is referenced in both the Editor role
definition and the endpoint mapping but is missing from the permissions registry
section which violates the single source of truth principle stated in the file.
Add the auth.authz.create permission declaration to the permissions registry
(lines 9-265) to complete the definition, ensuring all permissions used
throughout the configuration are properly declared in the central registry
location.
fd9030e to
c6c6e2b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@script/build_policy_wasm.sh`:
- Around line 112-128: The script currently downloads OPA checksums from the
same remote URL as the binary, which doesn't provide integrity verification
against a compromised endpoint. Replace the curl command that fetches from
`sha_url` and the awk command that extracts the checksum from the downloaded
file with a pinned checksum lookup mechanism. Create an in-repository mapping
(as a function or case statement) that associates each OPA_VERSION and asset
name combination with its expected checksum value, then validate the downloaded
binary against this pinned checksum instead of a remotely-fetched one.
- Around line 115-123: The two curl commands downloading the OPA binary and
checksum lack timeout and retry parameters, which can cause indefinite hangs on
network instability. For the first curl command that uses the url variable to
download the binary to tmp_bin, add --connect-timeout 10, --max-time 120,
--retry 3, and --retry-delay 2 flags to the curl invocation. For the second curl
command that uses the sha_url variable to download the checksum to tmp_sha, add
the same timeout and retry flags but use --max-time 30 instead of 120 to keep
the checksum download timeout shorter. Both curl commands should be updated with
the same --connect-timeout, --retry, and --retry-delay values, only differing in
their --max-time values.
In `@services/core/auth/src/nmp/core/auth/app/embedded_pdp/policy_wasm.py`:
- Around line 63-64: The freshness check in this code block uses file
modification times to determine if policy.wasm is up-to-date compared to source
files, which is unreliable across fresh checkouts or branch switches. Replace
the mtime comparison logic (the lines comparing wasm_path.stat().st_mtime_ns and
source.stat().st_mtime_ns) with a hash-based approach: either create and compare
a hash sidecar file containing hashes of the source files and OPA version
information, or implement a detection mechanism that forces a rebuild whenever a
source code checkout is detected. This ensures stale authorization logic will
not be loaded due to timestamp anomalies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 095b0949-5566-4b29-8c8b-673e2b74e3a2
⛔ Files ignored due to path filters (2)
sdk/python/nemo-platform/.nmpcontext/stainless.yamlis excluded by!sdk/**sdk/stainless.yamlis excluded by!sdk/**
📒 Files selected for processing (18)
.github/actions/build-policy-wasm/action.yamldocs/auth/authorization/policy-engine.mdxdocs/auth/deployment/configuration.mdxdocs/set-up/config-reference.mdxpackages/nmp_common/src/nmp/common/config/base.pypackages/nmp_platform_runner/src/nmp/platform_runner/run.pypackages/nmp_platform_runner/src/nmp/platform_runner/server.pypackages/nmp_platform_runner/tests/test_run_policy_wasm.pypackages/nmp_platform_runner/tests/test_server.pyscript/build_policy_wasm.shservices/core/auth/src/nmp/core/auth/app/embedded_pdp/__init__.pyservices/core/auth/src/nmp/core/auth/app/embedded_pdp/engine.pyservices/core/auth/src/nmp/core/auth/app/embedded_pdp/policy_wasm.pyservices/core/auth/src/nmp/core/auth/assets/static-authz.yamlservices/core/auth/src/nmp/core/auth/service.pyservices/core/auth/tests/test_config.pyservices/core/auth/tests/test_policy_wasm.pyservices/core/auth/tests/test_service_policy_wasm.py
💤 Files with no reviewable changes (1)
- .github/actions/build-policy-wasm/action.yaml
✅ Files skipped from review due to trivial changes (2)
- docs/auth/authorization/policy-engine.mdx
- docs/auth/deployment/configuration.mdx
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/nmp_common/src/nmp/common/config/base.py
- docs/set-up/config-reference.mdx
- packages/nmp_platform_runner/src/nmp/platform_runner/run.py
- services/core/auth/src/nmp/core/auth/app/embedded_pdp/init.py
- services/core/auth/tests/test_config.py
- packages/nmp_platform_runner/tests/test_run_policy_wasm.py
- services/core/auth/tests/test_service_policy_wasm.py
- services/core/auth/src/nmp/core/auth/service.py
- packages/nmp_platform_runner/tests/test_server.py
- services/core/auth/src/nmp/core/auth/app/embedded_pdp/engine.py
- services/core/auth/tests/test_policy_wasm.py
- packages/nmp_platform_runner/src/nmp/platform_runner/server.py
Comment on lines
+112
to
+128
| url="${OPA_DOWNLOAD_BASE_URL}/${OPA_VERSION}/${asset}" | ||
| sha_url="${url}.sha256" | ||
| echo "Downloading OPA ${OPA_VERSION} from ${url}..." >&2 | ||
| if ! curl -fsSL "${url}" -o "${tmp_bin}"; then | ||
| echo "Failed to download OPA binary from ${url}." >&2 | ||
| print_opa_help "${asset}" | ||
| exit 1 | ||
| fi | ||
| if ! curl -fsSL "${sha_url}" -o "${tmp_sha}"; then | ||
| echo "Failed to download OPA checksum from ${sha_url}." >&2 | ||
| print_opa_help "${asset}" | ||
| exit 1 | ||
| fi | ||
|
|
||
| expected="$(awk '{print $1; exit}' "${tmp_sha}")" | ||
| actual="$(sha256_file "${tmp_bin}")" | ||
| if [ "${expected}" != "${actual}" ]; then |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
version="v1.8.0"
for asset in \
opa_linux_amd64_static \
opa_linux_arm64_static \
opa_darwin_amd64_static \
opa_darwin_arm64_static
do
printf '%s ' "$asset"
curl -fsSL "https://openpolicyagent.org/downloads/${version}/${asset}.sha256"
doneRepository: NVIDIA-NeMo/nemo-platform
Length of output: 388
Pin OPA checksums in-repo.
Fetching checksums from the same remote as binaries doesn't verify integrity—a compromised endpoint can serve matching binary+checksum. Maintain a checked-in OPA_VERSION/asset checksum map and validate downloads against it. The shell evidence shows this remote is already unreliable (404 errors), reinforcing the need for pinned checksums.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@script/build_policy_wasm.sh` around lines 112 - 128, The script currently
downloads OPA checksums from the same remote URL as the binary, which doesn't
provide integrity verification against a compromised endpoint. Replace the curl
command that fetches from `sha_url` and the awk command that extracts the
checksum from the downloaded file with a pinned checksum lookup mechanism.
Create an in-repository mapping (as a function or case statement) that
associates each OPA_VERSION and asset name combination with its expected
checksum value, then validate the downloaded binary against this pinned checksum
instead of a remotely-fetched one.
Comment on lines
+63
to
+64
| wasm_mtime = wasm_path.stat().st_mtime_ns | ||
| return any(source.stat().st_mtime_ns > wasm_mtime for source in sources) |
There was a problem hiding this comment.
Do not trust mtimes for auth policy freshness.
A fresh checkout/branch switch can make a stale policy.wasm look newer than *.rego, so the embedded PDP can load old authorization logic. Compare a source/build-script/OPA-version hash sidecar, or rebuild whenever a source checkout is detected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/core/auth/src/nmp/core/auth/app/embedded_pdp/policy_wasm.py` around
lines 63 - 64, The freshness check in this code block uses file modification
times to determine if policy.wasm is up-to-date compared to source files, which
is unreliable across fresh checkouts or branch switches. Replace the mtime
comparison logic (the lines comparing wasm_path.stat().st_mtime_ns and
source.stat().st_mtime_ns) with a hash-based approach: either create and compare
a hash sidecar file containing hashes of the source files and OPA version
information, or implement a detection mechanism that forces a rebuild whenever a
source code checkout is detected. This ensures stale authorization logic will
not be loaded due to timestamp anomalies.
c6c6e2b to
06325e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@script/build_policy_wasm.sh`:
- Line 194: The glob pattern `*.rego` in the OPA build command is unsafe because
any .rego file starting with a dash character would be interpreted as a
command-line flag rather than a filename, potentially allowing option injection.
Change the glob pattern from `*.rego` to `./*.rego` to explicitly reference
files in the current directory, which prevents filenames beginning with a dash
from being misinterpreted as flags. This change should be made in the OPA build
command where the entrypoints are expanded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1935a072-5dcf-43b1-8fa1-b16e63a72659
⛔ Files ignored due to path filters (2)
sdk/python/nemo-platform/.nmpcontext/stainless.yamlis excluded by!sdk/**sdk/stainless.yamlis excluded by!sdk/**
📒 Files selected for processing (18)
.github/actions/build-policy-wasm/action.yamldocs/auth/authorization/policy-engine.mdxdocs/auth/deployment/configuration.mdxdocs/set-up/config-reference.mdxpackages/nmp_common/src/nmp/common/config/base.pypackages/nmp_platform_runner/src/nmp/platform_runner/run.pypackages/nmp_platform_runner/src/nmp/platform_runner/server.pypackages/nmp_platform_runner/tests/test_run_policy_wasm.pypackages/nmp_platform_runner/tests/test_server.pyscript/build_policy_wasm.shservices/core/auth/src/nmp/core/auth/app/embedded_pdp/__init__.pyservices/core/auth/src/nmp/core/auth/app/embedded_pdp/engine.pyservices/core/auth/src/nmp/core/auth/app/embedded_pdp/policy_wasm.pyservices/core/auth/src/nmp/core/auth/assets/static-authz.yamlservices/core/auth/src/nmp/core/auth/service.pyservices/core/auth/tests/test_config.pyservices/core/auth/tests/test_policy_wasm.pyservices/core/auth/tests/test_service_policy_wasm.py
💤 Files with no reviewable changes (9)
- services/core/auth/src/nmp/core/auth/service.py
- services/core/auth/tests/test_config.py
- services/core/auth/src/nmp/core/auth/assets/static-authz.yaml
- services/core/auth/src/nmp/core/auth/app/embedded_pdp/init.py
- services/core/auth/tests/test_service_policy_wasm.py
- services/core/auth/tests/test_policy_wasm.py
- .github/actions/build-policy-wasm/action.yaml
- services/core/auth/src/nmp/core/auth/app/embedded_pdp/engine.py
- services/core/auth/src/nmp/core/auth/app/embedded_pdp/policy_wasm.py
✅ Files skipped from review due to trivial changes (4)
- packages/nmp_common/src/nmp/common/config/base.py
- docs/set-up/config-reference.mdx
- docs/auth/authorization/policy-engine.mdx
- docs/auth/deployment/configuration.mdx
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/nmp_platform_runner/tests/test_run_policy_wasm.py
- packages/nmp_platform_runner/src/nmp/platform_runner/run.py
- packages/nmp_platform_runner/src/nmp/platform_runner/server.py
- packages/nmp_platform_runner/tests/test_server.py
| # shellcheck disable=SC2086 | ||
|
|
||
| (cd "${POLICY_DIR}" && opa build -t wasm ${ENTRYPOINTS} -o "${BUNDLE_TMP}/bundle.tar.gz" *.rego) | ||
| (cd "${POLICY_DIR}" && "${OPA}" build -t wasm ${ENTRYPOINTS} -o "${BUNDLE_TMP}/bundle.tar.gz" *.rego) |
There was a problem hiding this comment.
Glob safety: use ./*.rego to prevent option injection.
A .rego file starting with - would be interpreted as an OPA flag.
Proposed fix
-(cd "${POLICY_DIR}" && "${OPA}" build -t wasm ${ENTRYPOINTS} -o "${BUNDLE_TMP}/bundle.tar.gz" *.rego)
+(cd "${POLICY_DIR}" && "${OPA}" build -t wasm ${ENTRYPOINTS} -o "${BUNDLE_TMP}/bundle.tar.gz" ./*.rego)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| (cd "${POLICY_DIR}" && "${OPA}" build -t wasm ${ENTRYPOINTS} -o "${BUNDLE_TMP}/bundle.tar.gz" *.rego) | |
| (cd "${POLICY_DIR}" && "${OPA}" build -t wasm ${ENTRYPOINTS} -o "${BUNDLE_TMP}/bundle.tar.gz" ./*.rego) |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 194-194: Use ./glob or -- glob so names with dashes won't become options.
(SC2035)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@script/build_policy_wasm.sh` at line 194, The glob pattern `*.rego` in the
OPA build command is unsafe because any .rego file starting with a dash
character would be interpreted as a command-line flag rather than a filename,
potentially allowing option injection. Change the glob pattern from `*.rego` to
`./*.rego` to explicitly reference files in the current directory, which
prevents filenames beginning with a dash from being misinterpreted as flags.
This change should be made in the OPA build command where the entrypoints are
expanded.
Source: Linters/SAST tools
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
mckornfield
force-pushed
the
auth-opa-fix/mck
branch
from
06325e6 to
9888033
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
embedded_pdp_auto_build_wasmconfiguration option (defaults to enabled) to control auto-build behavior.Documentation
Chores