Use go-certmanager library for unified cert management#91
Conversation
Replace pre-release pseudo-version with go-certmanager v0.1.1, adapting to all API changes and introducing a two-manager architecture: server.Service for the gRPC API listener and client.Service for outgoing DKG sender connections. Key changes: - Remove fetcher/majordomo intermediary; use WithMajordomo() directly - Rename WithReloadTimeout -> WithLoadTimeout, TryReloadCertificate -> ReloadCertificate - Use credentials.NewGRPCClientCredentials() for sender TLS - Use credentials.NewServerTLSConfig() for API server TLS - Simplify SAN handling to DNS-only (IdentitySource is now uint) - Fix bug: sender parameters checked wrong nil variable - Fix bug: checker logged IdentitySource via string() instead of .String()
Reorder struct fields in sender/grpc parameters to satisfy attgo_struct_field_order. Reduce cyclomatic complexity in hasField (reflect-based numeric comparison) and SetupCerts (loop over slice).
|
|
||
| // ReleaseVersion is the release version for the code. | ||
| var ReleaseVersion = "1.2.1" | ||
| var ReleaseVersion = "1.2.1-dev-go-certmanager" |
There was a problem hiding this comment.
I think we can move to 1.3.0-rc.1
There was a problem hiding this comment.
Done in 8a5c3f2 — ReleaseVersion is now 1.3.0-rc.1.
| Dirk extracts the client identity from certificates following RFC 6125 compliance by prioritizing Subject Alternative Name (SAN) fields over the deprecated Common Name (CN). The identity extraction follows this priority order: | ||
|
|
||
| 1. **DNS names from SAN** - Most common for service-to-service authentication (e.g., `validator-01.example.com`) | ||
| 2. **IP addresses from SAN** - Valid for direct IP-based connections (e.g., `192.168.1.100` or `2001:db8::1`) |
There was a problem hiding this comment.
Are IP and email still valid or did we remove those from certmanager?
There was a problem hiding this comment.
You're right to flag this — the doc was overpromising. go-certmanager's san.ExtractIdentity only looks at DNS names in the SAN extension and falls back to the CN; IP, email, and URI SANs are never considered for identity extraction. The docs section has been rewritten in 750ccf1 to describe only DNS-SAN → CN, with an explicit note that other SAN types aren't used.
| - import latest go-eth2-wallet-store-s3 to enable force-path-style on S3 | ||
| - integrate go-certmanager for TLS certificate management | ||
| - use DNS SAN for client certificate identity extraction, with CN fallback | ||
| - support on-demand certificate reload on SIGHUP |
There was a problem hiding this comment.
Shamelessly copied from claude review:
SIGHUP only reloads the server cert, not the client cert. handleSignals (main.go:562) calls certManagerSvc.ReloadCertificate(ctx) only on the server cert manager. The client cert manager (used by sender / outbound DKG) is created with WithCACertURI and is never reloaded.
Maybe we should be explicit that we only reload the server cert and not the client cert?
There was a problem hiding this comment.
Good catch. Made it explicit in 26aa6b2 — the changelog line now reads
"support on-demand reload of the server certificate on SIGHUP (the client certificate used for outgoing DKG connections is not reloaded)".
Reloading the client cert is a follow-up if we ever need it; for now it's deliberately not in scope.
Shall we:
- Raise a new follow up issue for reloading client certs
- Rephrase the Changelog line
| **Note:** Dirk does not support URI-based SANs (such as SPIFFE IDs or HTTPS URIs) as these are not commonly used in validator/signer architectures and would complicate the permission matching system. | ||
|
|
||
| ### Client Identity Best Practices | ||
| Client identities should be fully qualified (_i.e._ `server.example.com` rather than just `server`) to avoid potential confusion with multiple clients of the same name in different domains. When using IP addresses, ensure they are static to maintain consistent authorization. |
There was a problem hiding this comment.
This says the identities should be fully qualified, but ClientCNOnlyCrt uses client-cn-only (unqualified). That's just a test cert, but the docs here warn against this.
Is ClientCNOnlyCrt purposely there to test the violation?
There was a problem hiding this comment.
Yes — ClientCNOnlyCrt is intentional. It's a backward-compat fixture (see testing/resources/certs.go:509) that exercises the CN fallback path for legacy certificates that pre-date RFC 6125-style DNS SANs.
The doc inconsistency was real, though: in 750ccf1 the Best Practices section now says fully qualified DNS SANs are recommended for new certificates while unqualified names (and the CN fallback) remain accepted for backward compatibility. That makes the fixture and the docs consistent.
| _, port, err := createTestServer(ctx, t, base, nil) | ||
| require.NoError(t, err) | ||
|
|
||
| time.Sleep(200 * time.Millisecond) |
There was a problem hiding this comment.
Is there a better way of handling this than a time.Sleep?
There was a problem hiding this comment.
Replaced in 1ca5151 with a waitForServerReady(t, port) helper that uses require.Eventually + net.DialTimeout to poll the listen address (5s deadline, 10ms interval). Since grpcapi.Service.serve calls lc.Listen synchronously before spawning the accept goroutine, the kernel-side bind happens during New(); the poll succeeds as soon as the accept loop is processing connections. All five time.Sleep(200ms) sites in the file now use the helper.
The previous wording claimed Dirk supports IP and email SANs as part of the identity extraction priority chain. This is not what the codebase actually does. The shared go-certmanager san.ExtractIdentity helper considers only DNS names from the SAN extension, falling back to the Common Name. IP, email, and URI SANs are never used for permission matching. Rewrite the Client Identity Extraction section to describe the real behaviour (DNS SAN -> CN fallback), and refresh the Best Practices section so it is consistent with the backward-compatibility allowance for CN-only certificates.
SIGHUP handling only invokes ReloadCertificate on the server certificate manager; the separate client certificate manager used for outgoing DKG connections is created once and never reloaded. Reflect this explicitly in the changelog so operators do not assume both certificates rotate together.
…d sleep The integration tests previously slept 200ms after starting the gRPC server to allow the accept goroutine to begin processing connections. Fixed sleeps are flaky under load and over-pad the common case. Replace them with a waitForServerReady helper that uses require.Eventually plus net.DialTimeout to poll until the listener accepts a TCP connection, with a 5s deadline. The helper drops in at each call site that previously called time.Sleep.
This branch introduces the go-certmanager integration, RFC 6125 SAN identity extraction, and on-demand server certificate reload, which are worth more than a patch bump on top of 1.2.1. Move to a 1.3.0 release candidate so the version reflects the scope of the change.
Running the custom golangci-lint with the attgo plugin surfaced a set
of housekeeping findings on files this branch already modifies:
- refresh copyright year ranges to include 2026 on files we have edited
(core/stores.go, main.go, services/api/grpc/parameters.go,
services/api/grpc/service.go, testing/logger/capture.go,
testing/resources/certs.go).
- reorder struct fields to satisfy the attgo_struct_field_order rule
(categories: dependency before data, metrics before data, data before
synchronization) in:
* Credentials in services/checker/service.go
* parameters in services/api/grpc/parameters.go
* Service in services/sender/grpc/service.go
* LogCapture in testing/logger/capture.go
The remaining attgo findings touch files that are unchanged on this
branch (package-level loggers and pre-existing struct ordering in
master), and are left for a follow-up clean-up pass so this PR stays
focused on the certmanager integration.
The EmptyContext setupCtx case wraps context.Background in a no-arg closure to match the table struct's func() context.Context type. The function value context.Background already has that signature, so use it directly. Removes a DeepSource CRT-A0018 finding.
testing/daemon.New is a single-purpose test harness that linearly constructs every service required to spin up a Dirk instance. It is already marked //nolint:maintidx for the same structural reason, and a meaningful refactor is out of scope for the certmanager integration. Add an explicit //skipcq:GO-R1005 directive so DeepSource stops flagging it for cyclomatic complexity.
Two extractions keep Check focused on the request-disposition decision while moving incidental work out of it: - buildCheckLogger encapsulates the client-identity logger construction (client, identity source, SAN DNS names). The nested SAN nil/length check no longer contributes to Check's branch count. - matchOperation walks a single path's operation entries and reports whether any entry matched and whether it allowed or denied the call. Check now skips non-matching paths with continue instead of nesting the operation loop inside a wallet/account-match conditional. DeepSource GO-R1005 previously reported Check at cyclomatic complexity 16. Behaviour is unchanged; existing static checker tests cover both allow and deny paths.
createTestServer was flagged by DeepSource for cyclomatic complexity 18 (GO-R1005), an unused testing.T parameter (RVV-B0012), and weak random number source for the listen port (GSC-G404). Address all three together: - Extract pickTestPort, which now sources the port from crypto/rand via cryptorand.Int. The t parameter is finally used to surface any unexpected reader failure through require.NoError, so the helper is also why t stops being unused. - Extract createCheckerService for the static-vs-mock checker choice, collapsing a duplicated if/else err-check pair into a single call. - Extract loadServerCertManager for the three test cert/key/CA reads plus the go-certmanager server manager construction, removing four err branches from the parent function. createTestServer now sits at cyclomatic complexity 13. Behaviour is unchanged; existing integration tests pass.
2 participants
Integrate go-certmanager library and unify certificate management by using server cert manager for both grpcapi (server) and sender (client) services and supporting on-demand certificate reload on SIGHUP.
This PR also adds RFC 6125-compliant SAN support for client certificate identity extraction on Dork.