Implement the Validate RPC on built-in plugins (#5303)

The old API performed all configuration checks coupled with plugin reconfiguration under the Configure() func. The new API adds a Validation() func that only performs configuration checks but has no impact on the running plugin. To facilitate easier user, the pluginconf package was added that makes it easier to handle the merged code streams through a pluginconf.Status struct that will capture the first error (for integration with Configure() while permitting the Validation() to capture all errors that can be captured. Unit tests had to be reworked, as a side-effect of using the new pluginconf package is that all plugins now automatically check their trustdomain, instead of each plugin checking it in a haphazard manner. Occasionally, very small fixes were performed on plugins, and plugin coding standards were tweaked in small ways to be more similar to each other. Signed-off-by: Edwin Buck <[email protected]>
spiffe · Sep 23, 2024 · 41746f0 · 41746f0
1 parent 2328715
commit 41746f0
Show file tree

Hide file tree

Showing 99 changed files with 2,969 additions and 1,827 deletions.
diff --git a/go.mod b/go.mod
@@ -73,7 +73,7 @@ require (
 	github.com/sirupsen/logrus v1.9.3
 	github.com/spiffe/go-spiffe/v2 v2.3.0
 	github.com/spiffe/spire-api-sdk v1.2.5-0.20240807182354-18e423ce2c1c
-	github.com/spiffe/spire-plugin-sdk v1.4.4-0.20230721151831-bf67dde4721d
+	github.com/spiffe/spire-plugin-sdk v1.4.4-0.20240701180828-594312f4444d
 	github.com/stretchr/testify v1.9.0
 	github.com/uber-go/tally/v4 v4.1.16
 	github.com/valyala/fastjson v1.6.4

diff --git a/go.sum b/go.sum
@@ -1393,8 +1393,8 @@ github.com/spiffe/go-spiffe/v2 v2.3.0 h1:g2jYNb/PDMB8I7mBGL2Zuq/Ur6hUhoroxGQFyD6
 github.com/spiffe/go-spiffe/v2 v2.3.0/go.mod h1:Oxsaio7DBgSNqhAO9i/9tLClaVlfRok7zvJnTV8ZyIY=
 github.com/spiffe/spire-api-sdk v1.2.5-0.20240807182354-18e423ce2c1c h1:lK/B2paDUiqbngUGsLxDBmNX/BsG2yKxS8W/iGT+x2c=
 github.com/spiffe/spire-api-sdk v1.2.5-0.20240807182354-18e423ce2c1c/go.mod h1:4uuhFlN6KBWjACRP3xXwrOTNnvaLp1zJs8Lribtr4fI=
-github.com/spiffe/spire-plugin-sdk v1.4.4-0.20230721151831-bf67dde4721d h1:LCRQGU6vOqKLfRrG+GJQrwMwDILcAddAEIf4/1PaSVc=
-github.com/spiffe/spire-plugin-sdk v1.4.4-0.20230721151831-bf67dde4721d/go.mod h1:GA6o2PVLwyJdevT6KKt5ZXCY/ziAPna13y/seGk49Ik=
+github.com/spiffe/spire-plugin-sdk v1.4.4-0.20240701180828-594312f4444d h1:Upcyq8u1aWFHTQSEskwxBE2PehobpY+M21LXXDS/mPw=
+github.com/spiffe/spire-plugin-sdk v1.4.4-0.20240701180828-594312f4444d/go.mod h1:GA6o2PVLwyJdevT6KKt5ZXCY/ziAPna13y/seGk49Ik=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw=

diff --git a/pkg/agent/plugin/nodeattestor/awsiid/iid.go b/pkg/agent/plugin/nodeattestor/awsiid/iid.go
@@ -15,6 +15,7 @@ import (
 	configv1 "github.com/spiffe/spire-plugin-sdk/proto/spire/service/common/config/v1"
 	"github.com/spiffe/spire/pkg/common/catalog"
 	caws "github.com/spiffe/spire/pkg/common/plugin/aws"
+	"github.com/spiffe/spire/pkg/common/pluginconf"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 )
@@ -40,6 +41,16 @@ type IIDAttestorConfig struct {
 	EC2MetadataEndpoint string `hcl:"ec2_metadata_endpoint"`
 }
 
+func buildConfig(coreConfig catalog.CoreConfig, hclText string, status *pluginconf.Status) *IIDAttestorConfig {
+	newConfig := &IIDAttestorConfig{}
+	if err := hcl.Decode(newConfig, hclText); err != nil {
+		status.ReportErrorf("unable to decode configuration: %v", err)
+		return nil
+	}
+
+	return newConfig
+}
+
 // IIDAttestorPlugin implements aws nodeattestation in the agent.
 type IIDAttestorPlugin struct {
 	nodeattestorv1.UnsafeNodeAttestorServer
@@ -155,20 +166,27 @@ func readStringAndClose(r io.ReadCloser) (string, error) {
 
 // Configure implements the Config interface method of the same name
 func (p *IIDAttestorPlugin) Configure(_ context.Context, req *configv1.ConfigureRequest) (*configv1.ConfigureResponse, error) {
-	// Parse HCL config payload into config struct
-	config := &IIDAttestorConfig{}
-	if err := hcl.Decode(config, req.HclConfiguration); err != nil {
-		return nil, status.Errorf(codes.InvalidArgument, "unable to decode configuration: %v", err)
+	newConfig, _, err := pluginconf.Build(req, buildConfig)
+	if err != nil {
+		return nil, err
 	}
 
 	p.mtx.Lock()
 	defer p.mtx.Unlock()
-
-	p.config = config
+	p.config = newConfig
 
 	return &configv1.ConfigureResponse{}, nil
 }
 
+func (p *IIDAttestorPlugin) Validate(_ context.Context, req *configv1.ValidateRequest) (*configv1.ValidateResponse, error) {
+	_, notes, err := pluginconf.Build(req, buildConfig)
+
+	return &configv1.ValidateResponse{
+		Valid: err == nil,
+		Notes: notes,
+	}, nil
+}
+
 func (p *IIDAttestorPlugin) getConfig() (*IIDAttestorConfig, error) {
 	p.mtx.RLock()
 	defer p.mtx.RUnlock()

diff --git a/pkg/agent/plugin/nodeattestor/awsiid/iid_test.go b/pkg/agent/plugin/nodeattestor/awsiid/iid_test.go
@@ -18,8 +18,10 @@ import (
 
 	"github.com/aws/aws-sdk-go-v2/feature/ec2/imds"
 	"github.com/fullsailor/pkcs7"
+	"github.com/spiffe/go-spiffe/v2/spiffeid"
 	"github.com/spiffe/spire/pkg/agent/plugin/nodeattestor"
 	nodeattestortest "github.com/spiffe/spire/pkg/agent/plugin/nodeattestor/test"
+	"github.com/spiffe/spire/pkg/common/catalog"
 	"github.com/spiffe/spire/pkg/common/pemutil"
 	"github.com/spiffe/spire/pkg/common/plugin/aws"
 	"github.com/spiffe/spire/test/plugintest"
@@ -93,6 +95,9 @@ func (s *Suite) SetupTest() {
 	}))
 
 	s.p = s.loadPlugin(
+		plugintest.CoreConfig(catalog.CoreConfig{
+			TrustDomain: spiffeid.RequireTrustDomainFromString("example.org"),
+		}),
 		plugintest.Configuref(`ec2_metadata_endpoint = "http://%s/latest"`, s.server.Listener.Addr()),
 	)
 
@@ -141,13 +146,21 @@ func (s *Suite) TestConfigure() {
 
 	var err error
 	s.loadPlugin(
+		plugintest.CoreConfig(catalog.CoreConfig{
+			TrustDomain: spiffeid.RequireTrustDomainFromString("example.org"),
+		}),
 		plugintest.CaptureConfigureError(&err),
 		plugintest.Configure("malformed"),
 	)
 	require.Error(err)
 
 	// success
-	s.loadPlugin(plugintest.Configure(""))
+	s.loadPlugin(
+		plugintest.CoreConfig(catalog.CoreConfig{
+			TrustDomain: spiffeid.RequireTrustDomainFromString("example.org"),
+		}),
+		plugintest.Configure(""),
+	)
 }
 
 func (s *Suite) loadPlugin(opts ...plugintest.Option) nodeattestor.NodeAttestor {

diff --git a/pkg/agent/plugin/nodeattestor/azuremsi/msi.go b/pkg/agent/plugin/nodeattestor/azuremsi/msi.go
@@ -11,6 +11,7 @@ import (
 	configv1 "github.com/spiffe/spire-plugin-sdk/proto/spire/service/common/config/v1"
 	"github.com/spiffe/spire/pkg/common/catalog"
 	"github.com/spiffe/spire/pkg/common/plugin/azure"
+	"github.com/spiffe/spire/pkg/common/pluginconf"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 )
@@ -40,6 +41,20 @@ type MSIAttestorConfig struct {
 	ResourceID string `hcl:"resource_id"`
 }
 
+func buildConfig(coreConfig catalog.CoreConfig, hclText string, status *pluginconf.Status) *MSIAttestorConfig {
+	newConfig := new(MSIAttestorConfig)
+	if err := hcl.Decode(newConfig, hclText); err != nil {
+		status.ReportErrorf("unable to decode configuration: %v", err)
+		return nil
+	}
+
+	if newConfig.ResourceID == "" {
+		newConfig.ResourceID = azure.DefaultMSIResourceID
+	}
+
+	return newConfig
+}
+
 type MSIAttestorPlugin struct {
 	nodeattestorv1.UnsafeNodeAttestorServer
 	configv1.UnsafeConfigServer
@@ -85,19 +100,27 @@ func (p *MSIAttestorPlugin) AidAttestation(stream nodeattestorv1.NodeAttestor_Ai
 }
 
 func (p *MSIAttestorPlugin) Configure(_ context.Context, req *configv1.ConfigureRequest) (*configv1.ConfigureResponse, error) {
-	config := new(MSIAttestorConfig)
-	if err := hcl.Decode(config, req.HclConfiguration); err != nil {
-		return nil, status.Errorf(codes.InvalidArgument, "unable to decode configuration: %v", err)
+	newConfig, _, err := pluginconf.Build(req, buildConfig)
+	if err != nil {
+		return nil, err
 	}
 
-	if config.ResourceID == "" {
-		config.ResourceID = azure.DefaultMSIResourceID
-	}
+	p.mu.Lock()
+	defer p.mu.Unlock()
+	p.config = newConfig
 
-	p.setConfig(config)
 	return &configv1.ConfigureResponse{}, nil
 }
 
+func (p *MSIAttestorPlugin) Validate(_ context.Context, req *configv1.ValidateRequest) (*configv1.ValidateResponse, error) {
+	_, notes, err := pluginconf.Build(req, buildConfig)
+
+	return &configv1.ValidateResponse{
+		Valid: err == nil,
+		Notes: notes,
+	}, nil
+}
+
 func (p *MSIAttestorPlugin) getConfig() (*MSIAttestorConfig, error) {
 	p.mu.RLock()
 	defer p.mu.RUnlock()
@@ -106,9 +129,3 @@ func (p *MSIAttestorPlugin) getConfig() (*MSIAttestorConfig, error) {
 	}
 	return p.config, nil
 }
-
-func (p *MSIAttestorPlugin) setConfig(config *MSIAttestorConfig) {
-	p.mu.Lock()
-	defer p.mu.Unlock()
-	p.config = config
-}
diff --git a/pkg/agent/plugin/nodeattestor/azuremsi/msi_test.go b/pkg/agent/plugin/nodeattestor/azuremsi/msi_test.go
@@ -9,8 +9,10 @@ import (
 
 	jose "github.com/go-jose/go-jose/v4"
 	"github.com/go-jose/go-jose/v4/jwt"
+	"github.com/spiffe/go-spiffe/v2/spiffeid"
 	"github.com/spiffe/spire/pkg/agent/plugin/nodeattestor"
 	nodeattestortest "github.com/spiffe/spire/pkg/agent/plugin/nodeattestor/test"
+	"github.com/spiffe/spire/pkg/common/catalog"
 	"github.com/spiffe/spire/pkg/common/plugin/azure"
 	"github.com/spiffe/spire/test/plugintest"
 	"github.com/spiffe/spire/test/spiretest"
@@ -49,7 +51,12 @@ func (s *MSIAttestorSuite) TestAidAttestationNotConfigured() {
 func (s *MSIAttestorSuite) TestAidAttestationFailedToObtainToken() {
 	s.tokenErr = errors.New("FAILED")
 
-	attestor := s.loadAttestor(plugintest.Configure(""))
+	attestor := s.loadAttestor(
+		plugintest.CoreConfig(catalog.CoreConfig{
+			TrustDomain: spiffeid.RequireTrustDomainFromString("example.org"),
+		}),
+		plugintest.Configure(""),
+	)
 	err := attestor.Attest(context.Background(), streamBuilder.Build())
 	s.RequireGRPCStatus(err, codes.Internal, "nodeattestor(azure_msi): unable to fetch token: FAILED")
 }
@@ -59,23 +66,46 @@ func (s *MSIAttestorSuite) TestAidAttestationSuccess() {
 
 	expectPayload := []byte(fmt.Sprintf(`{"token":%q}`, s.token))
 
-	attestor := s.loadAttestor(plugintest.Configure(""))
+	attestor := s.loadAttestor(
+		plugintest.CoreConfig(catalog.CoreConfig{
+			TrustDomain: spiffeid.RequireTrustDomainFromString("example.org"),
+		}),
+		plugintest.Configure(""),
+	)
 	err := attestor.Attest(context.Background(), streamBuilder.ExpectAndBuild(expectPayload))
 	s.Require().NoError(err)
 }
 
 func (s *MSIAttestorSuite) TestConfigure() {
 	// malformed configuration
 	var err error
-	s.loadAttestor(plugintest.CaptureConfigureError(&err), plugintest.Configure("blah"))
+	s.loadAttestor(
+		plugintest.CaptureConfigureError(&err),
+		plugintest.CoreConfig(catalog.CoreConfig{
+			TrustDomain: spiffeid.RequireTrustDomainFromString("example.org"),
+		}),
+		plugintest.Configure("blah"),
+	)
 	s.RequireGRPCStatusContains(err, codes.InvalidArgument, "unable to decode configuration")
 
 	// success
-	s.loadAttestor(plugintest.CaptureConfigureError(&err), plugintest.Configure(""))
+	s.loadAttestor(
+		plugintest.CaptureConfigureError(&err),
+		plugintest.CoreConfig(catalog.CoreConfig{
+			TrustDomain: spiffeid.RequireTrustDomainFromString("example.org"),
+		}),
+		plugintest.Configure(""),
+	)
 	s.Require().NoError(err)
 
 	// success with resource_id
-	s.loadAttestor(plugintest.CaptureConfigureError(&err), plugintest.Configure(`resource_id = "foo"`))
+	s.loadAttestor(
+		plugintest.CaptureConfigureError(&err),
+		plugintest.CoreConfig(catalog.CoreConfig{
+			TrustDomain: spiffeid.RequireTrustDomainFromString("example.org"),
+		}),
+		plugintest.Configure(`resource_id = "foo"`),
+	)
 	s.Require().NoError(err)
 }
 

diff --git a/pkg/agent/plugin/nodeattestor/gcpiit/iit.go b/pkg/agent/plugin/nodeattestor/gcpiit/iit.go
@@ -16,6 +16,7 @@ import (
 	configv1 "github.com/spiffe/spire-plugin-sdk/proto/spire/service/common/config/v1"
 	"github.com/spiffe/spire/pkg/common/catalog"
 	"github.com/spiffe/spire/pkg/common/plugin/gcp"
+	"github.com/spiffe/spire/pkg/common/pluginconf"
 )
 
 const (
@@ -50,6 +51,24 @@ type IITAttestorConfig struct {
 	ServiceAccount    string `hcl:"service_account"`
 }
 
+func buildConfig(coreConfig catalog.CoreConfig, hclText string, status *pluginconf.Status) *IITAttestorConfig {
+	newConfig := &IITAttestorConfig{}
+	if err := hcl.Decode(newConfig, hclText); err != nil {
+		status.ReportErrorf("unable to decode configuration: %v", err)
+		return nil
+	}
+
+	if newConfig.ServiceAccount == "" {
+		newConfig.ServiceAccount = defaultServiceAccount
+	}
+
+	if newConfig.IdentityTokenHost == "" {
+		newConfig.IdentityTokenHost = defaultIdentityTokenHost
+	}
+
+	return newConfig
+}
+
 // NewIITAttestorPlugin creates a new IITAttestorPlugin.
 func New() *IITAttestorPlugin {
 	return &IITAttestorPlugin{}
@@ -76,26 +95,27 @@ func (p *IITAttestorPlugin) AidAttestation(stream nodeattestorv1.NodeAttestor_Ai
 }
 
 func (p *IITAttestorPlugin) Configure(_ context.Context, req *configv1.ConfigureRequest) (*configv1.ConfigureResponse, error) {
-	config := &IITAttestorConfig{}
-	if err := hcl.Decode(config, req.HclConfiguration); err != nil {
-		return nil, status.Errorf(codes.InvalidArgument, "unable to decode configuration: %v", err)
-	}
-
-	if config.ServiceAccount == "" {
-		config.ServiceAccount = defaultServiceAccount
-	}
-
-	if config.IdentityTokenHost == "" {
-		config.IdentityTokenHost = defaultIdentityTokenHost
+	newConfig, _, err := pluginconf.Build(req, buildConfig)
+	if err != nil {
+		return nil, err
 	}
 
 	p.mtx.Lock()
 	defer p.mtx.Unlock()
-	p.config = config
+	p.config = newConfig
 
 	return &configv1.ConfigureResponse{}, nil
 }
 
+func (p *IITAttestorPlugin) Validate(_ context.Context, req *configv1.ValidateRequest) (*configv1.ValidateResponse, error) {
+	_, notes, err := pluginconf.Build(req, buildConfig)
+
+	return &configv1.ValidateResponse{
+		Valid: err == nil,
+		Notes: notes,
+	}, nil
+}
+
 func (p *IITAttestorPlugin) getConfig() (*IITAttestorConfig, error) {
 	p.mtx.Lock()
 	defer p.mtx.Unlock()

diff --git a/pkg/agent/plugin/nodeattestor/gcpiit/iit_test.go b/pkg/agent/plugin/nodeattestor/gcpiit/iit_test.go
@@ -11,8 +11,10 @@ import (
 	"github.com/go-jose/go-jose/v4"
 	"github.com/go-jose/go-jose/v4/cryptosigner"
 	"github.com/go-jose/go-jose/v4/jwt"
+	"github.com/spiffe/go-spiffe/v2/spiffeid"
 	"github.com/spiffe/spire/pkg/agent/plugin/nodeattestor"
 	nodeattestortest "github.com/spiffe/spire/pkg/agent/plugin/nodeattestor/test"
+	"github.com/spiffe/spire/pkg/common/catalog"
 	"github.com/spiffe/spire/pkg/common/plugin/gcp"
 	"github.com/spiffe/spire/test/plugintest"
 	"github.com/spiffe/spire/test/spiretest"
@@ -66,7 +68,10 @@ func (s *Suite) SetupSuite() {
 func (s *Suite) SetupTest() {
 	s.status = http.StatusOK
 	s.body = ""
-	s.na = s.loadPlugin(plugintest.Configuref(`
+	s.na = s.loadPlugin(plugintest.CoreConfig(catalog.CoreConfig{
+		TrustDomain: spiffeid.RequireTrustDomainFromString("example.org"),
+	}),
+		plugintest.Configuref(`
 		service_account = "%s"
 		identity_token_host = "%s"
 `, testServiceAccount, s.server.Listener.Addr().String()))
@@ -111,7 +116,12 @@ func (s *Suite) TestConfigure() {
 
 	// malformed
 	var err error
-	s.loadPlugin(plugintest.CaptureConfigureError(&err), plugintest.Configure("malformed"))
+	s.loadPlugin(plugintest.CaptureConfigureError(&err),
+		plugintest.CoreConfig(catalog.CoreConfig{
+			TrustDomain: spiffeid.RequireTrustDomainFromString("example.org"),
+		}),
+		plugintest.Configure("malformed"),
+	)
 	require.Error(err)
 }
 
@@ -135,7 +145,7 @@ func TestRetrieveIdentity(t *testing.T) {
 		},
 		{
 			msg:               "invalid port",
-			url:               "http://0.0.0.0:70000",
+			url:               "http://127.0.0.1:70000",
 			expectErrContains: "invalid port",
 		},
 		{