From 6c8a88c22748d1ffc50e32de617cfd75382d2b34 Mon Sep 17 00:00:00 2001 From: Edwin Buck Date: Tue, 26 Nov 2024 11:16:11 -0800 Subject: [PATCH 01/12] Squahed commit of adding support for cli plugin validation reporting. Signed-off-by: Edwin Buck --- cmd/spire-server/cli/cli.go | 2 +- cmd/spire-server/cli/run/run.go | 15 +-- cmd/spire-server/cli/validate/validate.go | 102 ++++++++++++++++-- .../cli/validate/validate_test.go | 5 +- pkg/agent/catalog/catalog.go | 2 +- pkg/common/catalog/catalog.go | 91 +++++++++++++--- pkg/common/catalog/catalog_test.go | 2 +- pkg/common/catalog/configure.go | 90 +++++++++++++--- pkg/common/catalog/plugin.go | 3 + pkg/common/pluginconf/pluginconf.go | 18 ++++ pkg/server/catalog/catalog.go | 91 +++++++++++----- pkg/server/catalog/catalog_test.go | 2 +- pkg/server/config.go | 31 +++++- pkg/server/datastore/sqlstore/sqlstore.go | 10 +- .../endpoints/bundle/internal/acmetest/ca.go | 2 +- .../bundle/internal/autocert/listener.go | 4 +- pkg/server/server.go | 26 +++-- 17 files changed, 405 insertions(+), 91 deletions(-) diff --git a/cmd/spire-server/cli/cli.go b/cmd/spire-server/cli/cli.go index e86c939305..034ee4ea1c 100644 --- a/cmd/spire-server/cli/cli.go +++ b/cmd/spire-server/cli/cli.go @@ -125,7 +125,7 @@ func (cc *CLI) Run(ctx context.Context, args []string) int { return jwt.NewMintCommand(), nil }, "validate": func() (cli.Command, error) { - return validate.NewValidateCommand(), nil + return validate.NewValidateCommand(ctx, cc.LogOptions), nil }, "localauthority x509 show": func() (cli.Command, error) { return localauthority_x509.NewX509ShowCommand(), nil diff --git a/cmd/spire-server/cli/run/run.go b/cmd/spire-server/cli/run/run.go index 97804a41b9..aee0f7c013 100644 --- a/cmd/spire-server/cli/run/run.go +++ b/cmd/spire-server/cli/run/run.go @@ -222,16 +222,16 @@ func (cmd *Command) Help() string { // Help is a standalone function that prints a help message to writer. // It is used by both the run and validate commands, so they can share flag usage messages. -func Help(name string, writer io.Writer) string { - _, err := parseFlags(name, []string{"-h"}, writer) +func Help(name string, writer io.Writer, options ...func(fs *flag.FlagSet)) string { + _, err := parseFlags(name, []string{"-h"}, writer, options...) // Error is always present because -h is passed return err.Error() } -func LoadConfig(name string, args []string, logOptions []log.Option, output io.Writer, allowUnknownConfig bool) (*server.Config, error) { +func LoadConfig(name string, args []string, logOptions []log.Option, output io.Writer, allowUnknownConfig bool, options ...func(*flag.FlagSet)) (*server.Config, error) { // First parse the CLI flags so we can get the config // file path, if set - cliInput, err := parseFlags(name, args, output) + cliInput, err := parseFlags(name, args, output, options...) if err != nil { return nil, err } @@ -267,7 +267,7 @@ func (cmd *Command) Run(args []string) int { // Set umask before starting up the server common_cli.SetUmask(c.Log) - s := server.New(*c) + s := server.New(c) ctx := cmd.ctx if ctx == nil { @@ -327,7 +327,7 @@ func ParseFile(path string, expandEnv bool) (*Config, error) { return c, nil } -func parseFlags(name string, args []string, output io.Writer) (*serverConfig, error) { +func parseFlags(name string, args []string, output io.Writer, options ...func(*flag.FlagSet)) (*serverConfig, error) { flags := flag.NewFlagSet(name, flag.ContinueOnError) flags.SetOutput(output) c := &serverConfig{} @@ -342,6 +342,9 @@ func parseFlags(name string, args []string, output io.Writer) (*serverConfig, er flags.StringVar(&c.LogLevel, "logLevel", "", "'debug', 'info', 'warn', or 'error'") flags.StringVar(&c.TrustDomain, "trustDomain", "", "The trust domain that this server belongs to") flags.BoolVar(&c.ExpandEnv, "expandEnv", false, "Expand environment variables in SPIRE config file") + for _, option := range options { + option(flags) + } c.addOSFlags(flags) err := flags.Parse(args) diff --git a/cmd/spire-server/cli/validate/validate.go b/cmd/spire-server/cli/validate/validate.go index 84c76b1234..3cf6c1fef0 100644 --- a/cmd/spire-server/cli/validate/validate.go +++ b/cmd/spire-server/cli/validate/validate.go @@ -1,42 +1,124 @@ package validate import ( + "context" + "errors" + "flag" + "fmt" + "github.com/mitchellh/cli" "github.com/spiffe/spire/cmd/spire-server/cli/run" commoncli "github.com/spiffe/spire/pkg/common/cli" + "github.com/spiffe/spire/pkg/common/cliprinter" + "github.com/spiffe/spire/pkg/common/log" + "github.com/spiffe/spire/pkg/server" ) const commandName = "validate" -func NewValidateCommand() cli.Command { - return newValidateCommand(commoncli.DefaultEnv) +func NewValidateCommand(ctx context.Context, logOptions []log.Option) cli.Command { + return newValidateCommand(ctx, commoncli.DefaultEnv, logOptions) } -func newValidateCommand(env *commoncli.Env) *validateCommand { +func newValidateCommand(ctx context.Context, env *commoncli.Env, logOptions []log.Option) *validateCommand { return &validateCommand{ - env: env, + ctx: ctx, + env: env, + logOptions: logOptions, } } type validateCommand struct { - env *commoncli.Env + ctx context.Context + logOptions []log.Option + env *commoncli.Env + printer cliprinter.Printer } // Help prints the server cmd usage func (c *validateCommand) Help() string { - return run.Help(commandName, c.env.Stderr) + return run.Help(commandName, c.env.Stderr, c.SetupPrinter) } func (c *validateCommand) Synopsis() string { return "Validates a SPIRE server configuration file" } +func (c *validateCommand) SetupPrinter(flags *flag.FlagSet) { + cliprinter.AppendFlagWithCustomPretty(&c.printer, flags, c.env, c.prettyPrintValidate) +} + func (c *validateCommand) Run(args []string) int { - if _, err := run.LoadConfig(commandName, args, nil, c.env.Stderr, false); err != nil { - // Ignore error since a failure to write to stderr cannot very well be reported - _ = c.env.ErrPrintf("SPIRE server configuration file is invalid: %v\n", err) + config, err := run.LoadConfig(commandName, args, c.logOptions, c.env.Stderr, false, c.SetupPrinter) + if err != nil { + _, _ = fmt.Fprintln(c.env.Stderr, err) + return 1 + } + config.ValidateOnly = true + + // Set umask before starting up the server + commoncli.SetUmask(config.Log) + + s := server.New(config) + + ctx := c.ctx + if ctx == nil { + ctx = context.Background() + } + + err = s.Run(ctx) + if err != nil { + config.Log.WithError(err).Error("Validation failed: validation server crashed") + return 1 + } + + err = c.printer.PrintStruct(&validateResult{ + Valid: config.ValidationError == "", + Error: config.ValidationError, + Notes: config.ValidationNotes, + }) + if err != nil { return 1 } - _ = c.env.Println("SPIRE server configuration file is valid.") return 0 } + +type validateResult struct { + Valid bool `json:"valid"` + Error string `json:"error"` + Notes []string `json:"notes"` +} + +func (c *validateCommand) prettyPrintValidate(env *commoncli.Env, results ...any) error { + if resultInterface, ok := results[0].([]any); ok { + result, ok := resultInterface[0].(*validateResult) + if !ok { + return errors.New("unexpected type") + } + // pretty print error section + if !result.Valid { + if err := env.Printf("Validation error:\n"); err != nil { + return err + } + if err := env.Printf(" %s\n", result.Error); err != nil { + return err + } + } + // pretty print notes section + if len(result.Notes) < 1 { + if err := env.Printf("No validation notes\n"); err != nil { + return err + } + return nil + } + if err := env.Printf("Validation notes:\n"); err != nil { + return err + } + for _, note := range result.Notes { + if err := env.Printf(" %s\n", note); err != nil { + return err + } + } + } + return cliprinter.ErrInternalCustomPrettyFunc +} diff --git a/cmd/spire-server/cli/validate/validate_test.go b/cmd/spire-server/cli/validate/validate_test.go index 26772f6ff1..06d0af73d9 100644 --- a/cmd/spire-server/cli/validate/validate_test.go +++ b/cmd/spire-server/cli/validate/validate_test.go @@ -2,6 +2,7 @@ package validate import ( "bytes" + "context" "testing" "github.com/mitchellh/cli" @@ -31,11 +32,11 @@ func (s *ValidateSuite) SetupTest() { s.stdout = new(bytes.Buffer) s.stderr = new(bytes.Buffer) - s.cmd = newValidateCommand(&common_cli.Env{ + s.cmd = newValidateCommand(context.Background(), &common_cli.Env{ Stdin: s.stdin, Stdout: s.stdout, Stderr: s.stderr, - }) + }, nil) } func (s *ValidateSuite) TestSynopsis() { diff --git a/pkg/agent/catalog/catalog.go b/pkg/agent/catalog/catalog.go index 0036bff1b9..7e1c28795a 100644 --- a/pkg/agent/catalog/catalog.go +++ b/pkg/agent/catalog/catalog.go @@ -91,7 +91,7 @@ func Load(ctx context.Context, config Config) (_ *Repository, err error) { repo := &Repository{ log: config.Log, } - repo.catalog, err = catalog.Load(ctx, catalog.Config{ + repo.catalog, err = catalog.Load(ctx, &catalog.Config{ Log: config.Log, CoreConfig: catalog.CoreConfig{ TrustDomain: config.TrustDomain, diff --git a/pkg/common/catalog/catalog.go b/pkg/common/catalog/catalog.go index d1ce9cd413..4490aa43a3 100644 --- a/pkg/common/catalog/catalog.go +++ b/pkg/common/catalog/catalog.go @@ -107,6 +107,34 @@ type Config struct { // CoreConfig is the core configuration provided to each plugin. CoreConfig CoreConfig + + // Validate plugins only + ValidateOnly bool + + // Validation findings + ValidationNotes []string + + // First validation error + ValidationError string +} + +func (c *Config) ReportInfo(message string) { + c.ValidationNotes = append(c.ValidationNotes, message) +} + +func (c *Config) ReportInfof(message string, args ...any) { + c.ReportInfo(fmt.Sprintf(message, args...)) +} + +func (c *Config) ReportError(message string) { + if c.ValidationError == "" { + c.ValidationError = message + } + c.ValidationNotes = append(c.ValidationNotes, message) +} + +func (c *Config) ReportErrorf(message string, args ...any) { + c.ReportError(fmt.Sprintf(message, args...)) } type Catalog struct { @@ -129,7 +157,7 @@ func (c *Catalog) Close() error { // given catalog are considered invalidated. If any plugin fails to load or // configure, all plugins are unloaded, the catalog is cleared, and the // function returns an error. -func Load(ctx context.Context, config Config, repo Repository) (_ *Catalog, err error) { +func Load(ctx context.Context, config *Config, repo Repository) (_ *Catalog, err error) { closers := make(closerGroup, 0) defer func() { // If loading fails, clear out the catalog and close down all plugins @@ -145,26 +173,39 @@ func Load(ctx context.Context, config Config, repo Repository) (_ *Catalog, err } }() + log := config.Log.WithFields(logrus.Fields{ + telemetry.SubsystemName: "common_catalog", + }) + pluginRepos, err := makeBindablePluginRepos(repo.Plugins()) if err != nil { return nil, err } + log.Infof("bindablePluginRepos: %+v", pluginRepos) + serviceRepos, err := makeBindableServiceRepos(repo.Services()) if err != nil { return nil, err } + log.Infof("bindableServiceRepos: %+v", serviceRepos) pluginCounts := make(map[string]int) var reconfigurers Reconfigurers for _, pluginConfig := range config.PluginConfigs { + log.Infof("plugin(%s): processing", pluginConfig.Name) + pluginLog := makePluginLog(config.Log, pluginConfig) pluginRepo, ok := pluginRepos[pluginConfig.Type] if !ok { - pluginLog.Error("Unsupported plugin type") - return nil, fmt.Errorf("unsupported plugin type %q", pluginConfig.Type) + config.ReportErrorf("common catalog: Unsupported plugin %q of type %q", pluginConfig.Name, pluginConfig.Type) + if !config.ValidateOnly { + return nil, fmt.Errorf("unsupported plugin type %q", pluginConfig.Type) + } + continue } + log.Infof("plugin(%s): supported", pluginConfig.Name) if pluginConfig.Disabled { pluginLog.Debug("Not loading plugin; disabled") @@ -173,8 +214,12 @@ func Load(ctx context.Context, config Config, repo Repository) (_ *Catalog, err plugin, err := loadPlugin(ctx, pluginRepo.BuiltIns(), pluginConfig, pluginLog, config.HostServices) if err != nil { + config.ReportErrorf("commmon catalog: plugin %q failed to load", pluginConfig.Name) pluginLog.WithError(err).Error("Failed to load plugin") - return nil, fmt.Errorf("failed to load plugin %q: %w", pluginConfig.Name, err) + if !config.ValidateOnly { + return nil, fmt.Errorf("failed to load plugin %q: %w", pluginConfig.Name, err) + } + continue } // Add the plugin to the closers even though it has not been completely @@ -182,20 +227,36 @@ func Load(ctx context.Context, config Config, repo Repository) (_ *Catalog, err // panic, etc.) we want the defer above to close the plugin. Failure to // do so can orphan external plugin processes. closers = append(closers, pluginCloser{plugin: plugin, log: pluginLog}) + log.Infof("plugin(%s): loaded", pluginConfig.Name) configurer, err := plugin.bindRepos(pluginRepo, serviceRepos) if err != nil { + config.ReportErrorf("commmon catalog: failed to bind plugin %q", pluginConfig.Name) pluginLog.WithError(err).Error("Failed to bind plugin") - return nil, fmt.Errorf("failed to bind plugin %q: %w", pluginConfig.Name, err) + if !config.ValidateOnly { + return nil, fmt.Errorf("failed to bind plugin %q: %w", pluginConfig.Name, err) + } } + log.Infof("plugin(%s): bound, configurer %+v", pluginConfig.Name, configurer) - reconfigurer, err := configurePlugin(ctx, pluginLog, config.CoreConfig, configurer, pluginConfig.DataSource) - if err != nil { - pluginLog.WithError(err).Error("Failed to configure plugin") - return nil, fmt.Errorf("failed to configure plugin %q: %w", pluginConfig.Name, err) - } - if reconfigurer != nil { - reconfigurers = append(reconfigurers, reconfigurer) + if !config.ValidateOnly { + reconfigurer, err := configurePlugin(ctx, pluginLog, config.CoreConfig, configurer, pluginConfig.DataSource) + if err != nil { + pluginLog.WithError(err).Error("Failed to configure plugin") + return nil, fmt.Errorf("failed to configure plugin %q: %w", pluginConfig.Name, err) + } + if reconfigurer != nil { + reconfigurers = append(reconfigurers, reconfigurer) + } + } else { + result, _ := validatePlugin(ctx, pluginLog, config.CoreConfig, configurer, pluginConfig.DataSource) + for _, note := range result.Notes { + if note == result.Error { + config.ReportErrorf("plugin %s(%q): %s", pluginConfig.Type, pluginConfig.Name, note) + } else { + config.ReportInfof("plugin %s(%q): %s", pluginConfig.Type, pluginConfig.Name, note) + } + } } pluginLog.Info("Plugin loaded") @@ -205,7 +266,11 @@ func Load(ctx context.Context, config Config, repo Repository) (_ *Catalog, err // Make sure all plugin constraints are satisfied for pluginType, pluginRepo := range pluginRepos { if err := pluginRepo.Constraints().Check(pluginCounts[pluginType]); err != nil { - return nil, fmt.Errorf("plugin type %q constraint not satisfied: %w", pluginType, err) + config.ReportErrorf("commmon catalog: plugin type %q constraint violation: %s", pluginType, err.Error()) + if !config.ValidateOnly { + return nil, fmt.Errorf("plugin type %q constraint not satisfied: %w", pluginType, err) + } + continue } } diff --git a/pkg/common/catalog/catalog_test.go b/pkg/common/catalog/catalog_test.go index badc35061e..f6c6c77280 100644 --- a/pkg/common/catalog/catalog_test.go +++ b/pkg/common/catalog/catalog_test.go @@ -391,7 +391,7 @@ func testLoad(t *testing.T, pluginPath string, tt loadTest) { tt.mutateServiceRepo(serviceRepo) } - cat, err := catalog.Load(context.Background(), config, repo) + cat, err := catalog.Load(context.Background(), &config, repo) if cat != nil { defer func() { cat.Close() diff --git a/pkg/common/catalog/configure.go b/pkg/common/catalog/configure.go index 5c381cd88b..51ecf4d498 100644 --- a/pkg/common/catalog/configure.go +++ b/pkg/common/catalog/configure.go @@ -29,17 +29,38 @@ func (c CoreConfig) v1() *configv1.CoreConfiguration { type Configurer interface { Configure(ctx context.Context, coreConfig CoreConfig, configuration string) error - Validate(ctx context.Context, coreConfig CoreConfig, configuration string) error + Validate(ctx context.Context, coreConfig CoreConfig, configuration string) (*ValidateResult, error) } -type ConfigurerFunc func(ctx context.Context, coreConfig CoreConfig, configuration string) error +type ValidateResult struct { + Notes []string + Error string +} + +func NewValidateResult() *ValidateResult { + return &ValidateResult{ + Notes: nil, + Error: "", + } +} + +func (vr *ValidateResult) ReportError(message string) { + if vr.Error == "" { + vr.Error = message + } + vr.Notes = append(vr.Notes, message) +} -func (fn ConfigurerFunc) Configure(ctx context.Context, coreConfig CoreConfig, configuration string) error { - return fn(ctx, coreConfig, configuration) +func (vr *ValidateResult) ReportErrorf(format string, parameters ...any) { + vr.ReportError(fmt.Sprintf(format, parameters...)) } -func (fn ConfigurerFunc) Validate(ctx context.Context, coreConfig CoreConfig, configuration string) error { - return fn(ctx, coreConfig, configuration) +func (vr *ValidateResult) ReportInfo(message string) { + vr.Notes = append(vr.Notes, message) +} + +func (vr *ValidateResult) ReportInfof(format string, parameters ...any) { + vr.ReportInfo(fmt.Sprintf(format, parameters...)) } func ConfigurePlugin(ctx context.Context, coreConfig CoreConfig, configurer Configurer, dataSource DataSource, lastHash string) (string, error) { @@ -57,6 +78,18 @@ func ConfigurePlugin(ctx context.Context, coreConfig CoreConfig, configurer Conf return dataHash, nil } +func ValidatePlugin(ctx context.Context, coreConfig CoreConfig, configurer Configurer, dataSource DataSource) (*ValidateResult, error) { + status := NewValidateResult() + data, err := dataSource.Load() + if err != nil { + status.ReportErrorf("failed to load plugin data: %s", err.Error()) + return status, err + } + status.ReportInfo("") + + return configurer.Validate(ctx, coreConfig, data) +} + func ReconfigureTask(log logrus.FieldLogger, reconfigurer Reconfigurer) func(context.Context) error { return func(ctx context.Context) error { return ReconfigureOnSignal(ctx, log, reconfigurer) @@ -129,6 +162,25 @@ func configurePlugin(ctx context.Context, pluginLog logrus.FieldLogger, coreConf }, nil } +func validatePlugin(ctx context.Context, pluginLog logrus.FieldLogger, coreConfig CoreConfig, configurer Configurer, dataSource DataSource) (*ValidateResult, error) { + pluginLog.Info("validating plugin") + switch { + case configurer == nil && dataSource == nil: + // The plugin doesn't support configuration and no data source was configured. Nothing to do. + return NewValidateResult(), nil + case configurer == nil && dataSource != nil: + // The plugin does not support configuration but a data source was configured. This is a failure. + return NewValidateResult(), errors.New("no supported configuration interface found") + case configurer != nil && dataSource == nil: + // The plugin supports configuration but no data source was configured. Default to an empty, fixed configuration. + dataSource = FixedData("") + case configurer != nil && dataSource != nil: + // The plugin supports configuration and there was a data source. + } + + return ValidatePlugin(ctx, coreConfig, configurer, dataSource) +} + type configurerRepo struct { configurer Configurer } @@ -166,9 +218,6 @@ var _ Configurer = (*configurerV1)(nil) func (v1 *configurerV1) InitInfo(PluginInfo) { } -func (v1 *configurerV1) InitLog(logrus.FieldLogger) { -} - func (v1 *configurerV1) Configure(ctx context.Context, coreConfig CoreConfig, hclConfiguration string) error { _, err := v1.ConfigServiceClient.Configure(ctx, &configv1.ConfigureRequest{ CoreConfiguration: coreConfig.v1(), @@ -177,12 +226,23 @@ func (v1 *configurerV1) Configure(ctx context.Context, coreConfig CoreConfig, hc return err } -func (v1 *configurerV1) Validate(ctx context.Context, coreConfig CoreConfig, hclConfiguration string) error { - _, err := v1.ConfigServiceClient.Validate(ctx, &configv1.ValidateRequest{ +func (v1 *configurerV1) Validate(ctx context.Context, coreConfig CoreConfig, hclConfiguration string) (*ValidateResult, error) { + response, err := v1.ConfigServiceClient.Validate(ctx, &configv1.ValidateRequest{ CoreConfiguration: coreConfig.v1(), HclConfiguration: hclConfiguration, }) - return err + result := NewValidateResult() + // TODO: The GRPC default behavior is to not return a *ValidateResponse when there is an error, and that destroys many + // of the validation messages, except for the error message. We may wish to not consider validation errors + // as errors going forward. + result.Notes = response.GetNotes() + if !response.GetValid() { + result.ReportErrorf("invalid configuration: %s", err.Error()) + } + return result, err +} + +func (v1 *configurerV1) InitLog(logrus.FieldLogger) { } type configurerUnsupported struct{} @@ -191,8 +251,10 @@ func (c configurerUnsupported) Configure(context.Context, CoreConfig, string) er return status.Error(codes.FailedPrecondition, "plugin does not support a configuration interface") } -func (c configurerUnsupported) Validate(context.Context, CoreConfig, string) error { - return status.Error(codes.FailedPrecondition, "plugin does not support a validation interface") +func (c configurerUnsupported) Validate(context.Context, CoreConfig, string) (*ValidateResult, error) { + result := NewValidateResult() + result.ReportInfo("plugin does not support a validation interface") + return result, status.Error(codes.FailedPrecondition, "plugin does not support a validation interface") } func hashData(data string) string { diff --git a/pkg/common/catalog/plugin.go b/pkg/common/catalog/plugin.go index c48357fe78..118e60b738 100644 --- a/pkg/common/catalog/plugin.go +++ b/pkg/common/catalog/plugin.go @@ -101,9 +101,11 @@ func (p *pluginImpl) initFacade(facade Facade) any { func (p *pluginImpl) bindRepos(pluginRepo bindablePluginRepo, serviceRepos []bindableServiceRepo) (Configurer, error) { grpcServiceNames := grpcServiceNameSet(p.grpcServiceNames) + p.log.Infof("plugin %q binding repos", p.info.Name()) impl := p.bindRepo(pluginRepo, grpcServiceNames) for _, serviceRepo := range serviceRepos { + p.log.Infof("plugin %q binding repo %+v", p.info.Name(), serviceRepo) p.bindRepo(serviceRepo, grpcServiceNames) } @@ -149,6 +151,7 @@ func (p *pluginImpl) bindRepo(repo bindableServiceRepo, grpcServiceNames map[str if impl != nil { continue } + p.log.Infof("found implementation for service %s", facade.GRPCServiceName()) warnIfDeprecated(p.log, version, versions[0]) impl = p.bindFacade(repo, facade) } diff --git a/pkg/common/pluginconf/pluginconf.go b/pkg/common/pluginconf/pluginconf.go index 4f74d44257..b0c5fabd88 100644 --- a/pkg/common/pluginconf/pluginconf.go +++ b/pkg/common/pluginconf/pluginconf.go @@ -15,6 +15,16 @@ type Status struct { err error } +func (s *Status) Append(other *Status) { + if other == nil { + return + } + s.notes = append(s.notes, other.notes...) + if s.err == nil { + s.err = other.err + } +} + func (s *Status) ReportInfo(message string) { s.notes = append(s.notes, message) } @@ -34,6 +44,14 @@ func (s *Status) ReportErrorf(format string, args ...any) { s.ReportError(fmt.Sprintf(format, args...)) } +func (s *Status) HasError() bool { + return s.err != nil +} + +func (s *Status) Error() error { + return s.err +} + type Request interface { GetCoreConfiguration() *configv1.CoreConfiguration GetHclConfiguration() string diff --git a/pkg/server/catalog/catalog.go b/pkg/server/catalog/catalog.go index 96deacfeb1..44402ba4c5 100644 --- a/pkg/server/catalog/catalog.go +++ b/pkg/server/catalog/catalog.go @@ -66,6 +66,30 @@ type Config struct { IdentityProvider *identityprovider.IdentityProvider AgentStore *agentstore.AgentStore HealthChecker health.Checker + + ValidateOnly bool + + ValidationNotes []string + ValidationError string +} + +func (c *Config) ReportInfo(message string) { + c.ValidationNotes = append(c.ValidationNotes, message) +} + +func (c *Config) ReportInfof(format string, args ...any) { + c.ReportInfo(fmt.Sprintf(format, args...)) +} + +func (c *Config) ReportError(message string) { + if c.ValidationError == "" { + c.ValidationError = message + } + c.ValidationNotes = append(c.ValidationNotes, message) +} + +func (c *Config) ReportErrorf(format string, args ...any) { + c.ReportError(fmt.Sprintf(format, args...)) } type datastoreRepository struct{ datastore.Repository } @@ -125,11 +149,11 @@ func (repo *Repository) Close() { } } -func Load(ctx context.Context, config Config) (_ *Repository, err error) { +func Load(ctx context.Context, config *Config) (_ *Repository, err error) { if c, ok := config.PluginConfigs.Find(nodeAttestorType, jointoken.PluginName); ok && c.IsEnabled() && c.IsExternal() { + config.ReportError("the built-in join_token node attestor cannot be overridden by an external plugin") return nil, fmt.Errorf("the built-in join_token node attestor cannot be overridden by an external plugin") } - repo := &Repository{ log: config.Log, } @@ -145,14 +169,13 @@ func Load(ctx context.Context, config Config) (_ *Repository, err error) { // Strip out the Datastore plugin configuration and load the SQL plugin // directly. This allows us to bypass gRPC and get rid of response limits. - dataStoreConfigs, pluginConfigs := config.PluginConfigs.FilterByType(dataStoreType) - sqlDataStore, err := loadSQLDataStore(ctx, config, coreConfig, dataStoreConfigs) + sqlDataStore, pluginConfigs, err := loadSQLDataStore(ctx, config, coreConfig) if err != nil { return nil, err } repo.dsCloser = sqlDataStore - repo.catalog, err = catalog.Load(ctx, catalog.Config{ + commonConfig := &catalog.Config{ Log: config.Log, CoreConfig: coreConfig, PluginConfigs: pluginConfigs, @@ -161,10 +184,16 @@ func Load(ctx context.Context, config Config) (_ *Repository, err error) { agentstorev1.AgentStoreServiceServer(config.AgentStore.V1()), metricsv1.MetricsServiceServer(metricsservice.V1(config.Metrics)), }, - }, repo) + ValidateOnly: config.ValidateOnly, + } + repo.catalog, err = catalog.Load(ctx, commonConfig, repo) if err != nil { return nil, err } + config.ValidationNotes = append(config.ValidationNotes, commonConfig.ValidationNotes...) + if config.ValidationError == "" { + config.ValidationError = commonConfig.ValidationError + } var dataStore datastore.DataStore = sqlDataStore _ = config.HealthChecker.AddCheck("catalog.datastore", &datastore.Health{ @@ -176,44 +205,52 @@ func Load(ctx context.Context, config Config) (_ *Repository, err error) { repo.SetDataStore(dataStore) repo.SetKeyManager(km_telemetry.WithMetrics(repo.GetKeyManager(), config.Metrics)) - return repo, nil } -func loadSQLDataStore(ctx context.Context, config Config, coreConfig catalog.CoreConfig, datastoreConfigs catalog.PluginConfigs) (*ds_sql.Plugin, error) { +func validateSQLConfig(config *Config) (catalog.PluginConfig, PluginConfigs, error) { + datastoreConfigs, pluginConfigs := config.PluginConfigs.FilterByType(dataStoreType) switch { case len(datastoreConfigs) == 0: - return nil, errors.New("expecting a DataStore plugin") + config.ReportError("expecting a DataStore plugin") + return catalog.PluginConfig{}, PluginConfigs(nil), errors.New("expecting a DataStore plugin") case len(datastoreConfigs) > 1: - return nil, errors.New("only one DataStore plugin is allowed") + config.ReportError("only one DataStore plugin is allowed") + return catalog.PluginConfig{}, PluginConfigs(nil), errors.New("only one DataStore plugin is allowed") } - sqlConfig := datastoreConfigs[0] + datastoreConfig := datastoreConfigs[0] - if sqlConfig.Name != ds_sql.PluginName { - return nil, fmt.Errorf("pluggability for the DataStore is deprecated; only the built-in %q plugin is supported", ds_sql.PluginName) + if datastoreConfig.Name != ds_sql.PluginName { + config.ReportErrorf("pluggability for the DataStore is deprecated; only the built-in %q plugin is supported", ds_sql.PluginName) + return catalog.PluginConfig{}, PluginConfigs(nil), fmt.Errorf("pluggability for the DataStore is deprecated; only the built-in %q plugin is supported", ds_sql.PluginName) } - if sqlConfig.IsExternal() { - return nil, fmt.Errorf("pluggability for the DataStore is deprecated; only the built-in %q plugin is supported", ds_sql.PluginName) + if datastoreConfig.IsExternal() { + config.ReportErrorf("pluggability for the DataStore is deprecated; only the built-in %q plugin is supported", ds_sql.PluginName) + return catalog.PluginConfig{}, PluginConfigs(nil), fmt.Errorf("pluggability for the DataStore is deprecated; only the built-in %q plugin is supported", ds_sql.PluginName) } - if sqlConfig.DataSource == nil { - sqlConfig.DataSource = catalog.FixedData("") + if datastoreConfig.DataSource.IsDynamic() { + config.ReportInfo("DataStore is not reconfigurable even with a dynamic data source") } - dsLog := config.Log.WithField(telemetry.SubsystemName, sqlConfig.Name) - ds := ds_sql.New(dsLog) - configurer := catalog.ConfigurerFunc(func(ctx context.Context, _ catalog.CoreConfig, configuration string) error { - return ds.Configure(ctx, configuration) - }) + return datastoreConfig, pluginConfigs, nil +} - if _, err := catalog.ConfigurePlugin(ctx, coreConfig, configurer, sqlConfig.DataSource, ""); err != nil { - return nil, err +func loadSQLDataStore(ctx context.Context, config *Config, coreConfig catalog.CoreConfig) (*ds_sql.Plugin, PluginConfigs, error) { + dataStoreConfig, pluginConfigs, err := validateSQLConfig(config) + if err != nil { + return nil, nil, err + } + if dataStoreConfig.DataSource == nil { + dataStoreConfig.DataSource = catalog.FixedData("") } - if sqlConfig.DataSource.IsDynamic() { - config.Log.Warn("DataStore is not reconfigurable even with a dynamic data source") + dsLog := config.Log.WithField(telemetry.SubsystemName, dataStoreConfig.Name) + ds := ds_sql.New(dsLog) + if _, err := catalog.ConfigurePlugin(ctx, coreConfig, ds, dataStoreConfig.DataSource, ""); err != nil { + return nil, nil, err } config.Log.WithField(telemetry.Reconfigurable, false).Info("Configured DataStore") - return ds, nil + return ds, pluginConfigs, nil } diff --git a/pkg/server/catalog/catalog_test.go b/pkg/server/catalog/catalog_test.go index 3358ee336b..14ee9ae453 100644 --- a/pkg/server/catalog/catalog_test.go +++ b/pkg/server/catalog/catalog_test.go @@ -73,7 +73,7 @@ func Test(t *testing.T) { if tt.prepareConfig != nil { tt.prepareConfig(dir, &config) } - repo, err := catalog.Load(context.Background(), config) + repo, err := catalog.Load(context.Background(), &config) if repo != nil { repo.Close() } diff --git a/pkg/server/config.go b/pkg/server/config.go index fdbef83671..221b73e75e 100644 --- a/pkg/server/config.go +++ b/pkg/server/config.go @@ -3,6 +3,7 @@ package server import ( "context" "crypto/x509/pkix" + "fmt" "net" "time" @@ -120,6 +121,15 @@ type Config struct { // calculation (prefer the TTL passed by the downstream caller, then fall // back to the default X509 CA TTL). UseLegacyDownstreamX509CATTL bool + + // load this server, validate configurations, and then shutdown + ValidateOnly bool + + // notes about the configuration + ValidationNotes []string + + // the first validation error message + ValidationError string } type ExperimentalConfig struct { @@ -133,8 +143,27 @@ type FederationConfig struct { FederatesWith map[spiffeid.TrustDomain]bundle_client.TrustDomainConfig } -func New(config Config) *Server { +func New(config *Config) *Server { return &Server{ config: config, } } + +func (c *Config) ReportInfo(message string) { + c.ValidationNotes = append(c.ValidationNotes, message) +} + +func (c *Config) ReportInfof(format string, args ...any) { + c.ReportInfo(fmt.Sprintf(format, args...)) +} + +func (c *Config) ReportError(message string) { + if c.ValidationError == "" { + c.ValidationError = message + } + c.ValidationNotes = append(c.ValidationNotes, message) +} + +func (c *Config) ReportErrorf(format string, args ...any) { + c.ReportError(fmt.Sprintf(format, args...)) +} diff --git a/pkg/server/datastore/sqlstore/sqlstore.go b/pkg/server/datastore/sqlstore/sqlstore.go index 76c4f3ed5c..97162a477a 100644 --- a/pkg/server/datastore/sqlstore/sqlstore.go +++ b/pkg/server/datastore/sqlstore/sqlstore.go @@ -24,6 +24,7 @@ import ( "github.com/spiffe/go-spiffe/v2/spiffeid" "github.com/spiffe/spire-api-sdk/proto/spire/api/types" "github.com/spiffe/spire/pkg/common/bundleutil" + "github.com/spiffe/spire/pkg/common/catalog" "github.com/spiffe/spire/pkg/common/protoutil" "github.com/spiffe/spire/pkg/common/telemetry" "github.com/spiffe/spire/pkg/common/x509util" @@ -90,6 +91,9 @@ type configuration struct { LogSQL bool `hcl:"log_sql" json:"log_sql"` } +type PluginAdapater struct { +} + type dbTypeConfig struct { AWSMySQL *awsConfig `hcl:"aws_mysql" json:"aws_mysql"` AWSPostgres *awsConfig `hcl:"aws_postgres" json:"aws_postgres"` @@ -834,7 +838,7 @@ checkAuthorities: // Configure parses HCL config payload into config struct, opens new DB based on the result, and // prunes all orphaned records -func (ds *Plugin) Configure(_ context.Context, hclConfiguration string) error { +func (ds *Plugin) Configure(_ context.Context, _ catalog.CoreConfig, hclConfiguration string) error { config := &configuration{} if err := hcl.Decode(config, hclConfiguration); err != nil { return err @@ -854,6 +858,10 @@ func (ds *Plugin) Configure(_ context.Context, hclConfiguration string) error { return ds.openConnections(config) } +func (ds *Plugin) Validate(_ context.Context, _ catalog.CoreConfig, hclConfiguration string) (*catalog.ValidateResult, error) { + return nil, nil +} + func (ds *Plugin) openConnections(config *configuration) error { ds.mu.Lock() defer ds.mu.Unlock() diff --git a/pkg/server/endpoints/bundle/internal/acmetest/ca.go b/pkg/server/endpoints/bundle/internal/acmetest/ca.go index 69fa8bff38..d226c7ed01 100644 --- a/pkg/server/endpoints/bundle/internal/acmetest/ca.go +++ b/pkg/server/endpoints/bundle/internal/acmetest/ca.go @@ -33,7 +33,7 @@ // appropriately by the SPIRE KeyManager signers. // - Fails new-reg requests if the terms-of-service has not been accepted -//nolint // forked code +// nolint // forked code package acmetest import ( diff --git a/pkg/server/endpoints/bundle/internal/autocert/listener.go b/pkg/server/endpoints/bundle/internal/autocert/listener.go index 0e37e8752f..ea9f8c8340 100644 --- a/pkg/server/endpoints/bundle/internal/autocert/listener.go +++ b/pkg/server/endpoints/bundle/internal/autocert/listener.go @@ -26,7 +26,7 @@ // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -//nolint // forked code +// nolint // forked code package autocert import ( @@ -45,7 +45,7 @@ import ( // // It enables one-line HTTPS servers: // -// log.Fatal(http.Serve(autocert.NewListener("example.com"), handler)) +// log.Fatal(http.Serve(autocert.NewListener("example.com"), handler)) // // NewListener is a convenience function for a common configuration. // More complex or custom configurations can use the autocert.Manager diff --git a/pkg/server/server.go b/pkg/server/server.go index 27db8ca41b..dfd7ce9b88 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -55,7 +55,7 @@ const ( ) type Server struct { - config Config + config *Config } // Run the server @@ -115,8 +115,14 @@ func (s *Server) run(ctx context.Context) (err error) { // until the call to SetDeps() below. agentStore := agentstore.New() - cat, err := s.loadCatalog(ctx, metrics, identityProvider, agentStore, healthChecker) - if err != nil { + catalogConfig := s.newCatalogConfig(metrics, identityProvider, agentStore, healthChecker) + catalogConfig.ValidateOnly = s.config.ValidateOnly + cat, err := catalog.Load(ctx, catalogConfig) + s.config.ValidationNotes = append(s.config.ValidationNotes, catalogConfig.ValidationNotes...) + if s.config.ValidationError == "" { + s.config.ValidationError = catalogConfig.ValidationError + } + if err != nil || s.config.ValidateOnly == true { return err } defer cat.Close() @@ -282,17 +288,17 @@ func (s *Server) setupProfiling(ctx context.Context) (stop func()) { } } -func (s *Server) loadCatalog(ctx context.Context, metrics telemetry.Metrics, identityProvider *identityprovider.IdentityProvider, agentStore *agentstore.AgentStore, - healthChecker health.Checker) (*catalog.Repository, error) { - return catalog.Load(ctx, catalog.Config{ - Log: s.config.Log.WithField(telemetry.SubsystemName, telemetry.Catalog), +func (s *Server) newCatalogConfig(metrics telemetry.Metrics, identityProvider *identityprovider.IdentityProvider, agentStore *agentstore.AgentStore, healthChecker health.Checker) *catalog.Config { + return &catalog.Config{ + Log: s.config.Log.WithField(telemetry.SubsystemName, telemetry.Catalog), + TrustDomain: s.config.TrustDomain, + PluginConfigs: s.config.PluginConfigs, + Metrics: metrics, - TrustDomain: s.config.TrustDomain, - PluginConfigs: s.config.PluginConfigs, IdentityProvider: identityProvider, AgentStore: agentStore, HealthChecker: healthChecker, - }) + } } func (s *Server) newCredBuilder(cat catalog.Catalog) (*credtemplate.Builder, error) { From 7c42e8f06aef315a7478ea908d34b1751bb73a1a Mon Sep 17 00:00:00 2001 From: Edwin Buck Date: Tue, 26 Nov 2024 11:36:21 -0800 Subject: [PATCH 02/12] Fix a few unit tests. Signed-off-by: Edwin Buck --- pkg/server/cache/entrycache/fullcache_test.go | 5 ++++- pkg/server/server_test.go | 2 +- test/fakes/fakedatastore/fakedatastore.go | 5 ++++- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/pkg/server/cache/entrycache/fullcache_test.go b/pkg/server/cache/entrycache/fullcache_test.go index 19122edc4e..705eb4a0bd 100644 --- a/pkg/server/cache/entrycache/fullcache_test.go +++ b/pkg/server/cache/entrycache/fullcache_test.go @@ -15,6 +15,7 @@ import ( "github.com/sirupsen/logrus/hooks/test" "github.com/spiffe/go-spiffe/v2/spiffeid" "github.com/spiffe/spire-api-sdk/proto/spire/api/types" + "github.com/spiffe/spire/pkg/common/catalog" "github.com/spiffe/spire/pkg/server/api" "github.com/spiffe/spire/pkg/server/datastore" sqlds "github.com/spiffe/spire/pkg/server/datastore/sqlstore" @@ -789,7 +790,9 @@ func newSQLPlugin(ctx context.Context, tb testing.TB) datastore.DataStore { require.FailNowf(tb, "Unsupported external test dialect %q", TestDialect) } - err := p.Configure(ctx, cfg) + err := p.Configure(ctx, catalog.CoreConfig{ + TrustDomain: spiffeid.RequireTrustDomainFromString("example.org"), + }, cfg) require.NoError(tb, err) return p diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index 1eb06ef237..7b4a7f4240 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -32,7 +32,7 @@ func (suite *ServerTestSuite) SetupTest() { logger.Out = suite.stdout logger.Level = logrusLevel - suite.server = New(Config{ + suite.server = New(&Config{ Log: logger, TrustDomain: spiffeid.RequireTrustDomainFromString("example.org"), }) diff --git a/test/fakes/fakedatastore/fakedatastore.go b/test/fakes/fakedatastore/fakedatastore.go index 13728a13ea..cfa21693ea 100644 --- a/test/fakes/fakedatastore/fakedatastore.go +++ b/test/fakes/fakedatastore/fakedatastore.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/require" "github.com/spiffe/go-spiffe/v2/spiffeid" + "github.com/spiffe/spire/pkg/common/catalog" "github.com/spiffe/spire-api-sdk/proto/spire/api/types" "github.com/spiffe/spire/pkg/common/util" "github.com/spiffe/spire/pkg/server/datastore" @@ -38,7 +39,9 @@ func New(tb testing.TB) *DataStore { ds := sql.New(log) ds.SetUseServerTimestamps(true) - err := ds.Configure(ctx, fmt.Sprintf(` + err := ds.Configure(ctx, catalog.CoreConfig{ + TrustDomain: spiffeid.RequireTrustDomainFromString("example.org"), + }, fmt.Sprintf(` database_type = "sqlite3" connection_string = "file:memdb%d?mode=memory&cache=shared" `, atomic.AddUint32(&nextID, 1))) From 4fccfd2e2b5d8838d7280fb39ff4f6bcc71ae9f1 Mon Sep 17 00:00:00 2001 From: Edwin Buck Date: Tue, 26 Nov 2024 11:52:55 -0800 Subject: [PATCH 03/12] Fix datastore unit tests not that sql datastores require trust domains. Signed-off-by: Edwin Buck --- .../datastore/sqlstore/sqlstore_test.go | 41 ++++++++++++++----- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/pkg/server/datastore/sqlstore/sqlstore_test.go b/pkg/server/datastore/sqlstore/sqlstore_test.go index e0d11ab020..ed60f77af8 100644 --- a/pkg/server/datastore/sqlstore/sqlstore_test.go +++ b/pkg/server/datastore/sqlstore/sqlstore_test.go @@ -24,6 +24,7 @@ import ( "github.com/spiffe/go-spiffe/v2/spiffeid" "github.com/spiffe/spire-api-sdk/proto/spire/api/types" "github.com/spiffe/spire/pkg/common/bundleutil" + "github.com/spiffe/spire/pkg/common/catalog" "github.com/spiffe/spire/pkg/common/protoutil" "github.com/spiffe/spire/pkg/common/telemetry" "github.com/spiffe/spire/pkg/common/util" @@ -140,7 +141,9 @@ func (s *PluginSuite) newPlugin() *Plugin { case "": s.nextID++ dbPath := filepath.ToSlash(filepath.Join(s.dir, fmt.Sprintf("db%d.sqlite3", s.nextID))) - err := ds.Configure(ctx, fmt.Sprintf(` + err := ds.Configure(ctx, catalog.CoreConfig{ + TrustDomain: spiffeid.RequireTrustDomainFromString("example.org"), + }, fmt.Sprintf(` database_type = "sqlite3" log_sql = true connection_string = "%s" @@ -164,7 +167,9 @@ func (s *PluginSuite) newPlugin() *Plugin { s.T().Logf("CONN STRING: %q", TestConnString) s.Require().NotEmpty(TestConnString, "connection string must be set") wipeMySQL(s.T(), TestConnString) - err := ds.Configure(ctx, fmt.Sprintf(` + err := ds.Configure(ctx, catalog.CoreConfig{ + TrustDomain: spiffeid.RequireTrustDomainFromString("example.org"), + }, fmt.Sprintf(` database_type = "mysql" log_sql = true connection_string = "%s" @@ -175,7 +180,9 @@ func (s *PluginSuite) newPlugin() *Plugin { s.T().Logf("CONN STRING: %q", TestConnString) s.Require().NotEmpty(TestConnString, "connection string must be set") wipePostgres(s.T(), TestConnString) - err := ds.Configure(ctx, fmt.Sprintf(` + err := ds.Configure(ctx, catalog.CoreConfig{ + TrustDomain: spiffeid.RequireTrustDomainFromString("example.org"), + }, fmt.Sprintf(` database_type = "postgres" log_sql = true connection_string = "%s" @@ -190,7 +197,9 @@ func (s *PluginSuite) newPlugin() *Plugin { } func (s *PluginSuite) TestInvalidPluginConfiguration() { - err := s.ds.Configure(ctx, ` + err := s.ds.Configure(ctx, catalog.CoreConfig{ + TrustDomain: spiffeid.RequireTrustDomainFromString("example.org"), + }, ` database_type = "wrong" connection_string = "bad" `) @@ -220,26 +229,34 @@ func (s *PluginSuite) TestInvalidAWSConfiguration() { } for _, testCase := range testCases { s.T().Run(testCase.name, func(t *testing.T) { - err := s.ds.Configure(ctx, testCase.config) + err := s.ds.Configure(ctx, catalog.CoreConfig{ + TrustDomain: spiffeid.RequireTrustDomainFromString("example.org"), + }, testCase.config) s.RequireErrorContains(err, testCase.expectedErr) }) } } func (s *PluginSuite) TestInvalidMySQLConfiguration() { - err := s.ds.Configure(ctx, ` + err := s.ds.Configure(ctx, catalog.CoreConfig{ + TrustDomain: spiffeid.RequireTrustDomainFromString("example.org"), + }, ` database_type = "mysql" connection_string = "username:@tcp(127.0.0.1)/spire_test" `) s.RequireErrorContains(err, "datastore-sql: invalid mysql config: missing parseTime=true param in connection_string") - err = s.ds.Configure(ctx, ` + err = s.ds.Configure(ctx, catalog.CoreConfig{ + TrustDomain: spiffeid.RequireTrustDomainFromString("example.org"), + }, ` database_type = "mysql" ro_connection_string = "username:@tcp(127.0.0.1)/spire_test" `) s.RequireErrorContains(err, "datastore-sql: connection_string must be set") - err = s.ds.Configure(ctx, ` + err = s.ds.Configure(ctx, catalog.CoreConfig{ + TrustDomain: spiffeid.RequireTrustDomainFromString("example.org"), + }, ` database_type = "mysql" `) s.RequireErrorContains(err, "datastore-sql: connection_string must be set") @@ -4924,7 +4941,9 @@ func (s *PluginSuite) TestMigration() { dump = minimalDB() } dumpDB(t, dbPath, dump) - err := s.ds.Configure(ctx, fmt.Sprintf(` + err := s.ds.Configure(ctx, catalog.CoreConfig{ + TrustDomain: spiffeid.RequireTrustDomainFromString("example.org"), + }, fmt.Sprintf(` database_type = "sqlite3" connection_string = %q `, dbURI)) @@ -5230,7 +5249,9 @@ func (s *PluginSuite) TestConfigure() { log, _ := test.NewNullLogger() p := New(log) - err := p.Configure(ctx, fmt.Sprintf(` + err := p.Configure(ctx, catalog.CoreConfig{ + TrustDomain: spiffeid.RequireTrustDomainFromString("example.org"), + }, fmt.Sprintf(` database_type = "sqlite3" log_sql = true connection_string = "%s" From 40c57bf79408f26b50792e7af713d54d229a5ca2 Mon Sep 17 00:00:00 2001 From: Edwin Buck Date: Wed, 27 Nov 2024 04:08:42 -0800 Subject: [PATCH 04/12] Fix the last unit test. This unit test crashes the server with an intentinally bad config. The problem was, it crashed in the wrong location. Signed-off-by: Edwin Buck --- pkg/server/catalog/catalog.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/server/catalog/catalog.go b/pkg/server/catalog/catalog.go index 44402ba4c5..699fdf2023 100644 --- a/pkg/server/catalog/catalog.go +++ b/pkg/server/catalog/catalog.go @@ -229,7 +229,9 @@ func validateSQLConfig(config *Config) (catalog.PluginConfig, PluginConfigs, err config.ReportErrorf("pluggability for the DataStore is deprecated; only the built-in %q plugin is supported", ds_sql.PluginName) return catalog.PluginConfig{}, PluginConfigs(nil), fmt.Errorf("pluggability for the DataStore is deprecated; only the built-in %q plugin is supported", ds_sql.PluginName) } - if datastoreConfig.DataSource.IsDynamic() { + if datastoreConfig.DataSource == nil { + config.ReportError("internal: DataStore is missing a configuration data source") + } else if datastoreConfig.DataSource.IsDynamic() { config.ReportInfo("DataStore is not reconfigurable even with a dynamic data source") } From 800f8905af265c75424328b8ba10d99c46051693 Mon Sep 17 00:00:00 2001 From: Edwin Buck Date: Wed, 27 Nov 2024 04:14:04 -0800 Subject: [PATCH 05/12] Fix linting issues. Fix import order on fakedatastore.go Comparison on boolean only, not boolean == true on server.go remove whitespace in "// nolint" for listener.go, ca.go, which was just added due to baseline merge conflict. Signed-off-by: Edwin Buck --- pkg/server/endpoints/bundle/internal/acmetest/ca.go | 2 +- pkg/server/endpoints/bundle/internal/autocert/listener.go | 2 +- pkg/server/server.go | 2 +- test/fakes/fakedatastore/fakedatastore.go | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/server/endpoints/bundle/internal/acmetest/ca.go b/pkg/server/endpoints/bundle/internal/acmetest/ca.go index d226c7ed01..69fa8bff38 100644 --- a/pkg/server/endpoints/bundle/internal/acmetest/ca.go +++ b/pkg/server/endpoints/bundle/internal/acmetest/ca.go @@ -33,7 +33,7 @@ // appropriately by the SPIRE KeyManager signers. // - Fails new-reg requests if the terms-of-service has not been accepted -// nolint // forked code +//nolint // forked code package acmetest import ( diff --git a/pkg/server/endpoints/bundle/internal/autocert/listener.go b/pkg/server/endpoints/bundle/internal/autocert/listener.go index ea9f8c8340..8bd45396f2 100644 --- a/pkg/server/endpoints/bundle/internal/autocert/listener.go +++ b/pkg/server/endpoints/bundle/internal/autocert/listener.go @@ -26,7 +26,7 @@ // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -// nolint // forked code +//nolint // forked code package autocert import ( diff --git a/pkg/server/server.go b/pkg/server/server.go index dfd7ce9b88..9ccb97236d 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -122,7 +122,7 @@ func (s *Server) run(ctx context.Context) (err error) { if s.config.ValidationError == "" { s.config.ValidationError = catalogConfig.ValidationError } - if err != nil || s.config.ValidateOnly == true { + if err != nil || s.config.ValidateOnly { return err } defer cat.Close() diff --git a/test/fakes/fakedatastore/fakedatastore.go b/test/fakes/fakedatastore/fakedatastore.go index cfa21693ea..ef2a8ebd87 100644 --- a/test/fakes/fakedatastore/fakedatastore.go +++ b/test/fakes/fakedatastore/fakedatastore.go @@ -12,8 +12,8 @@ import ( "github.com/stretchr/testify/require" "github.com/spiffe/go-spiffe/v2/spiffeid" - "github.com/spiffe/spire/pkg/common/catalog" "github.com/spiffe/spire-api-sdk/proto/spire/api/types" + "github.com/spiffe/spire/pkg/common/catalog" "github.com/spiffe/spire/pkg/common/util" "github.com/spiffe/spire/pkg/server/datastore" sql "github.com/spiffe/spire/pkg/server/datastore/sqlstore" From a005a10117beb2b79b0602ff782727be80d43a8b Mon Sep 17 00:00:00 2001 From: Edwin Buck Date: Wed, 27 Nov 2024 04:32:37 -0800 Subject: [PATCH 06/12] Fix windows unit test. Signed-off-by: Edwin Buck --- pkg/common/catalog/plugin.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/common/catalog/plugin.go b/pkg/common/catalog/plugin.go index 118e60b738..7804ddeb91 100644 --- a/pkg/common/catalog/plugin.go +++ b/pkg/common/catalog/plugin.go @@ -151,7 +151,6 @@ func (p *pluginImpl) bindRepo(repo bindableServiceRepo, grpcServiceNames map[str if impl != nil { continue } - p.log.Infof("found implementation for service %s", facade.GRPCServiceName()) warnIfDeprecated(p.log, version, versions[0]) impl = p.bindFacade(repo, facade) } From afe62633e3b6c1a8fe0779a444fa9af13097a5ff Mon Sep 17 00:00:00 2001 From: Edwin Buck Date: Tue, 3 Dec 2024 09:52:45 -0800 Subject: [PATCH 07/12] Remove plugin load / binding logging, per review comments. Signed-off-by: Edwin Buck --- pkg/common/catalog/catalog.go | 7 ------- pkg/common/catalog/plugin.go | 2 -- 2 files changed, 9 deletions(-) diff --git a/pkg/common/catalog/catalog.go b/pkg/common/catalog/catalog.go index 4490aa43a3..4efa946083 100644 --- a/pkg/common/catalog/catalog.go +++ b/pkg/common/catalog/catalog.go @@ -181,20 +181,16 @@ func Load(ctx context.Context, config *Config, repo Repository) (_ *Catalog, err if err != nil { return nil, err } - log.Infof("bindablePluginRepos: %+v", pluginRepos) serviceRepos, err := makeBindableServiceRepos(repo.Services()) if err != nil { return nil, err } - log.Infof("bindableServiceRepos: %+v", serviceRepos) pluginCounts := make(map[string]int) var reconfigurers Reconfigurers for _, pluginConfig := range config.PluginConfigs { - log.Infof("plugin(%s): processing", pluginConfig.Name) - pluginLog := makePluginLog(config.Log, pluginConfig) pluginRepo, ok := pluginRepos[pluginConfig.Type] @@ -205,7 +201,6 @@ func Load(ctx context.Context, config *Config, repo Repository) (_ *Catalog, err } continue } - log.Infof("plugin(%s): supported", pluginConfig.Name) if pluginConfig.Disabled { pluginLog.Debug("Not loading plugin; disabled") @@ -227,7 +222,6 @@ func Load(ctx context.Context, config *Config, repo Repository) (_ *Catalog, err // panic, etc.) we want the defer above to close the plugin. Failure to // do so can orphan external plugin processes. closers = append(closers, pluginCloser{plugin: plugin, log: pluginLog}) - log.Infof("plugin(%s): loaded", pluginConfig.Name) configurer, err := plugin.bindRepos(pluginRepo, serviceRepos) if err != nil { @@ -237,7 +231,6 @@ func Load(ctx context.Context, config *Config, repo Repository) (_ *Catalog, err return nil, fmt.Errorf("failed to bind plugin %q: %w", pluginConfig.Name, err) } } - log.Infof("plugin(%s): bound, configurer %+v", pluginConfig.Name, configurer) if !config.ValidateOnly { reconfigurer, err := configurePlugin(ctx, pluginLog, config.CoreConfig, configurer, pluginConfig.DataSource) diff --git a/pkg/common/catalog/plugin.go b/pkg/common/catalog/plugin.go index 7804ddeb91..c48357fe78 100644 --- a/pkg/common/catalog/plugin.go +++ b/pkg/common/catalog/plugin.go @@ -101,11 +101,9 @@ func (p *pluginImpl) initFacade(facade Facade) any { func (p *pluginImpl) bindRepos(pluginRepo bindablePluginRepo, serviceRepos []bindableServiceRepo) (Configurer, error) { grpcServiceNames := grpcServiceNameSet(p.grpcServiceNames) - p.log.Infof("plugin %q binding repos", p.info.Name()) impl := p.bindRepo(pluginRepo, grpcServiceNames) for _, serviceRepo := range serviceRepos { - p.log.Infof("plugin %q binding repo %+v", p.info.Name(), serviceRepo) p.bindRepo(serviceRepo, grpcServiceNames) } From 7064969122a10449ac03ded4971e23a1d897c7da Mon Sep 17 00:00:00 2001 From: Edwin Buck Date: Tue, 3 Dec 2024 10:19:10 -0800 Subject: [PATCH 08/12] Remove unused logger, now that plugin loading logging has been removed. Signed-off-by: Edwin Buck --- pkg/common/catalog/catalog.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pkg/common/catalog/catalog.go b/pkg/common/catalog/catalog.go index 4efa946083..624f9a6506 100644 --- a/pkg/common/catalog/catalog.go +++ b/pkg/common/catalog/catalog.go @@ -173,10 +173,6 @@ func Load(ctx context.Context, config *Config, repo Repository) (_ *Catalog, err } }() - log := config.Log.WithFields(logrus.Fields{ - telemetry.SubsystemName: "common_catalog", - }) - pluginRepos, err := makeBindablePluginRepos(repo.Plugins()) if err != nil { return nil, err From 4995563f4ed85e65d274b6012837d05614ca40e4 Mon Sep 17 00:00:00 2001 From: Edwin Buck Date: Tue, 3 Dec 2024 12:54:16 -0800 Subject: [PATCH 09/12] Minimal unit testing for spire-server validate. Tests include: 1. One minimal config file that should succeed. 2. One minimal config file that has an error in the disk keymanager. Signed-off-by: Edwin Buck --- .../cli/validate/validate_test.go | 10 ++++++ .../server_bad_disk_keymanager_plugin.conf | 34 +++++++++++++++++++ test/fixture/config/server_good_basic.conf | 33 ++++++++++++++++++ 3 files changed, 77 insertions(+) create mode 100644 test/fixture/config/server_bad_disk_keymanager_plugin.conf create mode 100644 test/fixture/config/server_good_basic.conf diff --git a/cmd/spire-server/cli/validate/validate_test.go b/cmd/spire-server/cli/validate/validate_test.go index 06d0af73d9..a39e2a2ba1 100644 --- a/cmd/spire-server/cli/validate/validate_test.go +++ b/cmd/spire-server/cli/validate/validate_test.go @@ -54,3 +54,13 @@ func (s *ValidateSuite) TestBadFlags() { s.Equal("", s.stdout.String(), "stdout") s.Contains(s.stderr.String(), "flag provided but not defined: -badflag") } + +func (s *ValidateSuite) TestValidate() { + code := s.cmd.Run([]string{"-config", "../../../../test/fixture/config/server_good_basic.conf"}) + s.Equal(0, code, "exit code") +} + +func (s *ValidateSuite) TestValidateFails() { + code := s.cmd.Run([]string{"-config", "../../../../test/fixture/config/server_bad_disk_keymanager_plugin.conf"}) + s.Equal(1, code, "exit code") +} diff --git a/test/fixture/config/server_bad_disk_keymanager_plugin.conf b/test/fixture/config/server_bad_disk_keymanager_plugin.conf new file mode 100644 index 0000000000..27c6af3c23 --- /dev/null +++ b/test/fixture/config/server_bad_disk_keymanager_plugin.conf @@ -0,0 +1,34 @@ +server { + bind_address = "127.0.0.1" + bind_port = "8081" + socket_path = "/tmp/spire-server/private/api.sock" + trust_domain = "example.org" + data_dir = "./.data" + log_level = "DEBUG" +} + +plugins { + DataStore "sql" { + plugin_data { + database_type = "sqlite3" + connection_string = "./.data/datastore2.sqlite3" + } + } + + NodeAttestor "join_token" { + plugin_data { + } + } + + # This configuration has an error: Disk keymanagers must have a set keys_path + KeyManager "disk" { + plugin_data = {} + } + + UpstreamAuthority "disk" { + plugin_data { + key_file_path = "./conf/server/dummy_upstream_ca.key" + cert_file_path = "./conf/server/dummy_upstream_ca.crt" + } + } +} diff --git a/test/fixture/config/server_good_basic.conf b/test/fixture/config/server_good_basic.conf new file mode 100644 index 0000000000..a21d53b93a --- /dev/null +++ b/test/fixture/config/server_good_basic.conf @@ -0,0 +1,33 @@ +server { + bind_address = "127.0.0.1" + bind_port = "8081" + socket_path = "/tmp/spire-server/private/api.sock" + trust_domain = "example.org" + data_dir = "./.data" + log_level = "DEBUG" +} + +plugins { + DataStore "sql" { + plugin_data { + database_type = "sqlite3" + connection_string = "./.data/datastore.sqlite3" + } + } + + NodeAttestor "join_token" { + plugin_data { + } + } + + KeyManager "memory" { + plugin_data = {} + } + + UpstreamAuthority "disk" { + plugin_data { + key_file_path = "./conf/server/dummy_upstream_ca.key" + cert_file_path = "./conf/server/dummy_upstream_ca.crt" + } + } +} From c0615b988a89a83f40f42b4420d44a5c7026181b Mon Sep 17 00:00:00 2001 From: Edwin Buck Date: Wed, 4 Dec 2024 07:23:12 -0800 Subject: [PATCH 10/12] Remove logger output for validat launch. The logging output is redirected to /dev/null, so the actual validation output can more easily be read. Signed-off-by: Edwin Buck --- cmd/spire-server/cli/cli.go | 2 +- cmd/spire-server/cli/validate/validate.go | 10 ++++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/cmd/spire-server/cli/cli.go b/cmd/spire-server/cli/cli.go index 034ee4ea1c..dfdb5ec202 100644 --- a/cmd/spire-server/cli/cli.go +++ b/cmd/spire-server/cli/cli.go @@ -125,7 +125,7 @@ func (cc *CLI) Run(ctx context.Context, args []string) int { return jwt.NewMintCommand(), nil }, "validate": func() (cli.Command, error) { - return validate.NewValidateCommand(ctx, cc.LogOptions), nil + return validate.NewValidateCommand(ctx), nil }, "localauthority x509 show": func() (cli.Command, error) { return localauthority_x509.NewX509ShowCommand(), nil diff --git a/cmd/spire-server/cli/validate/validate.go b/cmd/spire-server/cli/validate/validate.go index 3cf6c1fef0..c59446d700 100644 --- a/cmd/spire-server/cli/validate/validate.go +++ b/cmd/spire-server/cli/validate/validate.go @@ -16,15 +16,17 @@ import ( const commandName = "validate" -func NewValidateCommand(ctx context.Context, logOptions []log.Option) cli.Command { - return newValidateCommand(ctx, commoncli.DefaultEnv, logOptions) +func NewValidateCommand(ctx context.Context) cli.Command { + return newValidateCommand(ctx, commoncli.DefaultEnv) } -func newValidateCommand(ctx context.Context, env *commoncli.Env, logOptions []log.Option) *validateCommand { +func newValidateCommand(ctx context.Context, env *commoncli.Env) *validateCommand { return &validateCommand{ ctx: ctx, env: env, - logOptions: logOptions, + logOptions: []log.Option{ + log.WithOutputFile("/dev/null"), + }, } } From d05c2d164699be6fa05965807e4f021bba1f9575 Mon Sep 17 00:00:00 2001 From: Edwin Buck Date: Wed, 4 Dec 2024 08:32:03 -0800 Subject: [PATCH 11/12] Remove error output for all Validate() plugin commands. This permits getting the entire output of the notes, and aligns with the desired output in the code review. As we don't know which plugin note was the failure, an additional note is added when the plugin's configuration is invalid. Signed-off-by: Edwin Buck --- pkg/common/catalog/configure.go | 6 +----- .../bundlepublisher/awsrolesanywhere/awsrolesanywhere.go | 2 +- pkg/server/plugin/bundlepublisher/awss3/awss3.go | 2 +- .../bundlepublisher/gcpcloudstorage/gcpcloudstorage.go | 2 +- pkg/server/plugin/keymanager/awskms/awskms.go | 2 +- pkg/server/plugin/keymanager/disk/disk.go | 2 +- pkg/server/plugin/nodeattestor/jointoken/join_token.go | 2 +- pkg/server/plugin/nodeattestor/k8spsat/psat.go | 2 +- pkg/server/plugin/nodeattestor/k8ssat/sat.go | 2 +- pkg/server/plugin/nodeattestor/tpmdevid/devid.go | 2 +- pkg/server/plugin/nodeattestor/x509pop/x509pop.go | 2 +- pkg/server/plugin/notifier/gcsbundle/gcsbundle.go | 2 +- pkg/server/plugin/notifier/k8sbundle/k8sbundle.go | 2 +- pkg/server/plugin/upstreamauthority/awspca/pca.go | 2 +- pkg/server/plugin/upstreamauthority/awssecret/awssecret.go | 2 +- .../plugin/upstreamauthority/certmanager/certmanager.go | 2 +- pkg/server/plugin/upstreamauthority/disk/disk.go | 2 +- pkg/server/plugin/upstreamauthority/gcpcas/gcpcas.go | 2 +- pkg/server/plugin/upstreamauthority/spire/spire.go | 2 +- pkg/server/plugin/upstreamauthority/vault/vault.go | 2 +- 20 files changed, 20 insertions(+), 24 deletions(-) diff --git a/pkg/common/catalog/configure.go b/pkg/common/catalog/configure.go index 51ecf4d498..d086cd45da 100644 --- a/pkg/common/catalog/configure.go +++ b/pkg/common/catalog/configure.go @@ -85,7 +85,6 @@ func ValidatePlugin(ctx context.Context, coreConfig CoreConfig, configurer Confi status.ReportErrorf("failed to load plugin data: %s", err.Error()) return status, err } - status.ReportInfo("") return configurer.Validate(ctx, coreConfig, data) } @@ -232,12 +231,9 @@ func (v1 *configurerV1) Validate(ctx context.Context, coreConfig CoreConfig, hcl HclConfiguration: hclConfiguration, }) result := NewValidateResult() - // TODO: The GRPC default behavior is to not return a *ValidateResponse when there is an error, and that destroys many - // of the validation messages, except for the error message. We may wish to not consider validation errors - // as errors going forward. result.Notes = response.GetNotes() if !response.GetValid() { - result.ReportErrorf("invalid configuration: %s", err.Error()) + result.ReportError("plugin configuration is invalid") } return result, err } diff --git a/pkg/server/plugin/bundlepublisher/awsrolesanywhere/awsrolesanywhere.go b/pkg/server/plugin/bundlepublisher/awsrolesanywhere/awsrolesanywhere.go index f43ede99ff..0564b34e32 100644 --- a/pkg/server/plugin/bundlepublisher/awsrolesanywhere/awsrolesanywhere.go +++ b/pkg/server/plugin/bundlepublisher/awsrolesanywhere/awsrolesanywhere.go @@ -114,7 +114,7 @@ func (p *Plugin) Validate(ctx context.Context, req *configv1.ValidateRequest) (* return &configv1.ValidateResponse{ Valid: err == nil, Notes: notes, - }, err + }, nil } // PublishBundle puts the bundle in the Roles Anywhere trust anchor, with diff --git a/pkg/server/plugin/bundlepublisher/awss3/awss3.go b/pkg/server/plugin/bundlepublisher/awss3/awss3.go index 59c64a811f..df933b08a8 100644 --- a/pkg/server/plugin/bundlepublisher/awss3/awss3.go +++ b/pkg/server/plugin/bundlepublisher/awss3/awss3.go @@ -139,7 +139,7 @@ func (p *Plugin) Validate(ctx context.Context, req *configv1.ValidateRequest) (* return &configv1.ValidateResponse{ Valid: err == nil, Notes: notes, - }, err + }, nil } // PublishBundle puts the bundle in the configured S3 bucket name and diff --git a/pkg/server/plugin/bundlepublisher/gcpcloudstorage/gcpcloudstorage.go b/pkg/server/plugin/bundlepublisher/gcpcloudstorage/gcpcloudstorage.go index 4875b8c720..26f6465539 100644 --- a/pkg/server/plugin/bundlepublisher/gcpcloudstorage/gcpcloudstorage.go +++ b/pkg/server/plugin/bundlepublisher/gcpcloudstorage/gcpcloudstorage.go @@ -138,7 +138,7 @@ func (p *Plugin) Validate(ctx context.Context, req *configv1.ValidateRequest) (* return &configv1.ValidateResponse{ Valid: err == nil, Notes: notes, - }, err + }, nil } // PublishBundle puts the bundle in the configured GCS bucket and object name. diff --git a/pkg/server/plugin/keymanager/awskms/awskms.go b/pkg/server/plugin/keymanager/awskms/awskms.go index f2bdc74d1e..a860ad5dba 100644 --- a/pkg/server/plugin/keymanager/awskms/awskms.go +++ b/pkg/server/plugin/keymanager/awskms/awskms.go @@ -244,7 +244,7 @@ func (p *Plugin) Validate(ctx context.Context, req *configv1.ValidateRequest) (* return &configv1.ValidateResponse{ Valid: err == nil, Notes: notes, - }, err + }, nil } // GenerateKey creates a key in KMS. If a key already exists in the local storage, it is updated. diff --git a/pkg/server/plugin/keymanager/disk/disk.go b/pkg/server/plugin/keymanager/disk/disk.go index 087df5e552..afc9aa4166 100644 --- a/pkg/server/plugin/keymanager/disk/disk.go +++ b/pkg/server/plugin/keymanager/disk/disk.go @@ -91,7 +91,7 @@ func (m *KeyManager) Validate(_ context.Context, req *configv1.ValidateRequest) return &configv1.ValidateResponse{ Valid: err == nil, Notes: notes, - }, err + }, nil } func (m *KeyManager) configure(config *configuration) error { diff --git a/pkg/server/plugin/nodeattestor/jointoken/join_token.go b/pkg/server/plugin/nodeattestor/jointoken/join_token.go index 4237e58172..b5630f526b 100644 --- a/pkg/server/plugin/nodeattestor/jointoken/join_token.go +++ b/pkg/server/plugin/nodeattestor/jointoken/join_token.go @@ -74,5 +74,5 @@ func (p *Plugin) Validate(_ context.Context, req *configv1.ValidateRequest) (*co return &configv1.ValidateResponse{ Valid: err == nil, Notes: notes, - }, err + }, nil } diff --git a/pkg/server/plugin/nodeattestor/k8spsat/psat.go b/pkg/server/plugin/nodeattestor/k8spsat/psat.go index 2b71890ae4..0ddea993bb 100644 --- a/pkg/server/plugin/nodeattestor/k8spsat/psat.go +++ b/pkg/server/plugin/nodeattestor/k8spsat/psat.go @@ -288,7 +288,7 @@ func (p *AttestorPlugin) Validate(ctx context.Context, req *configv1.ValidateReq return &configv1.ValidateResponse{ Valid: err == nil, Notes: notes, - }, err + }, nil } func (p *AttestorPlugin) getConfig() (*attestorConfig, error) { diff --git a/pkg/server/plugin/nodeattestor/k8ssat/sat.go b/pkg/server/plugin/nodeattestor/k8ssat/sat.go index f80045a399..2a3e928082 100644 --- a/pkg/server/plugin/nodeattestor/k8ssat/sat.go +++ b/pkg/server/plugin/nodeattestor/k8ssat/sat.go @@ -356,7 +356,7 @@ func (p *AttestorPlugin) Validate(_ context.Context, req *configv1.ValidateReque return &configv1.ValidateResponse{ Valid: err == nil, Notes: notes, - }, err + }, nil } func (p *AttestorPlugin) getConfig() (*attestorConfig, error) { diff --git a/pkg/server/plugin/nodeattestor/tpmdevid/devid.go b/pkg/server/plugin/nodeattestor/tpmdevid/devid.go index eb55de2e3d..06df053862 100644 --- a/pkg/server/plugin/nodeattestor/tpmdevid/devid.go +++ b/pkg/server/plugin/nodeattestor/tpmdevid/devid.go @@ -242,7 +242,7 @@ func (p *Plugin) Validate(ctx context.Context, req *configv1.ValidateRequest) (* return &configv1.ValidateResponse{ Valid: err == nil, Notes: notes, - }, err + }, nil } func (p *Plugin) getConfiguration() *config { diff --git a/pkg/server/plugin/nodeattestor/x509pop/x509pop.go b/pkg/server/plugin/nodeattestor/x509pop/x509pop.go index d8ba2d7fae..69a6b4ef71 100644 --- a/pkg/server/plugin/nodeattestor/x509pop/x509pop.go +++ b/pkg/server/plugin/nodeattestor/x509pop/x509pop.go @@ -223,7 +223,7 @@ func (p *Plugin) Validate(_ context.Context, req *configv1.ValidateRequest) (*co return &configv1.ValidateResponse{ Valid: err == nil, Notes: notes, - }, err + }, nil } func (p *Plugin) getConfig() (*configuration, error) { diff --git a/pkg/server/plugin/notifier/gcsbundle/gcsbundle.go b/pkg/server/plugin/notifier/gcsbundle/gcsbundle.go index 8756b00368..9341e22f39 100644 --- a/pkg/server/plugin/notifier/gcsbundle/gcsbundle.go +++ b/pkg/server/plugin/notifier/gcsbundle/gcsbundle.go @@ -145,7 +145,7 @@ func (p *Plugin) Validate(_ context.Context, req *configv1.ValidateRequest) (res return &configv1.ValidateResponse{ Valid: err == nil, Notes: notes, - }, err + }, nil } func (p *Plugin) getConfig() (*configuration, error) { diff --git a/pkg/server/plugin/notifier/k8sbundle/k8sbundle.go b/pkg/server/plugin/notifier/k8sbundle/k8sbundle.go index bf2085a458..4fe608d59b 100644 --- a/pkg/server/plugin/notifier/k8sbundle/k8sbundle.go +++ b/pkg/server/plugin/notifier/k8sbundle/k8sbundle.go @@ -175,7 +175,7 @@ func (p *Plugin) Validate(_ context.Context, req *configv1.ValidateRequest) (*co return &configv1.ValidateResponse{ Valid: err == nil, Notes: notes, - }, err + }, nil } // startInformers creates informers to set CA Bundle in objects created after server has started diff --git a/pkg/server/plugin/upstreamauthority/awspca/pca.go b/pkg/server/plugin/upstreamauthority/awspca/pca.go index a4ece3856c..1c29a75077 100644 --- a/pkg/server/plugin/upstreamauthority/awspca/pca.go +++ b/pkg/server/plugin/upstreamauthority/awspca/pca.go @@ -196,7 +196,7 @@ func (p *PCAPlugin) Validate(ctx context.Context, req *configv1.ValidateRequest) return &configv1.ValidateResponse{ Valid: err == nil, Notes: notes, - }, err + }, nil } // MintX509CA mints an X509CA by submitting the CSR to ACM to be signed by the certificate authority diff --git a/pkg/server/plugin/upstreamauthority/awssecret/awssecret.go b/pkg/server/plugin/upstreamauthority/awssecret/awssecret.go index 5bdcca8949..2353e84c09 100644 --- a/pkg/server/plugin/upstreamauthority/awssecret/awssecret.go +++ b/pkg/server/plugin/upstreamauthority/awssecret/awssecret.go @@ -157,7 +157,7 @@ func (p *Plugin) Validate(ctx context.Context, req *configv1.ValidateRequest) (* return &configv1.ValidateResponse{ Valid: err == nil, Notes: notes, - }, err + }, nil } // MintX509CAAndSubscribe mints an X509CA by signing presented CSR with root CA fetched from AWS Secrets Manager diff --git a/pkg/server/plugin/upstreamauthority/certmanager/certmanager.go b/pkg/server/plugin/upstreamauthority/certmanager/certmanager.go index 3c93a775bf..7efb6c1511 100644 --- a/pkg/server/plugin/upstreamauthority/certmanager/certmanager.go +++ b/pkg/server/plugin/upstreamauthority/certmanager/certmanager.go @@ -155,7 +155,7 @@ func (p *Plugin) Validate(_ context.Context, req *configv1.ValidateRequest) (*co return &configv1.ValidateResponse{ Valid: err == nil, Notes: notes, - }, err + }, nil } func (p *Plugin) MintX509CAAndSubscribe(request *upstreamauthorityv1.MintX509CARequest, stream upstreamauthorityv1.UpstreamAuthority_MintX509CAAndSubscribeServer) error { diff --git a/pkg/server/plugin/upstreamauthority/disk/disk.go b/pkg/server/plugin/upstreamauthority/disk/disk.go index 1347e5e17a..e93aec47cd 100644 --- a/pkg/server/plugin/upstreamauthority/disk/disk.go +++ b/pkg/server/plugin/upstreamauthority/disk/disk.go @@ -120,7 +120,7 @@ func (p *Plugin) Validate(_ context.Context, req *configv1.ValidateRequest) (*co return &configv1.ValidateResponse{ Valid: err == nil, Notes: notes, - }, err + }, nil } func (p *Plugin) MintX509CAAndSubscribe(request *upstreamauthorityv1.MintX509CARequest, stream upstreamauthorityv1.UpstreamAuthority_MintX509CAAndSubscribeServer) error { diff --git a/pkg/server/plugin/upstreamauthority/gcpcas/gcpcas.go b/pkg/server/plugin/upstreamauthority/gcpcas/gcpcas.go index d6d4b9dced..ab96a23e77 100644 --- a/pkg/server/plugin/upstreamauthority/gcpcas/gcpcas.go +++ b/pkg/server/plugin/upstreamauthority/gcpcas/gcpcas.go @@ -184,7 +184,7 @@ func (p *Plugin) Validate(_ context.Context, req *configv1.ValidateRequest) (*co return &configv1.ValidateResponse{ Valid: err == nil, Notes: notes, - }, err + }, nil } func (p *Plugin) getConfig() (*Configuration, error) { diff --git a/pkg/server/plugin/upstreamauthority/spire/spire.go b/pkg/server/plugin/upstreamauthority/spire/spire.go index 55b7fbc65c..077b02bade 100644 --- a/pkg/server/plugin/upstreamauthority/spire/spire.go +++ b/pkg/server/plugin/upstreamauthority/spire/spire.go @@ -138,7 +138,7 @@ func (p *Plugin) Validate(_ context.Context, req *configv1.ValidateRequest) (*co return &configv1.ValidateResponse{ Valid: err == nil, Notes: notes, - }, err + }, nil } func (p *Plugin) SetLogger(log hclog.Logger) { diff --git a/pkg/server/plugin/upstreamauthority/vault/vault.go b/pkg/server/plugin/upstreamauthority/vault/vault.go index c2de0a8b45..70d95107e7 100644 --- a/pkg/server/plugin/upstreamauthority/vault/vault.go +++ b/pkg/server/plugin/upstreamauthority/vault/vault.go @@ -186,7 +186,7 @@ func (p *Plugin) Validate(_ context.Context, req *configv1.ValidateRequest) (*co return &configv1.ValidateResponse{ Valid: err == nil, Notes: notes, - }, err + }, nil } func (p *Plugin) MintX509CAAndSubscribe(req *upstreamauthorityv1.MintX509CARequest, stream upstreamauthorityv1.UpstreamAuthority_MintX509CAAndSubscribeServer) error { From 4f2cafab335990975b4fe64a15ca8324f58956cf Mon Sep 17 00:00:00 2001 From: Edwin Buck Date: Tue, 10 Dec 2024 10:40:48 -0800 Subject: [PATCH 12/12] Work to align to Agustin's design goals. Signed-off-by: Edwin Buck --- cmd/spire-server/cli/validate/validate.go | 16 +++-- pkg/common/catalog/catalog.go | 7 +++ pkg/common/validation/validationResult.go | 61 ++++++++++++++++++ pkg/server/catalog/catalog.go | 75 ++++++++++++++++++----- pkg/server/datastore/sqlstore/sqlstore.go | 2 +- 5 files changed, 138 insertions(+), 23 deletions(-) create mode 100644 pkg/common/validation/validationResult.go diff --git a/cmd/spire-server/cli/validate/validate.go b/cmd/spire-server/cli/validate/validate.go index c59446d700..efd04b07b3 100644 --- a/cmd/spire-server/cli/validate/validate.go +++ b/cmd/spire-server/cli/validate/validate.go @@ -68,20 +68,24 @@ func (c *validateCommand) Run(args []string) int { ctx = context.Background() } - err = s.Run(ctx) + result, err := s.ValidateConfig(ctx) if err != nil { - config.Log.WithError(err).Error("Validation failed: validation server crashed") + config.Log.WithError(err).Errorf("Validation failed: %s", err) return 1 } + // required to add Valid json output flag err = c.printer.PrintStruct(&validateResult{ - Valid: config.ValidationError == "", - Error: config.ValidationError, - Notes: config.ValidationNotes, + Valid: result.ValidationError == "", + Error: result.ValidationError, + Notes: result.ValidationNotes, }) + + if err != nil { return 1 } + return 0 } @@ -98,7 +102,7 @@ func (c *validateCommand) prettyPrintValidate(env *commoncli.Env, results ...any return errors.New("unexpected type") } // pretty print error section - if !result.Valid { + if result.Error != "" { if err := env.Printf("Validation error:\n"); err != nil { return err } diff --git a/pkg/common/catalog/catalog.go b/pkg/common/catalog/catalog.go index 624f9a6506..32d8501b4c 100644 --- a/pkg/common/catalog/catalog.go +++ b/pkg/common/catalog/catalog.go @@ -269,6 +269,13 @@ func Load(ctx context.Context, config *Config, repo Repository) (_ *Catalog, err }, nil } +func GetPluginConfigString(c PluginConfig) (string, error) { + if c.DataSource == nil { + return "", nil + } + return c.DataSource.Load() +} + func makePluginLog(log logrus.FieldLogger, pluginConfig PluginConfig) logrus.FieldLogger { return log.WithFields(logrus.Fields{ telemetry.PluginName: pluginConfig.Name, diff --git a/pkg/common/validation/validationResult.go b/pkg/common/validation/validationResult.go new file mode 100644 index 0000000000..1e79210a12 --- /dev/null +++ b/pkg/common/validation/validationResult.go @@ -0,0 +1,61 @@ +package validation + +import ( + "errors" + "fmt" +) + +/* Captures the result of a configuration validation */ +// TODO: make ValidationError validationError +// TODO: make ValidationNotes validationNotes +type ValidationResult struct { + ValidationError string `json:"error"` + ValidationNotes []string `json:"notes"` +} + +func (v *ValidationResult) Error() error { + if v.ValidationError != "" { + return errors.New(v.ValidationError) + } + return nil +} + +func (v *ValidationResult) HasError() bool { + return v.ValidationError != "" +} + +func (v *ValidationResult) ReportError(message string) { + if v.ValidationError == "" { + v.ValidationError = message + } + v.ValidationNotes = append(v.ValidationNotes, message) +} + +func (v *ValidationResult) ReportErrorf(format string, params ...any) { + v.ReportError(fmt.Sprintf(format, params...)) +} + +func (v *ValidationResult) ReportInfo(message string) { + v.ValidationNotes = append(v.ValidationNotes, message) +} + +func (v *ValidationResult) ReportInfof(format string, params ...any) { + v.ReportInfo(fmt.Sprintf(format, params...)) +} + +func (v *ValidationResult) Merge(other ValidationResult) { + if v.ValidationError == "" { + v.ValidationError = other.ValidationError + } + v.ValidationNotes = append(v.ValidationNotes, other.ValidationNotes...) +} + +func (v *ValidationResult) MergeWithFormat(format string, other ValidationResult) { + if v.ValidationError == "" && other.ValidationError != "" { + v.ValidationError = fmt.Sprintf(format, other.ValidationError) + } + for _, note := range other.ValidationNotes { + v.ValidationNotes = append(v.ValidationNotes, fmt.Sprintf(format, note)) + } +} + diff --git a/pkg/server/catalog/catalog.go b/pkg/server/catalog/catalog.go index 699fdf2023..ef79d68bfc 100644 --- a/pkg/server/catalog/catalog.go +++ b/pkg/server/catalog/catalog.go @@ -2,12 +2,12 @@ package catalog import ( "context" - "errors" "fmt" "io" "github.com/andres-erbsen/clock" "github.com/sirupsen/logrus" + "github.com/sirupsen/logrus/hooks/test" "github.com/spiffe/go-spiffe/v2/spiffeid" "github.com/spiffe/spire-plugin-sdk/pluginsdk" metricsv1 "github.com/spiffe/spire-plugin-sdk/proto/spire/hostservice/common/metrics/v1" @@ -17,6 +17,7 @@ import ( "github.com/spiffe/spire/pkg/common/health" "github.com/spiffe/spire/pkg/common/hostservice/metricsservice" "github.com/spiffe/spire/pkg/common/telemetry" + "github.com/spiffe/spire/pkg/common/validation" ds_telemetry "github.com/spiffe/spire/pkg/common/telemetry/server/datastore" km_telemetry "github.com/spiffe/spire/pkg/common/telemetry/server/keymanager" "github.com/spiffe/spire/pkg/server/cache/dscache" @@ -208,40 +209,41 @@ func Load(ctx context.Context, config *Config) (_ *Repository, err error) { return repo, nil } -func validateSQLConfig(config *Config) (catalog.PluginConfig, PluginConfigs, error) { +func validateSQLConfig(config *Config) (catalog.PluginConfig, PluginConfigs, validation.ValidationResult) { + vr := validation.ValidationResult{} datastoreConfigs, pluginConfigs := config.PluginConfigs.FilterByType(dataStoreType) + switch { case len(datastoreConfigs) == 0: - config.ReportError("expecting a DataStore plugin") - return catalog.PluginConfig{}, PluginConfigs(nil), errors.New("expecting a DataStore plugin") + vr.ReportError("expecting a DataStore plugin") case len(datastoreConfigs) > 1: - config.ReportError("only one DataStore plugin is allowed") - return catalog.PluginConfig{}, PluginConfigs(nil), errors.New("only one DataStore plugin is allowed") + vr.ReportError("only one DataStore plugin is allowed") } datastoreConfig := datastoreConfigs[0] if datastoreConfig.Name != ds_sql.PluginName { - config.ReportErrorf("pluggability for the DataStore is deprecated; only the built-in %q plugin is supported", ds_sql.PluginName) - return catalog.PluginConfig{}, PluginConfigs(nil), fmt.Errorf("pluggability for the DataStore is deprecated; only the built-in %q plugin is supported", ds_sql.PluginName) + vr.ReportErrorf("pluggability for the DataStore is deprecated; only the built-in %q plugin is supported", ds_sql.PluginName) } if datastoreConfig.IsExternal() { - config.ReportErrorf("pluggability for the DataStore is deprecated; only the built-in %q plugin is supported", ds_sql.PluginName) - return catalog.PluginConfig{}, PluginConfigs(nil), fmt.Errorf("pluggability for the DataStore is deprecated; only the built-in %q plugin is supported", ds_sql.PluginName) + vr.ReportErrorf("pluggability for the DataStore is deprecated; only the built-in %q plugin is supported", ds_sql.PluginName) } if datastoreConfig.DataSource == nil { - config.ReportError("internal: DataStore is missing a configuration data source") + vr.ReportError("internal: DataStore is missing a configuration data source") } else if datastoreConfig.DataSource.IsDynamic() { - config.ReportInfo("DataStore is not reconfigurable even with a dynamic data source") + vr.ReportInfo("DataStore is not reconfigurable even with a dynamic data source") } - return datastoreConfig, pluginConfigs, nil + if vr.HasError() { + return catalog.PluginConfig{}, PluginConfigs(nil), vr + } + return datastoreConfig, pluginConfigs, vr } func loadSQLDataStore(ctx context.Context, config *Config, coreConfig catalog.CoreConfig) (*ds_sql.Plugin, PluginConfigs, error) { - dataStoreConfig, pluginConfigs, err := validateSQLConfig(config) - if err != nil { - return nil, nil, err + dataStoreConfig, pluginConfigs, result := validateSQLConfig(config) + if result.HasError() { + return nil, nil, result.Error() } if dataStoreConfig.DataSource == nil { dataStoreConfig.DataSource = catalog.FixedData("") @@ -256,3 +258,44 @@ func loadSQLDataStore(ctx context.Context, config *Config, coreConfig catalog.Co config.Log.WithField(telemetry.Reconfigurable, false).Info("Configured DataStore") return ds, pluginConfigs, nil } + +func ValidateConfig(ctx context.Context, config *Config) (*validation.ValidationResult, error) { + validationResult := &validation.ValidationResult{} + if c, ok := config.PluginConfigs.Find(nodeAttestorType, jointoken.PluginName); ok && c.IsEnabled() && c.IsExternal() { + validationResult.ReportError("server catalog: the built-in join_token node attestor cannot be overridden by an external plugin") + // TODO: filter out plugin and continue + } + + nullLogger, _ := test.NewNullLogger() + repo := &Repository{ + log: nullLogger, + } + defer func() { + repo.Close() + }() + + coreConfig := catalog.CoreConfig{ + TrustDomain: config.TrustDomain, + } + + dataStoreConfig, _, result := validateSQLConfig(config) + validationResult.MergeWithFormat("server catalog: %s", result) + if result.HasError() { + return validationResult, nil + } + + ds := ds_sql.New(nullLogger) + dsConfigString, err := dataStoreConfig.DataSource.Load() + if err != nil { + validationResult.ReportError("server catalog: error loading sql datastore plugin config") + } + resp, err := ds.Validate(ctx, coreConfig, dsConfigString) + fmt.Printf("edwin: %+v, config=%s\n", ds, dsConfigString) + fmt.Printf("edwin: %+v, error=%+v\n", resp, err) + + if err != nil { + validationResult.ReportError("server catalog: error validating sql datastore plugin config") + } + + return validationResult, nil +} diff --git a/pkg/server/datastore/sqlstore/sqlstore.go b/pkg/server/datastore/sqlstore/sqlstore.go index 97162a477a..edd1e3cc7d 100644 --- a/pkg/server/datastore/sqlstore/sqlstore.go +++ b/pkg/server/datastore/sqlstore/sqlstore.go @@ -859,7 +859,7 @@ func (ds *Plugin) Configure(_ context.Context, _ catalog.CoreConfig, hclConfigur } func (ds *Plugin) Validate(_ context.Context, _ catalog.CoreConfig, hclConfiguration string) (*catalog.ValidateResult, error) { - return nil, nil + return nil, fmt.Errorf("%s", "edwin") } func (ds *Plugin) openConnections(config *configuration) error {