Skip to content

test(bdd): assert plaintext TLS rejection - #1323

Merged
sbaum1994 merged 3 commits into
test/bdd-1292-secure-multiclusterfrom
mcamp/fix/pr1308-coderabbit
Aug 28, 2026
Merged

test(bdd): assert plaintext TLS rejection#1323
sbaum1994 merged 3 commits into
test/bdd-1292-secure-multiclusterfrom
mcamp/fix/pr1308-coderabbit

Conversation

@mikeyrcamp

@mikeyrcamp mikeyrcamp commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

TL;DR

Strengthens the plaintext rejection coverage in #1308 so it accepts only the complete client-observable grpcurl dial-stage timeout from the expected TLS listener. A successful plaintext Watch snapshot followed by the stream deadline cannot satisfy the check.

Additional Details

  • Runs the exact plaintext WatchStargates probe and requires the complete evidenced diagnostic: Failed to dial target host "127.0.0.1:50071": context deadline exceeded.
  • Rejects JSON snapshots, RPC status output, additional output, wrong-target timeouts, missing-binary, connection-refused, proto/import, usage, and unexpected failures.
  • Uses the adjacent verified OpenSSL handshake and successful trusted TLS Watch to establish that the tested endpoint is healthy and TLS-only.
  • Adds focused red/green coverage for the successful-snapshot-plus-RPC-deadline false positive and focused rejection cases.
  • This PR is stacked on the head branch of test(bdd): cover secure multi-cluster Pylon registration #1308 and should land there before test(bdd): cover secure multi-cluster Pylon registration #1308 merges.

Before

flowchart LR
  A["TLS dial fails before any RPC"] --> C["Output contains context deadline exceeded"]
  B["Plaintext Watch returns JSON, then stream deadline"] --> C
  C --> D["Same rejection marker"]
  D --> E["False positive possible"]
Loading

After

flowchart LR
  A["OpenSSL verifies certificate and ALPN h2"] --> B["Exact plaintext Watch command"]
  B --> C{"Complete dial-stage line for 127.0.0.1:50071 only?"}
  C -->|Yes| D["Stable TLS-listener rejection marker"]
  C -->|No| E["Test fails"]
  F["JSON, RPC status, extra output, or wrong target"] --> E
  D --> G["Trusted TLS Watch succeeds"]
  G --> H["Healthy TLS endpoint rejects plaintext"]
Loading

For the Reviewer

Please review the exact diagnostic equality check and its focused red/green tests, plus the exact-command assertion in godog_test.go. The accepted line was reproduced with grpcurl 1.9.3 against a real local TLS listener on the BDD target port. The regression test proves a successful plaintext snapshot followed by DeadlineExceeded is rejected.

For QA

  • Real TLS-listener probe on port 50071: produced the complete expected dial line and normalized successfully.
  • Successful plaintext snapshot followed by RPC DeadlineExceeded: rejected.
  • Wrong-target, extra-output, missing-binary, connection-refused, proto/import, and usage cases: rejected.
  • Focused diagnostic and secure-registration wiring tests: passed.
  • go test -short -count=1 ./...: passed.
  • go vet ./...: passed.
  • Shell syntax and ShellCheck 0.11.0: passed.
  • Configured lint excluding the two pre-existing ST1005 findings in unchanged dsl/manifests.go: 0 issues.
  • git diff --check, gofmt, DCO, and sensitive-string checks: passed.

No separate QA run is needed; the parent PR's live topology test remains the integration gate.

Issues

Relates to #1305
Relates to #1292

Checklist

  • I am familiar with the Contributing Guidelines.
  • I have signed off my commits for Developer Certificate of Origin (DCO) compliance.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

Require the plaintext Watch probe to report the expected TLS transport failure, and assert that the wiring suite executes the exact probe.

Relates-to: #1305
Signed-off-by: Mike Camp <mcamp@nvidia.com>
@mikeyrcamp
mikeyrcamp requested a review from a team as a code owner August 28, 2026 19:00
@mikeyrcamp
mikeyrcamp requested a review from borao August 28, 2026 19:00
@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (1)
  • main

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 2542b2f2-8ea7-4f0b-8ce9-d6af75f8131b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

Assert the client-observable grpcurl deadline from a verified TLS listener while rejecting unrelated command, proto, usage, and endpoint failures.

Relates-to: #1305
Signed-off-by: Mike Camp <mcamp@nvidia.com>
@mikeyrcamp

Copy link
Copy Markdown
Contributor Author

Correction pushed in 760c73f5 after independent review.

Evidence:

  • grpcurl 1.9.3 against a real local TLS listener reports the client-observable context deadline exceeded diagnostic for the plaintext Watch attempt; the previously asserted TLS-parser phrase is server-side and cannot satisfy the client assertion.
  • The normalizer now accepts only that evidenced diagnostic and emits a stable rejection marker.
  • Focused red/green tests reject missing-binary, connection-refused, proto/import, and usage failures.
  • The exact plaintext Watch command assertion remains, with the adjacent verified OpenSSL handshake and successful trusted TLS Watch establishing endpoint health.
  • Focused tests, go test -short -count=1 ./..., go vet ./..., ShellCheck, diff/DCO, and sensitive-string checks pass. Configured lint is clean except for two pre-existing ST1005 findings in an unchanged file.

The PR body and before/after Mermaid diagrams now describe the client-observable behavior. Ready for independent re-review; no merge requested.

Match only the complete grpcurl dial-stage diagnostic for the expected listener so successful Watch snapshots followed by RPC deadlines cannot satisfy the plaintext rejection check.

Relates-to: #1305
Signed-off-by: Mike Camp <mcamp@nvidia.com>
@mikeyrcamp

Copy link
Copy Markdown
Contributor Author

Strict dial-stage correction pushed in 0a851365.

The previous substring match could accept a successful plaintext Watch snapshot followed by the normal stream DeadlineExceeded. The parser now requires the complete target-specific dial failure line and rejects all JSON, RPC status, additional output, and wrong-target diagnostics.

Validation:

  • Reproduced the exact accepted line with grpcurl 1.9.3 against a real local TLS listener on port 50071.
  • Added the snapshot-plus-RPC-deadline regression test and observed it fail before the strict comparison, then pass afterward.
  • Focused diagnostic and wiring tests, full short tests, vet, bash syntax, ShellCheck, diff/DCO, and sensitive-string checks pass.
  • Pinned lint reports only two pre-existing ST1005 findings in an unchanged file; the baseline-excluded run reports 0 issues.

The PR body and Mermaid diagrams now describe the strict full-line decision. Head 0a851365c5aa99b0092036cb5ceb0c6cc2540acd is ready for another independent re-review; no merge requested.

@mikeyrcamp

mikeyrcamp commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Independent review of current head 0a85136 found no Critical, Important, or Minor issues.

The strict target-specific dial-stage diagnostic and its snapshot-plus-RPC-deadline regression coverage passed, along with focused wiring tests, the short BDD suite, Go vet, ShellCheck, configured baseline-excluded lint, DCO, sensitive-string checks, and diff hygiene.

CodeRabbit skipped review because this PR targets a stacked, non-default base. #1323 is ready to integrate into #1308; the parent live topology test remains the integration gate. No merge was performed.

@sbaum1994
sbaum1994 merged commit 57d1b8b into test/bdd-1292-secure-multicluster Aug 28, 2026
3 checks passed
@sbaum1994
sbaum1994 deleted the mcamp/fix/pr1308-coderabbit branch August 28, 2026 20:16
sbaum1994 pushed a commit that referenced this pull request Aug 29, 2026
Signed-off-by: Mike Camp <mcamp@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants