Use go-certmanager library for unified cert management - #91
Open
AntiD2ta wants to merge 24 commits into
Open
Conversation
AntiD2ta
marked this pull request as draft
January 13, 2026 17:39
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).
AntiD2ta
marked this pull request as ready for review
April 16, 2026 08:16
Bez625
requested changes
May 27, 2026
|
|
||
| // ReleaseVersion is the release version for the code. | ||
| var ReleaseVersion = "1.2.1" | ||
| var ReleaseVersion = "1.2.1-dev-go-certmanager" |
Contributor
There was a problem hiding this comment.
I think we can move to 1.3.0-rc.1
Contributor
Author
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`) |
Contributor
There was a problem hiding this comment.
Are IP and email still valid or did we remove those from certmanager?
Contributor
Author
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 |
Contributor
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?
Contributor
Author
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. |
Contributor
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?
Contributor
Author
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) |
Contributor
There was a problem hiding this comment.
Is there a better way of handling this than a time.Sleep?
Contributor
Author
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.
When certificates.ca-cert is empty the API server now falls back to the host's system CA pool when building its client-verification TLS config, matching what docs/configuration.md already advertises and what the client cert manager has always done. Previously the server failed at startup with "CA certificate PEM is required when client cert verification is enabled", silently contradicting the docs. Also propagate certificates.load-timeout to the client certificate manager so a slow majordomo backend cannot hang the client manager's load indefinitely while the server manager honours the timeout.
…heck Identity extraction switched to prefer DNS SANs over the Common Name. For deployments whose client certificates still carry CN-keyed permissions that differ from the DNS SAN, requests now silently fail to match any rule. The static checker now emits a one-shot deprecation warning per (extracted_identity, common_name) pair whenever a request would have authorised under the legacy CN identity but is denied under the new SAN-DNS identity, giving operators time to retarget permissions.yaml before CN-based authorisation is removed. To support this, the certificate's Common Name is propagated through the context and onto checker.Credentials alongside the existing identity fields. Also replace the bare type assertion on grpcPeer.AuthInfo with a comma-ok variant; non-TLS or missing auth info now returns codes.Unauthenticated instead of crashing the request goroutine.
pickTestPort now probes the chosen port with net.Listen before returning it; previously a port already held by another process would surface as a confusing "address already in use" failure inside the test under test. The peer map in createTestServer also follows the actual server port so later peer-to-peer DKG coverage will not silently dial the wrong address. The grpc receiver test suite gains a per-test fixture (testServers) returned by createServers. The fixture owns the mock.Processes map and registers t.Cleanup to nil it out at test end, so the package-global no longer leaks state across tests. createServer now hands the process service back to the fixture rather than mutating the global itself. The duplicate WithSigner/WithLister mock overrides that silently shadowed the real signer/lister in TestEndToEnd are gone, and the unused createTestCertManager helper has been deleted. TestNonInitiator also resets mock.Processes on cleanup so its package-globals do not leak forward into the fixture-managed tests.
The integration tests now seed a wallet with one account and request that account's wallet path on every call. The static checker is always configured with a permission map keyed on the identity each test expects the server-side interceptor to extract; whether the response carries the seeded account therefore depends on what the interceptor actually put into the credentials. Concretely: - the DNS extraction test asserts the seeded account is listed only when permissions are keyed on the DNS-SAN identity; - the CN-only test asserts the same for the CN fallback; - the denied test connects as client-test02 against permissions for client-test01 and asserts the account is filtered out, replacing the previous empty-paths assertion that never reached the checker. The DNS-over-CN test now asserts that a permission keyed on a string absent from both CN and SAN does not authorise the seeded account — the strongest black-box assertion the bundled cert fixtures support (they share the same string for CN and DNS SAN).
SIGHUP reloads only the leaf server certificate. The trusted client CA pool and the outbound DKG client credentials are snapshotted at start and require a full restart to rotate — a behaviour that currently shows up only in folklore (and in the CHANGELOG for the DKG client). Promote that to a first-class operator note alongside the existing load-timeout description so rotations are planned, not discovered. While here, strengthen the startup warning emitted when certificates.ca-cert is empty. The server now falls back to the host's system trust store and will accept any publicly-trusted client certificate; that fact deserves a loud one-line warning at boot rather than the prior neutral message. A follow-up issue tracks adding actual refresh paths for the CA pool and the DKG client credentials.
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
2 participants
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.
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 Dirk.
ADR: client certificate identity source
Context. Identity extraction moved from
peerCert.Subject.CommonNametosan.ExtractIdentity, which prefers a DNS SAN over the CN (RFC 6125), with CN fallback. For a deployment whose client certificate carries both a CN that matchespermissions.yamland a different DNS SAN, the upgrade silently flips the extracted identity to the SAN and breaks permission matching with no migration signal.Decision (chosen). Ship a deprecation phase: when the extracted (SAN-DNS) identity differs from the certificate's CN and a permission entry exists for that CN, log a one-shot
warninstructing the operator to re-keypermissions.yamlon the SAN-DNS identity before CN-based authorisation is removed. This keeps the RFC 6125 behaviour as the default while giving operators an actionable, low-noise signal during migration. Implemented inservices/checker/static(warnCNFallbackIfApplicable), fired from the denial path and deduplicated per(extracted-identity, CN)pair.Alternatives considered (not chosen):
A
certificates.identity-sourceconfig flag (auto|san-dns|cn) to let operators pin extraction behaviour explicitly. Rejected for now: it adds a permanent configuration surface to paper over what should be a one-time migration, andautowould still need the same divergence heuristic underneath.A startup audit log of resolved identities — at boot, print the identity extracted from the first matching certificate of each configured permission, so operators can eyeball the mapping before serving traffic. Rejected as the primary mechanism: it fires only at startup (missing certificates first seen at runtime) and is noisier than the targeted on-divergence warning. It could complement the chosen approach later.