From 1dfe88af77b7ce78817c4521b59e8a6368272dd3 Mon Sep 17 00:00:00 2001 From: Thiago Durante Date: Mon, 14 Sep 2026 16:04:35 +0200 Subject: [PATCH 01/15] feat(protocol): add renew request/response commands Adds command bytes 8 (RENEW_REQUEST, agent -> server) and 9 (RENEW_RESPONSE, server -> agent) plus their codecs, so an agent can ask the backend to re-issue its client certificate over the existing tunnel instead of a new authenticated endpoint. RENEW_REQUEST carries a UTF-8 implementation/version identifier ("go/"). RENEW_RESPONSE carries [status:1][body] where status is 0 RENEWED (body = new certificate PEM), 1 CURRENT (no body) or 2 ERROR (body = diagnostic message). The same three bytes and statuses are implemented by the DeployHQ backend and the Ruby deploy-agent gem; they must not drift. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_011XaiFEqsyQGcLhRmBtKRFA --- internal/protocol/commands.go | 28 +++++++++++ internal/protocol/framing.go | 33 +++++++++++++ internal/protocol/renew_test.go | 82 +++++++++++++++++++++++++++++++++ 3 files changed, 143 insertions(+) create mode 100644 internal/protocol/renew_test.go diff --git a/internal/protocol/commands.go b/internal/protocol/commands.go index a62afa5..fdb988a 100644 --- a/internal/protocol/commands.go +++ b/internal/protocol/commands.go @@ -9,4 +9,32 @@ const ( CmdReject byte = 5 CmdReconnect byte = 6 CmdKeepalive byte = 7 + + // CmdRenewRequest is sent by the agent once per connection, immediately + // after the TLS handshake. Its payload is a UTF-8 implementation/version + // identifier such as "go/0.3.0"; the server decides whether the agent's + // client certificate needs re-issuing under a newer CA. + CmdRenewRequest byte = 8 + + // CmdRenewResponse is the server's answer to CmdRenewRequest. Its payload + // is [status:1][body], see the RenewStatus* constants. The server never + // sends it unsolicited. + CmdRenewResponse byte = 9 +) + +// Renewal response statuses, carried in the first payload byte of a +// CmdRenewResponse frame. +const ( + // RenewStatusRenewed means the body holds a new client certificate in PEM + // form, re-issued for the agent's existing key pair. + RenewStatusRenewed byte = 0 + + // RenewStatusCurrent means the agent's certificate is already current. + // There is no body. + RenewStatusCurrent byte = 1 + + // RenewStatusError means renewal failed server-side. The body is a UTF-8 + // diagnostic message; the agent logs it and carries on with its current + // certificate. + RenewStatusError byte = 2 ) diff --git a/internal/protocol/framing.go b/internal/protocol/framing.go index af65259..e786916 100644 --- a/internal/protocol/framing.go +++ b/internal/protocol/framing.go @@ -79,6 +79,21 @@ func EncodeKeepalive() []byte { return EncodePacket(CmdKeepalive, nil) } +// EncodeRenewRequest encodes a RENEW_REQUEST packet. The payload is the UTF-8 +// implementation/version identifier of the agent, e.g. "go/0.3.0". +func EncodeRenewRequest(identifier string) []byte { + return EncodePacket(CmdRenewRequest, []byte(identifier)) +} + +// EncodeRenewResponse encodes a RENEW_RESPONSE packet. Only the server sends +// these; the agent side exists so tests can drive the full exchange. +func EncodeRenewResponse(status byte, body []byte) []byte { + payload := make([]byte, 1+len(body)) + payload[0] = status + copy(payload[1:], body) + return EncodePacket(CmdRenewResponse, payload) +} + // --- Per-command parse helpers --- // ParseCreateRequest parses a CREATE_REQUEST payload into connID, host, port. @@ -122,6 +137,24 @@ func ParseDestroy(payload []byte) (connID uint16, ok bool) { return } +// ParseRenewRequest parses a RENEW_REQUEST payload, returning the agent's +// implementation/version identifier. An empty payload is valid but yields "". +func ParseRenewRequest(payload []byte) (identifier string) { + return string(payload) +} + +// ParseRenewResponse parses a RENEW_RESPONSE payload into its status byte and +// body. ok is false when the payload is too short to carry a status. +func ParseRenewResponse(payload []byte) (status byte, body []byte, ok bool) { + if len(payload) < 1 { + return + } + status = payload[0] + body = payload[1:] + ok = true + return +} + // ParseData parses a DATA payload and returns connID + data bytes. func ParseData(payload []byte) (connID uint16, data []byte, ok bool) { if len(payload) < 2 { diff --git a/internal/protocol/renew_test.go b/internal/protocol/renew_test.go new file mode 100644 index 0000000..7fcebad --- /dev/null +++ b/internal/protocol/renew_test.go @@ -0,0 +1,82 @@ +package protocol_test + +import ( + "bytes" + "testing" + + "github.com/deployhq/network-agent/internal/protocol" +) + +// The renewal command bytes are part of a wire contract shared with the Ruby +// agent and the DeployHQ backend. Pin them so a reorder of the const block +// cannot silently change what goes on the wire. +func TestRenewCommandBytes(t *testing.T) { + if protocol.CmdRenewRequest != 8 { + t.Errorf("CmdRenewRequest = %d, want 8", protocol.CmdRenewRequest) + } + if protocol.CmdRenewResponse != 9 { + t.Errorf("CmdRenewResponse = %d, want 9", protocol.CmdRenewResponse) + } + if protocol.RenewStatusRenewed != 0 || protocol.RenewStatusCurrent != 1 || protocol.RenewStatusError != 2 { + t.Errorf("renew statuses = %d/%d/%d, want 0/1/2", + protocol.RenewStatusRenewed, protocol.RenewStatusCurrent, protocol.RenewStatusError) + } +} + +func TestEncodeRenewRequest(t *testing.T) { + frame := protocol.EncodeRenewRequest("go/1.2.3") + + want := []byte{0x00, 0x0b, 0x08, 'g', 'o', '/', '1', '.', '2', '.', '3'} + if !bytes.Equal(frame, want) { + t.Fatalf("frame = % x, want % x", frame, want) + } + + packets, remaining := protocol.DecodePackets(frame) + if len(packets) != 1 || len(remaining) != 0 { + t.Fatalf("decoded %d packets, %d remaining bytes", len(packets), len(remaining)) + } + if got := protocol.ParseRenewRequest(packets[0].Payload); got != "go/1.2.3" { + t.Errorf("identifier = %q, want %q", got, "go/1.2.3") + } +} + +func TestRenewResponseRoundtrip(t *testing.T) { + tests := []struct { + name string + status byte + body []byte + }{ + {"renewed", protocol.RenewStatusRenewed, []byte("-----BEGIN CERTIFICATE-----\n")}, + {"current", protocol.RenewStatusCurrent, nil}, + {"error", protocol.RenewStatusError, []byte("no certificate on file")}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + frame := protocol.EncodeRenewResponse(tt.status, tt.body) + packets, remaining := protocol.DecodePackets(frame) + if len(packets) != 1 || len(remaining) != 0 { + t.Fatalf("decoded %d packets, %d remaining bytes", len(packets), len(remaining)) + } + if packets[0].Cmd != protocol.CmdRenewResponse { + t.Fatalf("cmd = %d, want %d", packets[0].Cmd, protocol.CmdRenewResponse) + } + status, body, ok := protocol.ParseRenewResponse(packets[0].Payload) + if !ok { + t.Fatal("ParseRenewResponse reported a malformed payload") + } + if status != tt.status { + t.Errorf("status = %d, want %d", status, tt.status) + } + if !bytes.Equal(body, tt.body) && !(len(body) == 0 && len(tt.body) == 0) { + t.Errorf("body = %q, want %q", body, tt.body) + } + }) + } +} + +func TestParseRenewResponseRejectsEmptyPayload(t *testing.T) { + if _, _, ok := protocol.ParseRenewResponse(nil); ok { + t.Error("empty payload should not parse as a renew response") + } +} From e392b07b2d6addfaead0aeefdfff2ccde479af54 Mon Sep 17 00:00:00 2001 From: Thiago Durante Date: Mon, 14 Sep 2026 16:06:06 +0200 Subject: [PATCH 02/15] feat(tls): reload client certificate per handshake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A single *tls.Config is built once at start-up and reused by RunAgent for every reconnect, so the client certificate cached in cfg.Certificates kept being presented until the process restarted. Replacing ~/.deploy/agent.crt on disk had no effect on a running agent — verified against a real backend before this change. Leave cfg.Certificates nil and load the key pair from disk in GetClientCertificate instead, so a renewed certificate takes effect on the next reconnect with no customer-side restart. The pair is still loaded once up front so a missing or mismatched cert/key is a hard error at start-up rather than a puzzling handshake failure later. Also extracts NewCertPool so the same bundle parsing is shared with certificate-renewal validation instead of being duplicated. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_011XaiFEqsyQGcLhRmBtKRFA --- internal/config/tls.go | 42 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 8 deletions(-) diff --git a/internal/config/tls.go b/internal/config/tls.go index 9f07128..db973a4 100644 --- a/internal/config/tls.go +++ b/internal/config/tls.go @@ -8,9 +8,21 @@ import ( "strings" ) +// NewCertPool parses a PEM bundle into a certificate pool. The bundle may hold +// more than one CA — that is how an agent trusts the old and the new DeployHQ +// CA simultaneously during a CA rotation. +func NewCertPool(caCert []byte) (*x509.CertPool, error) { + pool := x509.NewCertPool() + if !pool.AppendCertsFromPEM(caCert) { + return nil, fmt.Errorf("failed to parse CA certificate") + } + return pool, nil +} + // NewTLSConfig builds a mutual-TLS config for the agent: -// - Client certificate: ~/.deploy/agent.crt + agent.key -// - Server verification: embedded CA cert passed in via caCert +// - Client certificate: ~/.deploy/agent.crt + agent.key, re-read from disk on +// every handshake (see below) +// - Server verification: CA bundle passed in via caCert // // When verify is false, server certificate verification is skipped (dev/testing). // @@ -20,13 +32,27 @@ import ( // VerifyConnection callback that checks the CA chain and then falls back to CN // matching when no SANs are present. func NewTLSConfig(paths Paths, caCert []byte, verify bool) (*tls.Config, error) { - cert, err := tls.LoadX509KeyPair(paths.Certificate, paths.Key) - if err != nil { + // Load once up front so a missing, unreadable or mismatched key pair is a + // hard error at startup rather than a puzzling handshake failure later. + if _, err := tls.LoadX509KeyPair(paths.Certificate, paths.Key); err != nil { return nil, fmt.Errorf("loading agent certificate: %w", err) } cfg := &tls.Config{ - Certificates: []tls.Certificate{cert}, + // Certificates is deliberately left nil. A single *tls.Config is built + // once at start-up and reused for every reconnect, so a certificate + // cached here would keep being presented until the process restarted — + // which would defeat certificate renewal, where the agent replaces + // agent.crt in place and simply reconnects. Reading the key pair per + // handshake instead makes the renewed certificate take effect on the + // very next connection, with no customer-side restart. + GetClientCertificate: func(*tls.CertificateRequestInfo) (*tls.Certificate, error) { + cert, err := tls.LoadX509KeyPair(paths.Certificate, paths.Key) + if err != nil { + return nil, fmt.Errorf("loading agent certificate: %w", err) + } + return &cert, nil + }, } if !verify { @@ -34,9 +60,9 @@ func NewTLSConfig(paths Paths, caCert []byte, verify bool) (*tls.Config, error) return cfg, nil } - pool := x509.NewCertPool() - if !pool.AppendCertsFromPEM(caCert) { - return nil, fmt.Errorf("failed to parse CA certificate") + pool, err := NewCertPool(caCert) + if err != nil { + return nil, err } serverHost := ServerHost() From 6e9c37d858db4328d7592729bb23a1458089ccd8 Mon Sep 17 00:00:00 2001 From: Thiago Durante Date: Mon, 14 Sep 2026 16:16:19 +0200 Subject: [PATCH 03/15] feat(caroot): expose the embedded CA bundle as parsed certificates ca.crt is a PEM bundle, not a single certificate: during the CA rotation it will hold both the outgoing and the incoming DeployHQ CA so one binary verifies an agent server presenting a leaf from either. Certificates() parses every block in it and a test reports the count and subjects, so appending the new CA before a release is a visible, verifiable change rather than an unchecked assumption. Right now it holds exactly one: CN=Deploy Dev CA (charlie), expiring 2027-03-17. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_011XaiFEqsyQGcLhRmBtKRFA --- internal/caroot/caroot.go | 48 ++++++++++++++++++++++++++++- internal/caroot/caroot_test.go | 56 ++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 1 deletion(-) create mode 100644 internal/caroot/caroot_test.go diff --git a/internal/caroot/caroot.go b/internal/caroot/caroot.go index 5e9bdc5..5507e08 100644 --- a/internal/caroot/caroot.go +++ b/internal/caroot/caroot.go @@ -1,7 +1,53 @@ // Package caroot embeds the DeployHQ CA certificate used to verify the agent server. +// +// ca.crt is a PEM bundle, not necessarily a single certificate: during a CA +// rotation it holds both the outgoing and the incoming CA so that one binary +// verifies an agent server presenting a leaf from either. package caroot -import _ "embed" +import ( + "crypto/x509" + "encoding/pem" + "fmt" + + _ "embed" +) //go:embed ca.crt var CACert []byte + +// Certificates parses every certificate in the embedded bundle, in file order. +// +// It exists so the number of trusted CAs is observable — by `network-agent +// check`, by tests, and by anyone verifying that a release actually shipped the +// bundle it was supposed to. +func Certificates() ([]*x509.Certificate, error) { + return ParseBundle(CACert) +} + +// ParseBundle parses every CERTIFICATE block in a PEM bundle. Non-certificate +// blocks are skipped; any unparsable certificate block, or a bundle holding no +// certificates at all, is an error. +func ParseBundle(pemData []byte) ([]*x509.Certificate, error) { + var certs []*x509.Certificate + rest := pemData + for { + var block *pem.Block + block, rest = pem.Decode(rest) + if block == nil { + break + } + if block.Type != "CERTIFICATE" { + continue + } + cert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + return nil, fmt.Errorf("parsing certificate %d in bundle: %w", len(certs)+1, err) + } + certs = append(certs, cert) + } + if len(certs) == 0 { + return nil, fmt.Errorf("no certificates found in bundle") + } + return certs, nil +} diff --git a/internal/caroot/caroot_test.go b/internal/caroot/caroot_test.go new file mode 100644 index 0000000..6f0e155 --- /dev/null +++ b/internal/caroot/caroot_test.go @@ -0,0 +1,56 @@ +package caroot_test + +import ( + "testing" + "time" + + "github.com/deployhq/network-agent/internal/caroot" +) + +// TestEmbeddedBundleParses is the release-time guard for the CA bundle: it +// proves the embedded PEM is well formed and reports how many CAs it holds, so +// appending the new DeployHQ CA before a release is a visible, verifiable +// change rather than an unchecked assumption. +func TestEmbeddedBundleParses(t *testing.T) { + certs, err := caroot.Certificates() + if err != nil { + t.Fatalf("parsing embedded CA bundle: %v", err) + } + if len(certs) < 1 { + t.Fatal("embedded CA bundle holds no certificates") + } + + t.Logf("embedded CA bundle holds %d certificate(s)", len(certs)) + for i, c := range certs { + t.Logf(" [%d] subject=%q issuer=%q serial=%s notAfter=%s isCA=%v", + i, c.Subject.String(), c.Issuer.String(), c.SerialNumber, + c.NotAfter.UTC().Format(time.RFC3339), c.IsCA) + + if !c.IsCA { + t.Errorf("[%d] %q is not a CA certificate", i, c.Subject.CommonName) + } + } +} + +// TestParseBundleRejectsGarbage keeps ParseBundle honest: a bundle that does +// not parse must be an error, never an empty-but-successful result that would +// ship a binary trusting nothing. +func TestParseBundleRejectsGarbage(t *testing.T) { + tests := []struct { + name string + data []byte + }{ + {"empty", nil}, + {"not pem", []byte("hello, world")}, + {"pem but not a certificate", []byte("-----BEGIN RSA PRIVATE KEY-----\nZm9v\n-----END RSA PRIVATE KEY-----\n")}, + {"corrupt certificate block", []byte("-----BEGIN CERTIFICATE-----\nZm9v\n-----END CERTIFICATE-----\n")}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if _, err := caroot.ParseBundle(tt.data); err == nil { + t.Error("expected an error, got none") + } + }) + } +} From c21f94023db68023bedd3fefe8f06b69aa0b1fdc Mon Sep 17 00:00:00 2001 From: Thiago Durante Date: Mon, 14 Sep 2026 16:16:26 +0200 Subject: [PATCH 04/15] chore: remove unused root-level ca.crt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only internal/caroot/ca.crt is go:embed'ed. The root-level copy was byte-identical but nothing kept the two in sync and nothing read it — not the goreleaser file list, not install.sh, not the README. Two copies of a trust anchor with no mechanism keeping them equal is a trap during a CA rotation. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_011XaiFEqsyQGcLhRmBtKRFA --- ca.crt | 33 --------------------------------- 1 file changed, 33 deletions(-) delete mode 100644 ca.crt diff --git a/ca.crt b/ca.crt deleted file mode 100644 index 9233736..0000000 --- a/ca.crt +++ /dev/null @@ -1,33 +0,0 @@ ------BEGIN CERTIFICATE----- -MIIFszCCA5ugAwIBAgIBATANBgkqhkiG9w0BAQsFADB7MQswCQYDVQQGEwJHQjEP -MA0GA1UECAwGRG9yc2V0MSAwHgYDVQQDDBdEZXBsb3kgRGV2IENBIChjaGFybGll -KTEYMBYGA1UECgwPYVRlY2ggTWVkaWEgTHRkMQ4wDAYDVQQHDAVQb29sZTEPMA0G -A1UECwwGRGVwbG95MB4XDTE3MDMxNzE0MzE0NFoXDTI3MDMxNzE0MzE0NFowezEL -MAkGA1UEBhMCR0IxDzANBgNVBAgMBkRvcnNldDEgMB4GA1UEAwwXRGVwbG95IERl -diBDQSAoY2hhcmxpZSkxGDAWBgNVBAoMD2FUZWNoIE1lZGlhIEx0ZDEOMAwGA1UE -BwwFUG9vbGUxDzANBgNVBAsMBkRlcGxveTCCAiIwDQYJKoZIhvcNAQEBBQADggIP -ADCCAgoCggIBAM9uxfQPy7KGYOAgagcy1V+YqGsA2kXxtnmyg/tu+/JTJc+cfp6+ -UVh8T1uGf1gOVjTas4Zfz0iRvMxMNDz8AUBMv18Bxjrtw5J9vpSFvy5FKlxYBmbr -5CZf7OVtdpiCtK0dBxHnuasPbaBRLFfZuGEu8cA7Y2WgN1Sagj0254C0qGpVDgoF -ViB1cavGb9XZW4SDo4rTK/56YvxFCgy7E0Au/OSevvrfZloF6htUqkeq8RDl0Og6 -F0zFHUSK4w4tZhtpeov27cyacMoTP2ioXcWdUujuEz2yhgzVw/8h3obl84hqaXar -hWr+DUHbPw8EC0PI8qLNtmJHJ91zpEXgw0m2pyopyaOw60kmYo+P4HBEqYViWXMQ -2cYfi+5vWiVRQtaQKgd3+Ii8IPcU0GlQF1heMuX6Qnzf/3wIWKPTDKvly/seTC8A -8BYHABGFLWtir+rLaTD25fYySGzRlShWoT0otLZ5aOwA+W8CwJ+KLHvSNWaeRlqd -xz0NtJvsNnkY4mkIyWyDxCjiR14bOaC2TkCZgXh5teX9BuSKu6FX/ciwkZk4OpSW -VnDTQWERI1II/WjQlb6jPfkrcarld2PBaQidBdkZ72gCNp9lkvVeLLedkVhFPyIy -VXy6/tNOgPjlarjgDCrasWVKVFTGhbIobqgLPiKrJN6KWTjBIn4+o1TNAgMBAAGj -QjBAMB0GA1UdDgQWBBQqww4dAhh6km8+h6h+TQNOrbmPrTAPBgNVHRMBAf8EBTAD -AQH/MA4GA1UdDwEB/wQEAwIBBjANBgkqhkiG9w0BAQsFAAOCAgEAk3f/d6emYq5z -gTKa+mwvGZJEr4BvGUK47N+S9Se9zxXVL56gkxcPsfLqKkCijpx+0M1TR12+vY3u -niNj2FuxSdx9LOG5X+zXsNn0M3FkovMwIEulv5vQocHgONUvoQO8gh4QBhAADYhv -BFucBeda2zUtG/Tt1qREmpkOsBLFIl/9totULmXj47zpM57TqE8IVhfDD/hRB0Oy -sxRgtz5Ug4YYV0tf2rtO9pPbuKdZflAPS8S5x3bLHCMx6SFGpcP9jDxqCY4pA6RL -8HrT8kNjmgElKH8W/j6LAOK98znfKzWdXEuX/Wi2eb+wKSICvhY6qpuAmEAV0M95 -v799CtoDbiLdeSFR2i0KOoIcEkkyl66Icfbum8QMvyKqAP0+MFJw1f4u2ABvWrH2 -0IgC2ndJp5wopmYn2A0/cGHtlZs4COAK7whGQ3Y8cla+pat+/3YlG/tYnJm2yauv -pRlFQUHG1dQVTivWDCJReVbzFsxULxsLwRz+WteJVp3SL4J6A9cxZupNrr0EfHue -l4CPcGbB0L8yyIhGwiEfrZpjx6hOelX1daG8QPTvSSYpB6ODtQeb3zpDf8vU8M7T -oAwG8/0g1Owh/a970vIKu4TBa4D2IiCfA3KPWlsIUSoeu9uBTKmUQ0Raa0AhZWPv -JI4XgcL63KznYzLm0BOxvTYMxDfn7fs= ------END CERTIFICATE----- From 05a965c9cc8535154c6d1cb5a27b90f690ce4d03 Mon Sep 17 00:00:00 2001 From: Thiago Durante Date: Mon, 14 Sep 2026 16:16:41 +0200 Subject: [PATCH 05/15] test(testca): add DeployHQ-shaped certificate fixtures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Certificate shapes are load-bearing in this codebase and were previously re-derived ad hoc per test. DeployHQ's leaves carry a Common Name and nothing else — no subjectAltName, no extendedKeyUsage — which is exactly why NewTLSConfig needs a CN fallback and why renewal verification has to ask for ExtKeyUsageAny. Getting that wrong in a fixture makes a test pass for the wrong reason. testca mints CAs, CN-only leaves, and the renewal operation itself (re-signing an existing public key under another CA, preserving subject and serial). It is imported only from _test.go files, so it is not linked into the released binary. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_011XaiFEqsyQGcLhRmBtKRFA --- internal/testca/testca.go | 217 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 217 insertions(+) create mode 100644 internal/testca/testca.go diff --git a/internal/testca/testca.go b/internal/testca/testca.go new file mode 100644 index 0000000..0fe3143 --- /dev/null +++ b/internal/testca/testca.go @@ -0,0 +1,217 @@ +// Package testca mints certificates shaped like DeployHQ's real ones, for use +// in tests. It is imported only from _test.go files and so is not linked into +// the released binary. +// +// The shapes matter. DeployHQ's CA, its agent-server leaf and its agent client +// certificates all predate SAN-only verification: +// +// - CA: CN "Deploy Dev CA ()" under O=aTech Media Ltd, serial 1, +// basicConstraints CA:TRUE, keyUsage certSign+crlSign. +// - Leaves (both the server's and the agent's): a Common Name and nothing +// else — no subjectAltName, no extendedKeyUsage, no keyUsage. The missing +// SAN is exactly why config.NewTLSConfig needs its CN fallback, and the +// missing EKU is why certificate verification has to ask for +// ExtKeyUsageAny. +// +// Keys are ECDSA P-256 by default because tests generate a lot of them. +// Production uses 4096-bit RSA; pass KeyRSA where the key type is the thing +// under test. +package testca + +import ( + "crypto" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/rsa" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "math/big" + "testing" + "time" +) + +// KeyType selects the key algorithm for a generated certificate. +type KeyType int + +const ( + // KeyECDSA is a P-256 key: fast to generate, used everywhere the key + // algorithm is irrelevant to what the test is proving. + KeyECDSA KeyType = iota + // KeyRSA is a 2048-bit RSA key, matching production's algorithm (production + // uses 4096 bits; the size changes nothing that is under test here). + KeyRSA +) + +// CA is a self-signed certificate authority that can issue leaves. +type CA struct { + Cert *x509.Certificate + CertPEM []byte + + key crypto.Signer +} + +// Identity is a key pair plus the certificate currently issued for it. +type Identity struct { + Cert *x509.Certificate + CertPEM []byte + KeyPEM []byte + + key crypto.Signer +} + +// New creates a CA named "Deploy Dev CA ()". +func New(t *testing.T, name string) *CA { + t.Helper() + return NewWithKey(t, name, KeyECDSA) +} + +// NewWithKey creates a CA with an explicit key algorithm. +func NewWithKey(t *testing.T, name string, kt KeyType) *CA { + t.Helper() + key := generateKey(t, kt) + + tmpl := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: subject("Deploy Dev CA (" + name + ")"), + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(10 * 365 * 24 * time.Hour), + KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageCRLSign, + BasicConstraintsValid: true, + IsCA: true, + } + der, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, key.Public(), key) + if err != nil { + t.Fatalf("creating CA certificate: %v", err) + } + cert, err := x509.ParseCertificate(der) + if err != nil { + t.Fatalf("parsing CA certificate: %v", err) + } + return &CA{Cert: cert, CertPEM: certPEM(der), key: key} +} + +// Pool returns a certificate pool trusting just this CA. +func (ca *CA) Pool() *x509.CertPool { + pool := x509.NewCertPool() + pool.AddCert(ca.Cert) + return pool +} + +// Issue mints a CN-only leaf with a fresh key pair. +func (ca *CA) Issue(t *testing.T, commonName string, serial int64) *Identity { + t.Helper() + return ca.IssueWithKey(t, commonName, serial, KeyECDSA) +} + +// IssueWithKey mints a CN-only leaf with a fresh key pair of the given type. +func (ca *CA) IssueWithKey(t *testing.T, commonName string, serial int64, kt KeyType) *Identity { + t.Helper() + key := generateKey(t, kt) + certPEMBytes, cert := ca.sign(t, key.Public(), commonName, serial) + return &Identity{Cert: cert, CertPEM: certPEMBytes, KeyPEM: keyPEM(t, key), key: key} +} + +// Reissue signs an existing identity's public key again under this CA — the +// renewal operation the backend performs. Passing the identity's own common +// name and serial produces a valid renewal; passing different ones produces the +// certificates a correct agent must refuse. +func (ca *CA) Reissue(t *testing.T, id *Identity, commonName string, serial int64) []byte { + t.Helper() + certPEMBytes, _ := ca.sign(t, id.key.Public(), commonName, serial) + return certPEMBytes +} + +func (ca *CA) sign(t *testing.T, pub crypto.PublicKey, commonName string, serial int64) ([]byte, *x509.Certificate) { + t.Helper() + tmpl := &x509.Certificate{ + SerialNumber: big.NewInt(serial), + Subject: subject(commonName), + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(10 * 365 * 24 * time.Hour), + // No SANs, no KeyUsage, no ExtKeyUsage: exactly as DeployHQ mints them. + } + der, err := x509.CreateCertificate(rand.Reader, tmpl, ca.Cert, pub, ca.key) + if err != nil { + t.Fatalf("signing certificate for %q: %v", commonName, err) + } + cert, err := x509.ParseCertificate(der) + if err != nil { + t.Fatalf("parsing certificate for %q: %v", commonName, err) + } + return certPEM(der), cert +} + +// TLS returns the identity as a tls.Certificate. +func (id *Identity) TLS(t *testing.T) tls.Certificate { + t.Helper() + cert, err := tls.X509KeyPair(id.CertPEM, id.KeyPEM) + if err != nil { + t.Fatalf("building tls.Certificate: %v", err) + } + return cert +} + +// Bundle concatenates CA certificates into one PEM bundle, as internal/caroot's +// ca.crt holds them during a rotation. +func Bundle(cas ...*CA) []byte { + var out []byte + for _, ca := range cas { + out = append(out, ca.CertPEM...) + } + return out +} + +// Pool returns a pool trusting every given CA. +func Pool(cas ...*CA) *x509.CertPool { + pool := x509.NewCertPool() + for _, ca := range cas { + pool.AddCert(ca.Cert) + } + return pool +} + +// subject mirrors lib/certificate_authority.rb's distinguished name. +func subject(commonName string) pkix.Name { + return pkix.Name{ + Country: []string{"GB"}, + Province: []string{"Dorset"}, + Locality: []string{"Poole"}, + Organization: []string{"aTech Media Ltd"}, + OrganizationalUnit: []string{"Deploy"}, + CommonName: commonName, + } +} + +func generateKey(t *testing.T, kt KeyType) crypto.Signer { + t.Helper() + switch kt { + case KeyRSA: + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("generating RSA key: %v", err) + } + return key + default: + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatalf("generating ECDSA key: %v", err) + } + return key + } +} + +func certPEM(der []byte) []byte { + return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der}) +} + +func keyPEM(t *testing.T, key crypto.Signer) []byte { + t.Helper() + der, err := x509.MarshalPKCS8PrivateKey(key) + if err != nil { + t.Fatalf("marshalling private key: %v", err) + } + return pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: der}) +} From 2e48afc7a883ae7b87cab9e9bf3fcf843b7c9517 Mon Sep 17 00:00:00 2001 From: Thiago Durante Date: Mon, 14 Sep 2026 16:16:41 +0200 Subject: [PATCH 06/15] feat(config): allow overriding the CA bundle with DEPLOY_AGENT_CA_FILE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Until now the only way to change which CA an agent trusts was to ship a new binary. DEPLOY_AGENT_CA_FILE points the agent at a PEM bundle on disk instead — for staging, for a private agent server, and as the escape hatch that lets an operator trust a newly issued CA without waiting for a release. An unreadable or empty override is an error rather than a silent fall back to the embedded bundle: trusting a different CA than the operator asked for is worse than refusing to start. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_011XaiFEqsyQGcLhRmBtKRFA --- internal/config/ca.go | 35 ++++++++++++++++ internal/config/ca_test.go | 83 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+) create mode 100644 internal/config/ca.go create mode 100644 internal/config/ca_test.go diff --git a/internal/config/ca.go b/internal/config/ca.go new file mode 100644 index 0000000..44514c8 --- /dev/null +++ b/internal/config/ca.go @@ -0,0 +1,35 @@ +package config + +import ( + "fmt" + "os" +) + +// CAFileEnv names the environment variable that overrides the CA bundle used +// to verify the DeployHQ agent server. +const CAFileEnv = "DEPLOY_AGENT_CA_FILE" + +// CACert returns the CA bundle the agent should trust. +// +// By default this is the bundle compiled into the binary, passed in as +// embedded. Setting DEPLOY_AGENT_CA_FILE to a PEM file path replaces it — for +// staging, for a private agent server, and as the escape hatch that lets an +// operator trust a newly issued CA without waiting for a new release. +// +// An unreadable override is an error rather than a silent fall back to the +// embedded bundle: trusting a different CA than the operator asked for is worse +// than refusing to start. +func CACert(embedded []byte) ([]byte, error) { + path := os.Getenv(CAFileEnv) + if path == "" { + return embedded, nil + } + data, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("reading %s %q: %w", CAFileEnv, path, err) + } + if len(data) == 0 { + return nil, fmt.Errorf("reading %s %q: file is empty", CAFileEnv, path) + } + return data, nil +} diff --git a/internal/config/ca_test.go b/internal/config/ca_test.go new file mode 100644 index 0000000..3a38c6a --- /dev/null +++ b/internal/config/ca_test.go @@ -0,0 +1,83 @@ +package config_test + +import ( + "bytes" + "os" + "path/filepath" + "testing" + + "github.com/deployhq/network-agent/internal/config" + "github.com/deployhq/network-agent/internal/testca" +) + +func TestCACertDefaultsToTheEmbeddedBundle(t *testing.T) { + t.Setenv(config.CAFileEnv, "") + + embedded := testca.Bundle(testca.New(t, "embedded")) + got, err := config.CACert(embedded) + if err != nil { + t.Fatalf("CACert: %v", err) + } + if !bytes.Equal(got, embedded) { + t.Error("CACert did not return the embedded bundle") + } +} + +func TestCACertReadsTheOverrideFile(t *testing.T) { + embedded := testca.Bundle(testca.New(t, "embedded")) + override := testca.Bundle(testca.New(t, "operator supplied")) + + path := filepath.Join(t.TempDir(), "ca.pem") + if err := os.WriteFile(path, override, 0600); err != nil { + t.Fatal(err) + } + t.Setenv(config.CAFileEnv, path) + + got, err := config.CACert(embedded) + if err != nil { + t.Fatalf("CACert: %v", err) + } + if !bytes.Equal(got, override) { + t.Error("CACert did not return the override file's contents") + } + if bytes.Equal(got, embedded) { + t.Error("CACert returned the embedded bundle despite the override") + } +} + +// TestCACertFailsRatherThanFallingBack: silently trusting a different CA than +// the operator asked for is worse than refusing to start, so an unusable +// override must be an error and never a fallback to the embedded bundle. +func TestCACertFailsRatherThanFallingBack(t *testing.T) { + embedded := testca.Bundle(testca.New(t, "embedded")) + dir := t.TempDir() + + empty := filepath.Join(dir, "empty.pem") + if err := os.WriteFile(empty, nil, 0600); err != nil { + t.Fatal(err) + } + + tests := []struct { + name string + path string + }{ + {"missing file", filepath.Join(dir, "does-not-exist.pem")}, + {"a directory", dir}, + {"empty file", empty}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv(config.CAFileEnv, tt.path) + + got, err := config.CACert(embedded) + if err == nil { + t.Fatal("expected an error, got none") + } + if got != nil { + t.Error("CACert returned a bundle alongside an error") + } + t.Logf("rejected: %v", err) + }) + } +} From ace3bfa8113d0261b84d30925e32ca673a3ba5e0 Mon Sep 17 00:00:00 2001 From: Thiago Durante Date: Mon, 14 Sep 2026 16:16:41 +0200 Subject: [PATCH 07/15] test(config): cover bundle verification and per-handshake certificate reload internal/config had no tests at all. Adds the two properties the CA rotation depends on: - A two-CA bundle verifies an agent server presenting a leaf from either CA, in either bundle order, and rejects a third CA or a wrong CN. - Replacing agent.crt on disk changes what the very next handshake presents, proven against an in-process server with RequireAndVerifyClientCert reporting the issuer it was actually shown. Control run: reverting NewTLSConfig to the shipped behaviour (key pair cached in cfg.Certificates) fails the second test with 'second handshake presented issuer "Deploy Dev CA (old)"', reproducing the stale-certificate behaviour end to end. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_011XaiFEqsyQGcLhRmBtKRFA --- internal/config/tls_test.go | 304 ++++++++++++++++++++++++++++++++++++ 1 file changed, 304 insertions(+) create mode 100644 internal/config/tls_test.go diff --git a/internal/config/tls_test.go b/internal/config/tls_test.go new file mode 100644 index 0000000..388d785 --- /dev/null +++ b/internal/config/tls_test.go @@ -0,0 +1,304 @@ +package config_test + +import ( + "crypto/tls" + "crypto/x509" + "errors" + "net" + "os" + "path/filepath" + "testing" + "time" + + "github.com/deployhq/network-agent/internal/config" + "github.com/deployhq/network-agent/internal/testca" +) + +const serverCN = "agent.deployhq.com" + +// writeIdentity puts a key pair on disk the way `network-agent setup` does and +// returns the paths for it. +func writeIdentity(t *testing.T, id *testca.Identity) config.Paths { + t.Helper() + dir := t.TempDir() + paths := config.Paths{ + Config: dir, + Certificate: filepath.Join(dir, "agent.crt"), + Key: filepath.Join(dir, "agent.key"), + } + if err := os.WriteFile(paths.Certificate, id.CertPEM, 0600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(paths.Key, id.KeyPEM, 0600); err != nil { + t.Fatal(err) + } + return paths +} + +// TestVerifyConnectionAcceptsEitherCAInTheBundle is the property the whole CA +// rotation rests on: one binary carrying a two-CA bundle must accept an agent +// server presenting a leaf from either CA, and nothing else. +func TestVerifyConnectionAcceptsEitherCAInTheBundle(t *testing.T) { + t.Setenv("DEPLOY_AGENT_PROXY_IP", serverCN) + + oldCA := testca.New(t, "old") + newCA := testca.New(t, "new") + strangerCA := testca.New(t, "stranger") + + paths := writeIdentity(t, oldCA.Issue(t, "Deploy Agent #42", 42)) + + cfg, err := config.NewTLSConfig(paths, testca.Bundle(oldCA, newCA), true) + if err != nil { + t.Fatalf("NewTLSConfig: %v", err) + } + if cfg.VerifyConnection == nil { + t.Fatal("VerifyConnection was not installed") + } + + tests := []struct { + name string + leaf *x509.Certificate + wantErr bool + }{ + {"leaf from the outgoing CA", oldCA.Issue(t, serverCN, 1).Cert, false}, + {"leaf from the incoming CA", newCA.Issue(t, serverCN, 1).Cert, false}, + {"leaf from an untrusted CA", strangerCA.Issue(t, serverCN, 1).Cert, true}, + {"right CA, wrong common name", oldCA.Issue(t, "evil.example.com", 2).Cert, true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := cfg.VerifyConnection(tls.ConnectionState{ + PeerCertificates: []*x509.Certificate{tt.leaf}, + }) + if tt.wantErr && err == nil { + t.Error("expected verification to fail, it succeeded") + } + if !tt.wantErr && err != nil { + t.Errorf("expected verification to succeed, got: %v", err) + } + if err != nil { + t.Logf("rejected: %v", err) + } + }) + } + + t.Run("no certificate at all", func(t *testing.T) { + if err := cfg.VerifyConnection(tls.ConnectionState{}); err == nil { + t.Error("expected verification to fail when the server sends nothing") + } + }) +} + +// TestBundleOrderIsIrrelevant: nobody should have to think about which CA comes +// first in ca.crt when appending the new one. +func TestBundleOrderIsIrrelevant(t *testing.T) { + t.Setenv("DEPLOY_AGENT_PROXY_IP", serverCN) + + oldCA := testca.New(t, "old") + newCA := testca.New(t, "new") + paths := writeIdentity(t, oldCA.Issue(t, "Deploy Agent #42", 42)) + + leaves := map[string]*x509.Certificate{ + "old": oldCA.Issue(t, serverCN, 1).Cert, + "new": newCA.Issue(t, serverCN, 1).Cert, + } + + for name, bundle := range map[string][]byte{ + "old first": testca.Bundle(oldCA, newCA), + "new first": testca.Bundle(newCA, oldCA), + } { + t.Run(name, func(t *testing.T) { + cfg, err := config.NewTLSConfig(paths, bundle, true) + if err != nil { + t.Fatalf("NewTLSConfig: %v", err) + } + for which, leaf := range leaves { + if err := cfg.VerifyConnection(tls.ConnectionState{ + PeerCertificates: []*x509.Certificate{leaf}, + }); err != nil { + t.Errorf("%s leaf rejected: %v", which, err) + } + } + }) + } +} + +// TestGetClientCertificatePresentsRenewedCertificate is the regression test for +// the bug that made in-band renewal impossible: one *tls.Config is built at +// start-up and reused for every reconnect, so a client certificate cached in +// it would keep being presented until the process restarted. +// +// Replacing agent.crt on disk must change what the very next handshake +// presents, with no new config and no restart. +func TestGetClientCertificatePresentsRenewedCertificate(t *testing.T) { + oldCA := testca.New(t, "old") + newCA := testca.New(t, "new") + + agent := oldCA.Issue(t, "Deploy Agent #42", 42) + paths := writeIdentity(t, agent) + + server := newTestServer(t, oldCA.Issue(t, serverCN, 1), testca.Pool(oldCA, newCA)) + + // verify=false keeps the server's own certificate out of the picture; the + // client certificate is what is under test and it is loaded the same way + // on both paths. + cfg, err := config.NewTLSConfig(paths, testca.Bundle(oldCA, newCA), false) + if err != nil { + t.Fatalf("NewTLSConfig: %v", err) + } + // Structural checks first, but non-fatal: the handshakes below are the real + // evidence and should still run and report if these ever regress. + if cfg.Certificates != nil { + t.Error("Certificates is populated — a cached certificate cannot be renewed without a restart") + } + if cfg.GetClientCertificate == nil { + t.Error("GetClientCertificate was not installed") + } + + if got := server.issuerSeen(t, cfg); got != "Deploy Dev CA (old)" { + t.Fatalf("first handshake presented issuer %q, want the old CA", got) + } + + // The renewal: same key, same subject, same serial, new issuer. + renewed := newCA.Reissue(t, agent, "Deploy Agent #42", 42) + if err := os.WriteFile(paths.Certificate, renewed, 0600); err != nil { + t.Fatal(err) + } + + if got := server.issuerSeen(t, cfg); got != "Deploy Dev CA (new)" { + t.Errorf("second handshake presented issuer %q, want the new CA — the certificate was not reloaded", got) + } +} + +// TestNewTLSConfigRejectsBadInputs: every one of these must fail loudly at +// start-up rather than at handshake time behind a customer firewall. +func TestNewTLSConfigRejectsBadInputs(t *testing.T) { + ca := testca.New(t, "old") + good := ca.Issue(t, "Deploy Agent #42", 42) + other := ca.Issue(t, "Deploy Agent #42", 42) // same identity, different key + + t.Run("missing certificate", func(t *testing.T) { + paths := writeIdentity(t, good) + if err := os.Remove(paths.Certificate); err != nil { + t.Fatal(err) + } + if _, err := config.NewTLSConfig(paths, ca.CertPEM, true); err == nil { + t.Error("expected an error") + } + }) + + t.Run("missing key", func(t *testing.T) { + paths := writeIdentity(t, good) + if err := os.Remove(paths.Key); err != nil { + t.Fatal(err) + } + if _, err := config.NewTLSConfig(paths, ca.CertPEM, true); err == nil { + t.Error("expected an error") + } + }) + + t.Run("certificate and key do not match", func(t *testing.T) { + paths := writeIdentity(t, good) + if err := os.WriteFile(paths.Key, other.KeyPEM, 0600); err != nil { + t.Fatal(err) + } + if _, err := config.NewTLSConfig(paths, ca.CertPEM, true); err == nil { + t.Error("expected an error") + } + }) + + t.Run("unparsable CA bundle", func(t *testing.T) { + paths := writeIdentity(t, good) + if _, err := config.NewTLSConfig(paths, []byte("not a certificate"), true); err == nil { + t.Error("expected an error") + } + }) +} + +func TestNewCertPool(t *testing.T) { + oldCA := testca.New(t, "old") + newCA := testca.New(t, "new") + + if _, err := config.NewCertPool(testca.Bundle(oldCA, newCA)); err != nil { + t.Errorf("two-CA bundle: %v", err) + } + if _, err := config.NewCertPool(nil); err == nil { + t.Error("empty bundle should be an error") + } + if _, err := config.NewCertPool([]byte("garbage")); err == nil { + t.Error("unparsable bundle should be an error") + } +} + +// ── test server ────────────────────────────────────────────────────────────── + +// testServer accepts one mTLS connection at a time and reports the issuer of +// the client certificate it was shown. +type testServer struct { + ln net.Listener +} + +func newTestServer(t *testing.T, leaf *testca.Identity, clientCAs *x509.CertPool) *testServer { + t.Helper() + ln, err := tls.Listen("tcp", "127.0.0.1:0", &tls.Config{ + Certificates: []tls.Certificate{leaf.TLS(t)}, + ClientAuth: tls.RequireAndVerifyClientCert, + ClientCAs: clientCAs, + }) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { ln.Close() }) + return &testServer{ln: ln} +} + +// issuerSeen dials the server with cfg and returns the issuer common name of +// the client certificate the server was actually shown — the only authority on +// which certificate went on the wire. +func (s *testServer) issuerSeen(t *testing.T, cfg *tls.Config) string { + t.Helper() + + type result struct { + issuer string + err error + } + done := make(chan result, 1) + go func() { + conn, err := s.ln.Accept() + if err != nil { + done <- result{err: err} + return + } + defer conn.Close() + _ = conn.SetDeadline(time.Now().Add(10 * time.Second)) + tc := conn.(*tls.Conn) + if err := tc.Handshake(); err != nil { + done <- result{err: err} + return + } + peer := tc.ConnectionState().PeerCertificates + if len(peer) == 0 { + done <- result{err: errors.New("server saw no client certificate")} + return + } + done <- result{issuer: peer[0].Issuer.CommonName} + }() + + conn, err := tls.DialWithDialer(&net.Dialer{Timeout: 5 * time.Second}, "tcp", s.ln.Addr().String(), cfg) + if err != nil { + t.Fatalf("dialling test server: %v", err) + } + defer conn.Close() + + select { + case r := <-done: + if r.err != nil { + t.Fatalf("test server: %v", r.err) + } + return r.issuer + case <-time.After(10 * time.Second): + t.Fatal("timeout waiting for the test server") + return "" + } +} From e1dce4b6269fde71b270479d05acffcdd5272041 Mon Sep 17 00:00:00 2001 From: Thiago Durante Date: Mon, 14 Sep 2026 16:16:57 +0200 Subject: [PATCH 08/15] feat(certrenew): validate and atomically install a renewed certificate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The backend re-signs the public key it already holds for an agent under the new CA, preserving the subject and serial that identify it, so the agent keeps its private key and only agent.crt changes. Two properties drive the design. Nothing is written unless every check passes — the candidate must pair with the private key already on disk, carry the same subject and serial as the current certificate, and chain to a trusted CA — because a bad certificate that replaced a good one takes the agent offline behind a customer firewall where nobody can repair it. And the replacement is atomic: write agent.crt.tmp in the same directory, fsync, rename over the original. The rename is retried a few times. On Unix it is a single atomic syscall and never needs it; on Windows it is MoveFileEx(REPLACE_EXISTING), which fails with a sharing violation while antivirus or a backup agent holds the destination open. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_011XaiFEqsyQGcLhRmBtKRFA --- internal/certrenew/certrenew.go | 200 +++++++++++++++ internal/certrenew/certrenew_test.go | 364 +++++++++++++++++++++++++++ 2 files changed, 564 insertions(+) create mode 100644 internal/certrenew/certrenew.go create mode 100644 internal/certrenew/certrenew_test.go diff --git a/internal/certrenew/certrenew.go b/internal/certrenew/certrenew.go new file mode 100644 index 0000000..dbe3df4 --- /dev/null +++ b/internal/certrenew/certrenew.go @@ -0,0 +1,200 @@ +// Package certrenew validates a certificate the DeployHQ backend has re-issued +// for this agent and installs it over ~/.deploy/agent.crt. +// +// Renewal happens during a CA rotation: the backend re-signs the public key it +// already holds for the agent under the new CA, preserving the subject and the +// serial number that identify the agent, so the agent keeps its existing +// private key and only the certificate file changes. +// +// Two properties matter more than anything else here: +// +// - Nothing is written unless every check passes. A bad certificate that +// replaced a good one would take the agent offline behind a customer +// firewall, where nobody can fix it. +// - The replacement is atomic. A reader must see either the whole old +// certificate or the whole new one, never a truncated file. +package certrenew + +import ( + "bytes" + "crypto/tls" + "crypto/x509" + "encoding/pem" + "fmt" + "os" + "path/filepath" + "time" +) + +// renameAttempts / renameDelay bound the retry loop around the final rename. +// +// On Unix os.Rename is a single atomic syscall and never needs a retry. On +// Windows it is MoveFileEx(MOVEFILE_REPLACE_EXISTING), which fails with a +// sharing violation while another process (antivirus, a backup agent, an +// installer) holds the destination open without FILE_SHARE_DELETE. Those holds +// are short, so retrying a few times turns a transient collision into a +// successful renewal instead of a skipped one. +const ( + renameAttempts = 5 + renameDelay = 200 * time.Millisecond +) + +// Request describes a single renewal attempt. +type Request struct { + // CertPath is the certificate file to replace (~/.deploy/agent.crt). + CertPath string + + // KeyPath is the agent's private key. It is never written; the candidate + // certificate must pair with the key already on disk. + KeyPath string + + // Roots are the trust anchors the candidate certificate must chain to — + // the same CA bundle the agent uses to verify the server. + Roots *x509.CertPool + + // Current is the certificate the agent is using now. The candidate must + // carry the same subject and serial number, because those are how the + // backend identifies this agent. + Current *x509.Certificate + + // CandidatePEM is the PEM-encoded certificate offered by the server. + CandidatePEM []byte +} + +// Apply validates req.CandidatePEM and, only when every check passes, +// atomically replaces the file at req.CertPath with it. +// +// On any error nothing has been written and the existing certificate file is +// untouched. The returned certificate is the newly installed one. +func Apply(req Request) (*x509.Certificate, error) { + candidate, err := Validate(req) + if err != nil { + return nil, err + } + if err := writeAtomic(req.CertPath, req.CandidatePEM); err != nil { + return nil, err + } + return candidate, nil +} + +// Validate runs every check on the candidate certificate without touching the +// filesystem except to read the private key. It is separated from Apply so the +// checks can be exercised — and reasoned about — on their own. +func Validate(req Request) (*x509.Certificate, error) { + if req.Current == nil { + return nil, fmt.Errorf("no current certificate to compare against") + } + if req.Roots == nil { + return nil, fmt.Errorf("no CA roots to verify against") + } + + block, _ := pem.Decode(req.CandidatePEM) + if block == nil || block.Type != "CERTIFICATE" { + return nil, fmt.Errorf("renewed certificate is not a PEM certificate") + } + candidate, err := x509.ParseCertificate(block.Bytes) + if err != nil { + return nil, fmt.Errorf("parsing renewed certificate: %w", err) + } + + // (a) The certificate must pair with the private key already on disk. + // tls.X509KeyPair does exactly this comparison, for every key type Go + // supports, and fails with "private key does not match public key". + keyPEM, err := os.ReadFile(req.KeyPath) + if err != nil { + return nil, fmt.Errorf("reading private key %s: %w", req.KeyPath, err) + } + if _, err := tls.X509KeyPair(req.CandidatePEM, keyPEM); err != nil { + return nil, fmt.Errorf("renewed certificate does not pair with %s: %w", req.KeyPath, err) + } + + // (b) Subject and serial identify the agent to the backend. A renewal + // changes the issuer, never the identity — anything else is not a renewal + // of *this* agent's certificate and must not overwrite it. + if !bytes.Equal(candidate.RawSubject, req.Current.RawSubject) { + return nil, fmt.Errorf("renewed certificate subject %q does not match current %q", + candidate.Subject.String(), req.Current.Subject.String()) + } + if candidate.SerialNumber.Cmp(req.Current.SerialNumber) != 0 { + return nil, fmt.Errorf("renewed certificate serial %s does not match current %s", + candidate.SerialNumber, req.Current.SerialNumber) + } + + // (c) The certificate must chain to a CA this agent trusts. Mirrors the + // VerifyOptions used by config.NewTLSConfig's VerifyConnection: roots only + // and no DNSName, because DeployHQ's certificates are CN-only with no SANs. + // KeyUsageAny because an agent certificate carries no extended key usage + // extension at all and the zero value of KeyUsages would demand ServerAuth. + if _, err := candidate.Verify(x509.VerifyOptions{ + Roots: req.Roots, + KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageAny}, + }); err != nil { + return nil, fmt.Errorf("renewed certificate does not verify against the trusted CAs: %w", err) + } + + return candidate, nil +} + +// LoadCurrent reads and parses the certificate currently installed at path. +func LoadCurrent(path string) (*x509.Certificate, error) { + data, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("reading %s: %w", path, err) + } + block, _ := pem.Decode(data) + if block == nil || block.Type != "CERTIFICATE" { + return nil, fmt.Errorf("%s is not a PEM certificate", path) + } + cert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + return nil, fmt.Errorf("parsing %s: %w", path, err) + } + return cert, nil +} + +// writeAtomic writes data to ".tmp" in dst's own directory, flushes it to +// stable storage, and renames it over dst. Same directory so the rename stays +// within one filesystem, which is what makes it atomic. +func writeAtomic(dst string, data []byte) error { + tmp := dst + ".tmp" + + f, err := os.OpenFile(tmp, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600) + if err != nil { + return fmt.Errorf("creating %s: %w", tmp, err) + } + if _, err := f.Write(data); err != nil { + f.Close() + _ = os.Remove(tmp) + return fmt.Errorf("writing %s: %w", tmp, err) + } + if err := f.Sync(); err != nil { + f.Close() + _ = os.Remove(tmp) + return fmt.Errorf("syncing %s: %w", tmp, err) + } + if err := f.Close(); err != nil { + _ = os.Remove(tmp) + return fmt.Errorf("closing %s: %w", tmp, err) + } + + if err := renameWithRetry(tmp, dst); err != nil { + _ = os.Remove(tmp) + return err + } + return nil +} + +// renameWithRetry renames src over dst, retrying a few times so a transient +// Windows sharing violation does not abandon an otherwise valid renewal. +func renameWithRetry(src, dst string) error { + var err error + for attempt := 1; attempt <= renameAttempts; attempt++ { + if err = os.Rename(src, dst); err == nil { + return nil + } + if attempt < renameAttempts { + time.Sleep(renameDelay) + } + } + return fmt.Errorf("replacing %s after %d attempts: %w", filepath.Base(dst), renameAttempts, err) +} diff --git a/internal/certrenew/certrenew_test.go b/internal/certrenew/certrenew_test.go new file mode 100644 index 0000000..b88d18f --- /dev/null +++ b/internal/certrenew/certrenew_test.go @@ -0,0 +1,364 @@ +package certrenew_test + +import ( + "bytes" + "crypto/x509" + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/deployhq/network-agent/internal/certrenew" + "github.com/deployhq/network-agent/internal/testca" +) + +// agentCN and agentSerial mirror how DeployHQ identifies an agent: the subject +// common name and the serial number are the identity, and a renewal must +// preserve both while changing only the issuer. +const ( + agentCN = "Deploy Agent #42" + agentSerial = 42 +) + +type fixture struct { + dir string + certPath string + keyPath string + current *x509.Certificate + originalPEM []byte + + // renewed is the certificate the backend would legitimately hand back: + // the same key, subject and serial, re-signed by the new CA. + renewed []byte + + // The refusable variants. + wrongSerial []byte + wrongSubject []byte + wrongKey []byte + unknownCA []byte + + roots *x509.CertPool +} + +func newFixture(t *testing.T) *fixture { + t.Helper() + + oldCA := testca.New(t, "old") + newCA := testca.New(t, "new") + otherCA := testca.New(t, "someone else") + + agent := oldCA.Issue(t, agentCN, agentSerial) + impostor := newCA.Issue(t, agentCN, agentSerial) // same identity, different key + + dir := t.TempDir() + certPath := filepath.Join(dir, "agent.crt") + keyPath := filepath.Join(dir, "agent.key") + if err := os.WriteFile(certPath, agent.CertPEM, 0600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(keyPath, agent.KeyPEM, 0600); err != nil { + t.Fatal(err) + } + + return &fixture{ + dir: dir, + certPath: certPath, + keyPath: keyPath, + current: agent.Cert, + originalPEM: agent.CertPEM, + renewed: newCA.Reissue(t, agent, agentCN, agentSerial), + wrongSerial: newCA.Reissue(t, agent, agentCN, agentSerial+1), + wrongSubject: newCA.Reissue(t, agent, "Deploy Agent #99", agentSerial), + wrongKey: impostor.CertPEM, + unknownCA: otherCA.Reissue(t, agent, agentCN, agentSerial), + roots: testca.Pool(oldCA, newCA), + } +} + +func (f *fixture) request(candidate []byte) certrenew.Request { + return certrenew.Request{ + CertPath: f.certPath, + KeyPath: f.keyPath, + Roots: f.roots, + Current: f.current, + CandidatePEM: candidate, + } +} + +// onDisk reads the certificate file, so a test can assert it was left alone. +func (f *fixture) onDisk(t *testing.T) []byte { + t.Helper() + data, err := os.ReadFile(f.certPath) + if err != nil { + t.Fatalf("reading %s: %v", f.certPath, err) + } + return data +} + +// TestApplyInstallsRenewedCertificate is the happy path: a certificate +// re-signed under the new CA, keeping key, subject and serial, replaces the +// file on disk. +func TestApplyInstallsRenewedCertificate(t *testing.T) { + f := newFixture(t) + + installed, err := certrenew.Apply(f.request(f.renewed)) + if err != nil { + t.Fatalf("Apply: %v", err) + } + + if got := f.onDisk(t); !bytes.Equal(got, f.renewed) { + t.Error("certificate file does not hold the renewed certificate") + } + if installed.Issuer.CommonName != "Deploy Dev CA (new)" { + t.Errorf("issuer = %q, want %q", installed.Issuer.CommonName, "Deploy Dev CA (new)") + } + if installed.Subject.CommonName != agentCN { + t.Errorf("subject CN = %q, want %q", installed.Subject.CommonName, agentCN) + } + if installed.SerialNumber.Int64() != agentSerial { + t.Errorf("serial = %s, want %d", installed.SerialNumber, agentSerial) + } + if installed.Issuer.CommonName == f.current.Issuer.CommonName { + t.Error("issuer did not change — the fixture is not exercising a renewal") + } + + // The renewed certificate must still be usable: it has to load as a key + // pair with the untouched private key, or the agent is offline. + reloaded, err := certrenew.LoadCurrent(f.certPath) + if err != nil { + t.Fatalf("LoadCurrent after Apply: %v", err) + } + if !reloaded.Equal(installed) { + t.Error("certificate read back from disk differs from the one Apply reported") + } +} + +// TestApplyLeavesFileUntouchedOnEveryRejection is the safety property that +// matters most: any failed check must leave the existing certificate byte for +// byte intact, because a broken agent.crt takes the agent offline behind a +// customer firewall where nobody can repair it. +func TestApplyLeavesFileUntouchedOnEveryRejection(t *testing.T) { + tests := []struct { + name string + candidate func(f *fixture) []byte + }{ + {"wrong private key", func(f *fixture) []byte { return f.wrongKey }}, + {"wrong serial number", func(f *fixture) []byte { return f.wrongSerial }}, + {"wrong subject", func(f *fixture) []byte { return f.wrongSubject }}, + {"unknown issuer", func(f *fixture) []byte { return f.unknownCA }}, + {"malformed pem", func(f *fixture) []byte { return []byte("-----BEGIN CERTIFICATE-----\nnope\n") }}, + {"not pem at all", func(f *fixture) []byte { return []byte("certainly not a certificate") }}, + {"empty", func(f *fixture) []byte { return nil }}, + {"pem block of the wrong type", func(f *fixture) []byte { + return []byte("-----BEGIN PRIVATE KEY-----\nZm9v\n-----END PRIVATE KEY-----\n") + }}, + {"corrupt certificate der", func(f *fixture) []byte { + return []byte("-----BEGIN CERTIFICATE-----\nZm9vYmFy\n-----END CERTIFICATE-----\n") + }}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := newFixture(t) + before := f.onDisk(t) + + installed, err := certrenew.Apply(f.request(tt.candidate(f))) + if err == nil { + t.Fatalf("Apply accepted a %s certificate", tt.name) + } + if installed != nil { + t.Error("Apply returned a certificate alongside an error") + } + t.Logf("rejected: %v", err) + + if after := f.onDisk(t); !bytes.Equal(before, after) { + t.Error("certificate file was modified despite the rejection") + } + if !bytes.Equal(f.onDisk(t), f.originalPEM) { + t.Error("certificate file no longer holds the original certificate") + } + + // The temporary file must not be left behind either. + if _, err := os.Stat(f.certPath + ".tmp"); err == nil { + t.Error("temporary file left behind after a rejected renewal") + } + }) + } +} + +// TestApplyRejectsUntrustedRoots covers the failure this whole project exists +// to prevent: a certificate whose own dates are fine but whose issuer this +// agent does not trust must not be installed. +func TestApplyRejectsUntrustedRoots(t *testing.T) { + f := newFixture(t) + + req := f.request(f.renewed) + req.Roots = x509.NewCertPool() // trusts nothing + + if _, err := certrenew.Apply(req); err == nil { + t.Fatal("Apply accepted a certificate that chains to no trusted CA") + } + if !bytes.Equal(f.onDisk(t), f.originalPEM) { + t.Error("certificate file was modified") + } +} + +// TestValidateRejectsMissingInputs guards the caller contract: without a +// current certificate or a root pool there is nothing to validate against, and +// silently accepting would be the worst possible default. +func TestValidateRejectsMissingInputs(t *testing.T) { + f := newFixture(t) + + t.Run("no current certificate", func(t *testing.T) { + req := f.request(f.renewed) + req.Current = nil + if _, err := certrenew.Validate(req); err == nil { + t.Error("expected an error") + } + }) + + t.Run("no roots", func(t *testing.T) { + req := f.request(f.renewed) + req.Roots = nil + if _, err := certrenew.Validate(req); err == nil { + t.Error("expected an error") + } + }) + + t.Run("missing private key file", func(t *testing.T) { + req := f.request(f.renewed) + req.KeyPath = filepath.Join(f.dir, "does-not-exist.key") + if _, err := certrenew.Validate(req); err == nil { + t.Error("expected an error") + } + }) +} + +// TestValidateHasNoSideEffects: Validate is the pure half of Apply and must +// never touch the certificate file, not even on success. +func TestValidateHasNoSideEffects(t *testing.T) { + f := newFixture(t) + + if _, err := certrenew.Validate(f.request(f.renewed)); err != nil { + t.Fatalf("Validate: %v", err) + } + if !bytes.Equal(f.onDisk(t), f.originalPEM) { + t.Error("Validate modified the certificate file") + } +} + +// TestApplyWithRSAKeys pins the key-pairing check against production's key +// algorithm. DeployHQ mints 4096-bit RSA keys; the size is irrelevant here but +// the algorithm is not, because the pairing check is algorithm-specific. +func TestApplyWithRSAKeys(t *testing.T) { + oldCA := testca.New(t, "old") + newCA := testca.New(t, "new") + agent := oldCA.IssueWithKey(t, agentCN, agentSerial, testca.KeyRSA) + impostor := newCA.IssueWithKey(t, agentCN, agentSerial, testca.KeyRSA) + + dir := t.TempDir() + certPath := filepath.Join(dir, "agent.crt") + keyPath := filepath.Join(dir, "agent.key") + if err := os.WriteFile(certPath, agent.CertPEM, 0600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(keyPath, agent.KeyPEM, 0600); err != nil { + t.Fatal(err) + } + + req := certrenew.Request{ + CertPath: certPath, + KeyPath: keyPath, + Roots: testca.Pool(oldCA, newCA), + Current: agent.Cert, + } + + req.CandidatePEM = impostor.CertPEM + if _, err := certrenew.Apply(req); err == nil { + t.Error("Apply accepted an RSA certificate for a different key") + } + + req.CandidatePEM = newCA.Reissue(t, agent, agentCN, agentSerial) + if _, err := certrenew.Apply(req); err != nil { + t.Fatalf("Apply rejected a valid RSA renewal: %v", err) + } + data, err := os.ReadFile(certPath) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(data, req.CandidatePEM) { + t.Error("RSA renewal was not written") + } +} + +// TestApplyWritesPrivateFile: the certificate itself is not secret, but the +// agent's ~/.deploy directory is 0700 and the renewal must not widen anything. +func TestApplyWritesPrivateFile(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Unix file modes do not apply on Windows") + } + f := newFixture(t) + + if _, err := certrenew.Apply(f.request(f.renewed)); err != nil { + t.Fatalf("Apply: %v", err) + } + + info, err := os.Stat(f.certPath) + if err != nil { + t.Fatal(err) + } + if perm := info.Mode().Perm(); perm != 0600 { + t.Errorf("mode = %#o, want 0600", perm) + } +} + +// TestApplyReplacesAStaleTempFile: a crash mid-renewal can leave agent.crt.tmp +// behind. The next attempt must overwrite it rather than fail forever. +func TestApplyReplacesAStaleTempFile(t *testing.T) { + f := newFixture(t) + + stale := f.certPath + ".tmp" + if err := os.WriteFile(stale, []byte("leftover from a crashed renewal"), 0600); err != nil { + t.Fatal(err) + } + + if _, err := certrenew.Apply(f.request(f.renewed)); err != nil { + t.Fatalf("Apply: %v", err) + } + if !bytes.Equal(f.onDisk(t), f.renewed) { + t.Error("renewed certificate was not installed over a stale temp file") + } + if _, err := os.Stat(stale); err == nil { + t.Error("temp file still present after a successful renewal") + } +} + +// TestLoadCurrent covers the helper the tunnel uses to learn which certificate +// it is presenting before asking the server to renew it. +func TestLoadCurrent(t *testing.T) { + f := newFixture(t) + + cert, err := certrenew.LoadCurrent(f.certPath) + if err != nil { + t.Fatalf("LoadCurrent: %v", err) + } + if cert.Subject.CommonName != agentCN || cert.SerialNumber.Int64() != agentSerial { + t.Errorf("loaded %q serial %s", cert.Subject.CommonName, cert.SerialNumber) + } + + t.Run("missing file", func(t *testing.T) { + if _, err := certrenew.LoadCurrent(filepath.Join(f.dir, "nope.crt")); err == nil { + t.Error("expected an error") + } + }) + + t.Run("not a certificate", func(t *testing.T) { + path := filepath.Join(f.dir, "garbage.crt") + if err := os.WriteFile(path, []byte("hello"), 0600); err != nil { + t.Fatal(err) + } + if _, err := certrenew.LoadCurrent(path); err == nil { + t.Error("expected an error") + } + }) +} From 0974f2e67c17f0af35905e9d4fdc783191866750 Mon Sep 17 00:00:00 2001 From: Thiago Durante Date: Mon, 14 Sep 2026 16:16:57 +0200 Subject: [PATCH 09/15] feat(tunnel): in-band certificate renewal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On every connect the agent sends RENEW_REQUEST with "go/" and lets the server decide. RENEWED installs the returned certificate and closes the connection so the existing reconnect path presents it — deliberately without touching the TLS retry counter, since that connection succeeded. CURRENT and ERROR write nothing and leave the tunnel running. A renewal that cannot happen never takes down a working tunnel or the process: every failure path logs and carries on with the certificate the agent already has, and asks again on the next connection. Renewal is opt-in through tunnel.Options — without CA roots and both file paths the agent neither asks nor acts on an answer. The version is plumbed through Options rather than a package global, and RunAgent now takes Options in place of a bare Paths. Also wires both NewTLSConfig call sites in main.go through config.CACert so DEPLOY_AGENT_CA_FILE applies to `run` and `check` alike, and reports the trusted CAs in `check` output — which CAs a binary trusts is the thing this rotation turns on. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_011XaiFEqsyQGcLhRmBtKRFA --- cmd/network-agent/main.go | 49 ++- internal/tunnel/agent.go | 36 ++- internal/tunnel/integration_test.go | 13 +- internal/tunnel/renew.go | 121 ++++++++ internal/tunnel/renew_test.go | 442 ++++++++++++++++++++++++++++ internal/tunnel/server_conn.go | 8 +- 6 files changed, 661 insertions(+), 8 deletions(-) create mode 100644 internal/tunnel/renew.go create mode 100644 internal/tunnel/renew_test.go diff --git a/cmd/network-agent/main.go b/cmd/network-agent/main.go index 2ebc3be..b3816ea 100644 --- a/cmd/network-agent/main.go +++ b/cmd/network-agent/main.go @@ -153,12 +153,27 @@ func cmdRun(paths config.Paths, verbose bool) { } log := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: level})) - tlsCfg, err := config.NewTLSConfig(paths, caroot.CACert, config.VerifyTLS()) + caCert, err := config.CACert(caroot.CACert) + if err != nil { + log.Error("CA certificate error", "err", err) + os.Exit(1) + } + + tlsCfg, err := config.NewTLSConfig(paths, caCert, config.VerifyTLS()) if err != nil { log.Error("TLS configuration error", "err", err) os.Exit(1) } + // The same trust anchors gate in-band certificate renewal: a certificate + // the agent would not accept from the server is not one it will install + // for itself. + caRoots, err := config.NewCertPool(caCert) + if err != nil { + log.Error("CA certificate error", "err", err) + os.Exit(1) + } + // Handle SIGTERM / SIGINT for clean shutdown sigc := make(chan os.Signal, 1) signal.Notify(sigc, syscall.SIGTERM, syscall.SIGINT) @@ -173,7 +188,13 @@ func cmdRun(paths config.Paths, verbose bool) { // If PID already written by the parent (start), this is a no-op for our PID. _ = daemon.WritePID(paths.PID) - if err := tunnel.RunAgent(tlsCfg, paths, log); err != nil { + opts := tunnel.Options{ + Version: Version, + Paths: paths, + CARoots: caRoots, + } + + if err := tunnel.RunAgent(tlsCfg, opts, log); err != nil { log.Error("agent stopped with error", "err", err) daemon.RemovePID(paths.PID) os.Exit(1) @@ -260,11 +281,33 @@ func cmdCheck(paths config.Paths) { fmt.Println(")") } + // --- CA bundle --------------------------------------------------------- + // Which CAs this binary trusts is the thing a CA rotation turns on, so + // surface it rather than leaving operators to inspect the binary. + caCert, caErr := config.CACert(caroot.CACert) + if caErr != nil { + fmt.Printf(" CA bundle FAIL %v\n", caErr) + ok = false + } else if cas, err := caroot.ParseBundle(caCert); err != nil { + fmt.Printf(" CA bundle FAIL %v\n", err) + ok = false + } else { + source := "embedded" + if override := os.Getenv(config.CAFileEnv); override != "" { + source = override + } + fmt.Printf(" CA bundle OK %d CA(s) from %s\n", len(cas), source) + for _, c := range cas { + fmt.Printf(" %s (expires %s)\n", + c.Subject.CommonName, c.NotAfter.UTC().Format("2006-01-02")) + } + } + // --- Connectivity ------------------------------------------------------ serverAddr := config.ServerHost() + ":" + config.ServerPort fmt.Printf(" Connectivity ") - tlsCfg, err := config.NewTLSConfig(paths, caroot.CACert, config.VerifyTLS()) + tlsCfg, err := config.NewTLSConfig(paths, caCert, config.VerifyTLS()) if err != nil { fmt.Printf("FAIL TLS config error: %v\n", err) ok = false diff --git a/internal/tunnel/agent.go b/internal/tunnel/agent.go index f62d820..d51a295 100644 --- a/internal/tunnel/agent.go +++ b/internal/tunnel/agent.go @@ -2,6 +2,7 @@ package tunnel import ( "crypto/tls" + "crypto/x509" "errors" "fmt" "io" @@ -19,22 +20,42 @@ const ( reconnectMaxSecs = 20 ) +// Options carries what the tunnel needs beyond the TLS connection itself. +// +// The zero value is valid and disables in-band certificate renewal, which is +// what tests that only exercise the proxy protocol want. +type Options struct { + // Version is this build's version, reported to the server as + // "go/" when asking whether the client certificate needs + // re-issuing. The server has no other way to tell a Go agent from a Ruby + // one, or to know which release it is talking to. + Version string + + // Paths locates agent.crt and agent.key. Renewal replaces the certificate + // in place; the key is never touched. + Paths config.Paths + + // CARoots are the trust anchors a renewed certificate must chain to. + // Leaving it nil disables certificate renewal entirely. + CARoots *x509.CertPool +} + // RunAgent connects to the DeployHQ server and keeps reconnecting on transient // failures. It blocks until an unrecoverable error occurs (e.g. REJECT from // server, or a TLS authentication error that persists after maxSSLRetries). -func RunAgent(tlsCfg *tls.Config, paths config.Paths, log *slog.Logger) error { +func RunAgent(tlsCfg *tls.Config, opts Options, log *slog.Logger) error { serverAddr := fmt.Sprintf("%s:%s", config.ServerHost(), config.ServerPort) sslRetries := 0 for { - access, err := acl.LoadFile(paths.Access) + access, err := acl.LoadFile(opts.Paths.Access) if err != nil { return fmt.Errorf("loading access list: %w", err) } log.Info("connecting to server", "addr", serverAddr) - sc, err := Connect(tlsCfg, serverAddr, access, log) + sc, err := Connect(tlsCfg, serverAddr, access, opts, log) if err != nil { // TLS-level failure: apply retry counter (matches Ruby's 4-retry limit) sslRetries++ @@ -57,6 +78,15 @@ func RunAgent(tlsCfg *tls.Config, paths config.Paths, log *slog.Logger) error { return runErr } + if errors.Is(runErr, ErrCertificateRenewed) { + // Not a failure: the certificate on disk has been replaced and a + // fresh handshake is what makes the agent present it. Reconnect + // immediately, and deliberately without touching sslRetries — this + // connection succeeded. + log.Info("reconnecting to present the renewed certificate") + continue + } + if runErr == nil || errors.Is(runErr, io.EOF) { // Clean disconnect (RECONNECT or EOF): reconnect immediately log.Info("server disconnected, reconnecting") diff --git a/internal/tunnel/integration_test.go b/internal/tunnel/integration_test.go index 596643d..99fac0d 100644 --- a/internal/tunnel/integration_test.go +++ b/internal/tunnel/integration_test.go @@ -188,16 +188,27 @@ func (r *serverReader) next() protocol.Packet { // connectAgent starts the agent in a goroutine and returns immediately. // The TLS handshake completes once the caller's fake server calls accept(). +// +// Options are left at their zero value, which disables certificate renewal — +// these tests exercise the proxy protocol. See renew_test.go for the renewal +// exchange. func connectAgent(t *testing.T, serverAddr string, certs *testCerts, access *acl.AccessList) <-chan error { t.Helper() cfg := &tls.Config{ Certificates: []tls.Certificate{certs.clientCert}, RootCAs: certs.caPool, } + return runAgentConn(t, cfg, serverAddr, access, tunnel.Options{}) +} + +// runAgentConn drives one ServerConn to completion and reports the error Run +// returned. +func runAgentConn(t *testing.T, cfg *tls.Config, serverAddr string, access *acl.AccessList, opts tunnel.Options) <-chan error { + t.Helper() log := slog.New(slog.NewTextHandler(io.Discard, nil)) errc := make(chan error, 1) go func() { - sc, err := tunnel.Connect(cfg, serverAddr, access, log) + sc, err := tunnel.Connect(cfg, serverAddr, access, opts, log) if err != nil { errc <- err return diff --git a/internal/tunnel/renew.go b/internal/tunnel/renew.go new file mode 100644 index 0000000..9c7bfe5 --- /dev/null +++ b/internal/tunnel/renew.go @@ -0,0 +1,121 @@ +package tunnel + +import ( + "errors" + + "github.com/deployhq/network-agent/internal/certrenew" + "github.com/deployhq/network-agent/internal/protocol" +) + +// implementationPrefix identifies this agent to the server. The Ruby gem sends +// "ruby/" over the same command, which is the only way the backend can +// tell the two implementations apart — nothing else on the wire carries it. +const implementationPrefix = "go/" + +// devVersion is what an unreleased build reports, matching main.Version's +// default so a locally built agent is never mistaken for a release. +const devVersion = "dev" + +// ErrCertificateRenewed asks RunAgent for a fresh connection so the newly +// installed certificate is actually presented. It is a signal, not a failure, +// and must never count towards the TLS retry budget. +var ErrCertificateRenewed = errors.New("certificate renewed") + +// renewalEnabled reports whether this connection may install a new certificate. +// Renewal needs somewhere to write and something to verify against; without +// both, a RENEW_RESPONSE is ignored rather than acted on blindly. +func (sc *ServerConn) renewalEnabled() bool { + return sc.opts.CARoots != nil && + sc.opts.Paths.Certificate != "" && + sc.opts.Paths.Key != "" +} + +// sendRenewRequest asks the server, once per connection, whether this agent's +// client certificate needs re-issuing under a newer CA. The server decides; +// the agent only reports who it is. +func (sc *ServerConn) sendRenewRequest() { + if !sc.renewalEnabled() { + return + } + version := sc.opts.Version + if version == "" { + version = devVersion + } + identifier := implementationPrefix + version + + select { + case sc.serverSend <- protocol.EncodeRenewRequest(identifier): + sc.log.Debug("sent certificate renewal request", "agent", identifier) + default: + // Unreachable in practice — the queue is empty at this point — but a + // blocked send here would stall the whole connection, and renewal is + // never worth that. + sc.log.Warn("could not queue certificate renewal request") + } +} + +// handleRenewResponse acts on the server's answer to sendRenewRequest. +// +// It returns ErrCertificateRenewed when a new certificate has been installed, +// which tears this connection down so the next one presents it. Every other +// outcome — including every kind of failure — returns nil and leaves the +// connection running: a renewal that cannot happen is never a reason to take a +// working tunnel, or the process, down. +func (sc *ServerConn) handleRenewResponse(payload []byte) error { + status, body, ok := protocol.ParseRenewResponse(payload) + if !ok { + sc.log.Warn("malformed RENEW_RESPONSE") + return nil + } + + if !sc.renewalEnabled() { + sc.log.Warn("ignoring RENEW_RESPONSE: certificate renewal is not configured") + return nil + } + + switch status { + case protocol.RenewStatusCurrent: + sc.log.Debug("certificate is current, no renewal needed") + return nil + + case protocol.RenewStatusError: + sc.log.Warn("server could not renew certificate", "reason", string(body)) + return nil + + case protocol.RenewStatusRenewed: + return sc.installRenewedCertificate(body) + + default: + sc.log.Warn("unknown RENEW_RESPONSE status", "status", status) + return nil + } +} + +func (sc *ServerConn) installRenewedCertificate(certPEM []byte) error { + current, err := certrenew.LoadCurrent(sc.opts.Paths.Certificate) + if err != nil { + sc.log.Warn("certificate renewal skipped", "err", err) + return nil + } + + installed, err := certrenew.Apply(certrenew.Request{ + CertPath: sc.opts.Paths.Certificate, + KeyPath: sc.opts.Paths.Key, + Roots: sc.opts.CARoots, + Current: current, + CandidatePEM: certPEM, + }) + if err != nil { + // Nothing was written; the agent carries on with the certificate it + // has and will ask again on the next connection. + sc.log.Warn("certificate renewal rejected", "err", err) + return nil + } + + sc.log.Info("certificate renewed", + "issuer", installed.Issuer.CommonName, + "serial", installed.SerialNumber.String(), + "expires", installed.NotAfter.UTC().Format("2006-01-02")) + + return ErrCertificateRenewed +} diff --git a/internal/tunnel/renew_test.go b/internal/tunnel/renew_test.go new file mode 100644 index 0000000..f3ef45d --- /dev/null +++ b/internal/tunnel/renew_test.go @@ -0,0 +1,442 @@ +package tunnel_test + +// In-band certificate renewal over the tunnel. +// +// The exchange under test: the agent sends RENEW_REQUEST once per connection +// carrying "go/"; the server answers RENEW_RESPONSE with status +// RENEWED (a re-signed certificate), CURRENT, or ERROR. Only RENEWED writes +// anything, and only after the new certificate has been fully validated. + +import ( + "bytes" + "crypto/tls" + "crypto/x509" + "errors" + "net" + "os" + "path/filepath" + "testing" + "time" + + "github.com/deployhq/network-agent/internal/acl" + "github.com/deployhq/network-agent/internal/certrenew" + "github.com/deployhq/network-agent/internal/config" + "github.com/deployhq/network-agent/internal/protocol" + "github.com/deployhq/network-agent/internal/testca" + "github.com/deployhq/network-agent/internal/tunnel" +) + +const ( + agentCN = "Deploy Agent #42" + agentSerial = 42 + testVersion = "1.2.3" +) + +// renewFixture wires a real mTLS connection between a fake DeployHQ server and +// an agent whose client certificate lives on disk, so a renewal that replaces +// that file is observable exactly as it would be in production. +type renewFixture struct { + certPath string + keyPath string + + originalPEM []byte // what agent.crt held before the exchange + renewedPEM []byte // a legitimate renewal: same key, subject and serial + untrustedPEM []byte // signed by a CA the agent does not trust + + ln net.Listener + opts tunnel.Options + cfg *tls.Config +} + +func newRenewFixture(t *testing.T) *renewFixture { + t.Helper() + + oldCA := testca.New(t, "old") + newCA := testca.New(t, "new") + otherCA := testca.New(t, "someone else") + + agent := oldCA.Issue(t, agentCN, agentSerial) + server := oldCA.Issue(t, "agent.deployhq.com", 1) + + dir := t.TempDir() + f := &renewFixture{ + certPath: filepath.Join(dir, "agent.crt"), + keyPath: filepath.Join(dir, "agent.key"), + originalPEM: agent.CertPEM, + renewedPEM: newCA.Reissue(t, agent, agentCN, agentSerial), + untrustedPEM: otherCA.Reissue(t, agent, agentCN, agentSerial), + } + if err := os.WriteFile(f.certPath, agent.CertPEM, 0600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(f.keyPath, agent.KeyPEM, 0600); err != nil { + t.Fatal(err) + } + + // The server trusts both CAs, as the backend does throughout a rotation. + ln, err := tls.Listen("tcp", "127.0.0.1:0", &tls.Config{ + Certificates: []tls.Certificate{server.TLS(t)}, + ClientAuth: tls.RequireAndVerifyClientCert, + ClientCAs: testca.Pool(oldCA, newCA), + }) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { ln.Close() }) + f.ln = ln + + paths := config.Paths{Certificate: f.certPath, Key: f.keyPath} + + // verify=false: the server's own certificate is not what these tests are + // about, and it keeps the fixture free of hostname gymnastics. The client + // certificate is still loaded from disk per handshake either way. + cfg, err := config.NewTLSConfig(paths, testca.Bundle(oldCA, newCA), false) + if err != nil { + t.Fatal(err) + } + f.cfg = cfg + f.opts = tunnel.Options{ + Version: testVersion, + Paths: paths, + CARoots: testca.Pool(oldCA, newCA), + } + return f +} + +// connect starts the agent and returns the accepted server-side connection. +func (f *renewFixture) connect(t *testing.T, opts tunnel.Options) (net.Conn, <-chan error) { + t.Helper() + errc := runAgentConn(t, f.cfg, f.ln.Addr().String(), acl.Parse("127.0.0.1"), opts) + + connCh := make(chan net.Conn, 1) + go func() { + conn, err := f.ln.Accept() + if err == nil { + connCh <- conn + } + }() + select { + case conn := <-connCh: + conn.SetDeadline(time.Now().Add(10 * time.Second)) + t.Cleanup(func() { conn.Close() }) + // Accept returns before the handshake runs, so force it here: the + // client certificate the agent presented is the whole point of these + // tests and it is not observable until the handshake completes. + if err := conn.(*tls.Conn).Handshake(); err != nil { + t.Fatalf("server-side handshake: %v", err) + } + return conn, errc + case <-time.After(5 * time.Second): + t.Fatal("timeout accepting agent connection") + return nil, nil + } +} + +// peerIssuerCN reports the issuer of the client certificate the agent +// presented — the authoritative answer to "which certificate went on the wire". +func peerIssuerCN(t *testing.T, conn net.Conn) string { + t.Helper() + peer := conn.(*tls.Conn).ConnectionState().PeerCertificates + if len(peer) == 0 { + t.Fatal("server saw no client certificate") + } + return peer[0].Issuer.CommonName +} + +func (f *renewFixture) certOnDisk(t *testing.T) []byte { + t.Helper() + data, err := os.ReadFile(f.certPath) + if err != nil { + t.Fatalf("reading %s: %v", f.certPath, err) + } + return data +} + +// TestRenewRequestSentOnConnect: the request goes out once, immediately after +// the handshake, carrying the implementation and version the backend needs to +// tell Go agents from Ruby ones. +func TestRenewRequestSentOnConnect(t *testing.T) { + f := newRenewFixture(t) + serverConn, errc := f.connect(t, f.opts) + rd := newServerReader(t, serverConn) + + pkt := rd.next() + if pkt.Cmd != protocol.CmdRenewRequest { + t.Fatalf("first packet cmd = %d, want %d (RENEW_REQUEST)", pkt.Cmd, protocol.CmdRenewRequest) + } + if got := protocol.ParseRenewRequest(pkt.Payload); got != "go/"+testVersion { + t.Errorf("identifier = %q, want %q", got, "go/"+testVersion) + } + + stopAgent(t, serverConn, errc) +} + +// TestNoRenewRequestWhenRenewalDisabled: with zero Options — no CA roots, no +// paths — the agent must not ask. An agent that cannot safely install a +// certificate has no business requesting one. +func TestNoRenewRequestWhenRenewalDisabled(t *testing.T) { + f := newRenewFixture(t) + serverConn, errc := f.connect(t, tunnel.Options{}) + rd := newServerReader(t, serverConn) + + echoHost, echoPort := startEchoTarget(t) + if _, err := serverConn.Write(buildCreateRequest(1, echoHost, echoPort)); err != nil { + t.Fatal(err) + } + + // A renewal request would have been queued before anything else, so the + // first non-keepalive packet proves whether one was sent. + pkt := rd.next() + if pkt.Cmd == protocol.CmdRenewRequest { + t.Fatal("agent sent RENEW_REQUEST with renewal disabled") + } + if pkt.Cmd != protocol.CmdCreateResponse { + t.Fatalf("first packet cmd = %d, want %d (CREATE_RESPONSE)", pkt.Cmd, protocol.CmdCreateResponse) + } + + stopAgent(t, serverConn, errc) +} + +// TestRenewedCertificateIsInstalledAndConnectionRecycled is the happy path: +// the certificate on disk is replaced and Run returns the reconnect signal, so +// the next handshake presents the new certificate. +func TestRenewedCertificateIsInstalledAndConnectionRecycled(t *testing.T) { + f := newRenewFixture(t) + serverConn, errc := f.connect(t, f.opts) + + if got := peerIssuerCN(t, serverConn); got != "Deploy Dev CA (old)" { + t.Fatalf("first connection presented issuer %q, want the old CA", got) + } + + if _, err := serverConn.Write(protocol.EncodeRenewResponse(protocol.RenewStatusRenewed, f.renewedPEM)); err != nil { + t.Fatal(err) + } + + select { + case err := <-errc: + if !errors.Is(err, tunnel.ErrCertificateRenewed) { + t.Fatalf("Run returned %v, want ErrCertificateRenewed", err) + } + case <-time.After(5 * time.Second): + t.Fatal("timeout: agent did not recycle the connection after renewal") + } + + if got := f.certOnDisk(t); !bytes.Equal(got, f.renewedPEM) { + t.Fatal("agent.crt does not hold the renewed certificate") + } + + installed, err := certrenew.LoadCurrent(f.certPath) + if err != nil { + t.Fatal(err) + } + if installed.Issuer.CommonName != "Deploy Dev CA (new)" { + t.Errorf("issuer = %q, want the new CA", installed.Issuer.CommonName) + } + if installed.SerialNumber.Int64() != agentSerial || installed.Subject.CommonName != agentCN { + t.Errorf("renewal changed the agent's identity: %q serial %s", + installed.Subject.CommonName, installed.SerialNumber) + } + + // A reconnect is only useful if the renewed certificate is actually + // presented, which is what GetClientCertificate buys — prove it end to end. + serverConn.Close() + next, nextErrc := f.connect(t, f.opts) + if got := peerIssuerCN(t, next); got != "Deploy Dev CA (new)" { + t.Errorf("second connection presented issuer %q, want the new CA", got) + } + stopAgent(t, next, nextErrc) +} + +// TestRenewResponseStatusesThatWriteNothing: CURRENT and ERROR are normal +// answers, not failures. Neither may touch agent.crt and neither may drop the +// tunnel — the agent is mid-deployment as far as it knows. +func TestRenewResponseStatusesThatWriteNothing(t *testing.T) { + tests := []struct { + name string + status byte + body func(f *renewFixture) []byte + }{ + {"current", protocol.RenewStatusCurrent, func(*renewFixture) []byte { return nil }}, + {"error", protocol.RenewStatusError, func(*renewFixture) []byte { return []byte("agent has no certificate on file") }}, + {"renewed but untrusted issuer", protocol.RenewStatusRenewed, func(f *renewFixture) []byte { return f.untrustedPEM }}, + {"renewed but malformed", protocol.RenewStatusRenewed, func(*renewFixture) []byte { return []byte("not a certificate") }}, + {"renewed with an empty body", protocol.RenewStatusRenewed, func(*renewFixture) []byte { return nil }}, + {"unknown status", 200, func(*renewFixture) []byte { return nil }}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := newRenewFixture(t) + serverConn, errc := f.connect(t, f.opts) + rd := newServerReader(t, serverConn) + + if pkt := rd.next(); pkt.Cmd != protocol.CmdRenewRequest { + t.Fatalf("expected RENEW_REQUEST first, got cmd=%d", pkt.Cmd) + } + + if _, err := serverConn.Write(protocol.EncodeRenewResponse(tt.status, tt.body(f))); err != nil { + t.Fatal(err) + } + + // The connection must still be serving traffic afterwards. + echoHost, echoPort := startEchoTarget(t) + if _, err := serverConn.Write(buildCreateRequest(5, echoHost, echoPort)); err != nil { + t.Fatal(err) + } + resp := rd.next() + if resp.Cmd != protocol.CmdCreateResponse { + t.Fatalf("expected CREATE_RESPONSE, got cmd=%d — connection did not survive", resp.Cmd) + } + if _, status, reason, _ := protocol.ParseCreateResponse(resp.Payload); status != 0 { + t.Fatalf("CREATE_RESPONSE status=%d reason=%q", status, reason) + } + + if got := f.certOnDisk(t); !bytes.Equal(got, f.originalPEM) { + t.Error("agent.crt was modified") + } + if _, err := os.Stat(f.certPath + ".tmp"); err == nil { + t.Error("temporary file left behind") + } + + select { + case err := <-errc: + t.Fatalf("agent exited: %v", err) + default: + } + + stopAgent(t, serverConn, errc) + }) + } +} + +// TestMalformedRenewResponseIgnored: a zero-length payload cannot carry a +// status byte. It must be shrugged off, like any other frame the agent cannot +// make sense of. +func TestMalformedRenewResponseIgnored(t *testing.T) { + f := newRenewFixture(t) + serverConn, errc := f.connect(t, f.opts) + rd := newServerReader(t, serverConn) + + if pkt := rd.next(); pkt.Cmd != protocol.CmdRenewRequest { + t.Fatalf("expected RENEW_REQUEST first, got cmd=%d", pkt.Cmd) + } + + if _, err := serverConn.Write(protocol.EncodePacket(protocol.CmdRenewResponse, nil)); err != nil { + t.Fatal(err) + } + + echoHost, echoPort := startEchoTarget(t) + if _, err := serverConn.Write(buildCreateRequest(9, echoHost, echoPort)); err != nil { + t.Fatal(err) + } + if resp := rd.next(); resp.Cmd != protocol.CmdCreateResponse { + t.Fatalf("expected CREATE_RESPONSE, got cmd=%d", resp.Cmd) + } + if got := f.certOnDisk(t); !bytes.Equal(got, f.originalPEM) { + t.Error("agent.crt was modified") + } + + stopAgent(t, serverConn, errc) +} + +// TestRenewResponseIgnoredWhenRenewalDisabled: the server never sends one +// unsolicited, so this can only happen if something has gone wrong. Ignore it +// rather than writing a certificate that could not be verified. +func TestRenewResponseIgnoredWhenRenewalDisabled(t *testing.T) { + f := newRenewFixture(t) + serverConn, errc := f.connect(t, tunnel.Options{}) + rd := newServerReader(t, serverConn) + + if _, err := serverConn.Write(protocol.EncodeRenewResponse(protocol.RenewStatusRenewed, f.renewedPEM)); err != nil { + t.Fatal(err) + } + + echoHost, echoPort := startEchoTarget(t) + if _, err := serverConn.Write(buildCreateRequest(3, echoHost, echoPort)); err != nil { + t.Fatal(err) + } + if resp := rd.next(); resp.Cmd != protocol.CmdCreateResponse { + t.Fatalf("expected CREATE_RESPONSE, got cmd=%d", resp.Cmd) + } + if got := f.certOnDisk(t); !bytes.Equal(got, f.originalPEM) { + t.Error("agent.crt was modified with renewal disabled") + } + + stopAgent(t, serverConn, errc) +} + +// TestUnknownCommandsAreStillTolerated: adding commands 8 and 9 must not have +// narrowed what the agent shrugs off. Deployed v0.2.0 agents ignore unknown +// bytes, and this build has to keep doing the same for whatever comes next. +func TestUnknownCommandsAreStillTolerated(t *testing.T) { + certs := generateTestCerts(t) + srv := newFakeServer(t, certs) + errc := connectAgent(t, srv.addr(), certs, acl.Parse("127.0.0.1")) + + serverConn := srv.accept() + defer serverConn.Close() + rd := newServerReader(t, serverConn) + + for _, cmd := range []byte{10, 99, 200, 255} { + if _, err := serverConn.Write(protocol.EncodePacket(cmd, []byte("payload"))); err != nil { + t.Fatalf("writing cmd %d: %v", cmd, err) + } + } + + select { + case err := <-errc: + t.Fatalf("agent exited after unknown commands: %v", err) + case <-time.After(300 * time.Millisecond): + } + + echoHost, echoPort := startEchoTarget(t) + if _, err := serverConn.Write(buildCreateRequest(77, echoHost, echoPort)); err != nil { + t.Fatal(err) + } + resp := rd.next() + if resp.Cmd != protocol.CmdCreateResponse { + t.Fatalf("expected CREATE_RESPONSE after unknown commands, got cmd=%d", resp.Cmd) + } + + stopAgent(t, serverConn, errc) +} + +// TestRenewalDisabledWithoutRoots pins the individual preconditions: every one +// of them is required, so a half-configured agent never writes a certificate. +func TestRenewalRequiresCompleteOptions(t *testing.T) { + full := tunnel.Options{ + Version: testVersion, + Paths: config.Paths{Certificate: "/tmp/agent.crt", Key: "/tmp/agent.key"}, + CARoots: x509.NewCertPool(), + } + + tests := []struct { + name string + mutate func(o *tunnel.Options) + }{ + {"no CA roots", func(o *tunnel.Options) { o.CARoots = nil }}, + {"no certificate path", func(o *tunnel.Options) { o.Paths.Certificate = "" }}, + {"no key path", func(o *tunnel.Options) { o.Paths.Key = "" }}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := newRenewFixture(t) + opts := full + tt.mutate(&opts) + + serverConn, errc := f.connect(t, opts) + rd := newServerReader(t, serverConn) + + echoHost, echoPort := startEchoTarget(t) + if _, err := serverConn.Write(buildCreateRequest(1, echoHost, echoPort)); err != nil { + t.Fatal(err) + } + if pkt := rd.next(); pkt.Cmd == protocol.CmdRenewRequest { + t.Error("agent asked for renewal despite incomplete options") + } + + stopAgent(t, serverConn, errc) + }) + } +} diff --git a/internal/tunnel/server_conn.go b/internal/tunnel/server_conn.go index f71414b..c8e5169 100644 --- a/internal/tunnel/server_conn.go +++ b/internal/tunnel/server_conn.go @@ -26,6 +26,7 @@ type ServerConn struct { done chan struct{} // closed when Run() begins shutdown access *acl.AccessList + opts Options mu sync.Mutex dests map[uint16]*destConn @@ -34,7 +35,7 @@ type ServerConn struct { } // Connect dials the server and performs the mTLS handshake. -func Connect(tlsCfg *tls.Config, serverAddr string, access *acl.AccessList, log *slog.Logger) (*ServerConn, error) { +func Connect(tlsCfg *tls.Config, serverAddr string, access *acl.AccessList, opts Options, log *slog.Logger) (*ServerConn, error) { conn, err := tls.DialWithDialer( &net.Dialer{Timeout: 30 * time.Second}, "tcp", @@ -51,6 +52,7 @@ func Connect(tlsCfg *tls.Config, serverAddr string, access *acl.AccessList, log serverSend: make(chan []byte, 256), done: make(chan struct{}), access: access, + opts: opts, dests: make(map[uint16]*destConn), log: log, }, nil @@ -60,6 +62,7 @@ func Connect(tlsCfg *tls.Config, serverAddr string, access *acl.AccessList, log // It blocks until the connection is closed and returns ErrRejected on REJECT. func (sc *ServerConn) Run() error { go sc.writeLoop() + sc.sendRenewRequest() go sc.keepaliveLoop() err := sc.readLoop() @@ -176,6 +179,9 @@ func (sc *ServerConn) handlePacket(pkt protocol.Packet) error { case protocol.CmdKeepalive: // no-op + case protocol.CmdRenewResponse: + return sc.handleRenewResponse(pkt.Payload) + default: sc.log.Warn("unknown command", "cmd", pkt.Cmd) } From f25da13ca81f8419d1cd0c3b68390a4ace540962 Mon Sep 17 00:00:00 2001 From: Thiago Durante Date: Mon, 14 Sep 2026 16:17:48 +0200 Subject: [PATCH 10/15] docs: document certificate renewal and DEPLOY_AGENT_CA_FILE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit README gains an Upgrading section (`network-agent update && network-agent restart` — both steps, there is no background update check), a customer-facing note on what certificate renewal does automatically, and the new environment variable. CLAUDE.md records the two new command bytes and their payload shapes as a contract shared with the backend and the Ruby gem, the unknown-command tolerance deployed agents depend on, and a warning against folding the per-handshake certificate reload back into tls.Config.Certificates. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_011XaiFEqsyQGcLhRmBtKRFA --- CLAUDE.md | 35 ++++++++++++++++++++++++++++++++--- README.md | 28 ++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 58e0af7..c4ff00a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -32,11 +32,13 @@ cmd/network-agent/ # CLI entry point (main.go) internal/ protocol/ # Binary wire format (encode/decode) — no external deps acl/ # IP/CIDR access list parser - config/ # File paths, TLS config, env vars - caroot/ # Embedded CA certificate (ca.crt) + config/ # File paths, TLS config, CA bundle, env vars + caroot/ # Embedded CA bundle (ca.crt) + bundle parsing + certrenew/ # Validate + atomically install a renewed client cert tunnel/ # mTLS connection + goroutine lifecycle setup/ # Interactive setup wizard daemon/ # PID file, background process management + testca/ # Test-only: DeployHQ-shaped certificate fixtures ``` ## Protocol (Wire Format) @@ -50,7 +52,23 @@ Frame layout — must not change without coordinating with the server side: `total_frame_size` includes the 2-byte length field itself. Commands: `CREATE_REQUEST(1)`, `CREATE_RESPONSE(2)`, `DESTROY(3)`, `DATA(4)`, -`REJECT(5)`, `RECONNECT(6)`, `KEEPALIVE(7)`. +`REJECT(5)`, `RECONNECT(6)`, `KEEPALIVE(7)`, `RENEW_REQUEST(8)`, +`RENEW_RESPONSE(9)`. + +`RENEW_REQUEST` / `RENEW_RESPONSE` carry in-band certificate renewal and are +implemented identically in the DeployHQ backend and the Ruby `deploy-agent` +gem — the three must not drift: + +- `RENEW_REQUEST` (agent → server), sent once per connection immediately after + the handshake. Payload: UTF-8 `"go/"` (the gem sends `"ruby/"`). + Nothing else on the wire identifies the implementation or version. +- `RENEW_RESPONSE` (server → agent, never unsolicited). Payload + `[status:1][body]`: `0` RENEWED (body = new client certificate PEM), `1` + CURRENT (no body), `2` ERROR (body = UTF-8 message, logged and ignored). + +Unknown command bytes must stay tolerated — the `default:` branch in +`server_conn.go` only logs. Deployed agents rely on that to survive new +commands. Reference implementations: - Agent side: `../deploy-agent/lib/deploy_agent/server_connection.rb` @@ -73,6 +91,11 @@ Reference implementations: | `DEPLOY_AGENT_PROXY_IP` | `agent.deployhq.com` | | `DEPLOY_AGENT_CERTIFICATE_URL` | `https://api.deployhq.com/api/v1/agents/create` | | `DEPLOY_AGENT_NOVERIFY` | unset (set to skip TLS verification in dev) | +| `DEPLOY_AGENT_CA_FILE` | unset (uses `internal/caroot`'s embedded bundle) | + +`DEPLOY_AGENT_CA_FILE` replaces the embedded CA bundle with a PEM file on disk. +Read through `config.CACert(caroot.CACert)`, which both `run` and `check` go +through. An unreadable or empty file is an error, never a silent fall back. ## Testing @@ -100,5 +123,11 @@ GOOS=windows GOARCH=amd64 CGO_ENABLED=0 go build ./cmd/network-agent - Keep the protocol package dependency-free — it is the ground truth for wire format - The access list is reloaded on each reconnect (see `tunnel/agent.go`) — no restart needed for ACL changes +- The client certificate is reloaded from disk on **every handshake** + (`config.NewTLSConfig`'s `GetClientCertificate`), not cached in + `tls.Config.Certificates`. One `*tls.Config` is built at start-up and reused + for every reconnect, so caching it there would mean a renewed `agent.crt` + never took effect without a process restart — the thing certificate renewal + exists to avoid. Do not "optimise" this back into `Certificates`. - Daemon backgrounding uses re-exec (not fork) — see `internal/daemon/` - Build tags `!windows` / `windows` separate Unix/Windows daemon code diff --git a/README.md b/README.md index b7e4ef4..db52f01 100644 --- a/README.md +++ b/README.md @@ -70,6 +70,33 @@ Add `-v` / `--verbose` before the command for debug logging. 10.0.0.0/8 ``` +## Upgrading + +```bash +network-agent update && network-agent restart +``` + +`update` replaces the binary in place; `restart` is what makes the running +agent pick it up. Both steps are needed — there is no background update check. + +## Certificate renewal + +DeployHQ is rotating the certificate authority behind the agent connection. The +agent handles its side automatically: on each connection it tells DeployHQ which +version it is running, and if DeployHQ has re-issued its client certificate the +agent validates the new one, replaces `~/.deploy/agent.crt`, and reconnects to +start using it. Nothing is written unless the new certificate pairs with the +existing private key, keeps the same identity, and is signed by a trusted CA — +and the replacement is atomic, so a failure never leaves a half-written +certificate behind. + +Nothing is required of you beyond running a recent version. There is no +interruption to deployments, no new claim code, and no change to +`~/.deploy/agent.key`, which never leaves the machine. + +`network-agent check` prints the certificate authorities the binary trusts and +when they expire. + ## Migration from Ruby gem Users already running the Ruby gem can migrate in place — the Go binary uses the @@ -88,6 +115,7 @@ To roll back: `network-agent stop`, then `gem install deploy-agent` and `deploy- | `DEPLOY_AGENT_PROXY_IP` | `agent.deployhq.com` | Agent server hostname/IP | | `DEPLOY_AGENT_CERTIFICATE_URL` | `https://api.deployhq.com/api/v1/agents/create` | Certificate provisioning endpoint | | `DEPLOY_AGENT_NOVERIFY` | unset | Set to skip TLS server verification | +| `DEPLOY_AGENT_CA_FILE` | unset (uses the bundled CAs) | Path to a PEM bundle of certificate authorities to trust instead of the ones built into the binary. Unreadable or empty is an error, not a fall back. | ## Building from source From c4fb6a1fb82b44a75cbfad5958e3b1e9c562ae26 Mon Sep 17 00:00:00 2001 From: Thiago Durante Date: Mon, 14 Sep 2026 16:20:53 +0200 Subject: [PATCH 11/15] feat(certrenew): treat an already-installed certificate as a no-op A server that answers RENEWED with the certificate the agent already presents would otherwise send it round connect - renew - reconnect forever, since installing it and recycling the connection changes nothing. After every validation check passes, compare the candidate's DER with the installed certificate's. If they match, write nothing, return ErrUnchanged and keep the connection. DER rather than PEM, so re-encoded line endings or an added PEM header cannot make an identical certificate look new. Matches the guard already implemented in the Ruby deploy-agent gem. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_011XaiFEqsyQGcLhRmBtKRFA --- internal/certrenew/certrenew.go | 26 +++++++++++-- internal/certrenew/certrenew_test.go | 58 ++++++++++++++++++++++++++++ internal/tunnel/renew.go | 11 +++++- internal/tunnel/renew_test.go | 4 ++ 4 files changed, 94 insertions(+), 5 deletions(-) diff --git a/internal/certrenew/certrenew.go b/internal/certrenew/certrenew.go index dbe3df4..aaaa89a 100644 --- a/internal/certrenew/certrenew.go +++ b/internal/certrenew/certrenew.go @@ -20,12 +20,21 @@ import ( "crypto/tls" "crypto/x509" "encoding/pem" + "errors" "fmt" "os" "path/filepath" "time" ) +// ErrUnchanged reports that the offered certificate is byte for byte the one +// already installed, so there is nothing to write. +// +// It is a normal outcome, not a failure: a server that keeps answering RENEWED +// with the certificate the agent already has must not push it through a +// connect-renew-reconnect loop forever. +var ErrUnchanged = errors.New("certificate is already installed") + // renameAttempts / renameDelay bound the retry loop around the final rename. // // On Unix os.Rename is a single atomic syscall and never needs a retry. On @@ -61,11 +70,13 @@ type Request struct { CandidatePEM []byte } -// Apply validates req.CandidatePEM and, only when every check passes, -// atomically replaces the file at req.CertPath with it. +// Apply validates req.CandidatePEM and, only when every check passes and the +// certificate actually differs from the one installed, atomically replaces the +// file at req.CertPath with it. // -// On any error nothing has been written and the existing certificate file is -// untouched. The returned certificate is the newly installed one. +// On any error — ErrUnchanged included — nothing has been written and the +// existing certificate file is untouched. The returned certificate is the newly +// installed one. func Apply(req Request) (*x509.Certificate, error) { candidate, err := Validate(req) if err != nil { @@ -132,6 +143,13 @@ func Validate(req Request) (*x509.Certificate, error) { return nil, fmt.Errorf("renewed certificate does not verify against the trusted CAs: %w", err) } + // Last, once the candidate is known good: is it the one already installed? + // Compare the DER, so PEM line endings or trailing whitespace cannot make + // an identical certificate look like a new one. + if bytes.Equal(candidate.Raw, req.Current.Raw) { + return candidate, ErrUnchanged + } + return candidate, nil } diff --git a/internal/certrenew/certrenew_test.go b/internal/certrenew/certrenew_test.go index b88d18f..10bb92a 100644 --- a/internal/certrenew/certrenew_test.go +++ b/internal/certrenew/certrenew_test.go @@ -3,6 +3,8 @@ package certrenew_test import ( "bytes" "crypto/x509" + "encoding/pem" + "errors" "os" "path/filepath" "runtime" @@ -133,6 +135,62 @@ func TestApplyInstallsRenewedCertificate(t *testing.T) { } } +// TestApplyIsANoOpWhenTheCertificateIsAlreadyInstalled: a server that keeps +// answering RENEWED with the certificate the agent already presents must not +// push it through a connect-renew-reconnect loop forever. The comparison is on +// the DER, so re-encoded PEM does not read as a different certificate. +func TestApplyIsANoOpWhenTheCertificateIsAlreadyInstalled(t *testing.T) { + tests := []struct { + name string + candidate func(f *fixture) []byte + }{ + {"the exact bytes on disk", func(f *fixture) []byte { return f.originalPEM }}, + {"the same certificate, re-encoded", func(f *fixture) []byte { + block, _ := pem.Decode(f.originalPEM) + if block == nil { + t.Fatal("fixture certificate is not PEM") + } + // Same DER, different PEM framing: extra trailing newline and a + // header the original does not carry. + out := pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", + Headers: map[string]string{"Comment": "re-encoded"}, + Bytes: block.Bytes, + }) + return append(out, '\n') + }}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := newFixture(t) + before, err := os.Stat(f.certPath) + if err != nil { + t.Fatal(err) + } + + candidate := tt.candidate(f) + if _, err := certrenew.Apply(f.request(candidate)); !errors.Is(err, certrenew.ErrUnchanged) { + t.Fatalf("Apply returned %v, want ErrUnchanged", err) + } + + if !bytes.Equal(f.onDisk(t), f.originalPEM) { + t.Error("certificate file was rewritten") + } + after, err := os.Stat(f.certPath) + if err != nil { + t.Fatal(err) + } + if !after.ModTime().Equal(before.ModTime()) { + t.Error("certificate file was touched") + } + if _, err := os.Stat(f.certPath + ".tmp"); err == nil { + t.Error("temporary file left behind") + } + }) + } +} + // TestApplyLeavesFileUntouchedOnEveryRejection is the safety property that // matters most: any failed check must leave the existing certificate byte for // byte intact, because a broken agent.crt takes the agent offline behind a diff --git a/internal/tunnel/renew.go b/internal/tunnel/renew.go index 9c7bfe5..2886fcb 100644 --- a/internal/tunnel/renew.go +++ b/internal/tunnel/renew.go @@ -105,7 +105,16 @@ func (sc *ServerConn) installRenewedCertificate(certPEM []byte) error { Current: current, CandidatePEM: certPEM, }) - if err != nil { + switch { + case errors.Is(err, certrenew.ErrUnchanged): + // The server offered the certificate the agent already presents. + // Reconnecting would change nothing, and a server that keeps answering + // this way would spin the agent through connect-renew-reconnect + // forever. Stay on this connection. + sc.log.Debug("server offered the certificate already installed, nothing to do") + return nil + + case err != nil: // Nothing was written; the agent carries on with the certificate it // has and will ask again on the next connection. sc.log.Warn("certificate renewal rejected", "err", err) diff --git a/internal/tunnel/renew_test.go b/internal/tunnel/renew_test.go index f3ef45d..ff838cc 100644 --- a/internal/tunnel/renew_test.go +++ b/internal/tunnel/renew_test.go @@ -258,6 +258,10 @@ func TestRenewResponseStatusesThatWriteNothing(t *testing.T) { }{ {"current", protocol.RenewStatusCurrent, func(*renewFixture) []byte { return nil }}, {"error", protocol.RenewStatusError, func(*renewFixture) []byte { return []byte("agent has no certificate on file") }}, + // A server that answered RENEWED with the certificate the agent already + // presents would otherwise spin it through connect-renew-reconnect + // forever, so this must not recycle the connection either. + {"renewed with the certificate already installed", protocol.RenewStatusRenewed, func(f *renewFixture) []byte { return f.originalPEM }}, {"renewed but untrusted issuer", protocol.RenewStatusRenewed, func(f *renewFixture) []byte { return f.untrustedPEM }}, {"renewed but malformed", protocol.RenewStatusRenewed, func(*renewFixture) []byte { return []byte("not a certificate") }}, {"renewed with an empty body", protocol.RenewStatusRenewed, func(*renewFixture) []byte { return nil }}, From 9319c3e6cbdad714b1ee083d8c1ebddebc059b0f Mon Sep 17 00:00:00 2001 From: Thiago Durante Date: Tue, 15 Sep 2026 18:09:19 +0200 Subject: [PATCH 12/15] feat(caroot): trust the new DeployHQ Agent CA alongside the current one Appends the public certificate of the CA that agent certificates are issued under from the rotation onwards: CN=DeployHQ Agent CA, O=DeployHQ, valid until 2036-09-15 SHA256 4D:96:83:F0:CA:37:C8:4F:4A:52:E5:0E:E9:4E:61:5F:3B:CD:37:3A:29:F1:0B:5E:87:BE:C5:87:26:57:C7:6C The current CA stays first and unchanged. With both in the bundle this build accepts a renewed certificate and trusts the agent server whichever of the two signs its certificate. Co-Authored-By: Claude Opus 5 (1M context) --- internal/caroot/ca.crt | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/internal/caroot/ca.crt b/internal/caroot/ca.crt index 9233736..17c3696 100644 --- a/internal/caroot/ca.crt +++ b/internal/caroot/ca.crt @@ -31,3 +31,33 @@ l4CPcGbB0L8yyIhGwiEfrZpjx6hOelX1daG8QPTvSSYpB6ODtQeb3zpDf8vU8M7T oAwG8/0g1Owh/a970vIKu4TBa4D2IiCfA3KPWlsIUSoeu9uBTKmUQ0Raa0AhZWPv JI4XgcL63KznYzLm0BOxvTYMxDfn7fs= -----END CERTIFICATE----- +-----BEGIN CERTIFICATE----- +MIIFGzCCAwOgAwIBAgIBATANBgkqhkiG9w0BAQsFADAvMRowGAYDVQQDDBFEZXBs +b3lIUSBBZ2VudCBDQTERMA8GA1UECgwIRGVwbG95SFEwHhcNMjYwOTE1MDczODI4 +WhcNMzYwOTE1MDczODI4WjAvMRowGAYDVQQDDBFEZXBsb3lIUSBBZ2VudCBDQTER +MA8GA1UECgwIRGVwbG95SFEwggIiMA0GCSqGSIb3DQEBAQUAA4ICDwAwggIKAoIC +AQC4bMAQgGZC7ZUO9Wq2+rWodOXL4IZ6Ae+Dfce+SlTsQUx5HrYrmMFTE9NJTxcE +dGYjAVVwpdvgT8flFKRiQa7j1xVOLr73ra7Uro0ZGHw1TOBFFAhXLZ25oXT8SsB7 +1aaRZksZAPGR1ZkWRvKqUkRge9LERtcmZDOSeKyf3i3PQp3AaODA4tRwsQnyKWoA +6h9Ruk9jnHqNRapiEjjdkubKKRjgU28IO0Abom5dRsDvvFYVquaQmud745kdaxDr +fQA9Kyz8lzNSglmTHACjzxtX1AGRpn5yLO7PqaV0M/QqnKZhaKW+tzP3pWdsDZ8v +RBGDhyVYeyHYSPAEOgsRdTqQYpnXkR1FLLS/FCFQ/UgyWFStFVv3l9mzq/fQLr/O +sdPzG9KNNTz4OqMrGOeRMcBXsKQJjIRS6TSzggPagcxT2iPRHCGQknko8/O3IyM1 +9YIsLm7kxw2oEIzhHx8Wv3s+SafJr4cuTu1n9RfyP3LmU/C6GbfrH/jDvVuLvFdH +9AY55RNCfCgCKWjqDeYrzGxIcuoZ57H6DJlGouw0MiSg4hUUqNnM1EQG410bJouA +itprB7FIGcZ9uHQvpEgs8NP/DTRsmUP+51Vdp6SLbFSNrt5v/APDrd3ya8R3f7g0 +XVLFl/jxNM4Ki+L6ZGwRz4SYThew0OZXsezbnYiGEro1JwIDAQABo0IwQDAdBgNV +HQ4EFgQUgnCdcTQCEbmADry95bq7UAoX71AwDwYDVR0TAQH/BAUwAwEB/zAOBgNV +HQ8BAf8EBAMCAQYwDQYJKoZIhvcNAQELBQADggIBAHaN63Zqs4W5FTTqQPYnkMH0 +/hMzRVVcQ8tZng6rGy4HdONixQ2XOwFOCMbl8uqRB/ihyLbwiLIVgsqG3wMEIiqV +lxNDYerUtdBolam/lqA371d18lEqKFiFfncCRrd+AzTMcYnYZZK026smx1JCxDCP +t1rB/T9Lv0g2aRyWTKsKBeI5GFOIEIHScP1k/R7549kNEwerFKX5USWszDfnouiV +hSz9UjlY1vQQ//qYgimevJxlGudt2b+73+BkT5THdNKBxeyn02/DbhaH/1zafE/7 +lxAKmN94HkAOM2TD27ZPRDIK4jmKK/Hx03X0kYD+KL/9Mr/4PIjlObRFF8BUawp1 +Kpn/Caw4gNz76IujGXGvJp9FxsZUscRRRdikIZ3AtR0/6Wyf8fFaAd89DoBGSzLM +mRynJbnK5ROlVzvxX43ow7sBLtoh1+/RCsWcCVgOC2EMCW2n4b5M0USv7FBrhPie +F50NkMqsTtEoxw8kcKvLd4FbPvWuIxPsXzxlnFaHTbVgTevgL6O8IVeWLqdqwFGK +owB7z1tEQTN6sV4+fDDtaVQFCalEV9QPAolVLYdBUApc866heDHmzW85tkq2zoDG +2cj80hCcI42DPvrb9nEcIcZiFZDCcIlDjRf6kDmqBW4oXr5V1yAxrC+i0UA2+LWj +NVgD1tDYCLJlNY3GA6j8 +-----END CERTIFICATE----- From e9ce952d5e05b3c21ca2c40efc8ce48c8f070270 Mon Sep 17 00:00:00 2001 From: Thiago Durante Date: Thu, 17 Sep 2026 14:22:47 +0200 Subject: [PATCH 13/15] fix(certrenew): require client authentication when validating a renewal A replacement that pairs with the agent key, keeps subject and serial and chains to a trusted CA still has to be usable for the one thing the agent does with it. ExtKeyUsageAny also accepted a certificate whose extended key usage ruled client authentication out: it would be written over agent.crt and then refused by the server on every reconnect, leaving an agent behind a firewall offline with its working certificate already gone. ExtKeyUsageClientAuth costs nothing for real certificates -- the backend mints them with no extended key usage at all, which stays valid for every usage -- and refuses the broken case before anything is written. The Ruby agent makes the same check with PURPOSE_SSL_CLIENT. testca can now mint a leaf carrying an explicit extended key usage, which is what the new tests need; real ones carry none. Reported by Codex on #5. Co-Authored-By: Claude Opus 5 (1M context) --- internal/certrenew/certrenew.go | 15 ++++++----- internal/certrenew/certrenew_test.go | 40 ++++++++++++++++++++++++++++ internal/testca/testca.go | 16 +++++++++-- 3 files changed, 63 insertions(+), 8 deletions(-) diff --git a/internal/certrenew/certrenew.go b/internal/certrenew/certrenew.go index aaaa89a..326c457 100644 --- a/internal/certrenew/certrenew.go +++ b/internal/certrenew/certrenew.go @@ -131,14 +131,17 @@ func Validate(req Request) (*x509.Certificate, error) { candidate.SerialNumber, req.Current.SerialNumber) } - // (c) The certificate must chain to a CA this agent trusts. Mirrors the - // VerifyOptions used by config.NewTLSConfig's VerifyConnection: roots only - // and no DNSName, because DeployHQ's certificates are CN-only with no SANs. - // KeyUsageAny because an agent certificate carries no extended key usage - // extension at all and the zero value of KeyUsages would demand ServerAuth. + // (c) The certificate must chain to a CA this agent trusts, AND be usable + // for the one thing the agent does with it: client authentication. Roots + // only and no DNSName, because DeployHQ's certificates are CN-only with no + // SANs. ClientAuth rather than Any: a certificate carrying no extended key + // usage -- which is what the backend actually mints -- is valid for every + // usage and still passes, while one whose EKU rules client authentication + // out is refused here rather than on every reconnect after it has already + // replaced the working certificate. if _, err := candidate.Verify(x509.VerifyOptions{ Roots: req.Roots, - KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageAny}, + KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, }); err != nil { return nil, fmt.Errorf("renewed certificate does not verify against the trusted CAs: %w", err) } diff --git a/internal/certrenew/certrenew_test.go b/internal/certrenew/certrenew_test.go index 10bb92a..0d2bdf4 100644 --- a/internal/certrenew/certrenew_test.go +++ b/internal/certrenew/certrenew_test.go @@ -39,6 +39,10 @@ type fixture struct { wrongKey []byte unknownCA []byte + // Kept so a test can mint a variant the fixture does not carry. + newCA *testca.CA + agent *testca.Identity + roots *x509.CertPool } @@ -73,6 +77,8 @@ func newFixture(t *testing.T) *fixture { wrongSubject: newCA.Reissue(t, agent, "Deploy Agent #99", agentSerial), wrongKey: impostor.CertPEM, unknownCA: otherCA.Reissue(t, agent, agentCN, agentSerial), + newCA: newCA, + agent: agent, roots: testca.Pool(oldCA, newCA), } } @@ -135,6 +141,40 @@ func TestApplyInstallsRenewedCertificate(t *testing.T) { } } +// TestApplyRejectsACertificateThatCannotDoClientAuth: the agent presents its +// certificate for client authentication and nothing else. A replacement that +// pairs with the key, keeps subject and serial and chains to a trusted CA, but +// whose extended key usage rules client authentication out, would be written to +// disk and then refused by the server on every reconnect -- the agent is behind +// a customer's firewall with its working certificate already gone. A real agent +// certificate carries no extended key usage at all, which remains valid for any +// usage, so requiring ClientAuth costs nothing and closes that door. +func TestApplyRejectsACertificateThatCannotDoClientAuth(t *testing.T) { + f := newFixture(t) + + serverAuthOnly := f.newCA.ReissueWithExtKeyUsage(t, f.agent, agentCN, agentSerial, x509.ExtKeyUsageServerAuth) + + if _, err := certrenew.Apply(f.request(serverAuthOnly)); err == nil { + t.Fatal("Apply accepted a certificate that cannot be used for client authentication") + } + if got := f.onDisk(t); !bytes.Equal(got, f.originalPEM) { + t.Error("the working certificate was replaced by one the server would reject") + } +} + +// TestApplyAcceptsAnExplicitClientAuthCertificate is the control for the test +// above: tightening the check must not reject a certificate that names the +// usage explicitly, only one that excludes it. +func TestApplyAcceptsAnExplicitClientAuthCertificate(t *testing.T) { + f := newFixture(t) + + clientAuth := f.newCA.ReissueWithExtKeyUsage(t, f.agent, agentCN, agentSerial, x509.ExtKeyUsageClientAuth) + + if _, err := certrenew.Apply(f.request(clientAuth)); err != nil { + t.Fatalf("Apply rejected an explicit clientAuth certificate: %v", err) + } +} + // TestApplyIsANoOpWhenTheCertificateIsAlreadyInstalled: a server that keeps // answering RENEWED with the certificate the agent already presents must not // push it through a connect-renew-reconnect loop forever. The comparison is on diff --git a/internal/testca/testca.go b/internal/testca/testca.go index 0fe3143..88dbf16 100644 --- a/internal/testca/testca.go +++ b/internal/testca/testca.go @@ -124,14 +124,26 @@ func (ca *CA) Reissue(t *testing.T, id *Identity, commonName string, serial int6 return certPEMBytes } -func (ca *CA) sign(t *testing.T, pub crypto.PublicKey, commonName string, serial int64) ([]byte, *x509.Certificate) { +// ReissueWithExtKeyUsage is Reissue for a certificate that DOES carry an +// extended key usage extension. Real agent certificates carry none, so this +// exists to build the ones an agent must refuse -- a server-auth-only +// certificate that would be rejected by the server's client authentication. +func (ca *CA) ReissueWithExtKeyUsage(t *testing.T, id *Identity, commonName string, serial int64, eku ...x509.ExtKeyUsage) []byte { + t.Helper() + certPEMBytes, _ := ca.sign(t, id.key.Public(), commonName, serial, eku...) + return certPEMBytes +} + +func (ca *CA) sign(t *testing.T, pub crypto.PublicKey, commonName string, serial int64, eku ...x509.ExtKeyUsage) ([]byte, *x509.Certificate) { t.Helper() tmpl := &x509.Certificate{ SerialNumber: big.NewInt(serial), Subject: subject(commonName), NotBefore: time.Now().Add(-time.Hour), NotAfter: time.Now().Add(10 * 365 * 24 * time.Hour), - // No SANs, no KeyUsage, no ExtKeyUsage: exactly as DeployHQ mints them. + // No SANs, no KeyUsage, and no ExtKeyUsage unless a test asks for one: + // exactly as DeployHQ mints them. + ExtKeyUsage: eku, } der, err := x509.CreateCertificate(rand.Reader, tmpl, ca.Cert, pub, ca.key) if err != nil { From 73da9a437f3e1989aa07ec38b1c1813f082746e7 Mon Sep 17 00:00:00 2001 From: Thiago Durante Date: Thu, 17 Sep 2026 14:22:47 +0200 Subject: [PATCH 14/15] fix(config): reject a CA bundle whose certificates do not all parse AppendCertsFromPEM reports success as soon as one block parses and silently drops the rest, so a DEPLOY_AGENT_CA_FILE bundle whose incoming CA is corrupt started the agent trusting only the CA it already had. The override is the escape hatch for trusting a new CA without waiting for a release, so failing silently is the one thing it must not do: it looks like it worked and then fails at the moment the server rotates. Every PEM block is now parsed, a non-certificate block is an error, and trailing junk after the last block is too. A test asserts the embedded bundle this binary ships with still parses through the stricter path. Reported by Codex on #5. Co-Authored-By: Claude Opus 5 (1M context) --- internal/config/tls.go | 31 ++++++++++++++++++++++++++++- internal/config/tls_test.go | 39 +++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/internal/config/tls.go b/internal/config/tls.go index db973a4..5f8936b 100644 --- a/internal/config/tls.go +++ b/internal/config/tls.go @@ -3,6 +3,7 @@ package config import ( "crypto/tls" "crypto/x509" + "encoding/pem" "fmt" "os" "strings" @@ -11,11 +12,39 @@ import ( // NewCertPool parses a PEM bundle into a certificate pool. The bundle may hold // more than one CA — that is how an agent trusts the old and the new DeployHQ // CA simultaneously during a CA rotation. +// +// Every block has to parse. AppendCertsFromPEM would report success as soon as +// ONE did, silently dropping the rest: a bundle whose incoming CA is corrupt +// would start the agent trusting only the CA it already had, so the override +// looks like it worked and then fails the moment the server rotates. func NewCertPool(caCert []byte) (*x509.CertPool, error) { pool := x509.NewCertPool() - if !pool.AppendCertsFromPEM(caCert) { + rest := caCert + added := 0 + + for { + var block *pem.Block + block, rest = pem.Decode(rest) + if block == nil { + break + } + if block.Type != "CERTIFICATE" { + return nil, fmt.Errorf("CA bundle holds a %q block, not a certificate", block.Type) + } + cert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + return nil, fmt.Errorf("parsing certificate %d in the CA bundle: %w", added+1, err) + } + pool.AddCert(cert) + added++ + } + + if added == 0 { return nil, fmt.Errorf("failed to parse CA certificate") } + if trailing := strings.TrimSpace(string(rest)); trailing != "" { + return nil, fmt.Errorf("CA bundle has %d trailing bytes that are not a PEM certificate", len(trailing)) + } return pool, nil } diff --git a/internal/config/tls_test.go b/internal/config/tls_test.go index 388d785..8c73619 100644 --- a/internal/config/tls_test.go +++ b/internal/config/tls_test.go @@ -10,6 +10,7 @@ import ( "testing" "time" + "github.com/deployhq/network-agent/internal/caroot" "github.com/deployhq/network-agent/internal/config" "github.com/deployhq/network-agent/internal/testca" ) @@ -231,6 +232,44 @@ func TestNewCertPool(t *testing.T) { } } +// TestNewCertPoolRejectsAPartiallyMalformedBundle: DEPLOY_AGENT_CA_FILE is the +// escape hatch for trusting a new CA without waiting for a release, so it has +// to fail loudly when it is wrong. Go's AppendCertsFromPEM reports success as +// long as ONE block parsed, which would start the agent trusting only the CA it +// already had -- the override appears to have worked and then fails at the +// moment of rotation, which is the moment it exists for. +func TestNewCertPoolRejectsAPartiallyMalformedBundle(t *testing.T) { + good := testca.Bundle(testca.New(t, "old")) + corrupt := []byte("-----BEGIN CERTIFICATE-----\nbm90IGEgY2VydGlmaWNhdGU=\n-----END CERTIFICATE-----\n") + + if _, err := config.NewCertPool(append(append([]byte{}, good...), corrupt...)); err == nil { + t.Error("a bundle whose second certificate is malformed should be an error") + } + if _, err := config.NewCertPool(append(append([]byte{}, corrupt...), good...)); err == nil { + t.Error("a bundle whose first certificate is malformed should be an error") + } +} + +// TestNewCertPoolAcceptsTheShippedBundle: the stricter parsing above must not +// reject what this binary actually ships with -- the embedded bundle holds both +// DeployHQ CAs during the rotation, and it is read through this same function. +func TestNewCertPoolAcceptsTheShippedBundle(t *testing.T) { + if _, err := config.NewCertPool(caroot.CACert); err != nil { + t.Fatalf("the embedded CA bundle does not parse: %v", err) + } +} + +// TestNewCertPoolRejectsANonCertificateBlock: a PEM file of the right shape but +// the wrong contents -- a private key pasted in place of a certificate -- is a +// plausible operator mistake and must not read as an empty-but-valid bundle. +func TestNewCertPoolRejectsANonCertificateBlock(t *testing.T) { + notACert := []byte("-----BEGIN PRIVATE KEY-----\nMC4CAQAwBQYDK2VwBCIEIA==\n-----END PRIVATE KEY-----\n") + + if _, err := config.NewCertPool(notACert); err == nil { + t.Error("a bundle holding no certificate at all should be an error") + } +} + // ── test server ────────────────────────────────────────────────────────────── // testServer accepts one mTLS connection at a time and reports the issuer of From b695b3a01635e21d7cbb577d115c68033dbd952e Mon Sep 17 00:00:00 2001 From: Thiago Durante Date: Thu, 17 Sep 2026 15:41:48 +0200 Subject: [PATCH 15/15] fix(config): refuse a CA bundle with anything pem.Decode would skip Parsing every block closed the back of the file but not the front: encoding/pem skips whatever it cannot read AHEAD of a block it can, so junk before the first certificate, or a BEGIN line with no matching END, left the agent quietly trusting whichever CA did parse. Probed against the shipped bundle: both were accepted. Each chunk must now begin with a PEM header once whitespace is stripped, and the number of certificates read has to match the number of BEGIN lines in the file -- which is what catches the unterminated block that a later, valid one would otherwise cover for. Reported by CodeRabbit on #5. Co-Authored-By: Claude Opus 5 (1M context) --- internal/config/tls.go | 26 +++++++++++++++++++++++--- internal/config/tls_test.go | 19 +++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/internal/config/tls.go b/internal/config/tls.go index 5f8936b..707c0ac 100644 --- a/internal/config/tls.go +++ b/internal/config/tls.go @@ -1,6 +1,7 @@ package config import ( + "bytes" "crypto/tls" "crypto/x509" "encoding/pem" @@ -9,6 +10,10 @@ import ( "strings" ) +// pemBegin opens every PEM block; a bundle that does not start with it, or +// that carries more of them than parse, is malformed. +const pemBegin = "-----BEGIN " + // NewCertPool parses a PEM bundle into a certificate pool. The bundle may hold // more than one CA — that is how an agent trusts the old and the new DeployHQ // CA simultaneously during a CA rotation. @@ -22,11 +27,26 @@ func NewCertPool(caCert []byte) (*x509.CertPool, error) { rest := caCert added := 0 + // pem.Decode SKIPS anything it cannot parse ahead of a block it can, so + // neither junk before the first block nor a BEGIN line with no matching END + // would be reported by decoding alone. Both are refused here: each chunk has + // to start with a BEGIN line, and the number of blocks decoded has to match + // the number of BEGIN lines in the file. + begins := bytes.Count(caCert, []byte(pemBegin)) + for { + rest = bytes.TrimLeft(rest, " \t\r\n") + if len(rest) == 0 { + break + } + if !bytes.HasPrefix(rest, []byte(pemBegin)) { + return nil, fmt.Errorf("CA bundle has data that is not a PEM certificate, after %d certificate(s)", added) + } + var block *pem.Block block, rest = pem.Decode(rest) if block == nil { - break + return nil, fmt.Errorf("CA bundle has an unreadable PEM block after %d certificate(s)", added) } if block.Type != "CERTIFICATE" { return nil, fmt.Errorf("CA bundle holds a %q block, not a certificate", block.Type) @@ -42,8 +62,8 @@ func NewCertPool(caCert []byte) (*x509.CertPool, error) { if added == 0 { return nil, fmt.Errorf("failed to parse CA certificate") } - if trailing := strings.TrimSpace(string(rest)); trailing != "" { - return nil, fmt.Errorf("CA bundle has %d trailing bytes that are not a PEM certificate", len(trailing)) + if added != begins { + return nil, fmt.Errorf("CA bundle declares %d certificates but only %d could be read", begins, added) } return pool, nil } diff --git a/internal/config/tls_test.go b/internal/config/tls_test.go index 8c73619..18a73f3 100644 --- a/internal/config/tls_test.go +++ b/internal/config/tls_test.go @@ -250,6 +250,25 @@ func TestNewCertPoolRejectsAPartiallyMalformedBundle(t *testing.T) { } } +// TestNewCertPoolRejectsWhatPemDecodeWouldSkip: encoding/pem skips anything it +// cannot parse BEFORE a block it can -- leading junk, or a BEGIN line with no +// matching END. Either would leave the agent trusting whichever CA happened to +// parse, which is the same silent half-trust as a malformed block at the end of +// the file, just at the other end of it. +func TestNewCertPoolRejectsWhatPemDecodeWouldSkip(t *testing.T) { + good := testca.Bundle(testca.New(t, "old")) + + leadingGarbage := append([]byte("garbage before the block\n"), good...) + if _, err := config.NewCertPool(leadingGarbage); err == nil { + t.Error("data before the first certificate should be an error") + } + + unterminated := append([]byte("-----BEGIN CERTIFICATE-----\nbm90IGEgY2VydA==\n"), good...) + if _, err := config.NewCertPool(unterminated); err == nil { + t.Error("an unterminated certificate block should be an error") + } +} + // TestNewCertPoolAcceptsTheShippedBundle: the stricter parsing above must not // reject what this binary actually ships with -- the embedded bundle holds both // DeployHQ CAs during the rotation, and it is read through this same function.