Skip to content

fix(trillianclient): strip dns:/// scheme from TLS ServerName in gRPC dial#2812

Merged
Hayden-IO merged 3 commits into
sigstore:mainfrom
kdacosta0:fix/strip-dns
May 21, 2026
Merged

fix(trillianclient): strip dns:/// scheme from TLS ServerName in gRPC dial#2812
Hayden-IO merged 3 commits into
sigstore:mainfrom
kdacosta0:fix/strip-dns

Conversation

@kdacosta0

Copy link
Copy Markdown
Contributor

Summary

When the Trillian gRPC address includes a dns:/// resolver scheme (used to enable client-side round_robin load balancing with headless Kubernetes Services), the dial() function passes the raw address, scheme included, into tls.Config.ServerName and the gRPC channel authority

This breaks TLS: the x509 certificate verification matches SANs against "dns" (the URI scheme) instead of the actual hostname, failing with:

x509: certificate is valid for trillian-logserver.ns.svc, not dns

The fix strips the dns:/// prefix into a cleanHostname for TLS ServerName and grpc.WithAuthority(), while preserving the full scheme in the grpc.NewClient target so the DNS resolver stays active

strings.TrimPrefix is a no-op when the prefix is absent, so callers passing plain hostnames are unaffected

Tested on an OpenShift cluster with internal TLS:

  • before: every Trillian gRPC call fails with the x509 error above
  • after: TLS handshake succeeds and round-robin load balancing works across all trillian-logserver pod IPs

Release Note

Fixed a bug where using the dns:/// gRPC resolver scheme in the Trillian server address caused TLS certificate verification to fail. The URI scheme was incorrectly used as the TLS ServerName instead of the actual hostname. This affected deployments using client-side gRPC load balancing (round_robin) with TLS enabled.

Documentation

@bobcallaway

Copy link
Copy Markdown
Member

could you add a test case here to ensure this doesn't break going forward?

@codecov

codecov Bot commented Apr 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 26.33%. Comparing base (488eb97) to head (a44c1f7).
⚠️ Report is 679 commits behind head on main.

Files with missing lines Patch % Lines
pkg/trillianclient/manager.go 75.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2812       +/-   ##
===========================================
- Coverage   66.46%   26.33%   -40.13%     
===========================================
  Files          92      191       +99     
  Lines        9258    20261    +11003     
===========================================
- Hits         6153     5336      -817     
- Misses       2359    14088    +11729     
- Partials      746      837       +91     
Flag Coverage Δ
e2etests 49.44% <75.00%> (+1.88%) ⬆️
unittests 16.95% <25.00%> (-30.73%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bobcallaway

Copy link
Copy Markdown
Member

could you add a test case here to ensure this doesn't break going forward?

sorry, i should have been clearer. in tests/e2e_test.go, TestDialE2E I think can be easily altered to add setting dns://localhost as a value (instead of hardcoding it to localhost which will provide functional coverage of this.

@bobcallaway

Copy link
Copy Markdown
Member

can you rebase this please and i'll merge

@kdacosta0

Copy link
Copy Markdown
Contributor Author

@bobcallaway Rebased and pushed, lmk if you need anything else

@Hayden-IO Hayden-IO merged commit a10818a into sigstore:main May 21, 2026
16 checks passed
@kdacosta0 kdacosta0 deleted the fix/strip-dns branch May 26, 2026 10:17
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.

3 participants