Allow more complex errors from plugins (no interface change) (#3444)

* Allow more complex errors from plugins

This enables more complex types to be registered and returned from plugins.

*  Register common error types

This is a slightly less drastic change, which keeps the HTTPCodedError
as an interface.

* Remove replication error from list

logical/error.go

@@ -5,21 +5,24 @@ type HTTPCodedError interface {
 	Code() int
 }
 
-func CodedError(c int, s string) HTTPCodedError {
-	return &codedError{s, c}
+func CodedError(status int, msg string) HTTPCodedError {
+	return &codedError{
+		Status:  status,
+		Message: msg,
+	}
 }
 
 type codedError struct {
-	s    string
-	code int
+	Status  int
+	Message string
 }
 
 func (e *codedError) Error() string {
-	return e.s
+	return e.Message
 }
 
 func (e *codedError) Code() int {
-	return e.code
+	return e.Status
 }
 
 // Struct to identify user input errors.  This is helpful in responding the
@@ -34,9 +37,9 @@ func (s *StatusBadRequest) Error() string {
 }
 
 // This is a new type declared to not cause potential compatibility problems if
-// the logic around the HTTPCodedError interface changes; in particular for
-// logical request paths it is basically ignored, and changing that behavior
-// might cause unforseen issues.
+// the logic around the CodedError changes; in particular for logical request
+// paths it is basically ignored, and changing that behavior might cause
+// unforseen issues.
 type ReplicationCodedError struct {
 	Msg  string
 	Code int

logical/plugin/backend_client.go

@@ -33,7 +33,7 @@ type HandleRequestArgs struct {
 // HandleRequestReply is the reply for HandleRequest method.
 type HandleRequestReply struct {
 	Response *logical.Response
-	Error    *plugin.BasicError
+	Error    error
 }
 
 // SpecialPathsReply is the reply for SpecialPaths method.
@@ -44,7 +44,7 @@ type SpecialPathsReply struct {
 // SystemReply is the reply for System method.
 type SystemReply struct {
 	SystemView logical.SystemView
-	Error      *plugin.BasicError
+	Error      error
 }
 
 // HandleExistenceCheckArgs is the args for HandleExistenceCheck method.
@@ -57,7 +57,7 @@ type HandleExistenceCheckArgs struct {
 type HandleExistenceCheckReply struct {
 	CheckFound bool
 	Exists     bool
-	Error      *plugin.BasicError
+	Error      error
 }
 
 // SetupArgs is the args for Setup method.
@@ -70,7 +70,7 @@ type SetupArgs struct {
 
 // SetupReply is the reply for Setup method.
 type SetupReply struct {
-	Error *plugin.BasicError
+	Error error
 }
 
 // TypeReply is the reply for the Type method.
@@ -85,7 +85,7 @@ type RegisterLicenseArgs struct {
 
 // RegisterLicenseReply is the reply for the RegisterLicense method.
 type RegisterLicenseReply struct {
-	Error *plugin.BasicError
+	Error error
 }
 
 func (b *backendPluginClient) HandleRequest(req *logical.Request) (*logical.Response, error) {

logical/plugin/backend_server.go

@@ -41,7 +41,7 @@ func (b *backendPluginServer) HandleRequest(args *HandleRequestArgs, reply *Hand
 	resp, err := b.backend.HandleRequest(args.Request)
 	*reply = HandleRequestReply{
 		Response: resp,
-		Error:    plugin.NewBasicError(err),
+		Error:    wrapError(err),
 	}
 
 	return nil
@@ -66,7 +66,7 @@ func (b *backendPluginServer) HandleExistenceCheck(args *HandleExistenceCheckArg
 	*reply = HandleExistenceCheckReply{
 		CheckFound: checkFound,
 		Exists:     exists,
-		Error:      plugin.NewBasicError(err),
+		Error:      wrapError(err),
 	}
 
 	return nil
@@ -108,7 +108,7 @@ func (b *backendPluginServer) Setup(args *SetupArgs, reply *SetupReply) error {
 	storageConn, err := b.broker.Dial(args.StorageID)
 	if err != nil {
 		*reply = SetupReply{
-			Error: plugin.NewBasicError(err),
+			Error: wrapError(err),
 		}
 		return nil
 	}
@@ -121,7 +121,7 @@ func (b *backendPluginServer) Setup(args *SetupArgs, reply *SetupReply) error {
 	loggerConn, err := b.broker.Dial(args.LoggerID)
 	if err != nil {
 		*reply = SetupReply{
-			Error: plugin.NewBasicError(err),
+			Error: wrapError(err),
 		}
 		return nil
 	}
@@ -134,7 +134,7 @@ func (b *backendPluginServer) Setup(args *SetupArgs, reply *SetupReply) error {
 	sysViewConn, err := b.broker.Dial(args.SysViewID)
 	if err != nil {
 		*reply = SetupReply{
-			Error: plugin.NewBasicError(err),
+			Error: wrapError(err),
 		}
 		return nil
 	}
@@ -155,7 +155,7 @@ func (b *backendPluginServer) Setup(args *SetupArgs, reply *SetupReply) error {
 	backend, err := b.factory(config)
 	if err != nil {
 		*reply = SetupReply{
-			Error: plugin.NewBasicError(err),
+			Error: wrapError(err),
 		}
 	}
 	b.backend = backend
@@ -179,7 +179,7 @@ func (b *backendPluginServer) RegisterLicense(args *RegisterLicenseArgs, reply *
 	err := b.backend.RegisterLicense(args.License)
 	if err != nil {
 		*reply = RegisterLicenseReply{
-			Error: plugin.NewBasicError(err),
+			Error: wrapError(err),
 		}
 	}
 

logical/plugin/logger.go

@@ -3,7 +3,6 @@ package plugin
 import (
 	"net/rpc"
 
-	plugin "github.com/hashicorp/go-plugin"
 	log "github.com/mgutz/logxi/v1"
 )
 
@@ -131,7 +130,7 @@ func (l *LoggerServer) Warn(args *LoggerArgs, reply *LoggerReply) error {
 	err := l.logger.Warn(args.Msg, args.Args...)
 	if err != nil {
 		*reply = LoggerReply{
-			Error: plugin.NewBasicError(err),
+			Error: wrapError(err),
 		}
 		return nil
 	}
@@ -142,7 +141,7 @@ func (l *LoggerServer) Error(args *LoggerArgs, reply *LoggerReply) error {
 	err := l.logger.Error(args.Msg, args.Args...)
 	if err != nil {
 		*reply = LoggerReply{
-			Error: plugin.NewBasicError(err),
+			Error: wrapError(err),
 		}
 		return nil
 	}
@@ -201,5 +200,5 @@ type LoggerArgs struct {
 // for a particular RPC call.
 type LoggerReply struct {
 	IsTrue bool
-	Error  *plugin.BasicError
+	Error  error
 }

logical/plugin/plugin.go

@@ -15,8 +15,8 @@ import (
 	log "github.com/mgutz/logxi/v1"
 )
 
-// Register these types since we have to serialize and de-serialize tls.ConnectionState
-// over the wire as part of logical.Request.Connection.
+// init registers basic structs with gob which will be used to transport complex
+// types through the plugin server and client.
 func init() {
 	// Common basic structs
 	gob.Register([]interface{}{})
@@ -24,10 +24,17 @@ func init() {
 	gob.Register(map[string]string{})
 	gob.Register(map[string]int{})
 
-	// tls.ConnectionState structs
+	// Register these types since we have to serialize and de-serialize
+	// tls.ConnectionState over the wire as part of logical.Request.Connection.
 	gob.Register(rsa.PublicKey{})
 	gob.Register(ecdsa.PublicKey{})
 	gob.Register(time.Duration(0))
+
+	// Custom common error types for requests. If you add something here, you must
+	// also add it to the switch statement in `wrapError`!
+	gob.Register(&plugin.BasicError{})
+	gob.Register(logical.CodedError(0, ""))
+	gob.Register(&logical.StatusBadRequest{})
 }
 
 // BackendPluginClient is a wrapper around backendPluginClient
@@ -124,3 +131,22 @@ func newPluginClient(sys pluginutil.RunnerUtil, pluginRunner *pluginutil.PluginR
 		backendPluginClient: backendRPC,
 	}, nil
 }
+
+// wrapError takes a generic error type and makes it usable with the plugin
+// interface. Only errors which have exported fields and have been registered
+// with gob can be unwrapped and transported. This checks error types and, if
+// none match, wrap the error in a plugin.BasicError.
+func wrapError(err error) error {
+	if err == nil {
+		return nil
+	}
+
+	switch err.(type) {
+	case *plugin.BasicError,
+		logical.HTTPCodedError,
+		*logical.StatusBadRequest:
+		return err
+	}
+
+	return plugin.NewBasicError(err)
+}

logical/plugin/storage.go

@@ -3,7 +3,6 @@ package plugin
 import (
 	"net/rpc"
 
-	"github.com/hashicorp/go-plugin"
 	"github.com/hashicorp/vault/logical"
 )
 
@@ -70,7 +69,7 @@ func (s *StorageServer) List(prefix string, reply *StorageListReply) error {
 	keys, err := s.impl.List(prefix)
 	*reply = StorageListReply{
 		Keys:  keys,
-		Error: plugin.NewBasicError(err),
+		Error: wrapError(err),
 	}
 	return nil
 }
@@ -79,7 +78,7 @@ func (s *StorageServer) Get(key string, reply *StorageGetReply) error {
 	storageEntry, err := s.impl.Get(key)
 	*reply = StorageGetReply{
 		StorageEntry: storageEntry,
-		Error:        plugin.NewBasicError(err),
+		Error:        wrapError(err),
 	}
 	return nil
 }
@@ -87,7 +86,7 @@ func (s *StorageServer) Get(key string, reply *StorageGetReply) error {
 func (s *StorageServer) Put(entry *logical.StorageEntry, reply *StoragePutReply) error {
 	err := s.impl.Put(entry)
 	*reply = StoragePutReply{
-		Error: plugin.NewBasicError(err),
+		Error: wrapError(err),
 	}
 	return nil
 }
@@ -95,27 +94,27 @@ func (s *StorageServer) Put(entry *logical.StorageEntry, reply *StoragePutReply)
 func (s *StorageServer) Delete(key string, reply *StorageDeleteReply) error {
 	err := s.impl.Delete(key)
 	*reply = StorageDeleteReply{
-		Error: plugin.NewBasicError(err),
+		Error: wrapError(err),
 	}
 	return nil
 }
 
 type StorageListReply struct {
 	Keys  []string
-	Error *plugin.BasicError
+	Error error
 }
 
 type StorageGetReply struct {
 	StorageEntry *logical.StorageEntry
-	Error        *plugin.BasicError
+	Error        error
 }
 
 type StoragePutReply struct {
-	Error *plugin.BasicError
+	Error error
 }
 
 type StorageDeleteReply struct {
-	Error *plugin.BasicError
+	Error error
 }
 
 // NOOPStorage is used to deny access to the storage interface while running a

logical/plugin/system.go

@@ -6,7 +6,6 @@ import (
 
 	"fmt"
 
-	plugin "github.com/hashicorp/go-plugin"
 	"github.com/hashicorp/vault/helper/consts"
 	"github.com/hashicorp/vault/helper/pluginutil"
 	"github.com/hashicorp/vault/helper/wrapping"
@@ -182,7 +181,7 @@ func (s *SystemViewServer) ResponseWrapData(args *ResponseWrapDataArgs, reply *R
 	info, err := s.impl.ResponseWrapData(args.Data, args.TTL, false)
 	if err != nil {
 		*reply = ResponseWrapDataReply{
-			Error: plugin.NewBasicError(err),
+			Error: wrapError(err),
 		}
 		return nil
 	}
@@ -239,7 +238,7 @@ type ResponseWrapDataArgs struct {
 
 type ResponseWrapDataReply struct {
 	ResponseWrapInfo *wrapping.ResponseWrapInfo
-	Error            *plugin.BasicError
+	Error            error
 }
 
 type MlockEnabledReply struct {

vault/logical_system.go

@@ -2639,7 +2639,7 @@ and is unaffected by replication.`,
 	},
 
 	"mount_plugin_name": {
-		`Name of the plugin to mount based from the name registered 
+		`Name of the plugin to mount based from the name registered
 in the plugin catalog.`,
 	},
 
@@ -2989,7 +2989,7 @@ This path responds to the following HTTP methods.
 		"",
 	},
 	"plugin-catalog_sha-256": {
-		`The SHA256 sum of the executable used in the 
+		`The SHA256 sum of the executable used in the
 command field. This should be HEX encoded.`,
 		"",
 	},