Fix three security issues: revocation bypass, serial truncation, CSR rate limit

- MEDIUM: auth middleware now calls IsRevokedSerial(cert.SerialNumber) instead
  of IsRevoked(CN), closing a bypass window where an old revoked credential
  could still authenticate after re-issuance for the same CN. Fail-closed:
  CRL read errors are treated as denials (was fail-open).
  New IsRevokedSerial method added to ca.Revoke; delegates to the existing
  private isRevokedSerial (used by OCSP) with a proper read lock.

- LOW: CertStatusResponse.SerialNumber changed from *int64 to *string using
  big.Int.Text(10), preventing silent truncation of 128-bit random serials.

- INFO: per-IP fixed-window rate limiter (ratelimit.go) added to the
  unauthenticated PUT /certificate_request endpoint. Configured via
  Server.CSRRateLimit (0 = disabled, default). Does not trust proxy headers.

Tests updated across auth_test.go, ca_test.go, and api_test.go to detect
regressions for all three issues, including the re-issuance bypass scenario
and CRL-unavailable fail-closed behaviour.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Author: trevor-vaughan

internal/api/api_test.go

@@ -884,4 +884,81 @@ var _ = Describe("API Workflow", func() {
 		})
 	})
 
+	Context("per-IP rate limiting on CSR submission", func() {
+		It("should return 429 when the per-IP limit is exceeded within the window", func() {
+			// Build a server with a tight limit of 2 requests/minute.
+			limitedServer := api.New(myCA)
+			limitedServer.CSRRateLimit = 2
+			limitedMux := limitedServer.Routes()
+
+			// First two requests succeed (200 or 409 — both mean the limiter allowed them through).
+			for i := 0; i < 2; i++ {
+				csrPEM, err := testutil.GenerateCSR("rl-node")
+				Expect(err).NotTo(HaveOccurred())
+				req := httptest.NewRequest("PUT", "/certificate_request/rl-node", bytes.NewReader(csrPEM))
+				rr := httptest.NewRecorder()
+				limitedMux.ServeHTTP(rr, req)
+				Expect(rr.Code).NotTo(Equal(http.StatusTooManyRequests))
+			}
+
+			// Third request from the same IP must be rate-limited.
+			csrPEM, err := testutil.GenerateCSR("rl-node")
+			Expect(err).NotTo(HaveOccurred())
+			req := httptest.NewRequest("PUT", "/certificate_request/rl-node", bytes.NewReader(csrPEM))
+			rr := httptest.NewRecorder()
+			limitedMux.ServeHTTP(rr, req)
+			Expect(rr.Code).To(Equal(http.StatusTooManyRequests))
+		})
+
+		It("should not rate-limit when CSRRateLimit is zero (default)", func() {
+			// The shared server has no rate limit set; submit many requests.
+			for i := 0; i < 5; i++ {
+				csrPEM, err := testutil.GenerateCSR("nolimit-node")
+				Expect(err).NotTo(HaveOccurred())
+				req := httptest.NewRequest("PUT", "/certificate_request/nolimit-node", bytes.NewReader(csrPEM))
+				rr := httptest.NewRecorder()
+				mux.ServeHTTP(rr, req)
+				Expect(rr.Code).NotTo(Equal(http.StatusTooManyRequests))
+			}
+		})
+	})
+
+	Context("serial_number in status response is a full decimal string", func() {
+		It("should return serial_number as a non-empty decimal string without truncation", func() {
+			subject := "serial-node"
+			csrPEM, err := testutil.GenerateCSR(subject)
+			Expect(err).NotTo(HaveOccurred())
+
+			// Submit CSR and sign it.
+			mux.ServeHTTP(httptest.NewRecorder(),
+				httptest.NewRequest("PUT", "/certificate_request/"+subject, bytes.NewReader(csrPEM)))
+			body, _ := json.Marshal(api.PutStatusBody{DesiredState: "signed"})
+			mux.ServeHTTP(httptest.NewRecorder(),
+				httptest.NewRequest("PUT", "/certificate_status/"+subject, bytes.NewReader(body)))
+
+			// Fetch the signed cert and parse its serial for comparison.
+			certRR := httptest.NewRecorder()
+			mux.ServeHTTP(certRR, httptest.NewRequest("GET", "/certificate/"+subject, nil))
+			Expect(certRR.Code).To(Equal(http.StatusOK))
+			block, _ := pem.Decode(certRR.Body.Bytes())
+			Expect(block).NotTo(BeNil())
+			cert, err := x509.ParseCertificate(block.Bytes)
+			Expect(err).NotTo(HaveOccurred())
+			expectedSerial := cert.SerialNumber.Text(10)
+
+			// Fetch status and confirm serial_number matches exactly.
+			statusRR := httptest.NewRecorder()
+			mux.ServeHTTP(statusRR, httptest.NewRequest("GET", "/certificate_status/"+subject, nil))
+			Expect(statusRR.Code).To(Equal(http.StatusOK))
+
+			var resp api.CertStatusResponse
+			Expect(json.Unmarshal(statusRR.Body.Bytes(), &resp)).To(Succeed())
+			Expect(resp.SerialNumber).NotTo(BeNil())
+			// Must be a pure decimal string.
+			Expect(*resp.SerialNumber).To(MatchRegexp(`^[0-9]+$`))
+			// Must be the full, un-truncated value.
+			Expect(*resp.SerialNumber).To(Equal(expectedSerial))
+		})
+	})
+
 })

internal/api/auth.go

@@ -98,9 +98,18 @@ func newAuthMiddleware(cfg *AuthConfig, myCA *ca.CA, next http.Handler) http.Han
 
 		clientCN := clientCert.Subject.CommonName
 
-		// Check whether the client cert has been revoked.
-		if myCA.IsRevoked(clientCN) {
-			slog.Debug("Auth: client cert is revoked", "cn", clientCN)
+		// Check whether the presented cert's serial appears in the CRL.
+		// We check the serial of the actual presented certificate — not the
+		// serial of whatever cert happens to be on disk for the same CN —
+		// so that old revoked credentials are rejected even after a
+		// re-issuance for the same CN.  Fail-closed: a CRL read error is
+		// also treated as a denial.
+		if revoked, err := myCA.IsRevokedSerial(clientCert.SerialNumber); err != nil || revoked {
+			if err != nil {
+				slog.Warn("Auth: CRL check failed (denying)", "cn", clientCN, "error", err)
+			} else {
+				slog.Debug("Auth: client cert is revoked", "cn", clientCN)
+			}
 			http.Error(w, "access denied", http.StatusForbidden)
 			return
 		}

internal/api/auth_test.go

@@ -265,8 +265,34 @@ var _ = Describe("Auth Middleware", func() {
 	// ── Revoked client cert ────────────────────────────────────────────────────
 
 	Context("revoked client cert", func() {
-		It("returns 403 even though the cert is CA-signed", func() {
-			// Register the CN in the CA so Revoke can find it in inventory.
+		It("returns 403 when the presented cert's serial is in the CRL", func() {
+			// Sign a cert through the CA so its serial is tracked.
+			csrPEM, err := testutil.GenerateCSR("revoked-client")
+			Expect(err).NotTo(HaveOccurred())
+			_, err = myCA.SaveRequest("revoked-client", csrPEM)
+			Expect(err).NotTo(HaveOccurred())
+			certPEM, err := myCA.Sign("revoked-client")
+			Expect(err).NotTo(HaveOccurred())
+
+			// Parse the issued cert so we can present it in the TLS request.
+			block, _ := pem.Decode(certPEM)
+			issuedCert, err := x509.ParseCertificate(block.Bytes)
+			Expect(err).NotTo(HaveOccurred())
+
+			// Revoke the cert — its serial is now in the CRL.
+			Expect(myCA.Revoke("revoked-client")).To(Succeed())
+
+			// Present the revoked cert; the middleware checks its serial
+			// directly against the CRL and must deny access.
+			req := httptest.NewRequest("GET", "/certificate_request/revoked-client", nil)
+			req = withClientCert(req, issuedCert)
+			rr := httptest.NewRecorder()
+			mux.ServeHTTP(rr, req)
+			Expect(rr.Code).To(Equal(http.StatusForbidden))
+		})
+
+		It("allows a cert whose serial is NOT in the CRL even when another cert for the same CN was revoked", func() {
+			// Sign and revoke "revoked-client".
 			csrPEM, err := testutil.GenerateCSR("revoked-client")
 			Expect(err).NotTo(HaveOccurred())
 			_, err = myCA.SaveRequest("revoked-client", csrPEM)
@@ -275,14 +301,90 @@ var _ = Describe("Auth Middleware", func() {
 			Expect(err).NotTo(HaveOccurred())
 			Expect(myCA.Revoke("revoked-client")).To(Succeed())
 
-			// Issue a fresh TLS cert with the revoked CN.
-			// IsRevoked looks up the on-disk cert for the CN, reads its serial
-			// number, and checks whether that serial is in the CRL.  The TLS-
-			// presented cert's serial is not consulted; only the CN is used to
-			// locate the revoked record on disk.
-			clientCert := issueClientCert("revoked-client", caCert, caKey)
+			// A separately-issued cert with the same CN but a different serial
+			// (not in the CRL) must pass the revocation check.
+			freshCert := issueClientCert("revoked-client", caCert, caKey)
 			req := httptest.NewRequest("GET", "/certificate_request/revoked-client", nil)
-			req = withClientCert(req, clientCert)
+			req = withClientCert(req, freshCert)
+			rr := httptest.NewRecorder()
+			mux.ServeHTTP(rr, req)
+			// The cert is not revoked — access is denied only if it also
+			// fails the tier check (self-or-admin: CN matches path subject).
+			Expect(rr.Code).NotTo(Equal(http.StatusForbidden))
+		})
+	})
+
+	// ── Revocation bypass prevention (re-issuance regression) ─────────────────
+	// Before the fix, IsRevoked looked up the cert *on disk* for the CN and
+	// checked that cert's serial.  After a revocation + re-issuance the disk
+	// cert had a new (clean) serial, so the old revoked cert would pass.
+	// IsRevokedSerial checks the serial of the PRESENTED cert, closing the gap.
+
+	Context("revocation bypass prevention after re-issuance", func() {
+		It("denies an old revoked cert even after the same CN has been re-issued", func() {
+			// Step 1: issue the first cert for "puppet-server" (admin CN).
+			csrPEM1, err := testutil.GenerateCSR("puppet-server")
+			Expect(err).NotTo(HaveOccurred())
+			_, err = myCA.SaveRequest("puppet-server", csrPEM1)
+			Expect(err).NotTo(HaveOccurred())
+			certPEM1, err := myCA.Sign("puppet-server")
+			Expect(err).NotTo(HaveOccurred())
+			block1, _ := pem.Decode(certPEM1)
+			oldCert, err := x509.ParseCertificate(block1.Bytes)
+			Expect(err).NotTo(HaveOccurred())
+
+			// Step 2: revoke it — serial1 is now in the CRL.
+			Expect(myCA.Revoke("puppet-server")).To(Succeed())
+
+			// Step 3: re-register and sign a new cert for the same CN.
+			csrPEM2, err := testutil.GenerateCSR("puppet-server")
+			Expect(err).NotTo(HaveOccurred())
+			_, err = myCA.SaveRequest("puppet-server", csrPEM2) // evicts the revoked cert
+			Expect(err).NotTo(HaveOccurred())
+			certPEM2, err := myCA.Sign("puppet-server")
+			Expect(err).NotTo(HaveOccurred())
+			block2, _ := pem.Decode(certPEM2)
+			newCert, err := x509.ParseCertificate(block2.Bytes)
+			Expect(err).NotTo(HaveOccurred())
+
+			// OLD cert (revoked serial) must be denied — regression test.
+			req := httptest.NewRequest("POST", "/sign/all", nil)
+			req = withClientCert(req, oldCert)
+			rr := httptest.NewRecorder()
+			mux.ServeHTTP(rr, req)
+			Expect(rr.Code).To(Equal(http.StatusForbidden))
+
+			// NEW cert (clean serial, same admin CN) must be allowed.
+			req2 := httptest.NewRequest("POST", "/sign/all", nil)
+			req2 = withClientCert(req2, newCert)
+			rr2 := httptest.NewRecorder()
+			mux.ServeHTTP(rr2, req2)
+			Expect(rr2.Code).NotTo(Equal(http.StatusForbidden))
+		})
+	})
+
+	// ── CRL unavailable → fail-closed ─────────────────────────────────────────
+
+	Context("CRL unavailable", func() {
+		It("returns 403 when the CRL file cannot be read (fail-closed)", func() {
+			// Sign a cert through the CA so it is a valid client cert.
+			csrPEM, err := testutil.GenerateCSR("crl-test-node")
+			Expect(err).NotTo(HaveOccurred())
+			_, err = myCA.SaveRequest("crl-test-node", csrPEM)
+			Expect(err).NotTo(HaveOccurred())
+			certPEM, err := myCA.Sign("crl-test-node")
+			Expect(err).NotTo(HaveOccurred())
+			block, _ := pem.Decode(certPEM)
+			issuedCert, err := x509.ParseCertificate(block.Bytes)
+			Expect(err).NotTo(HaveOccurred())
+
+			// Remove the CRL file to simulate a disk fault.
+			Expect(os.Remove(store.CRLPath())).To(Succeed())
+
+			// The middleware must deny the request (fail-closed) rather than
+			// allowing access because it cannot check revocation status.
+			req := httptest.NewRequest("GET", "/certificate_request/crl-test-node", nil)
+			req = withClientCert(req, issuedCert)
 			rr := httptest.NewRecorder()
 			mux.ServeHTTP(rr, req)
 			Expect(rr.Code).To(Equal(http.StatusForbidden))

internal/api/handlers.go

@@ -46,6 +46,12 @@ type AuthConfig struct {
 type Server struct {
 	CA         *ca.CA
 	AuthConfig *AuthConfig
+	// CSRRateLimit is the maximum number of CSR submissions allowed per IP
+	// address per minute on the unauthenticated PUT /certificate_request
+	// endpoint. Zero (the default) disables rate limiting.
+	CSRRateLimit int
+
+	csrLimiter *ipRateLimiter
 }
 
 func New(c *ca.CA) *Server {
@@ -56,6 +62,10 @@ func New(c *ca.CA) *Server {
 // Puppet agents use the /puppet-ca/v1/ prefix; we support both bare and prefixed paths
 // so the Go CA can be used directly or behind a stripping proxy.
 func (s *Server) Routes() http.Handler {
+	if s.CSRRateLimit > 0 {
+		s.csrLimiter = newIPRateLimiter(s.CSRRateLimit, time.Minute)
+	}
+
 	mux := http.NewServeMux()
 
 	routes := []struct {
@@ -109,7 +119,9 @@ type CertStatusResponse struct {
 	// Always present, empty map when none exist.
 	AuthorizationExtensions map[string]string `json:"authorization_extensions"`
 	// Populated when signed or revoked.
-	SerialNumber *int64  `json:"serial_number,omitempty"`
+	// SerialNumber is a decimal string to preserve the full 128-bit value
+	// without loss; int64 would silently truncate random CA/B-Forum serials.
+	SerialNumber *string `json:"serial_number,omitempty"`
 	NotBefore    *string `json:"not_before,omitempty"`
 	NotAfter     *string `json:"not_after,omitempty"`
 }
@@ -272,6 +284,11 @@ func (s *Server) handleGetRequest(w http.ResponseWriter, r *http.Request) {
 }
 
 func (s *Server) handlePutRequest(w http.ResponseWriter, r *http.Request) {
+	if s.csrLimiter != nil && !s.csrLimiter.Allow(clientIP(r)) {
+		http.Error(w, "rate limit exceeded", http.StatusTooManyRequests)
+		return
+	}
+
 	subject := r.PathValue("subject")
 	if err := ca.ValidateSubject(subject); err != nil {
 		http.Error(w, "invalid subject", http.StatusBadRequest)
@@ -428,7 +445,7 @@ func noNilSlice(s []string) []string {
 func certStatusFromCert(subject string, certPEM []byte, state string) CertStatusResponse {
 	cert := parseCert(certPEM)
 	fp := fingerprint(certPEM)
-	serial := cert.SerialNumber.Int64()
+	serial := cert.SerialNumber.Text(10) // decimal string; preserves full 128-bit value
 	nb := cert.NotBefore.UTC().Format(time.RFC3339)
 	na := cert.NotAfter.UTC().Format(time.RFC3339)
 	dnsNames := noNilSlice(cert.DNSNames)

internal/api/ratelimit.go

@@ -0,0 +1,77 @@
+// Copyright (C) 2026 Trevor Vaughan
+//
+// This program is free software; you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation; either version 2 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License along
+// with this program; if not, write to the Free Software Foundation, Inc.,
+// 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+package api
+
+import (
+	"net"
+	"net/http"
+	"sync"
+	"time"
+)
+
+// ipRateLimiter is a fixed-window per-IP rate limiter.
+// Each IP address is allowed at most maxReqs requests per window duration.
+// Old windows are evicted lazily on access so memory stays bounded over time.
+type ipRateLimiter struct {
+	mu      sync.Mutex
+	window  time.Duration
+	maxReqs int
+	entries map[string]*rlEntry
+}
+
+type rlEntry struct {
+	start time.Time
+	count int
+}
+
+func newIPRateLimiter(maxReqs int, window time.Duration) *ipRateLimiter {
+	return &ipRateLimiter{
+		window:  window,
+		maxReqs: maxReqs,
+		entries: make(map[string]*rlEntry),
+	}
+}
+
+// Allow reports whether the request from ip should be allowed.
+// Returns false when the per-window request count has been exceeded.
+func (l *ipRateLimiter) Allow(ip string) bool {
+	now := time.Now()
+	l.mu.Lock()
+	defer l.mu.Unlock()
+
+	e, ok := l.entries[ip]
+	if !ok || now.Sub(e.start) >= l.window {
+		l.entries[ip] = &rlEntry{start: now, count: 1}
+		return true
+	}
+	if e.count >= l.maxReqs {
+		return false
+	}
+	e.count++
+	return true
+}
+
+// clientIP extracts the remote IP address from r, stripping the port.
+// It does not trust X-Forwarded-For or similar headers since the server
+// accepts direct connections (no trusted reverse proxy layer).
+func clientIP(r *http.Request) string {
+	host, _, err := net.SplitHostPort(r.RemoteAddr)
+	if err != nil {
+		return r.RemoteAddr
+	}
+	return host
+}