Skip to content

[creds]: Split transport credentials into role-typed resolvers#94

Open
pseudomuto wants to merge 1 commit into
mainfrom
creds_refactor
Open

[creds]: Split transport credentials into role-typed resolvers#94
pseudomuto wants to merge 1 commit into
mainfrom
creds_refactor

Conversation

@pseudomuto

Copy link
Copy Markdown
Collaborator

The creds package was a shallow catalog: five near-parallel constructors, two loaders, and a wide set of validator methods. The decision of which credential a TLS config actually wants was re-derived at three call sites (proxy dialing, server listening, and config validation), which could drift; worse, the outbound validation rules (cert and key must be set together, a client cert requires a CA) lived only in config validation while the runtime trusted that validation had already run.

Replace the catalog with two role-typed constructors, NewDialer and NewListener, built from options (Insecure, WithCA, WithCertificate). Each resolves the TLS mode and cross-field legality once, and Validate, DialOption, and ServerOption all run the same checks, so validation and runtime can no longer disagree.

Secure is now the default: only Insecure() yields plaintext, and an empty client config verifies against the system root pool rather than silently downgrading (the public-CA upstream case, e.g. Temporal Cloud). Certificate material is parsed once per Dialer and reused across per-request dials.

Migrate config, proxy, and server to the new constructors, removing the hand-rolled decision trees and the per-upstream loader plumbing.

The creds package was a shallow catalog: five near-parallel
constructors, two loaders, and a wide set of validator methods. The
decision of which credential a TLS config actually wants was re-derived
at three call sites (proxy dialing, server listening, and config
validation), which could drift; worse, the outbound validation rules
(cert and key must be set together, a client cert requires a CA) lived
only in config validation while the runtime trusted that validation had
already run.

Replace the catalog with two role-typed constructors, NewDialer and
NewListener, built from options (Insecure, WithCA, WithCertificate).
Each resolves the TLS mode and cross-field legality once, and Validate,
DialOption, and ServerOption all run the same checks, so validation and
runtime can no longer disagree.

Secure is now the default: only Insecure() yields plaintext, and an
empty client config verifies against the system root pool rather than
silently downgrading (the public-CA upstream case, e.g. Temporal Cloud).
Certificate material is parsed once per Dialer and reused across
per-request dials.

Migrate config, proxy, and server to the new constructors, removing the
hand-rolled decision trees and the per-upstream loader plumbing.
@pseudomuto
pseudomuto requested a review from Copilot July 25, 2026 14:33
@pseudomuto
pseudomuto requested a review from a team as a code owner July 25, 2026 14:33
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.00000% with 24 lines in your changes missing coverage. Please review.

📢 Thoughts on this report? Let us know!

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors transport credential handling to be role-aware (client dialer vs server listener), centralizing TLS mode resolution and validation so config validation and runtime behavior can’t drift, while making “secure by default” the standard for outbound connections when TLS is configured.

Changes:

  • Introduces creds.Dialer and creds.Listener built from shared options (Insecure, WithCA, WithCertificate) and a resolved Mode.
  • Extracts reusable X.509/PEM validation into pkg/validation/certs and reuses it from transport credential validation.
  • Migrates config/proxy/server wiring and tests to the new credential constructors; adds an RSA cert test helper for real keypair loading.

Reviewed changes

Copilot reviewed 30 out of 31 changed files in this pull request and generated no comments.

Show a summary per file
File Description
pkg/validation/certs/doc.go Adds package documentation for reusable certificate validation checks.
pkg/validation/certs/certs.go Renames/reshapes certificate validators into certs package checks and updates APIs (Check, NotExpired, etc.).
pkg/validation/certs/certs_test.go Updates and extends tests for the new certs package API and behavior.
pkg/testutil/certs.go Adds GenerateRSACert helper producing a real RSA cert+key pair usable with tls.LoadX509KeyPair.
pkg/testutil/certs_test.go Adds test coverage for GenerateRSACert.
internal/transport/creds/validate.go Adds shared certificate-material validation logic per resolved TLS Mode.
internal/transport/creds/validate_test.go Adds tests asserting file/material validation behavior for dialer/listener modes.
internal/transport/creds/resolve.go Adds centralized TLS mode resolution for client vs server roles.
internal/transport/creds/options.go Adds options-based configuration and cross-field legality validation for client/server roles.
internal/transport/creds/material.go Adds lazy, cached parsing for outbound (dialer) cert/CA material.
internal/transport/creds/listener.go Introduces Listener to build inbound grpc.ServerOption with consistent legality checks.
internal/transport/creds/listener_test.go Adds listener mode/validation/server-option/encryption tests.
internal/transport/creds/dialer.go Introduces Dialer to build outbound grpc.DialOption, caching parsed material across calls.
internal/transport/creds/dialer_test.go Adds dialer mode/validation/dial-option tests including cached and concurrent reuse.
internal/transport/creds/tls.go Removes legacy TLS credential implementation (superseded by Listener/Dialer).
internal/transport/creds/tls_test.go Removes tests for the legacy TLS credential.
internal/transport/creds/mtls.go Removes legacy MTLS credential implementation (superseded by Listener/Dialer).
internal/transport/creds/mtls_test.go Removes tests for the legacy MTLS credential.
internal/transport/creds/insecure.go Removes legacy Insecure credential implementation (replaced by Insecure() option).
internal/transport/creds/insecure_test.go Removes tests for the legacy Insecure credential.
internal/transport/creds/doc.go Updates package docs to describe role-typed credentials, options, and secure defaults.
internal/server/server.go Switches default inbound credentials to creds.NewListener(creds.Insecure()).
internal/server/fx.go Simplifies server credential wiring to p.Config.Listen.TLS.Listener().
internal/proxy/server.go Switches local proxy inbound credentials to the new listener insecure option.
internal/proxy/fx.go Replaces per-request loader plumbing with a single per-upstream Dialer reused across dynamic resolution.
internal/config/upstream_test.go Updates expected error message for cert/key pairing.
internal/config/listen.go Moves TLS validation and outbound validation to Listener/Dialer derived from config options.
internal/config/listen_test.go Updates tests to reflect new “legality-first” listener validation semantics.
internal/config/config_test.go Updates expected validation tuples consistent with new listener legality errors.
e2e/harness_test.go Reuses testutil.GenerateRSACert instead of local RSA keypair generator.
.gitignore Adds docs/ to ignored paths.

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