Implement GetSerialMetadata on StorageAuthorityRO (#6780)

When external clients make POST requests to our ARI endpoint, they're
getting 404s even when a GET request with the same exact CertID
succeeds. Logs show that this is because the SA is returning "method
GetSerialMetadata not implemented" when the WFE attempts that gRPC
request. This is due to an oversight: the GetSerialMetadata method is
not implemented on the SQLStorageAuthorityRO object, only on the
SQLStorageAuthority object. The unit tests did not catch this bug
because they supply a mock SA, which does implement the method in
question.

Update the receiver and add a wrapper so that GetSerialMetadata is
implemented on both the read-write and read-only SA implementation
types. Add a new kind of test assertion which helps ensure this won't
happen again. Add a TODO for an integration test covering the ARI POST
codepath to prevent a regression.

Fixes #6778

sa/precertificates.go

@@ -35,7 +35,7 @@ func (ssa *SQLStorageAuthority) AddSerial(ctx context.Context, req *sapb.AddSeri
 // GetSerialMetadata returns metadata stored alongside the serial number,
 // such as the RegID whose certificate request created that serial, and when
 // the certificate with that serial will expire.
-func (ssa *SQLStorageAuthority) GetSerialMetadata(ctx context.Context, req *sapb.Serial) (*sapb.SerialMetadata, error) {
+func (ssa *SQLStorageAuthorityRO) GetSerialMetadata(ctx context.Context, req *sapb.Serial) (*sapb.SerialMetadata, error) {
 	if req == nil || req.Serial == "" {
 		return nil, errIncompleteRequest
 	}
@@ -65,6 +65,10 @@ func (ssa *SQLStorageAuthority) GetSerialMetadata(ctx context.Context, req *sapb
 	}, nil
 }
 
+func (ssa *SQLStorageAuthority) GetSerialMetadata(ctx context.Context, req *sapb.Serial) (*sapb.SerialMetadata, error) {
+	return ssa.SQLStorageAuthorityRO.GetSerialMetadata(ctx, req)
+}
+
 // AddPrecertificate writes a record of a precertificate generation to the DB.
 // Note: this is not idempotent: it does not protect against inserting the same
 // certificate multiple times. Calling code needs to first insert the cert's

sa/sa_test.go

@@ -54,6 +54,11 @@ var (
 }`
 )
 
+func TestImplementation(t *testing.T) {
+	test.AssertImplements(t, &SQLStorageAuthority{}, sapb.UnimplementedStorageAuthorityServer{})
+	test.AssertImplements(t, &SQLStorageAuthorityRO{}, sapb.UnimplementedStorageAuthorityReadOnlyServer{})
+}
+
 // initSA constructs a SQLStorageAuthority and a clean up function that should
 // be defer'ed to the end of the test.
 func initSA(t *testing.T) (*SQLStorageAuthority, clock.FakeClock, func()) {

test/asserts.go

@@ -249,3 +249,44 @@ loop:
 	}
 	AssertEquals(t, total, expected)
 }
+
+// AssertImplements guarantees that impl, which must be a pointer to one of our
+// gRPC service implementation types, implements all of the methods expected by
+// unimpl, which must be the auto-generated gRPC type which is embedded by impl.
+// This function incidentally also guarantees that impl does not have any
+// methods with non-pointer receivers.
+func AssertImplements(t *testing.T, impl any, unimpl any) {
+	// This type is auto-generated by grpc-go, and has methods which implement
+	// the auto-generated gRPC interface. These methods all have value receivers,
+	// not pointer receivers, so we're not using a pointer type here.
+	unimplType := reflect.TypeOf(unimpl)
+
+	// This is the type we use to manually implement the same auto-generated gRPC
+	// interface. We implement all of our methods with pointer receivers. This
+	// type is required to embed the auto-generated "unimplemented" type above,
+	// so it inherits all of the methods implemented by that type as well. But
+	// we can use the fact that we use pointer receivers while the auto-generated
+	// type uses value receivers to our advantage.
+	implType := reflect.TypeOf(impl).Elem()
+
+	// Iterate over all of the methods which are provided by a *non-pointer*
+	// receiver of our type. These will be only the methods which we *don't*
+	// manually implement ourselves, because when we implement the method ourself
+	// we use a pointer receiver. So this loop will only iterate over those
+	// methods which "fall through" to the embedded "unimplemented" type. Ideally,
+	// this loop executes zero times. If there are any methods at all on the
+	// non-pointer receiver, something has gone wrong.
+	for i := 0; i < implType.NumMethod(); i++ {
+		method := implType.Method(i)
+		_, ok := unimplType.MethodByName(method.Name)
+		if ok {
+			// If the lookup worked, then we know this is a method which we were
+			// supposed to implement, but didn't. Oops.
+			t.Errorf("%s does not implement method %s", implType.Name(), method.Name)
+		} else {
+			// If the lookup failed, then we have accidentally implemented some other
+			// method with a non-pointer receiver. We probably didn't mean to do that.
+			t.Errorf("%s.%s has non-pointer receiver", implType.Name(), method.Name)
+		}
+	}
+}

test/integration/ari_test.go

@@ -135,4 +135,7 @@ func TestARI(t *testing.T) {
 	resp, err = http.Get(url)
 	test.AssertNotError(t, err, "ARI request should have succeeded")
 	test.AssertEquals(t, resp.StatusCode, http.StatusNotFound)
+
+	// TODO(#6781): Test the ARI POST path as soon as it's supported in the acme
+	// client we use here.
 }