feat(security): implement SEC13-01 mutual TLS validation#538
Open
abegnoche wants to merge 4 commits into
Open
Conversation
Add MutualTlsCheck for north-south and east-west mTLS (or equivalent), with a shared probe, AWS wrapper (east-west provider-hidden by default), and my-isv DEMO_MODE wiring. Closes #309 Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com>
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds SEC13-01 mutual TLS probing, structured validation, provider and suite integration, and tests. AWS tenant teardown now uses bounded polling that handles pending EC2 termination states before network cleanup. ChangesMutual TLS validation
AWS tenant teardown polling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SecurityCommand
participant mutual_tls_test
participant TLSEndpoint
participant MutualTlsCheck
SecurityCommand->>mutual_tls_test: Run SEC13-01 probe
mutual_tls_test->>TLSEndpoint: Test anonymous and authenticated TLS
TLSEndpoint-->>mutual_tls_test: Return handshake results
mutual_tls_test-->>MutualTlsCheck: Provide structured JSON contract
MutualTlsCheck-->>SecurityCommand: Report validation status
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
boto3's InstanceTerminated waiter treats pending as terminal failure, so terminating a still-booting tenant fixture failed the step after probes passed. Wait for running on launch and poll terminate without that race. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
…apper Wire aws/config/security.yaml straight to the shared mutual TLS probe via --east-west-provider-hidden-message instead of a 131-line wrapper that re-implemented the shared main() and reached into its private helpers. In the shared probe, collapse the duplicate skip payload into a single guard, extract _bad_input_result, loop over the two traffic planes instead of copy-pasting them, build the SSL contexts once per run instead of two per endpoint, and reduce the handshake except tuple to OSError. Also drop the instance_running launch waiter from the SEC11 tenant fixture: the pending-tolerant terminate poller already covers the race it guarded, and the waiter cost up to a minute per tenant of setup wall-clock on every run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Member
Author
|
/ok to test 0f8b1be |
🔐 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 20:07:17 UTC | Commit: 0f8b1be |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
isvctl/tests/providers/aws/test_aws_security_scripts.py (1)
3644-3708: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage of the pending-tolerant path; consider adding the two untested branches.
_wait_instance_terminated'sInvalidInstanceID.NotFoundshort-circuit and itsTimeoutErrorpath (instance never reachesterminated) aren't exercised by any test. Both are cheap to add and cover failure modes on a resource-cleanup path.♻️ Example additional tests
def test_wait_instance_terminated_returns_on_not_found( monkeypatch: pytest.MonkeyPatch, tenant_isolation_module: ModuleType, ) -> None: """Already-gone instances should not be treated as a failure.""" class FakeEc2: def describe_instances(self, InstanceIds: list[str]) -> dict[str, Any]: raise tenant_isolation_module.ClientError( {"Error": {"Code": "InvalidInstanceID.NotFound", "Message": "gone"}}, "DescribeInstances", ) monkeypatch.setattr(tenant_isolation_module.time, "sleep", lambda _seconds: None) tenant_isolation_module._wait_instance_terminated(FakeEc2(), "i-gone123", delay=0.0, max_attempts=3) def test_wait_instance_terminated_times_out( monkeypatch: pytest.MonkeyPatch, tenant_isolation_module: ModuleType, ) -> None: """An instance stuck in shutting-down must raise TimeoutError, not hang forever.""" class FakeEc2: def describe_instances(self, InstanceIds: list[str]) -> dict[str, Any]: return {"Reservations": [{"Instances": [{"State": {"Name": "shutting-down"}}]}]} monkeypatch.setattr(tenant_isolation_module.time, "sleep", lambda _seconds: None) with pytest.raises(TimeoutError): tenant_isolation_module._wait_instance_terminated(FakeEc2(), "i-stuck123", delay=0.0, max_attempts=3)🤖 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/aws/test_aws_security_scripts.py` around lines 3644 - 3708, Add tests for the two untested branches of _wait_instance_terminated: verify an InvalidInstanceID.NotFound ClientError returns without failure, and verify an instance remaining in shutting-down raises TimeoutError after max_attempts. Reuse the existing monkeypatched sleep and FakeEc2 test patterns.isvtest/src/isvtest/validations/security.py (1)
251-260: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate the skipped-step guard
BaseValidation.execute()already skips beforerun()is entered, so this branch only duplicates the control flow and can drift from the shared skip message.🤖 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 `@isvtest/src/isvtest/validations/security.py` around lines 251 - 260, Remove the step_output["skipped"] guard from the mTLS validation run method, leaving skip handling to BaseValidation.execute(). Preserve the existing failure handling for unsuccessful step output and the remaining validation behavior.
🤖 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/security/tenant_isolation_test.py`:
- Around line 413-424: Update the instance teardown handling around
ec2.terminate_instances and _wait_instance_terminated to also catch
BotoCoreError alongside the existing ClientError and TimeoutError cases, append
the failure to errors, and allow the remaining tenant cleanup steps to continue.
In `@isvctl/configs/providers/shared/mutual_tls_test.py`:
- Around line 145-166: Update _handshake and probe_mtls_endpoint so DNS,
connection, timeout, and other transport failures return a distinct
transport-error classification rather than accepted=False. Compute
anonymous_rejected only when the anonymous attempt has a TLS-level rejection,
while preserving authenticated_accepted only for a successful authenticated
handshake.
- Around line 292-320: The missing-plane handling in the loop over planes
currently treats every omitted plane as passed. Update this logic so
_provider_hidden_plane is used only for the explicit AWS east-west provider
exception; return a failing missing-configuration result for all other absent
required planes, while preserving the existing endpoint aggregation and
REQUIRED_TESTS success calculation.
- Around line 120-126: Update the SSLContext configuration in the mutual TLS
setup to keep server hostname verification enabled by removing the assignment
that disables check_hostname, while preserving the existing CA verification and
client certificate loading behavior.
In `@scripts/tests/test_mutual_tls_probe.py`:
- Around line 98-106: Add a PEP 257-compliant docstring directly inside the test
helper function fake_probe, describing its purpose and relevant probe parameters
while leaving its signature and behavior unchanged.
---
Nitpick comments:
In `@isvctl/tests/providers/aws/test_aws_security_scripts.py`:
- Around line 3644-3708: Add tests for the two untested branches of
_wait_instance_terminated: verify an InvalidInstanceID.NotFound ClientError
returns without failure, and verify an instance remaining in shutting-down
raises TimeoutError after max_attempts. Reuse the existing monkeypatched sleep
and FakeEc2 test patterns.
In `@isvtest/src/isvtest/validations/security.py`:
- Around line 251-260: Remove the step_output["skipped"] guard from the mTLS
validation run method, leaving skip handling to BaseValidation.execute().
Preserve the existing failure handling for unsuccessful step output and the
remaining validation behavior.
🪄 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: 2b9da885-2043-4ffd-bf1c-2227c0b95ce7
📒 Files selected for processing (12)
docs/test-plan.yamlisvctl/configs/providers/aws/config/security.yamlisvctl/configs/providers/aws/scripts/security/tenant_isolation_test.pyisvctl/configs/providers/my-isv/config/security.yamlisvctl/configs/providers/shared/mutual_tls_test.pyisvctl/configs/suites/README.mdisvctl/configs/suites/security.yamlisvctl/tests/providers/aws/test_aws_security_scripts.pyisvtest/src/isvtest/validations/__init__.pyisvtest/src/isvtest/validations/security.pyisvtest/tests/test_security.pyscripts/tests/test_mutual_tls_probe.py
Enable hostname verification with a CA, require TLS-layer rejection for anonymous probes, fail omitted planes unless east-west is explicitly provider-hidden, catch BotoCoreError during SEC11 terminate waits, and document the probe test helper. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Member
Author
|
/ok to test ee05570 |
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 B network-security issue:
MutualTlsCheckChanges
MutualTlsCheckrequiringtests.north_south_mtls_enforced+tests.east_west_mtls_enforcedproviders/shared/mutual_tls_test.py): anonymous reject + client-cert acceptEDGE_ENDPOINTSwhen certs are set; marks east-westprovider_hiddenunlessEAST_WEST_ENDPOINTSis set; structured skip when nothing configuredmake demo-testOperator env (real probes)
EDGE_ENDPOINTS/EAST_WEST_ENDPOINTS—host:port,...MTLS_CA_CERT_PATH,MTLS_CLIENT_CERT_PATH,MTLS_CLIENT_KEY_PATHTest plan
pytest isvtest/tests/test_security.py::TestMutualTlsCheck scripts/tests/test_mutual_tls_probe.pyISVCTL_DEMO_MODE=1 ISVTEST_INCLUDE_UNRELEASED=1my-isv security (MutualTlsCheckPASS)make lintCloses #309
Summary by CodeRabbit
Summary by CodeRabbit
pendingand enhanced cleanup timeout/error handling.