feat(iam,vm): implement IAM03 credential access and AUTH03 key access#537
Open
abegnoche wants to merge 4 commits into
Open
feat(iam,vm): implement IAM03 credential access and AUTH03 key access#537abegnoche wants to merge 4 commits into
abegnoche wants to merge 4 commits into
Conversation
Strengthen IAM03-01 beyond FieldExistsCheck by validating identity and authorized-resource probes, and add AUTH03-01 component key access for SOL (with provider-hidden network devices on AWS). Closes #341 Closes #248 Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com>
📝 WalkthroughWalkthroughAdds IAM credential-access validation and VM component key-access checks. AWS probes serial-console key authorization and reports provider-hidden network-device access, while provider configuration, suite validation, documentation, and contract tests are updated. ChangesAccess validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant VMValidation as VM validation suite
participant ComponentProbe as component_key_access.py
participant EC2Console as EC2 serial console
participant InstanceConnect as EC2 Instance Connect
participant ComponentCheck as ComponentKeyAccessCheck
VMValidation->>ComponentProbe: Run with instance and key parameters
ComponentProbe->>EC2Console: Check serial access
ComponentProbe->>InstanceConnect: Send serial-console SSH public key
InstanceConnect-->>ComponentProbe: Return authorization result
ComponentProbe-->>ComponentCheck: Return JSON probe results
ComponentCheck-->>VMValidation: Validate required access tests
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Cleanup pass over the IAM03 credential-access and AUTH03 key-access change; no behavior change. - component_key_access.py: drop _make_test_result/_mark_skipped/ REQUIRED_TESTS copied from console_rbac.py; emit a minimal top-level skip instead of the unconsumed per-subtest fan-out; remove the isinstance dance and the unconsumed request_id field; stop fabricating a passing network_device_access on key-load failure - hoist SERIAL_ACCESS_DISABLED_SKIP_REASON into common/serial_console and import it in both console_rbac.py and component_key_access.py - ComponentKeyAccessCheck: remove the inline pytest.skip already handled by BaseValidation.execute(), the dead requested_key_name fallback, and a redundant isinstance guard - suites: drop required_tests overrides that duplicated class defaults - contract test: reuse the cached conftest load_vm_script instead of a local uncached loader; use tmp_path instead of mkstemp (leaked fd) - test_credentials.py: simplify the success expression; hoist local imports in tests Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Member
Author
|
/ok to test d6dd460 |
🔐 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-13 18:31:09 UTC | Commit: d6dd460 |
![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 (2)
isvctl/configs/providers/aws/scripts/iam/test_credentials.py (1)
96-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
main()is missing a docstring.Coding guidelines require every function to have a PEP 257 docstring;
main()has none.📝 Proposed fix
def main() -> int: + """Test IAM credentials by validating STS identity and authorized-resource access.""" parser = argparse.ArgumentParser(description="Test IAM credentials")Based on learnings, coding guidelines state "Every function and class must have docstrings following PEP 257."
🤖 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/configs/providers/aws/scripts/iam/test_credentials.py` around lines 96 - 97, Add a concise PEP 257-compliant docstring as the first statement in the main() function, describing that it tests IAM credentials and returns the resulting process exit status. Preserve the existing argument parsing and execution flow.Source: Coding guidelines
isvctl/configs/providers/my-isv/scripts/vm/component_key_access.py (1)
89-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNon-demo failure path leaves
testsempty instead of per-test{"passed": False}stubs.Other my-isv scaffolds (e.g.
api_endpoint_test.py,bmc_isolation_test.py,sa_credential_test.py) initialize each subtest entry with{"passed": False}and set a single top-levelresult["error"]. This scaffold leavesresult["tests"]as{}in the not-implemented branch, so it doesn't follow that established convention.♻️ Proposed fix to match the established scaffold convention
if DEMO_MODE: result = _demo_result(args.instance_id, args.key_name) else: result["error"] = "Not implemented - replace with your platform's component key access logic" + result["tests"]["sol_access"] = {"passed": False} + result["tests"]["network_device_access"] = {"passed": False}Based on learnings, "follow the established non-demo failure convention: initialize each test entry with
{"passed": False}and record a single top-levelresult["error"]string for failures" for my-isv scaffolds.🤖 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/configs/providers/my-isv/scripts/vm/component_key_access.py` around lines 89 - 121, Update the non-DEMO_MODE branch in the component key access flow to initialize every expected entry in result["tests"] with {"passed": False}, matching the scaffold convention used by the related test scripts. Keep a single top-level result["error"] string for the not-implemented failure, while preserving the existing demo result behavior and success-based exit code.
🤖 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/aws/scripts/iam/test_credentials.py`:
- Around line 120-132: Update the IAM access test around iam.get_user() to catch
BotoCoreError alongside ClientError, recording the access test as failed with
the exception details so the script still writes its JSON result. Preserve the
existing AccessDenied success handling and ClientError behavior.
In `@isvctl/configs/providers/aws/scripts/vm/component_key_access.py`:
- Around line 81-94: Update the send_serial_console_ssh_public_key call in the
component key access probe to pass SerialPort=0 instead of 1, while preserving
the existing error handling and response evaluation.
In `@isvctl/configs/providers/my-isv/config/vm.yaml`:
- Around line 48-52: Update the numbered step list in the VM configuration
comments to include the active virtual_device_hardening step after
component_key_access, then renumber deploy_nim, teardown_nim, and teardown to
11, 12, and 13 respectively.
In `@isvctl/configs/suites/README.md`:
- Line 88: Update the component_key_access entry in the suites README to
document that tests.sol_access.passed and tests.network_device_access.passed are
required only for non-skipped results, matching ComponentKeyAccessCheck’s early
skipped response when serial access is disabled.
---
Nitpick comments:
In `@isvctl/configs/providers/aws/scripts/iam/test_credentials.py`:
- Around line 96-97: Add a concise PEP 257-compliant docstring as the first
statement in the main() function, describing that it tests IAM credentials and
returns the resulting process exit status. Preserve the existing argument
parsing and execution flow.
In `@isvctl/configs/providers/my-isv/scripts/vm/component_key_access.py`:
- Around line 89-121: Update the non-DEMO_MODE branch in the component key
access flow to initialize every expected entry in result["tests"] with
{"passed": False}, matching the scaffold convention used by the related test
scripts. Keep a single top-level result["error"] string for the not-implemented
failure, while preserving the existing demo result behavior and success-based
exit code.
🪄 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: d19a0da1-3240-45ae-a499-80ac51ebfd1a
📒 Files selected for processing (17)
docs/test-plan.yamlisvctl/configs/providers/aws/config/vm.yamlisvctl/configs/providers/aws/scripts/common/serial_console.pyisvctl/configs/providers/aws/scripts/iam/test_credentials.pyisvctl/configs/providers/aws/scripts/vm/component_key_access.pyisvctl/configs/providers/aws/scripts/vm/console_rbac.pyisvctl/configs/providers/my-isv/config/vm.yamlisvctl/configs/providers/my-isv/scripts/README.mdisvctl/configs/providers/my-isv/scripts/vm/component_key_access.pyisvctl/configs/suites/README.mdisvctl/configs/suites/iam.yamlisvctl/configs/suites/vm.yamlisvctl/tests/test_component_key_access_contract.pyisvtest/src/isvtest/validations/iam.pyisvtest/src/isvtest/validations/instance.pyisvtest/tests/test_iam.pyisvtest/tests/test_instance.py
AWS only accepts serial port 0 for SendSerialConsoleSSHPublicKey; SerialPort=1 would fail every SOL probe. Also sync my-isv step comments and document the skipped component_key_access contract. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Member
Author
|
/ok to test 7b061e9 |
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
Implements M7 Batch A foundational identity/access issues:
IamCredentialAccessCheckComponentKeyAccessCheckChanges
IAM03-01 (#341)
FieldExistsCheck(account_id)withIamCredentialAccessCheckrequiringtests.identity+tests.accesstest_credentials.pyJSON to the provider-neutralidentity/accesscontract (already used by my-isv)AUTH03-01 (#248)
ComponentKeyAccessCheckon a newcomponent_key_accessVM stepprovider_hiddenmake demo-testTest plan
make lint/make testCloses #341
Closes #248
Summary by CodeRabbit
component_key_accessVM validation for launch-key access to serial console and network devices (with SOL-disabled skipping).AUTH03-01component key access checks and a my-isv demo-mode implementation.component_key_accessstep and JSON field expectations.