ociregistry: improve errors with respect to HTTP status

Currently the standard wire representation of an error is duplicated
across both client and server, and there is no way for:
- a client to access the actual HTTP status of a response
- an `Interface` implementation to cause the `ociserver` to return
  a specific HTTP error code for a error code that it doesn't know
  about.

This change addresses that by moving the wire representation into the
top level `ociregistry` package, splitting `HTTPError` into its own
type, and making both `ociclient` and `ociserver` aware of them.

One deliberate decision made here guards against some scenarios when
nesting an `ociclient` implementation inside `ociserver`. There is a
risk that, due to the fact we're using the same `HTTPError` in
`httpclient` and `httpserve, if `ociclient` talks to a misbehaving
server that returns inappropriate HTTP response codes, those codes could
propagate back through `ociserver`, causing that to return inappropriate
codes too.

So in `ociserver` we only use the `HTTPError` code if there is no known
HTTP status for the error code. This seems like a reasonable
half-way-house compromise.

Fixes #26.

Signed-off-by: Roger Peppe <[email protected]>
Change-Id: I5a79c1c6fec9f22c1f565830d73486b406dd181d

ociregistry/error.go

@@ -14,6 +14,104 @@
 
 package ociregistry
 
+import (
+	"bytes"
+	"encoding/json"
+	"errors"
+	"fmt"
+	"net/http"
+	"strconv"
+	"strings"
+	"unicode"
+)
+
+// WireErrors is the JSON format used for error responses in
+// the OCI HTTP API. It should always contain at least one
+// error.
+type WireErrors struct {
+	Errors []WireError `json:"errors"`
+}
+
+// Unwrap allows [errors.Is] and [errors.As] to
+// see the errors inside e.
+func (e *WireErrors) Unwrap() []error {
+	// TODO we could do this only once.
+	errs := make([]error, len(e.Errors))
+	for i := range e.Errors {
+		errs[i] = &e.Errors[i]
+	}
+	return errs
+}
+
+func (e *WireErrors) Error() string {
+	var buf strings.Builder
+	buf.WriteString(e.Errors[0].Error())
+	for i := range e.Errors[1:] {
+		buf.WriteString("; ")
+		buf.WriteString(e.Errors[i+1].Error())
+	}
+	return buf.String()
+}
+
+// WireError holds a single error in an OCI HTTP response.
+type WireError struct {
+	Code_   string `json:"code"`
+	Message string `json:"message,omitempty"`
+	// Detail_ holds the JSON detail for the message.
+	// It's assumed to be valid JSON if non-empty.
+	Detail_ json.RawMessage `json:"detail,omitempty"`
+}
+
+// Is makes it possible for users to write `if errors.Is(err, ociregistry.ErrBlobUnknown)`
+// even when the error hasn't exactly wrapped that error.
+func (e *WireError) Is(err error) bool {
+	var rerr Error
+	return errors.As(err, &rerr) && rerr.Code() == e.Code()
+}
+
+// Error implements the [error] interface.
+func (e *WireError) Error() string {
+	var buf strings.Builder
+	for _, r := range e.Code_ {
+		if r == '_' {
+			buf.WriteByte(' ')
+		} else {
+			buf.WriteRune(unicode.ToLower(r))
+		}
+	}
+	if buf.Len() == 0 {
+		buf.WriteString("(no code)")
+	}
+	if e.Message != "" {
+		buf.WriteString(": ")
+		buf.WriteString(e.Message)
+	}
+	if len(e.Detail_) != 0 && !bytes.Equal(e.Detail_, []byte("null")) {
+		buf.WriteString("; detail: ")
+		buf.Write(e.Detail_)
+	}
+	return buf.String()
+}
+
+// Code implements [Error.Code].
+func (e *WireError) Code() string {
+	return e.Code_
+}
+
+// Detail implements [Error.Detail].
+// It panics if e.Detail_ contains invalid JSON.
+func (e *WireError) Detail() any {
+	if len(e.Detail_) == 0 {
+		return nil
+	}
+	// TODO do this once only?
+	var d any
+	if err := json.Unmarshal(e.Detail_, &d); err != nil {
+		panic(fmt.Errorf("invalid error detail JSON %q: %v", e.Detail_, err))
+	}
+	return d
+}
+
 // NewError returns a new error with the given code, message and detail.
 func NewError(msg string, code string, detail any) Error {
 	return &registryError{
@@ -31,14 +129,110 @@ type Error interface {
 	// error.Error provides the error message.
 	error
 
-	// Code returns the error code. See
+	// Code returns the error code.
 	Code() string
 
 	// Detail returns any detail to be associated with the error; it should
 	// be JSON-marshable.
 	Detail() any
 }
 
+// HTTPError is optionally implemented by an error when
+// the error has originated from an HTTP request
+// or might be returned from one.
+type HTTPError interface {
+	error
+
+	// StatusCode returns the HTTP status code of the response.
+	StatusCode() int
+
+	// Response holds the HTTP response that caused the HTTPError to
+	// be created. It will return nil if the error was not created
+	// as a result of an HTTP response.
+	//
+	// The caller should not read the response body or otherwise
+	// change the response (mutation of errors is a Bad Thing).
+	//
+	// Use the ResponseBody method to obtain the body of the
+	// response if needed.
+	Response() *http.Response
+
+	// ResponseBody returns the contents of the response body. It
+	// will return nil if the error was not created as a result of
+	// an HTTP response.
+	//
+	// The caller should not change or append to the returned data.
+	ResponseBody() []byte
+}
+
+// NewHTTPError returns an error that wraps err to make an [HTTPError]
+// that represents the given status code, response and response body.
+// Both response and body may be nil.
+//
+// A shallow copy is made of the response.
+func NewHTTPError(err error, statusCode int, response *http.Response, body []byte) HTTPError {
+	herr := &httpError{
+		underlying: err,
+		statusCode: statusCode,
+	}
+	if response != nil {
+		herr.response = ref(*response)
+		herr.response.Body = nil
+		herr.body = body
+	}
+	return herr
+}
+
+type httpError struct {
+	underlying error
+	statusCode int
+	response   *http.Response
+	body       []byte
+}
+
+// Unwrap implements the [errors] Unwrap interface.
+func (e *httpError) Unwrap() error {
+	return e.underlying
+}
+
+// Is makes it possible for users to write `if errors.Is(err, ociregistry.ErrRangeInvalid)`
+// even when the error hasn't exactly wrapped that error.
+func (e *httpError) Is(err error) bool {
+	switch e.statusCode {
+	case http.StatusRequestedRangeNotSatisfiable:
+		return err == ErrRangeInvalid
+	}
+	return false
+}
+
+// Error implements [error.Error].
+func (e *httpError) Error() string {
+	var buf strings.Builder
+	buf.WriteString(strconv.Itoa(e.statusCode))
+	buf.WriteString(" ")
+	buf.WriteString(http.StatusText(e.statusCode))
+	if e.underlying != nil {
+		buf.WriteString(": ")
+		buf.WriteString(e.underlying.Error())
+	}
+	return buf.String()
+}
+
+// StatusCode implements [HTTPError.StatusCode].
+func (e *httpError) StatusCode() int {
+	return e.statusCode
+}
+
+// Response implements [HTTPError.Response].
+func (e *httpError) Response() *http.Response {
+	return e.response
+}
+
+// ResponseBody implements [HTTPError.ResponseBody].
+func (e *httpError) ResponseBody() []byte {
+	return e.body
+}
+
 // The following values represent the known error codes.
 var (
 	ErrBlobUnknown         = NewError("blob unknown to registry", "BLOB_UNKNOWN", nil)
@@ -83,3 +277,7 @@ func (e *registryError) Error() string {
 func (e *registryError) Detail() any {
 	return e.detail
 }
+
+func ref[T any](x T) *T {
+	return &x
+}

ociregistry/ociauth/auth.go

@@ -12,6 +12,8 @@ import (
 	"strings"
 	"sync"
 	"time"
+
+	"cuelabs.dev/go/oci/ociregistry"
 )
 
 // TODO decide on a good value for this.
@@ -298,8 +300,8 @@ func (r *registry) acquireAccessToken(ctx context.Context, requiredScope, wantSc
 	scope := requiredScope.Union(wantScope)
 	tok, err := r.acquireToken(ctx, scope)
 	if err != nil {
-		var rerr *responseError
-		if !errors.As(err, &rerr) || rerr.statusCode != http.StatusUnauthorized {
+		var herr ociregistry.HTTPError
+		if !errors.As(err, &herr) || herr.StatusCode() != http.StatusUnauthorized {
 			return "", err
 		}
 		// The documentation says this:
@@ -372,8 +374,8 @@ func (r *registry) acquireToken(ctx context.Context, scope Scope) (*wireToken, e
 		if err == nil {
 			return tok, nil
 		}
-		var rerr *responseError
-		if !errors.As(err, &rerr) || rerr.statusCode != http.StatusNotFound {
+		var herr ociregistry.HTTPError
+		if !errors.As(err, &herr) || herr.StatusCode() != http.StatusNotFound {
 			return tok, err
 		}
 		// The request to the endpoint returned 404 from the POST request,
@@ -449,7 +451,8 @@ func (r *registry) doTokenRequest(req *http.Request) (*wireToken, error) {
 	}
 	defer resp.Body.Close()
 	if resp.StatusCode != http.StatusOK {
-		return nil, errorFromResponse(resp)
+		// TODO include body of response in error message.
+		return nil, ociregistry.NewHTTPError(nil, resp.StatusCode, resp, nil)
 	}
 	data, err := io.ReadAll(resp.Body)
 	if err != nil {
@@ -462,22 +465,6 @@ func (r *registry) doTokenRequest(req *http.Request) (*wireToken, error) {
 	return &tok, nil
 }
 
-type responseError struct {
-	statusCode int
-	msg        string
-}
-
-func errorFromResponse(resp *http.Response) error {
-	// TODO include body of response in error message.
-	return &responseError{
-		statusCode: resp.StatusCode,
-	}
-}
-
-func (e *responseError) Error() string {
-	return fmt.Sprintf("unexpected HTTP response %d", e.statusCode)
-}
-
 // deleteExpiredTokens removes all tokens from r that expire after the given
 // time.
 // TODO ask the store to remove expired tokens?