Merge pull request #3 from esnet/develop

Implement cert revocation, option to configure max cert lifetime

Author: netops2devops

.github/workflows/ci.yml

@@ -50,6 +50,28 @@ jobs:
       - name: Run go vet
         run: go vet ./...
 
+  test:
+    name: Run unit tests
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v4
+
+      - name: Set up Go
+        uses: actions/setup-go@v5
+        with:
+          go-version: ${{ env.GO_VERSION }}
+
+      - name: Install dependencies
+        run: |
+          sudo apt-get update
+          sudo apt-get install -y libpcsclite-dev
+
+      - name: Run tests
+        run: |
+          make dev
+          go test -v -race ./...
+
   security-scan:
     name: Security Scan
     runs-on: ubuntu-latest
@@ -83,7 +105,7 @@ jobs:
   build:
     name: Build
     runs-on: ubuntu-latest
-    needs: [lint, security-scan]
+    needs: [lint, security-scan, test]
     steps:
       - name: Checkout code
         uses: actions/checkout@v4
@@ -110,7 +132,7 @@ jobs:
   container-build-and-push:
     name: Build and Push Container
     runs-on: ubuntu-latest
-    needs: [lint, security-scan]
+    needs: [lint, security-scan, test]
     permissions:
       contents: read
       packages: write

.pre-commit-config.yaml

@@ -7,3 +7,12 @@ repos:
       - id: end-of-file-fixer
       - id: check-yaml
       - id: check-added-large-files
+
+  - repo: local
+    hooks:
+      - id: go-test
+        name: go test
+        entry: go test ./externalcas
+        language: system
+        pass_filenames: false
+        types: [go]

README.md

@@ -11,7 +11,7 @@ This is a work in progress. Not quite ready for production but will be soon.
 TODO
 
 - [x] Move config bits from env vars to `ca.json`
-- [ ] Implement Revoke method
+- [x] Implement Revoke method
 - [x] Re-assess if `GetCertificateAuthority` is a requirement or not
 - [ ] Write unit tests
 - [ ] Prometheus metrics
@@ -54,6 +54,7 @@ The most important part of the config is this section
       "account_email": "",
       "eab_kid": "",
       "eab_hmac_key": "",
+      "certlifetime": 30,
       "metrics": {
         "enabled": true,
         "port": 9123

ca.json

@@ -15,6 +15,7 @@
       "account_email": "",
       "eab_kid": "",
       "eab_hmac_key": "",
+      "certlifetime": 30,
       "metrics": {
         "enabled": true,
         "port": 9123

externalcas/external.go

@@ -10,7 +10,7 @@ import (
 	"encoding/pem"
 	"errors"
 	"fmt"
-	"log"
+	"log/slog"
 	"net/http"
 	"sync"
 	"time"
@@ -42,7 +42,7 @@ type AcmeProxyConfig struct {
 	HmacKey string `json:"eab_hmac_key"`
 
 	// Certificate lifetime in days
-	ValidFor int `json:"validity,omitempty"`
+	CertLifetime int `json:"certlifetime,omitempty"`
 
 	// Prometheus metrics endpoint
 	Metrics Metrics `json:"metrics"`
@@ -91,7 +91,7 @@ func (c *ExternalCAS) Type() apiv1.Type {
 
 func (c *ExternalCAS) initClient() error {
 	c.initOnce.Do(func() {
-		log.Println("Initializing ACME client...")
+		slog.Info("initializing ACME client")
 
 		privateKey, err := rsa.GenerateKey(rand.Reader, 2048)
 		if err != nil {
@@ -102,7 +102,9 @@ func (c *ExternalCAS) initClient() error {
 		// Unmarshal EAB config from ca.json
 		var eab AcmeProxyConfig
 		if err = json.Unmarshal(c.config, &eab); err != nil {
-			log.Fatal("Error unmarshalling EAB config from ca.json", err)
+			slog.Error("failed to unmarshal EAB config", "error", err)
+			c.initError = fmt.Errorf("failed to unmarshal EAB config: %w", err)
+			return
 		}
 
 		user := User{
@@ -138,7 +140,7 @@ func (c *ExternalCAS) initClient() error {
 		}
 		c.user.Registration = reg
 
-		log.Println("ACME client initialized successfully")
+		slog.Info("ACME client initialized")
 	})
 	return c.initError
 }
@@ -158,14 +160,14 @@ func (c *ExternalCAS) CreateCertificate(req *apiv1.CreateCertificateRequest) (*a
 		return nil, fmt.Errorf("failed to initialize ACME client: %w", err)
 	}
 
-	log.Printf("Processing certificate request for domains: %v", req.CSR.DNSNames)
+	slog.Info("processing certificate request", "domains", req.CSR.DNSNames)
 
 	resultChan := make(chan *certificateResult, 1)
 
 	go func() {
 		defer func() {
 			if r := recover(); r != nil {
-				log.Printf("Recovered from panic in processCertificateRequest: %v", r)
+				slog.Error("recovered from panic in processCertificateRequest", "panic", r)
 				resultChan <- &certificateResult{
 					err: fmt.Errorf("internal error: %v", r),
 				}
@@ -176,7 +178,7 @@ func (c *ExternalCAS) CreateCertificate(req *apiv1.CreateCertificateRequest) (*a
 		select {
 		case resultChan <- result:
 		case <-ctx.Done():
-			log.Printf("Certificate request timed out or cancelled for domains: %v", req.CSR.DNSNames)
+			slog.Warn("certificate request timed out or cancelled", "domains", req.CSR.DNSNames)
 		}
 	}()
 
@@ -192,7 +194,7 @@ func (c *ExternalCAS) CreateCertificate(req *apiv1.CreateCertificateRequest) (*a
 }
 
 func (c *ExternalCAS) processCertificateRequest(ctx context.Context, req *apiv1.CreateCertificateRequest) *certificateResult {
-	log.Printf("Starting certificate request processing for domains: %v", req.CSR.DNSNames)
+	slog.Debug("starting certificate request processing", "domains", req.CSR.DNSNames)
 
 	select {
 	case <-ctx.Done():
@@ -202,18 +204,31 @@ func (c *ExternalCAS) processCertificateRequest(ctx context.Context, req *apiv1.
 	default:
 	}
 
-	cert, err := c.client.Certificate.ObtainForCSR(certificate.ObtainForCSRRequest{
-		CSR:      req.CSR,
-		Bundle:   true,
-		NotAfter: time.Now().Add(1 * 24 * time.Hour),
-	})
+	var cfg AcmeProxyConfig
+	if err := json.Unmarshal(c.config, &cfg); err != nil {
+		return &certificateResult{
+			err: fmt.Errorf("failed to parse acmeproxy config: %v", err),
+		}
+	}
+
+	// Build certificate request - only set NotAfter if CertLifetime is configured
+	csrRequest := certificate.ObtainForCSRRequest{
+		CSR:    req.CSR,
+		Bundle: true,
+	}
+	if cfg.CertLifetime > 0 {
+		csrRequest.NotAfter = time.Now().Add(time.Duration(cfg.CertLifetime) * 24 * time.Hour)
+		slog.Debug("using configured certificate lifetime", "days", cfg.CertLifetime)
+	}
+
+	cert, err := c.client.Certificate.ObtainForCSR(csrRequest)
 	if err != nil {
 		return &certificateResult{
 			err: fmt.Errorf("failed to obtain certificate from InCommon: %v", err),
 		}
 	}
 
-	log.Printf("Successfully obtained certificate from InCommon for domains: %v", req.CSR.DNSNames)
+	slog.Info("obtained certificate from external CA", "domains", req.CSR.DNSNames)
 
 	leaf, intermediates, err := c.splitCertificateBundle(cert.Certificate)
 	if err != nil {
@@ -271,6 +286,38 @@ func (c *ExternalCAS) RenewCertificate(req *apiv1.RenewCertificateRequest) (*api
 }
 
 func (c *ExternalCAS) RevokeCertificate(req *apiv1.RevokeCertificateRequest) (*apiv1.RevokeCertificateResponse, error) {
-	c.client.Certificate.Revoke(req.Certificate.Raw)
-	return nil, apiv1.NotImplementedError{}
+	if req == nil || req.Certificate == nil {
+		return nil, errors.New("certificate cannot be nil")
+	}
+
+	if err := c.initClient(); err != nil {
+		return nil, fmt.Errorf("failed to initialize ACME client: %w", err)
+	}
+
+	// Convert DER-encoded certificate to PEM (lego expects PEM)
+	pemBytes := pem.EncodeToMemory(&pem.Block{
+		Type:  "CERTIFICATE",
+		Bytes: req.Certificate.Raw,
+	})
+
+	slog.Info("revoking certificate",
+		"serial", req.Certificate.SerialNumber.String(),
+		"subject", req.Certificate.Subject.CommonName,
+	)
+
+	if err := c.client.Certificate.Revoke(pemBytes); err != nil {
+		slog.Error("failed to revoke certificate",
+			"serial", req.Certificate.SerialNumber.String(),
+			"error", err,
+		)
+		return nil, fmt.Errorf("failed to revoke certificate: %w", err)
+	}
+
+	slog.Info("certificate revoked successfully",
+		"serial", req.Certificate.SerialNumber.String(),
+	)
+
+	return &apiv1.RevokeCertificateResponse{
+		Certificate: req.Certificate,
+	}, nil
 }

externalcas/external_test.go

@@ -142,6 +142,48 @@ func TestRenewCertificate(t *testing.T) {
 }
 
 func TestRevokeCertificate(t *testing.T) {
+	ctx := context.Background()
+	opts := apiv1.Options{
+		Type:   "externalcas",
+		Config: []byte("{}"),
+	}
+
+	extcas, err := New(ctx, opts)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Table-driven tests for validation logic (no network calls)
+	tests := []struct {
+		name    string
+		req     *apiv1.RevokeCertificateRequest
+		wantErr string
+	}{
+		{
+			name:    "nil request returns error",
+			req:     nil,
+			wantErr: "certificate cannot be nil",
+		},
+		{
+			name:    "nil certificate returns error",
+			req:     &apiv1.RevokeCertificateRequest{Certificate: nil},
+			wantErr: "certificate cannot be nil",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			_, err := extcas.RevokeCertificate(tt.req)
+
+			if err == nil {
+				t.Fatalf("expected error containing %q, got nil", tt.wantErr)
+			}
+
+			if !strings.Contains(err.Error(), tt.wantErr) {
+				t.Errorf("expected error containing %q, got %q", tt.wantErr, err.Error())
+			}
+		})
+	}
 }
 
 // Helper function that generates a self-signed test certificate in PEM format.