Always return error interface rather than specific types

[jsha]

In this excellent talk, Francesc Campoy recommends to always return the error interface for errors, rather than specific examples. This helps avoid tricky bugs related to the "non-nil interface can contain a boxed nil" pattern. We return *ProblemDetails in a few places, intending to more clearly document that those functions always return a *ProblemDetails, not some other error. However, we should probably change that.

Direct link to most relevant bit: https://youtu.be/ynoY2xz-F8s?t=699

[jsha]

Another possibility: Really we'd like to do a better job of distinguishing ProblemDetails from other errors (like when the RPC layer gives an error to the RA on validation). Instead of using error as a return type more broadly, we could prevent "boxed nil" errors from the other direction: by preventing ProblemDetails from being boxed into an error. We would do this by removing its .Error() method so it no loner satisfies the error interface.

[jsha]

One particular place where this is an issue: In the wrappers for VA.PerformValidation, we box a nil *probs.ProblemDetails into an error:

boulder/grpc/va-wrappers.go

Lines 57 to 72 in 56c18f6

	func (vac ValidationAuthorityGRPCClient) PerformValidation(ctx context.Context, domain string, challenge core.Challenge, authz core.Authorization) ([]core.ValidationRecord, error) {
	req, err := argsToPerformValidationRequest(domain, challenge, authz)
	if err != nil {
	return nil, err
	}
	gRecords, err := vac.gc.PerformValidation(ctx, req)
	if err != nil {
	return nil, err
	}
	records, prob, err := pbToValidationResult(gRecords)
	if err != nil {
	return nil, err
	}
	return records, prob
	}

boulder/grpc/pb-marshalling.go

Line 221 in 56c18f6

func pbToValidationResult(in *vapb.ValidationResult) ([]core.ValidationRecord, *probs.ProblemDetails, error) {

Specifically, the PerformValidation wrapper takes a *probs.ProblemDetails from pbToValidationResult and returns it as an error interface, whether it's nil or not. This means we have to deal with typed nils.