Lightly refactor nonce service to clean up abstraction

aarongable · aarongable · commit 1d49e4f4e7a0 · 2025-10-17T17:20:42.000-07:00
diff --git a/cmd/nonce-service/main.go b/cmd/nonce-service/main.go
@@ -93,9 +93,8 @@ func main() {
 	tlsConfig, err := c.NonceService.TLS.Load(scope)
 	cmd.FailOnError(err, "tlsConfig config")
 
-	nonceServer := nonce.NewServer(ns)
 	start, err := bgrpc.NewServer(c.NonceService.GRPC, logger).Add(
-		&noncepb.NonceService_ServiceDesc, nonceServer).Build(tlsConfig, scope, clock.New())
+		&noncepb.NonceService_ServiceDesc, ns).Build(tlsConfig, scope, clock.New())
 	cmd.FailOnError(err, "Unable to setup nonce service gRPC server")
 
 	logger.Info(fmt.Sprintf("Nonce server listening on %s with prefix %q", c.NonceService.GRPC.Address, noncePrefix))
diff --git a/nonce/nonce.go b/nonce/nonce.go
@@ -63,6 +63,7 @@ func DerivePrefix(grpcAddr string, key []byte) string {
 
 // NonceService generates, cancels, and tracks Nonces.
 type NonceService struct {
+	noncepb.UnsafeNonceServiceServer
 	mu               sync.Mutex
 	latest           int64
 	earliest         int64
@@ -232,8 +233,8 @@ func (ns *NonceService) decrypt(nonce string) (int64, error) {
 	return ctr.Int64(), nil
 }
 
-// Nonce provides a new Nonce.
-func (ns *NonceService) Nonce() (string, error) {
+// nonce provides a new Nonce.
+func (ns *NonceService) nonce() (string, error) {
 	ns.mu.Lock()
 	ns.latest++
 	latest := ns.latest
@@ -242,30 +243,30 @@ func (ns *NonceService) Nonce() (string, error) {
 	return ns.encrypt(latest)
 }
 
-// Valid determines whether the provided Nonce string is valid, returning
+// valid determines whether the provided Nonce string is valid, returning
 // true if so.
-func (ns *NonceService) Valid(nonce string) bool {
+func (ns *NonceService) valid(nonce string) error {
 	c, err := ns.decrypt(nonce)
 	if err != nil {
 		ns.nonceRedeems.WithLabelValues("invalid", "decrypt").Inc()
-		return false
+		return fmt.Errorf("unable to decrypt nonce: %s", err)
 	}
 
 	ns.mu.Lock()
 	defer ns.mu.Unlock()
 	if c > ns.latest {
 		ns.nonceRedeems.WithLabelValues("invalid", "too high").Inc()
-		return false
+		return fmt.Errorf("nonce greater than highest dispensed nonce: %d > %d", c, ns.latest)
 	}
 
 	if c <= ns.earliest {
 		ns.nonceRedeems.WithLabelValues("invalid", "too low").Inc()
-		return false
+		return fmt.Errorf("nonce less than lowest eligible nonce: %d < %d", c, ns.earliest)
 	}
 
 	if ns.used[c] {
 		ns.nonceRedeems.WithLabelValues("invalid", "already used").Inc()
-		return false
+		return fmt.Errorf("nonce already marked as used: %d ∈ used{%d}", c, len(ns.used))
 	}
 
 	ns.used[c] = true
@@ -279,7 +280,7 @@ func (ns *NonceService) Valid(nonce string) bool {
 	}
 
 	ns.nonceRedeems.WithLabelValues("valid", "").Inc()
-	return true
+	return nil
 }
 
 // splitNonce splits a nonce into a prefix and a body.
@@ -290,27 +291,18 @@ func (ns *NonceService) splitNonce(nonce string) (string, string, error) {
 	return nonce[:PrefixLen], nonce[PrefixLen:], nil
 }
 
-// NewServer returns a new Server, wrapping a NonceService.
-func NewServer(inner *NonceService) *Server {
-	return &Server{inner: inner}
-}
-
-// Server implements the gRPC nonce service.
-type Server struct {
-	noncepb.UnsafeNonceServiceServer
-	inner *NonceService
-}
-
-var _ noncepb.NonceServiceServer = (*Server)(nil)
-
 // Redeem accepts a nonce from a gRPC client and redeems it using the inner nonce service.
-func (ns *Server) Redeem(ctx context.Context, msg *noncepb.NonceMessage) (*noncepb.ValidMessage, error) {
-	return &noncepb.ValidMessage{Valid: ns.inner.Valid(msg.Nonce)}, nil
+func (ns *NonceService) Redeem(ctx context.Context, msg *noncepb.NonceMessage) (*noncepb.ValidMessage, error) {
+	err := ns.valid(msg.Nonce)
+	if err != nil {
+		return nil, err
+	}
+	return &noncepb.ValidMessage{Valid: true}, nil
 }
 
 // Nonce generates a nonce and sends it to a gRPC client.
-func (ns *Server) Nonce(_ context.Context, _ *emptypb.Empty) (*noncepb.NonceMessage, error) {
-	nonce, err := ns.inner.Nonce()
+func (ns *NonceService) Nonce(_ context.Context, _ *emptypb.Empty) (*noncepb.NonceMessage, error) {
+	nonce, err := ns.nonce()
 	if err != nil {
 		return nil, err
 	}
diff --git a/nonce/nonce_test.go b/nonce/nonce_test.go
@@ -11,32 +11,32 @@ import (
 func TestValidNonce(t *testing.T) {
 	ns, err := NewNonceService(metrics.NoopRegisterer, 0, "")
 	test.AssertNotError(t, err, "Could not create nonce service")
-	n, err := ns.Nonce()
+	n, err := ns.nonce()
 	test.AssertNotError(t, err, "Could not create nonce")
-	test.Assert(t, ns.Valid(n), fmt.Sprintf("Did not recognize fresh nonce %s", n))
+	test.AssertNotError(t, ns.valid(n), fmt.Sprintf("Did not recognize fresh nonce %s", n))
 }
 
 func TestAlreadyUsed(t *testing.T) {
 	ns, err := NewNonceService(metrics.NoopRegisterer, 0, "")
 	test.AssertNotError(t, err, "Could not create nonce service")
-	n, err := ns.Nonce()
+	n, err := ns.nonce()
 	test.AssertNotError(t, err, "Could not create nonce")
-	test.Assert(t, ns.Valid(n), "Did not recognize fresh nonce")
-	test.Assert(t, !ns.Valid(n), "Recognized the same nonce twice")
+	test.AssertNotError(t, ns.valid(n), "Did not recognize fresh nonce")
+	test.AssertError(t, ns.valid(n), "Recognized the same nonce twice")
 }
 
 func TestRejectMalformed(t *testing.T) {
 	ns, err := NewNonceService(metrics.NoopRegisterer, 0, "")
 	test.AssertNotError(t, err, "Could not create nonce service")
-	n, err := ns.Nonce()
+	n, err := ns.nonce()
 	test.AssertNotError(t, err, "Could not create nonce")
-	test.Assert(t, !ns.Valid("asdf"+n), "Accepted an invalid nonce")
+	test.AssertError(t, ns.valid("asdf"+n), "Accepted an invalid nonce")
 }
 
 func TestRejectShort(t *testing.T) {
 	ns, err := NewNonceService(metrics.NoopRegisterer, 0, "")
 	test.AssertNotError(t, err, "Could not create nonce service")
-	test.Assert(t, !ns.Valid("aGkK"), "Accepted an invalid nonce")
+	test.AssertError(t, ns.valid("aGkK"), "Accepted an invalid nonce")
 }
 
 func TestRejectUnknown(t *testing.T) {
@@ -45,48 +45,48 @@ func TestRejectUnknown(t *testing.T) {
 	ns2, err := NewNonceService(metrics.NoopRegisterer, 0, "")
 	test.AssertNotError(t, err, "Could not create nonce service")
 
-	n, err := ns1.Nonce()
+	n, err := ns1.nonce()
 	test.AssertNotError(t, err, "Could not create nonce")
-	test.Assert(t, !ns2.Valid(n), "Accepted a foreign nonce")
+	test.AssertError(t, ns2.valid(n), "Accepted a foreign nonce")
 }
 
 func TestRejectTooLate(t *testing.T) {
 	ns, err := NewNonceService(metrics.NoopRegisterer, 0, "")
 	test.AssertNotError(t, err, "Could not create nonce service")
 
 	ns.latest = 2
-	n, err := ns.Nonce()
+	n, err := ns.nonce()
 	test.AssertNotError(t, err, "Could not create nonce")
 	ns.latest = 1
-	test.Assert(t, !ns.Valid(n), "Accepted a nonce with a too-high counter")
+	test.AssertError(t, ns.valid(n), "Accepted a nonce with a too-high counter")
 }
 
 func TestRejectTooEarly(t *testing.T) {
 	ns, err := NewNonceService(metrics.NoopRegisterer, 0, "")
 	test.AssertNotError(t, err, "Could not create nonce service")
 
-	n0, err := ns.Nonce()
+	n0, err := ns.nonce()
 	test.AssertNotError(t, err, "Could not create nonce")
 
 	for range ns.maxUsed {
-		n, err := ns.Nonce()
+		n, err := ns.nonce()
 		test.AssertNotError(t, err, "Could not create nonce")
-		if !ns.Valid(n) {
+		if ns.valid(n) != nil {
 			t.Errorf("generated invalid nonce")
 		}
 	}
 
-	n1, err := ns.Nonce()
+	n1, err := ns.nonce()
 	test.AssertNotError(t, err, "Could not create nonce")
-	n2, err := ns.Nonce()
+	n2, err := ns.nonce()
 	test.AssertNotError(t, err, "Could not create nonce")
-	n3, err := ns.Nonce()
+	n3, err := ns.nonce()
 	test.AssertNotError(t, err, "Could not create nonce")
 
-	test.Assert(t, ns.Valid(n3), "Rejected a valid nonce")
-	test.Assert(t, ns.Valid(n2), "Rejected a valid nonce")
-	test.Assert(t, ns.Valid(n1), "Rejected a valid nonce")
-	test.Assert(t, !ns.Valid(n0), "Accepted a nonce that we should have forgotten")
+	test.AssertNotError(t, ns.valid(n3), "Rejected a valid nonce")
+	test.AssertNotError(t, ns.valid(n2), "Rejected a valid nonce")
+	test.AssertNotError(t, ns.valid(n1), "Rejected a valid nonce")
+	test.AssertError(t, ns.valid(n0), "Accepted a nonce that we should have forgotten")
 }
 
 func BenchmarkNonces(b *testing.B) {
@@ -96,23 +96,23 @@ func BenchmarkNonces(b *testing.B) {
 	}
 
 	for range ns.maxUsed {
-		n, err := ns.Nonce()
+		n, err := ns.nonce()
 		if err != nil {
 			b.Fatal("noncing", err)
 		}
-		if !ns.Valid(n) {
+		if ns.valid(n) != nil {
 			b.Fatal("generated invalid nonce")
 		}
 	}
 
 	b.ResetTimer()
 	b.RunParallel(func(pb *testing.PB) {
 		for pb.Next() {
-			n, err := ns.Nonce()
+			n, err := ns.nonce()
 			if err != nil {
 				b.Fatal("noncing", err)
 			}
-			if !ns.Valid(n) {
+			if ns.valid(n) != nil {
 				b.Fatal("generated invalid nonce")
 			}
 		}
@@ -123,18 +123,18 @@ func TestNoncePrefixing(t *testing.T) {
 	ns, err := NewNonceService(metrics.NoopRegisterer, 0, "aluminum")
 	test.AssertNotError(t, err, "Could not create nonce service")
 
-	n, err := ns.Nonce()
+	n, err := ns.nonce()
 	test.AssertNotError(t, err, "Could not create nonce")
-	test.Assert(t, ns.Valid(n), "Valid nonce rejected")
+	test.AssertNotError(t, ns.valid(n), "Valid nonce rejected")
 
-	n, err = ns.Nonce()
+	n, err = ns.nonce()
 	test.AssertNotError(t, err, "Could not create nonce")
 	n = n[1:]
-	test.Assert(t, !ns.Valid(n), "Valid nonce with incorrect prefix accepted")
+	test.AssertError(t, ns.valid(n), "Valid nonce with incorrect prefix accepted")
 
-	n, err = ns.Nonce()
+	n, err = ns.nonce()
 	test.AssertNotError(t, err, "Could not create nonce")
-	test.Assert(t, !ns.Valid(n[6:]), "Valid nonce without prefix accepted")
+	test.AssertError(t, ns.valid(n[6:]), "Valid nonce without prefix accepted")
 }
 
 func TestNoncePrefixValidation(t *testing.T) {
diff --git a/test/inmem/nonce/nonce.go b/test/inmem/nonce/nonce.go
@@ -11,33 +11,27 @@ import (
 	noncepb "github.com/letsencrypt/boulder/nonce/proto"
 )
 
-// Service implements noncepb.NonceServiceClient for tests.
-type Service struct {
-	*nonce.NonceService
+// NonceService implements noncepb.NonceServiceClient for tests.
+type NonceService struct {
+	noncepb.NonceServiceClient
+	Impl *nonce.NonceService
 }
 
-var _ noncepb.NonceServiceClient = &Service{}
+var _ noncepb.NonceServiceClient = &NonceService{}
 
-// Nonce implements proto.NonceServiceClient
-func (imns *Service) Nonce(ctx context.Context, in *emptypb.Empty, opts ...grpc.CallOption) (*noncepb.NonceMessage, error) {
-	n, err := imns.NonceService.Nonce()
-	if err != nil {
-		return nil, err
-	}
-	return &noncepb.NonceMessage{Nonce: n}, nil
+func (ns *NonceService) Nonce(ctx context.Context, req *emptypb.Empty, opts ...grpc.CallOption) (*noncepb.NonceMessage, error) {
+	return ns.Impl.Nonce(ctx, req)
 }
 
-// Redeem implements proto.NonceServiceClient
-func (imns *Service) Redeem(ctx context.Context, in *noncepb.NonceMessage, opts ...grpc.CallOption) (*noncepb.ValidMessage, error) {
-	valid := imns.NonceService.Valid(in.Nonce)
-	return &noncepb.ValidMessage{Valid: valid}, nil
+func (ns *NonceService) Redeem(ctx context.Context, req *noncepb.NonceMessage, opts ...grpc.CallOption) (*noncepb.ValidMessage, error) {
+	return ns.Impl.Redeem(ctx, req)
 }
 
 // AsSource returns a wrapper type that implements jose.NonceSource using this
 // inmemory service. This is useful so that tests can get nonces for signing
 // their JWS that will be accepted by the test WFE configured using this service.
-func (imns *Service) AsSource() jose.NonceSource {
-	return nonceServiceAdapter{imns}
+func (ns *NonceService) AsSource() jose.NonceSource {
+	return nonceServiceAdapter{ns}
 }
 
 // nonceServiceAdapter changes the gRPC nonce service interface to the one
diff --git a/wfe2/wfe_test.go b/wfe2/wfe_test.go
@@ -404,7 +404,7 @@ func setupWFE(t *testing.T) (WebFrontEndImpl, clock.FakeClock, requestSigner) {
 	nonceService, err := nonce.NewNonceService(metrics.NoopRegisterer, 100, noncePrefix)
 	test.AssertNotError(t, err, "making nonceService")
 
-	inmemNonceService := &inmemnonce.Service{NonceService: nonceService}
+	inmemNonceService := &inmemnonce.NonceService{Impl: nonceService}
 	gnc := inmemNonceService
 	rnc := inmemNonceService
 
