diff --git a/cmd/nonce-service/main.go b/cmd/nonce-service/main.go index b4de83c6446..25ca4a17266 100644 --- 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/email/exporter_test.go b/email/exporter_test.go index e9beca3961a..03f00735c91 100644 --- a/email/exporter_test.go +++ b/email/exporter_test.go @@ -27,11 +27,10 @@ type mockPardotClientImpl struct { // newMockPardotClientImpl returns a MockPardotClientImpl, implementing the // PardotClient interface. Both refer to the same instance, with the interface // for mock interaction and the struct for state inspection and modification. -func newMockPardotClientImpl() (PardotClient, *mockPardotClientImpl) { - mockImpl := &mockPardotClientImpl{ +func newMockPardotClientImpl() *mockPardotClientImpl { + return &mockPardotClientImpl{ CreatedContacts: []string{}, } - return mockImpl, mockImpl } // SendContact adds an email to CreatedContacts. @@ -55,10 +54,10 @@ func (m *mockPardotClientImpl) getCreatedContacts() []string { // ExporterImpl queue and cleanup() to drain and shutdown. If start() is called, // cleanup() must be called. func setup() (*ExporterImpl, *mockPardotClientImpl, func(), func()) { - mockClient, clientImpl := newMockPardotClientImpl() + mockClient := newMockPardotClientImpl() exporter := NewExporterImpl(mockClient, nil, 1000000, 5, metrics.NoopRegisterer, blog.NewMock()) daemonCtx, cancel := context.WithCancel(context.Background()) - return exporter, clientImpl, + return exporter, mockClient, func() { exporter.Start(daemonCtx) }, func() { cancel() @@ -167,7 +166,7 @@ func TestSendContactDeduplication(t *testing.T) { t.Parallel() cache := NewHashedEmailCache(1000, metrics.NoopRegisterer) - mockClient, clientImpl := newMockPardotClientImpl() + mockClient := newMockPardotClientImpl() exporter := NewExporterImpl(mockClient, cache, 1000000, 5, metrics.NoopRegisterer, blog.NewMock()) daemonCtx, cancel := context.WithCancel(context.Background()) @@ -182,7 +181,7 @@ func TestSendContactDeduplication(t *testing.T) { cancel() exporter.Drain() - contacts := clientImpl.getCreatedContacts() + contacts := mockClient.getCreatedContacts() test.AssertEquals(t, 1, len(contacts)) test.AssertEquals(t, "duplicate@example.com", contacts[0]) diff --git a/nonce/nonce.go b/nonce/nonce.go index c056855c27c..5e2bee53fc0 100644 --- a/nonce/nonce.go +++ b/nonce/nonce.go @@ -32,6 +32,7 @@ import ( "google.golang.org/grpc" "google.golang.org/protobuf/types/known/emptypb" + berrors "github.com/letsencrypt/boulder/errors" noncepb "github.com/letsencrypt/boulder/nonce/proto" ) @@ -63,6 +64,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 @@ -73,7 +75,9 @@ type NonceService struct { prefix string nonceCreates prometheus.Counter nonceEarliest prometheus.Gauge + nonceLatest prometheus.Gauge nonceRedeems *prometheus.CounterVec + nonceAges *prometheus.HistogramVec nonceHeapLatency prometheus.Histogram } @@ -143,11 +147,22 @@ func NewNonceService(stats prometheus.Registerer, maxUsed int, prefix string) (* Help: "A gauge with the current earliest valid nonce value", }) stats.MustRegister(nonceEarliest) + nonceLatest := prometheus.NewGauge(prometheus.GaugeOpts{ + Name: "nonce_latest", + Help: "A gauge with the current latest valid nonce value", + }) + stats.MustRegister(nonceLatest) nonceRedeems := prometheus.NewCounterVec(prometheus.CounterOpts{ Name: "nonce_redeems", Help: "A counter of nonce validations labelled by result", }, []string{"result", "error"}) stats.MustRegister(nonceRedeems) + nonceAges := prometheus.NewHistogramVec(prometheus.HistogramOpts{ + Name: "nonce_ages", + Help: "A histogram of nonce ages at the time they were (attempted to be) redeemed, expressed as fractions of the valid nonce window", + Buckets: []float64{-0.01, 0, .1, .2, .3, .4, .5, .6, .7, .8, .9, 1, 1.1, 1.2, 1.5, 2, 5}, + }, []string{"result"}) + stats.MustRegister(nonceAges) nonceHeapLatency := prometheus.NewHistogram(prometheus.HistogramOpts{ Name: "nonce_heap_latency", Help: "A histogram of latencies of heap pop operations", @@ -164,7 +179,9 @@ func NewNonceService(stats prometheus.Registerer, maxUsed int, prefix string) (* prefix: prefix, nonceCreates: nonceCreates, nonceEarliest: nonceEarliest, + nonceLatest: nonceLatest, nonceRedeems: nonceRedeems, + nonceAges: nonceAges, nonceHeapLatency: nonceHeapLatency, }, nil } @@ -232,40 +249,53 @@ 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 ns.mu.Unlock() - defer ns.nonceCreates.Inc() + ns.nonceCreates.Inc() + ns.nonceLatest.Set(float64(latest)) 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 berrors.BadNonceError("unable to decrypt nonce: %s", err) } ns.mu.Lock() defer ns.mu.Unlock() - if c > ns.latest { + + // age represents how "far back" in the valid nonce window this nonce is. + // If it is very recent, then the numerator is very small and the age is close + // to zero. If it is old but still valid, the numerator is slightly smaller + // than the denominator, and the age is close to one. If it is too old, then + // the age is greater than one. If it is magically too new (i.e. greater than + // the largest nonce we've actually handed out), then the age is negative. + age := float64(ns.latest-c) / float64(ns.latest-ns.earliest) + + if c > ns.latest { // i.e. age < 0 ns.nonceRedeems.WithLabelValues("invalid", "too high").Inc() - return false + ns.nonceAges.WithLabelValues("invalid").Observe(age) + return berrors.BadNonceError("nonce greater than highest dispensed nonce: %d > %d", c, ns.latest) } - if c <= ns.earliest { + if c <= ns.earliest { // i.e. age >= 1 ns.nonceRedeems.WithLabelValues("invalid", "too low").Inc() - return false + ns.nonceAges.WithLabelValues("invalid").Observe(age) + return berrors.BadNonceError("nonce less than lowest eligible nonce: %d < %d", c, ns.earliest) } if ns.used[c] { ns.nonceRedeems.WithLabelValues("invalid", "already used").Inc() - return false + ns.nonceAges.WithLabelValues("invalid").Observe(age) + return berrors.BadNonceError("nonce already marked as used: %d in [%d]used", c, len(ns.used)) } ns.used[c] = true @@ -279,7 +309,8 @@ func (ns *NonceService) Valid(nonce string) bool { } ns.nonceRedeems.WithLabelValues("valid", "").Inc() - return true + ns.nonceAges.WithLabelValues("valid").Observe(age) + return nil } // splitNonce splits a nonce into a prefix and a body. @@ -290,27 +321,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 index 42b43649117..edc2885b695 100644 --- a/nonce/nonce_test.go +++ b/nonce/nonce_test.go @@ -4,6 +4,8 @@ import ( "fmt" "testing" + "github.com/prometheus/client_golang/prometheus" + "github.com/letsencrypt/boulder/metrics" "github.com/letsencrypt/boulder/test" ) @@ -11,32 +13,50 @@ 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)) + test.AssertMetricWithLabelsEquals(t, ns.nonceRedeems, prometheus.Labels{ + "result": "valid", "error": "", + }, 1) + test.AssertHistogramBucketCount(t, ns.nonceAges, prometheus.Labels{ + "result": "valid", + }, 0, 1) } 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") + test.AssertMetricWithLabelsEquals(t, ns.nonceRedeems, prometheus.Labels{ + "result": "invalid", "error": "already used", + }, 1) + test.AssertHistogramBucketCount(t, ns.nonceAges, prometheus.Labels{ + "result": "invalid", + }, 0, 1) } 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") + test.AssertMetricWithLabelsEquals(t, ns.nonceRedeems, prometheus.Labels{ + "result": "invalid", "error": "decrypt", + }, 1) } 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") + test.AssertMetricWithLabelsEquals(t, ns.nonceRedeems, prometheus.Labels{ + "result": "invalid", "error": "decrypt", + }, 1) } func TestRejectUnknown(t *testing.T) { @@ -45,9 +65,12 @@ 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") + test.AssertMetricWithLabelsEquals(t, ns2.nonceRedeems, prometheus.Labels{ + "result": "invalid", "error": "decrypt", + }, 1) } func TestRejectTooLate(t *testing.T) { @@ -55,38 +78,79 @@ func TestRejectTooLate(t *testing.T) { 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") + test.AssertMetricWithLabelsEquals(t, ns.nonceRedeems, prometheus.Labels{ + "result": "invalid", "error": "too high", + }, 1) + test.AssertHistogramBucketCount(t, ns.nonceAges, prometheus.Labels{ + "result": "invalid", + }, -1, 1) } func TestRejectTooEarly(t *testing.T) { - ns, err := NewNonceService(metrics.NoopRegisterer, 0, "") + // Use a very low value for maxUsed so the loop below can be short. + ns, err := NewNonceService(metrics.NoopRegisterer, 2, "") test.AssertNotError(t, err, "Could not create nonce service") - n0, err := ns.Nonce() + n, err := ns.nonce() test.AssertNotError(t, err, "Could not create nonce") - for range ns.maxUsed { - n, err := ns.Nonce() + // Generate and redeem enough nonces to surpass maxUsed, forcing the nonce + // service to move ns.earliest upwards, invalidating n. + for range ns.maxUsed + 1 { + n, err := ns.nonce() test.AssertNotError(t, err, "Could not create nonce") - if !ns.Valid(n) { - t.Errorf("generated invalid nonce") - } + test.AssertNotError(t, ns.valid(n), "Rejected a valid nonce") } - n1, err := ns.Nonce() - test.AssertNotError(t, err, "Could not create nonce") - n2, err := ns.Nonce() - test.AssertNotError(t, err, "Could not create nonce") - n3, err := ns.Nonce() - test.AssertNotError(t, err, "Could not create nonce") + test.AssertError(t, ns.valid(n), "Accepted a nonce that we should have forgotten") + test.AssertMetricWithLabelsEquals(t, ns.nonceRedeems, prometheus.Labels{ + "result": "invalid", "error": "too low", + }, 1) + test.AssertHistogramBucketCount(t, ns.nonceAges, prometheus.Labels{ + "result": "invalid", + }, 1.5, 1) +} + +func TestNonceMetrics(t *testing.T) { + // Use a low value for maxUsed so the loop below can be short. + ns, err := NewNonceService(metrics.NoopRegisterer, 2, "") + test.AssertNotError(t, err, "Could not create nonce service") - 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") + // After issuing (but not redeeming) many nonces, the latest should have + // increased by the same amount and the earliest should have moved at all. + var nonces []string + for range 10 * ns.maxUsed { + n, err := ns.nonce() + test.AssertNotError(t, err, "Could not create nonce") + nonces = append(nonces, n) + } + test.AssertMetricWithLabelsEquals(t, ns.nonceEarliest, nil, 0) + test.AssertMetricWithLabelsEquals(t, ns.nonceLatest, nil, 20) + + // Redeeming maxUsed nonces shouldn't cause either metric to change, because + // no redeemed nonces have been dropped from the used heap yet. + test.AssertNotError(t, ns.valid(nonces[0]), "Rejected a valid nonce") + test.AssertNotError(t, ns.valid(nonces[1]), "Rejected a valid nonce") + test.AssertMetricWithLabelsEquals(t, ns.nonceEarliest, nil, 0) + test.AssertMetricWithLabelsEquals(t, ns.nonceLatest, nil, 20) + + // Redeeming one more nonce should cause the earliest to move forward one, as + // the earliest redeemed nonce is popped from the heap. + test.AssertNotError(t, ns.valid(nonces[2]), "Rejected a valid nonce") + test.AssertMetricWithLabelsEquals(t, ns.nonceEarliest, nil, 1) + test.AssertMetricWithLabelsEquals(t, ns.nonceLatest, nil, 20) + + // Redeeming maxUsed+1 much later nonces should cause the earliest to skip + // forward to the first of those. + test.AssertNotError(t, ns.valid(nonces[17]), "Rejected a valid nonce") + test.AssertNotError(t, ns.valid(nonces[18]), "Rejected a valid nonce") + test.AssertNotError(t, ns.valid(nonces[19]), "Rejected a valid nonce") + test.AssertMetricWithLabelsEquals(t, ns.nonceEarliest, nil, 18) + test.AssertMetricWithLabelsEquals(t, ns.nonceLatest, nil, 20) } func BenchmarkNonces(b *testing.B) { @@ -96,11 +160,11 @@ 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") } } @@ -108,11 +172,11 @@ func BenchmarkNonces(b *testing.B) { 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 +187,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/asserts.go b/test/asserts.go index 97b23822c18..b87490f0dcb 100644 --- a/test/asserts.go +++ b/test/asserts.go @@ -250,3 +250,53 @@ loop: t.Errorf("metric with labels %+v: got %g, want %g", l, total, expected) } } + +// AssertHistogramBucketCount is similar to AssertMetricWithLabelsEquals, in +// that it determines whether the number of samples within a given histogram +// bucket matches the expectation. The bucket to check is indicated by a single +// exemplar value; whichever bucket that value falls into is the bucket whose +// sample count will be compared to the expected value. +func AssertHistogramBucketCount(t *testing.T, c prometheus.Collector, l prometheus.Labels, b float64, expected uint64) { + t.Helper() + ch := make(chan prometheus.Metric) + done := make(chan struct{}) + go func() { + c.Collect(ch) + close(done) + }() + var total uint64 + timeout := time.After(time.Second) +loop: + for { + metric: + select { + case <-timeout: + t.Fatal("timed out collecting metrics") + case <-done: + break loop + case m := <-ch: + var iom io_prometheus_client.Metric + _ = m.Write(&iom) + for _, lp := range iom.Label { + // If any of the labels on this metric have the same name as but + // different value than a label in `l`, skip this metric. + val, ok := l[lp.GetName()] + if ok && lp.GetValue() != val { + break metric + } + } + lowerBucketsCount := uint64(0) + for _, bucket := range iom.Histogram.Bucket { + if b <= bucket.GetUpperBound() { + total += bucket.GetCumulativeCount() - lowerBucketsCount + break + } else { + lowerBucketsCount += bucket.GetCumulativeCount() + } + } + } + } + if total != expected { + t.Errorf("histogram with labels %+v at bucket %g: got %d, want %d", l, b, total, expected) + } +} diff --git a/test/inmem/nonce/nonce.go b/test/inmem/nonce/nonce.go index bdebdae3a01..55d98f5a285 100644 --- 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/web/probs.go b/web/probs.go index 1f1c9c8a90b..236375e2e0e 100644 --- a/web/probs.go +++ b/web/probs.go @@ -57,7 +57,9 @@ func problemDetailsForBoulderError(err *berrors.BoulderError, msg string) *probs case berrors.AccountDoesNotExist: outProb = probs.AccountDoesNotExist(fmt.Sprintf("%s :: %s", msg, err)) case berrors.BadNonce: - outProb = probs.BadNonce(fmt.Sprintf("%s :: %s", msg, err)) + // We stuff extra internal info into bad nonce errors, but none of it is + // really actionable by the end-user, so overwrite the user-visible message. + outProb = probs.BadNonce("JWS has an invalid anti-replay nonce") default: // Internal server error messages may include sensitive data, so we do // not include it. diff --git a/wfe2/verify.go b/wfe2/verify.go index cda6dc41f82..70a655a1050 100644 --- a/wfe2/verify.go +++ b/wfe2/verify.go @@ -235,15 +235,24 @@ func (wfe *WebFrontEndImpl) validNonce(ctx context.Context, header jose.Header) // should retry their request with a fresh nonce. wfe.stats.nonceNoMatchingBackendCount.Inc() wfe.stats.joseErrorCount.With(prometheus.Labels{"type": "JWSNoBackendNonce"}).Inc() - return berrors.BadNonceError("JWS has an invalid anti-replay nonce: %q", header.Nonce) + return berrors.BadNonceError("JWS has a nonce whose prefix matches no nonce service: %q", header.Nonce) + } + + if errors.Is(err, berrors.BadNonce) { + // Getting a berrors.BadNonce means that the nonce service itself had + // something to say about why the nonce was invalid; no need to wrap it. + wfe.stats.joseErrorCount.With(prometheus.Labels{"type": "JWSUnredeemableNonce"}).Inc() + return err } // We don't recognize this error, so just pass it upwards. return fmt.Errorf("failed to redeem nonce: %w", err) } + // TODO: Remove this clause, as we're updating the NonceService to return an + // error rather than Valid=false when redemption fails. if !resp.Valid { - wfe.stats.joseErrorCount.With(prometheus.Labels{"type": "JWSExpiredNonce"}).Inc() + wfe.stats.joseErrorCount.With(prometheus.Labels{"type": "JWSUnredeemableNonce"}).Inc() return berrors.BadNonceError("JWS has an expired anti-replay nonce: %q", header.Nonce) } diff --git a/wfe2/verify_test.go b/wfe2/verify_test.go index 8edef17ae3c..e861a43f014 100644 --- a/wfe2/verify_test.go +++ b/wfe2/verify_test.go @@ -175,9 +175,7 @@ func (rs requestSigner) byKeyID( return parsedJWS, jwk, body } -// missingNonce returns an otherwise well-signed request that is missing its -// nonce. -func (rs requestSigner) missingNonce() *jose.JSONWebSignature { +func (rs requestSigner) makeJWS(ns jose.NonceSource) *jose.JSONWebSignature { privateKey := loadKey(rs.t, []byte(test1KeyPrivatePEM)) jwk := &jose.JSONWebKey{ Key: privateKey, @@ -190,101 +188,7 @@ func (rs requestSigner) missingNonce() *jose.JSONWebSignature { } opts := &jose.SignerOptions{ - ExtraHeaders: map[jose.HeaderKey]any{ - "url": "https://example.com/acme/foo", - }, - } - - signer, err := jose.NewSigner(signerKey, opts) - test.AssertNotError(rs.t, err, "Failed to make signer") - jws, err := signer.Sign([]byte("")) - test.AssertNotError(rs.t, err, "Failed to sign req") - - return jws -} - -// expiredNonce returns an otherwise well-signed request with a nonce that the -// nonce service doesn't remember giving out (i.e. is expired). -func (rs requestSigner) expiredNonce() *jose.JSONWebSignature { - privateKey := loadKey(rs.t, []byte(test1KeyPrivatePEM)) - jwk := &jose.JSONWebKey{ - Key: privateKey, - Algorithm: keyAlgForKey(rs.t, privateKey), - KeyID: "http://localhost/acme/acct/1", - } - signerKey := jose.SigningKey{ - Key: jwk, - Algorithm: jose.RS256, - } - - opts := &jose.SignerOptions{ - NonceSource: badNonceProvider{}, - ExtraHeaders: map[jose.HeaderKey]any{ - "url": "https://example.com/acme/foo", - }, - } - - signer, err := jose.NewSigner(signerKey, opts) - test.AssertNotError(rs.t, err, "Failed to make signer") - jws, err := signer.Sign([]byte("")) - test.AssertNotError(rs.t, err, "Failed to sign req") - - body := jws.FullSerialize() - parsedJWS, err := jose.ParseSigned(body, getSupportedAlgs()) - test.AssertNotError(rs.t, err, "Failed to parse generated JWS") - - return parsedJWS -} - -// malformedNonce returns an otherwise well-signed request with a malformed -// nonce. -func (rs requestSigner) malformedNonce() *jose.JSONWebSignature { - privateKey := loadKey(rs.t, []byte(test1KeyPrivatePEM)) - jwk := &jose.JSONWebKey{ - Key: privateKey, - Algorithm: keyAlgForKey(rs.t, privateKey), - KeyID: "http://localhost/acme/acct/1", - } - signerKey := jose.SigningKey{ - Key: jwk, - Algorithm: jose.RS256, - } - - opts := &jose.SignerOptions{ - NonceSource: badNonceProvider{malformed: true}, - ExtraHeaders: map[jose.HeaderKey]any{ - "url": "https://example.com/acme/foo", - }, - } - - signer, err := jose.NewSigner(signerKey, opts) - test.AssertNotError(rs.t, err, "Failed to make signer") - jws, err := signer.Sign([]byte("")) - test.AssertNotError(rs.t, err, "Failed to sign req") - - body := jws.FullSerialize() - parsedJWS, err := jose.ParseSigned(body, getSupportedAlgs()) - test.AssertNotError(rs.t, err, "Failed to parse generated JWS") - - return parsedJWS -} - -// shortNonce returns an otherwise well-signed request with a nonce shorter than -// the prefix length. -func (rs requestSigner) shortNonce() *jose.JSONWebSignature { - privateKey := loadKey(rs.t, []byte(test1KeyPrivatePEM)) - jwk := &jose.JSONWebKey{ - Key: privateKey, - Algorithm: keyAlgForKey(rs.t, privateKey), - KeyID: "http://localhost/acme/acct/1", - } - signerKey := jose.SigningKey{ - Key: jwk, - Algorithm: jose.RS256, - } - - opts := &jose.SignerOptions{ - NonceSource: badNonceProvider{shortNonce: true}, + NonceSource: ns, ExtraHeaders: map[jose.HeaderKey]any{ "url": "https://example.com/acme/foo", }, @@ -701,31 +605,31 @@ func TestValidNonce(t *testing.T) { }{ { Name: "No nonce in JWS", - JWS: signer.missingNonce(), + JWS: signer.makeJWS(nil), WantErrType: berrors.BadNonce, WantErrDetail: "JWS has no anti-replay nonce", WantStatType: "JWSMissingNonce", }, { Name: "Malformed nonce in JWS", - JWS: signer.malformedNonce(), + JWS: signer.makeJWS(badNonceProvider{malformed: true}), WantErrType: berrors.BadNonce, WantErrDetail: "JWS has a malformed anti-replay nonce: \"im-a-nonce\"", WantStatType: "JWSMalformedNonce", }, { Name: "Canned nonce shorter than prefixLength in JWS", - JWS: signer.shortNonce(), + JWS: signer.makeJWS(badNonceProvider{shortNonce: true}), WantErrType: berrors.BadNonce, WantErrDetail: "JWS has a malformed anti-replay nonce: \"woww\"", WantStatType: "JWSMalformedNonce", }, { - Name: "Expired nonce in JWS", - JWS: signer.expiredNonce(), + Name: "Unrecognized nonce in JWS", + JWS: signer.makeJWS(badNonceProvider{}), WantErrType: berrors.BadNonce, - WantErrDetail: "JWS has an expired anti-replay nonce: \"mlolmlol3ov77I5Ui-cdaY_k8IcjK58FvbG0y_BCRrx5rGQ8rjA\"", - WantStatType: "JWSExpiredNonce", + WantErrDetail: "unable to decrypt nonce", + WantStatType: "JWSUnredeemableNonce", }, // We don't have a test case for "invalid" (i.e. no backend matching the // prefix) because the unit tests don't use the noncebalancer that does @@ -782,7 +686,7 @@ func TestValidNonce_NoMatchingBackendFound(t *testing.T) { err := wfe.validNonce(context.Background(), goodJWS.Signatures[0].Header) test.AssertError(t, err, "Expected error for valid nonce with no backend") test.AssertErrorIs(t, err, berrors.BadNonce) - test.AssertContains(t, err.Error(), "JWS has an invalid anti-replay nonce") + test.AssertContains(t, err.Error(), "JWS has a nonce whose prefix matches no nonce service") test.AssertMetricWithLabelsEquals(t, wfe.stats.nonceNoMatchingBackendCount, prometheus.Labels{}, 1) } @@ -1352,12 +1256,12 @@ func TestValidJWSForKey(t *testing.T) { WantStatType: "JWSAlgorithmCheckFailed", }, { - Name: "JWS with an expired nonce", - JWS: bJSONWebSignature{signer.expiredNonce()}, + Name: "JWS with an unredeemable nonce", + JWS: bJSONWebSignature{signer.makeJWS(badNonceProvider{})}, JWK: goodJWK, WantErrType: berrors.BadNonce, - WantErrDetail: "JWS has an expired anti-replay nonce: \"mlolmlol3ov77I5Ui-cdaY_k8IcjK58FvbG0y_BCRrx5rGQ8rjA\"", - WantStatType: "JWSExpiredNonce", + WantErrDetail: "unable to decrypt nonce", + WantStatType: "JWSUnredeemableNonce", }, { Name: "JWS with broken signature", diff --git a/wfe2/wfe_test.go b/wfe2/wfe_test.go index affa9e85419..c20f1cfc26c 100644 --- 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 @@ -1358,7 +1358,7 @@ func TestBadNonce(t *testing.T) { test.AssertNotError(t, err, "Failed to sign body") wfe.NewAccount(ctx, newRequestEvent(), responseWriter, makePostRequestWithPath("nonce", result.FullSerialize())) - test.AssertUnmarshaledEquals(t, responseWriter.Body.String(), `{"type":"`+probs.ErrorNS+`badNonce","detail":"Unable to validate JWS :: JWS has no anti-replay nonce","status":400}`) + test.AssertUnmarshaledEquals(t, responseWriter.Body.String(), `{"type":"`+probs.ErrorNS+`badNonce","detail":"JWS has an invalid anti-replay nonce","status":400}`) } func TestNewECDSAAccount(t *testing.T) {