[creds]: Split transport credentials into role-typed resolvers#94
Open
pseudomuto wants to merge 1 commit into
Open
[creds]: Split transport credentials into role-typed resolvers#94pseudomuto wants to merge 1 commit into
pseudomuto wants to merge 1 commit into
Conversation
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.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
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.Dialerandcreds.Listenerbuilt from shared options (Insecure,WithCA,WithCertificate) and a resolvedMode. - Extracts reusable X.509/PEM validation into
pkg/validation/certsand 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. |
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
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.
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.