feat(nico): add AUTH-XX-03 specified-key access validation#469
Open
abegnoche wants to merge 10 commits into
Open
feat(nico): add AUTH-XX-03 specified-key access validation#469abegnoche wants to merge 10 commits into
abegnoche wants to merge 10 commits into
Conversation
📝 WalkthroughWalkthroughAdds AUTH03-01 specified-key access validation for serial-console and network-device targets, with NICo query, ephemeral setup/teardown workflows, configuration wiring, HTTP client extensions, labels, and comprehensive unit and integration tests. ChangesSpecified-key access
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SetupKeyAccess
participant NICoAPI
participant QueryKeyAccess
participant SpecifiedKeyAccessCheck
participant TeardownKeyAccess
SetupKeyAccess->>NICoAPI: create temporary SSH key and key group
SetupKeyAccess->>NICoAPI: wait for synchronization and enable site access
QueryKeyAccess->>NICoAPI: query synced groups and site configuration
QueryKeyAccess-->>SpecifiedKeyAccessCheck: emit access evidence JSON
SpecifiedKeyAccessCheck-->>TeardownKeyAccess: complete validation workflow
TeardownKeyAccess->>NICoAPI: delete temporary resources and restore site flag
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Verify a tenant-specified key (SSH key) can access out-of-band components, with the serial console (SOL) as the verifiable example. - nico_client: add NICo tenant REST (/nico/) path helpers alongside the Forge control-plane (/carbide/) helpers; SSH key groups and per-site serial-console config live only on the tenant REST API. - query_key_access.py: read the site's SSH key groups and serial-console configuration and emit the provider-neutral access contract; network devices are provider-managed and reported as unverified. - SpecifiedKeyAccessCheck: pass when a specified key can reach at least one component, fail when a key-access path is disabled/broken, skip when access can only be left unverified. - Wire query_key_access into the bare_metal suite + NICo config. Ships unreleased (not added to released_tests.json). Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com>
The deployed NICo clusters serve the API under the /carbide/ segment (/nico/ is the newer name). Use the existing forge_get/forge_get_all helpers like every other NICo script instead of introducing /nico/ helpers, which 404 on current clusters. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com>
When no SSH key group with a key is synced to the site, emit a structured skip that distinguishes 'no key groups exist at all' from 'key groups exist but none are synced to this site' (carrying an org_key_groups count), so the operator knows what to configure. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com>
Add a key_access.yaml config that provisions an ephemeral SSH key + key group synced to the site (setup), runs SpecifiedKeyAccessCheck (test), then deletes them (teardown), so AUTH-XX-03 can go green in CI without manual key setup. The throwaway private key is generated and discarded immediately; only the unusable public key is registered. - nico_client: add forge_post/forge_patch/forge_delete write helpers. - setup_key_access.py / teardown_key_access.py scripts (best-effort, emit created IDs even on failure so teardown stays reliable). - Unit coverage for the query/setup/teardown scripts in test_nico_provider.py. Read-only bare_metal.yaml flow is unchanged. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
abegnoche
force-pushed
the
cursor/auth-xx-03-specified-key-access-42e9
branch
from
766776b to
6fcfa6d
Compare
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Make query_key_access a read-only test-phase step (like the other NICo checks) so a key-access failure no longer skips the whole bare_metal run. Restore the dedicated key_access.yaml for the opt-in setup -> test -> teardown auto-provisioning path. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Cleanup pass over the AUTH-XX-03 key-access change; no new behavior except teardown now failing when a deletion fails: - fold forge_get into forge_request (params + GET default) so URL building and HTTPError body rewrap live in one place - hoist the shared sshkeygroup_synced_to_site predicate and SYNCED_STATUS into common/nico_client.py (was duplicated in the setup and query scripts) - query_key_access: fetch the site only on the non-skip path, drop the always-true keys_synced parameter and its unreachable branch, single print/return exit, and drop the unread key_groups / keys_synced_to_site output fields per JSON contract discipline - setup_key_access: short-circuit _wait_for_sync on an empty group id instead of polling the collection URL for 3 minutes - teardown_key_access: report success = no cleanup_errors (matches teardown_nim.py) so leaked resources fail the step visibly; shrink _as_bool to the spellings the pipeline can produce - drop the dead labels ClassVar on SpecifiedKeyAccessCheck (labels are declared only in suite YAML) and the min_accessible_targets: 1 suite param that restated the code default - collapse three copy-pasted test script loaders into _load_nico_script Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Legacy carbide DELETE returns a bare OK body, which broke forge_request with JSONDecodeError and failed teardown even when the API accepted cleanup. Treat non-JSON 2xx bodies as success, make DELETE 404 idempotent, and surface cleanup_errors in the step error field for easier diagnosis. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-14 21:04:21 UTC | Commit: 26d19e3 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
isvctl/tests/providers/nico/test_nico_provider.py (1)
377-385: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the new test helpers with the repository’s Python standards.
Add a PEP 257 docstring to
_RawResponse, parameterize the baredict/listannotations, and moveHTTPErrorinto the top-level import block.Representative fixes
+from urllib.error import HTTPError + class _RawResponse: + """Provide a context-managed byte response for mocked HTTP calls.""" + - def fake_all(..., params: dict | None = None, ...) -> list: + def fake_all(..., params: dict[str, str] | None = None, ...) -> list[dict[str, Any]]: - def fake_post(..., body: dict, ...) -> dict: + def fake_post(..., body: dict[str, Any], ...) -> dict[str, Any]: - from urllib.error import HTTPErrorAs per coding guidelines, all classes require PEP 257 docstrings, collection annotations require generic parameters, and imports belong at module scope.
Also applies to: 3182-3182, 3211-3221, 3253-3253, 3318-3318
🤖 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 `@isvctl/tests/providers/nico/test_nico_provider.py` around lines 377 - 385, Update the test helpers in test_nico_provider.py to follow repository standards: add a PEP 257 class docstring to _RawResponse, replace every bare dict and list annotation in the affected helpers with parameterized generics, and move the HTTPError import into the module’s top-level import block.Source: Coding guidelines
🤖 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 `@isvctl/configs/providers/nico/config/key_access.yaml`:
- Around line 65-75: Increase the setup_key_access command timeout from 240
seconds to allow the 180-second polling period plus authentication and all
subsequent HTTP operations to complete and print the resource IDs.
In `@isvctl/configs/providers/nico/scripts/auth/setup_key_access.py`:
- Around line 152-175: Update the setup flow around _wait_for_sync so a false
synchronization result stops provisioning before setting result["success"] =
True or proceeding with site-flag handling. Mark the operation as failed using
the existing failure/error path, while preserving the successful flow only when
the key group synchronizes.
In `@isvctl/configs/providers/nico/scripts/common/nico_client.py`:
- Around line 211-238: Update the response parsing in the client request method
around Request and urlopen so empty or malformed successful bodies return {}
only for DELETE requests. For GET, POST, and other methods requiring structured
data, propagate a clear parsing error instead of silently returning {}, while
preserving the existing HTTPError behavior and legacy DELETE handling.
In `@isvtest/src/isvtest/validations/key_access.py`:
- Around line 120-125: Update the reachable handling in the key-access
validation flow to require an exact boolean True instead of coercing
target.get("reachable"), so string values such as "false" do not increment
accessible. Add a regression test covering non-boolean reachable output while
preserving the existing enabled and detail behavior.
---
Nitpick comments:
In `@isvctl/tests/providers/nico/test_nico_provider.py`:
- Around line 377-385: Update the test helpers in test_nico_provider.py to
follow repository standards: add a PEP 257 class docstring to _RawResponse,
replace every bare dict and list annotation in the affected helpers with
parameterized generics, and move the HTTPError import into the module’s
top-level import block.
🪄 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: 0fbc35ea-e36c-4918-ad63-bf1289ddc036
📒 Files selected for processing (13)
docs/test-plan.adocdocs/test-plan.yamlisvctl/configs/providers/nico/config/bare_metal.yamlisvctl/configs/providers/nico/config/key_access.yamlisvctl/configs/providers/nico/scripts/auth/query_key_access.pyisvctl/configs/providers/nico/scripts/auth/setup_key_access.pyisvctl/configs/providers/nico/scripts/auth/teardown_key_access.pyisvctl/configs/providers/nico/scripts/common/nico_client.pyisvctl/configs/suites/bare_metal.yamlisvctl/configs/suites/vm.yamlisvctl/tests/providers/nico/test_nico_provider.pyisvtest/src/isvtest/validations/key_access.pyisvtest/tests/test_key_access.py
Scope non-JSON DELETE tolerance to DELETE only, fail setup when the key group never syncs, require boolean True for reachable, and budget more setup timeout for the 180s sync poll. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Member
Author
|
Addressed all four CodeRabbit actionable comments in
Also fixed ruff import order ( |
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
2 participants
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
Adds
SpecifiedKeyAccessCheck(AUTH-XX-03), a provider-neutral "a tenant-specified key can access out-of-band components" check, wired for NICo via thebare_metalsuite. Following the recent NICo-validation pattern (#457/#458/#466). Ships unreleased (released_tests.jsonuntouched); run withISVTEST_INCLUDE_UNRELEASED=1.NICo models a specified key as an SSH Key Group synced to a Site; the Site's serial console (SOL) is key-accessible when the provider enables it (
isSerialConsoleEnabled), SSH-key auth is on (isSerialConsoleSSHKeysEnabled), and a key group is synced to the site. The serial console is the verifiable target; network-device access is provider-managed and reported asnull(unverified) rather than passing vacuously. Key material is never emitted.Two ways to run it:
bare_metal.yaml): reports against whatever keys exist. When nothing is synced to the site it emits a structured skip whose reason distinguishes "no key groups exist at all" from "key groups exist but none synced to this site" (with anorg_key_groupscount), so the operator knows what to configure.key_access.yaml): asetup → test → teardownconfig that provisions an ephemeral SSH key + key group synced to the site, runs the check, then deletes them — so AUTH-XX-03 goes green in CI with no manual key setup. The throwaway private key is generated and discarded immediately; only the unusable public key is registered.Changes
SpecifiedKeyAccessCheck: passes when a specified key can reach ≥1 component, fails when a key-access path is disabled/broken, skips (with a precise reason) when access can't be evidenced.query_key_access.py(read-only) +setup_key_access.py/teardown_key_access.py(provisioning), via the existing/carbide/helpers; addsforge_post/forge_patch/forge_deletetonico_client.suites/bare_metal.yaml+providers/nico/config/bare_metal.yaml; adds the dedicatedproviders/nico/config/key_access.yaml.isvtest/tests/test_key_access.pyand the query/setup/teardown scripts inisvctl/tests/providers/nico/test_nico_provider.py.Testing
make test(full suite, incl. new validation + script tests)make lintmake demo-testuvx pre-commit run -a/carbide/stub — auto-provisioningkey_access.yamlruns setup → check PASS → teardown, and the throwaway key group is removed afterward; read-only skip diagnostics and the disabled-key fail path also verified.auth_xx_03_key_access_provision_e2e.log
auth_xx_03_key_access_e2e.log
Closes #248
To show artifacts inline, enable in settings.
Summary by CodeRabbit
New Features
Bug Fixes