Skip to content

test(bdd): require TLS rejection diagnostics - #1324

Merged
sbaum1994 merged 2 commits into
test/bdd-1292-fail-closed-tlsfrom
mcamp/fix/pr1309-coderabbit
Aug 28, 2026
Merged

test(bdd): require TLS rejection diagnostics#1324
sbaum1994 merged 2 commits into
test/bdd-1292-fail-closed-tlsfrom
mcamp/fix/pr1309-coderabbit

Conversation

@mikeyrcamp

@mikeyrcamp mikeyrcamp commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

TL;DR

Fixes the two unresolved CodeRabbit findings on #1309 by making the fail-closed BDD checks prove certificate identity and the intended rejection cause.

Additional Details

  • Adds explicit hostname verification to the successful OpenSSL probe.
  • Checks grpcurl availability before the topology bootstrap.
  • Captures each negative probe's diagnostic and accepts only the expected trust, hostname, plaintext-protocol, or invalid-authority rejection.
  • Updates the non-live Godog wiring test to preserve those requirements.

This PR is stacked on #1309 so its diff remains limited to the review fixes.

Before and after

Before

flowchart LR
  S["Successful TLS probe"] --> I["Set SNI only"]
  I --> V["Chain and ALPN checks pass"]
  V --> G["Hostname identity is not explicitly verified"]
  N["Negative probe"] --> E["Any nonzero exit"]
  U["Missing grpcurl, proto error, or unrelated render failure"] --> E
  E --> P["Negative case passes"]
Loading

After

flowchart LR
  P["Preflight grpcurl"] --> A{"Available?"}
  A -- "No" --> F["Test fails"]
  A -- "Yes" --> S["Successful TLS probe"]
  S --> V["Verify SNI, hostname, trust chain, and ALPN h2"]
  A -- "Yes" --> N["Negative probe"]
  N --> C["Capture diagnostic"]
  C --> M{"Matches the case-specific rejection?"}
  M -- "Trust, hostname, plaintext, or authority" --> X["Negative case passes"]
  M -- "Anything else" --> F
Loading

Integration dependency

#1309 was opened before #1300 merged. Its current base rejects the explicit HTTPS worker URI and emits the legacy authority-validation diagnostic, while this companion intentionally follows the secure-ingress behavior now on main.

Safest integration order:

  1. Incorporate this companion into test(bdd): cover fail-closed Pylon TLS registration #1309.
  2. Rebase test(bdd): cover fail-closed Pylon TLS registration #1309 onto current main, which contains feat(self-managed): secure LLM gRPC ingress #1300.
  3. Rerun the live fail-closed feature against that combined history.

Do not weaken the HTTPS expectation or restore the legacy diagnostic to make the pre-#1300 base pass.

For the Reviewer

Please review the negative command assertions in tests/bdd/features/multi-cluster-helmfile-llm-registration-tls-fail-closed.feature and their wiring coverage in tests/bdd/godog_test.go.

For QA

  • go test -short -count=1 ./... from tests/bdd: passed.
  • go vet ./... from tests/bdd: passed.
  • Revive line-length policy on the changed Go file: passed.
  • git diff --check: passed.
  • Public-snapshot sensitive-string scan: passed.

A fresh destructive live run remains part of the dependent secure multi-cluster validation matrix.

Issues

Relates to #1306
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.

Signed-off-by: Mike Camp <mcamp@nvidia.com>
@mikeyrcamp
mikeyrcamp requested a review from a team as a code owner August 28, 2026 19:03
@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: 2278f2d0-25ef-4d16-bd74-1519c8654350

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.

Signed-off-by: Mike Camp <mcamp@nvidia.com>
@mikeyrcamp

Copy link
Copy Markdown
Contributor Author

Independent review follow-up:

  • Fixed the grpcurl preflight in c7d7850f90dca96cba681f213c523a0dc98f20b0. The BDD runner executes shlex-parsed argv directly, so the feature now invokes the shell builtin through /bin/sh -c, with an exact wiring assertion to prevent regression.
  • Verified that current main contains feat(self-managed): secure LLM gRPC ingress #1300's explicit HTTP/HTTPS authority parsing and secure-ingress validation. test(bdd): cover fail-closed Pylon TLS registration #1309 predates that merge, so its current base cannot validate this companion's HTTPS behavior standalone.

Safest integration order: incorporate #1324 into #1309, rebase #1309 onto current main, then rerun the live fail-closed feature. The HTTPS expectation and current validation diagnostic should not be weakened for the older base.

Post-fix validation passed: focused wiring test, full short BDD suite, go vet, changed-file Revive policy, diff check, DCO, and sensitive-string scan.

@mikeyrcamp

mikeyrcamp commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

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

The corrected grpcurl preflight and exact wiring assertion passed, along with focused coverage, the short BDD suite, Go vet, changed-file revive policy, DCO, sensitive-string checks, and diff hygiene.

CodeRabbit skipped review because this PR targets a stacked, non-default base. Readiness remains gated on incorporating #1324 into #1309, rebasing #1309 onto current main with #1300, and rerunning the live fail-closed feature. No merge was performed.

@sbaum1994
sbaum1994 merged commit fdc3be3 into test/bdd-1292-fail-closed-tls Aug 28, 2026
3 checks passed
@sbaum1994
sbaum1994 deleted the mcamp/fix/pr1309-coderabbit branch August 28, 2026 20:18
sbaum1994 pushed a commit that referenced this pull request Aug 29, 2026
Signed-off-by: Mike Camp <mcamp@nvidia.com>
sbaum1994 pushed a commit that referenced this pull request Aug 30, 2026
Signed-off-by: Mike Camp <mcamp@nvidia.com>
sbaum1994 pushed a commit that referenced this pull request Aug 30, 2026
Signed-off-by: Mike Camp <mcamp@nvidia.com>
sbaum1994 pushed a commit that referenced this pull request Aug 30, 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