sa: truncate all timestamps to seconds

cmd/bad-key-revoker/main.go

@@ -141,7 +141,7 @@ func (bkr *badKeyRevoker) findUnrevoked(ctx context.Context, unchecked unchecked
 			"SELECT id, certSerial FROM keyHashToSerial WHERE keyHash = ? AND id > ? AND certNotAfter > ? ORDER BY id LIMIT ?",
 			unchecked.KeyHash,
 			initialID,
-			bkr.clk.Now(),
+			bkr.clk.Now().Truncate(time.Second),
 			bkr.serialBatchSize,
 		)
 		if err != nil {

docs/CONTRIBUTING.md

@@ -324,6 +324,13 @@ value. The `core.IsAnyNilOrZero` function can check these cases.
 Senders must check that timestamps are non-zero before sending them. Receivers
 must check that timestamps are non-zero before accepting them.
 
+# Rounding time in DB
+
+All times that we write to the database are truncated to one second's worth of
+precision. This reduces the size of indexes that include timestamps, and makes
+querying them more efficient. The Storage Authority (SA) is responsible for this
+truncation, and performs it for SELECT queries as well as INSERT and UPDATE.
+
 # Release Process
 
 The current Boulder release process is described in

sa/sa.go

@@ -108,7 +108,7 @@ func (ssa *SQLStorageAuthority) NewRegistration(ctx context.Context, req *corepb
 		return nil, err
 	}
 
-	reg.CreatedAt = ssa.clk.Now()
+	reg.CreatedAt = ssa.clk.Now().Truncate(time.Second)
 
 	err = ssa.dbMap.Insert(ctx, reg)
 	if err != nil {
@@ -141,6 +141,10 @@ func (ssa *SQLStorageAuthority) UpdateRegistration(ctx context.Context, req *cor
 		return nil, err
 	}
 
+	// The CreatedAt field shouldn't change from the original, so we copy it straight through.
+	// This also ensures that it's already truncated to second (which happened on creation).
+	update.CreatedAt = curr.CreatedAt
+
 	// Copy the existing registration model's LockCol to the new updated
 	// registration model's LockCol
 	update.LockCol = curr.LockCol
@@ -169,8 +173,8 @@ func (ssa *SQLStorageAuthority) AddSerial(ctx context.Context, req *sapb.AddSeri
 	err := ssa.dbMap.Insert(ctx, &recordedSerialModel{
 		Serial:         req.Serial,
 		RegistrationID: req.RegID,
-		Created:        req.Created.AsTime(),
-		Expires:        req.Expires.AsTime(),
+		Created:        req.Created.AsTime().Truncate(time.Second),
+		Expires:        req.Expires.AsTime().Truncate(time.Second),
 	})
 	if err != nil {
 		return nil, err
@@ -223,7 +227,7 @@ func (ssa *SQLStorageAuthority) AddPrecertificate(ctx context.Context, req *sapb
 		Serial:         serialHex,
 		RegistrationID: req.RegID,
 		DER:            req.Der,
-		Issued:         req.Issued.AsTime(),
+		Issued:         req.Issued.AsTime().Truncate(time.Second),
 		Expires:        parsed.NotAfter,
 	}
 
@@ -252,13 +256,15 @@ func (ssa *SQLStorageAuthority) AddPrecertificate(ctx context.Context, req *sapb
 		cs := &core.CertificateStatus{
 			Serial:                serialHex,
 			Status:                status,
-			OCSPLastUpdated:       ssa.clk.Now(),
+			OCSPLastUpdated:       ssa.clk.Now().Truncate(time.Second),
 			RevokedDate:           time.Time{},
 			RevokedReason:         0,
 			LastExpirationNagSent: time.Time{},
-			NotAfter:              parsed.NotAfter,
-			IsExpired:             false,
-			IssuerNameID:          req.IssuerNameID,
+			// No need to truncate because it's already truncated to encode
+			// per https://datatracker.ietf.org/doc/html/rfc5280#section-4.1.2.5.1
+			NotAfter:     parsed.NotAfter,
+			IsExpired:    false,
+			IssuerNameID: req.IssuerNameID,
 		}
 		err = ssa.dbMap.Insert(ctx, cs)
 		if err != nil {
@@ -317,7 +323,7 @@ func (ssa *SQLStorageAuthority) AddCertificate(ctx context.Context, req *sapb.Ad
 		Serial:         serial,
 		Digest:         digest,
 		DER:            req.Der,
-		Issued:         req.Issued.AsTime(),
+		Issued:         req.Issued.AsTime().Truncate(time.Second),
 		Expires:        parsedCertificate.NotAfter,
 	}
 
@@ -476,13 +482,15 @@ func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb
 				if err != nil {
 					return nil, err
 				}
+				// These parameters correspond to the fields listed in `authzFields`, as used in the
+				// `db.NewMultiInserter` call above, and occur in the same order.
 				err = inserter.Add([]interface{}{
 					am.ID,
 					am.IdentifierType,
 					am.IdentifierValue,
 					am.RegistrationID,
 					statusToUint[core.StatusPending],
-					am.Expires,
+					am.Expires.Truncate(time.Second),
 					am.Challenges,
 					nil,
 					nil,
@@ -503,11 +511,12 @@ func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb
 		// Second, insert the new order.
 		var orderID int64
 		var err error
-		created := ssa.clk.Now()
+		created := ssa.clk.Now().Truncate(time.Second)
+		expires := req.NewOrder.Expires.AsTime().Truncate(time.Second)
 		if features.Get().MultipleCertificateProfiles {
 			omv2 := orderModelv2{
 				RegistrationID:         req.NewOrder.RegistrationID,
-				Expires:                req.NewOrder.Expires.AsTime(),
+				Expires:                expires,
 				Created:                created,
 				CertificateProfileName: req.NewOrder.CertificateProfileName,
 			}
@@ -516,7 +525,7 @@ func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb
 		} else {
 			omv1 := orderModelv1{
 				RegistrationID: req.NewOrder.RegistrationID,
-				Expires:        req.NewOrder.Expires.AsTime(),
+				Expires:        expires,
 				Created:        created,
 			}
 			err = tx.Insert(ctx, &omv1)
@@ -549,19 +558,25 @@ func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb
 		}
 
 		// Fourth, insert the FQDNSet entry for the order.
-		err = addOrderFQDNSet(ctx, tx, req.NewOrder.Names, orderID, req.NewOrder.RegistrationID, req.NewOrder.Expires.AsTime())
+		err = addOrderFQDNSet(ctx,
+			tx,
+			req.NewOrder.Names,
+			orderID,
+			req.NewOrder.RegistrationID,
+			expires,
+		)
 		if err != nil {
 			return nil, err
 		}
 
-		// Finally, build the overall Order PB.
+		// Finally, build the overall Order PB to return.
 		res := &corepb.Order{
 			// ID and Created were auto-populated on the order model when it was inserted.
 			Id:      orderID,
 			Created: timestamppb.New(created),
 			// These are carried over from the original request unchanged.
 			RegistrationID: req.NewOrder.RegistrationID,
-			Expires:        req.NewOrder.Expires,
+			Expires:        timestamppb.New(expires),
 			Names:          req.NewOrder.Names,
 			// Have to combine the already-associated and newly-reacted authzs.
 			V2Authorizations: append(req.NewOrder.V2Authorizations, newAuthzIDs...),
@@ -576,7 +591,12 @@ func (ssa *SQLStorageAuthority) NewOrderAndAuthzs(ctx context.Context, req *sapb
 		if req.NewOrder.ReplacesSerial != "" {
 			// Update the replacementOrders table to indicate that this order
 			// replaces the provided certificate serial.
-			err := addReplacementOrder(ctx, tx, req.NewOrder.ReplacesSerial, orderID, req.NewOrder.Expires.AsTime())
+			err := addReplacementOrder(ctx,
+				tx,
+				req.NewOrder.ReplacesSerial,
+				orderID,
+				req.NewOrder.Expires.AsTime().Truncate(time.Second),
+			)
 			if err != nil {
 				return nil, err
 			}
@@ -789,7 +809,7 @@ func (ssa *SQLStorageAuthority) FinalizeAuthorization2(ctx context.Context, req
 	// database attemptedAt field Null instead of 1970-01-01 00:00:00.
 	var attemptedTime *time.Time
 	if !core.IsAnyNilOrZero(req.AttemptedAt) {
-		val := req.AttemptedAt.AsTime()
+		val := req.AttemptedAt.AsTime().Truncate(time.Second)
 		attemptedTime = &val
 	}
 	params := map[string]interface{}{
@@ -799,7 +819,7 @@ func (ssa *SQLStorageAuthority) FinalizeAuthorization2(ctx context.Context, req
 		"validationRecord": vrJSON,
 		"id":               req.Id,
 		"pending":          statusUint(core.StatusPending),
-		"expires":          req.Expires.AsTime(),
+		"expires":          req.Expires.AsTime().Truncate(time.Second),
 		// if req.ValidationError is nil veJSON should also be nil
 		// which should result in a NULL field
 		"validationError": veJSON,
@@ -867,7 +887,7 @@ func (ssa *SQLStorageAuthority) RevokeCertificate(ctx context.Context, req *sapb
 	}
 
 	_, overallError := db.WithTransaction(ctx, ssa.dbMap, func(tx db.Executor) (interface{}, error) {
-		revokedDate := req.Date.AsTime()
+		revokedDate := req.Date.AsTime().Truncate(time.Second)
 
 		res, err := tx.ExecContext(ctx,
 			`UPDATE certificateStatus SET
@@ -924,8 +944,8 @@ func (ssa *SQLStorageAuthority) UpdateRevokedCertificate(ctx context.Context, re
 	}
 
 	_, overallError := db.WithTransaction(ctx, ssa.dbMap, func(tx db.Executor) (interface{}, error) {
-		thisUpdate := req.Date.AsTime()
-		revokedDate := req.Backdate.AsTime()
+		thisUpdate := req.Date.AsTime().Truncate(time.Second)
+		revokedDate := req.Backdate.AsTime().Truncate(time.Second)
 
 		res, err := tx.ExecContext(ctx,
 			`UPDATE certificateStatus SET
@@ -1007,7 +1027,7 @@ func (ssa *SQLStorageAuthority) AddBlockedKey(ctx context.Context, req *sapb.Add
 	cols, qs := blockedKeysColumns, "?, ?, ?, ?"
 	vals := []interface{}{
 		req.KeyHash,
-		req.Added.AsTime(),
+		req.Added.AsTime().Truncate(time.Second),
 		sourceInt,
 		req.Comment,
 	}
@@ -1232,7 +1252,10 @@ func (ssa *SQLStorageAuthority) leaseSpecificCRLShard(ctx context.Context, req *
 
 // UpdateCRLShard updates the thisUpdate and nextUpdate timestamps of a CRL
 // shard. It rejects the update if it would cause the thisUpdate timestamp to
-// move backwards. It does *not* reject the update if the shard is no longer
+// move backwards, but if thisUpdate would stay the same (for instance, multiple
+// CRL generations within a single second), it will succeed.
+//
+// It does *not* reject the update if the shard is no longer
 // leased: although this would be unexpected (because the lease timestamp should
 // be the same as the crl-updater's context expiration), it's not inherently a
 // sign of an update that should be skipped. It does reject the update if the
@@ -1247,24 +1270,25 @@ func (ssa *SQLStorageAuthority) UpdateCRLShard(ctx context.Context, req *sapb.Up
 	// Only set the nextUpdate if it's actually present in the request message.
 	var nextUpdate *time.Time
 	if req.NextUpdate != nil {
-		nut := req.NextUpdate.AsTime()
+		nut := req.NextUpdate.AsTime().Truncate(time.Second)
 		nextUpdate = &nut
 	}
 
 	_, err := db.WithTransaction(ctx, ssa.dbMap, func(tx db.Executor) (interface{}, error) {
+		thisUpdate := req.ThisUpdate.AsTime().Truncate(time.Second)
 		res, err := tx.ExecContext(ctx,
 			`UPDATE crlShards
 				SET thisUpdate = ?, nextUpdate = ?, leasedUntil = ?
 				WHERE issuerID = ?
 				AND idx = ?
-				AND (thisUpdate is NULL OR thisUpdate < ?)
+				AND (thisUpdate is NULL OR thisUpdate <= ?)
 				LIMIT 1`,
-			req.ThisUpdate.AsTime(),
+			thisUpdate,
 			nextUpdate,
-			req.ThisUpdate.AsTime(),
+			thisUpdate,
 			req.IssuerNameID,
 			req.ShardIdx,
-			req.ThisUpdate.AsTime(),
+			thisUpdate,
 		)
 		if err != nil {
 			return nil, err
@@ -1275,7 +1299,7 @@ func (ssa *SQLStorageAuthority) UpdateCRLShard(ctx context.Context, req *sapb.Up
 			return nil, err
 		}
 		if rowsAffected == 0 {
-			return nil, fmt.Errorf("unable to update shard %d for issuer %d", req.ShardIdx, req.IssuerNameID)
+			return nil, fmt.Errorf("unable to update shard %d for issuer %d; possibly because shard exists", req.ShardIdx, req.IssuerNameID)
 		}
 		if rowsAffected != 1 {
 			return nil, errors.New("update affected unexpected number of rows")

sa/sa_test.go

@@ -4012,7 +4012,7 @@ func TestUpdateCRLShard(t *testing.T) {
 		`SELECT thisUpdate FROM crlShards WHERE issuerID = 1 AND idx = 0 LIMIT 1`,
 	)
 	test.AssertNotError(t, err, "getting updated thisUpdate timestamp")
-	test.Assert(t, crlModel.ThisUpdate.Equal(thisUpdate), "checking updated thisUpdate timestamp")
+	test.AssertEquals(t, *crlModel.ThisUpdate, thisUpdate)
 
 	// Updating an unleased shard should work.
 	_, err = sa.UpdateCRLShard(