diff --git a/command/operator_diagnose.go b/command/operator_diagnose.go index 93fd96457a..02d72dc248 100644 --- a/command/operator_diagnose.go +++ b/command/operator_diagnose.go @@ -37,9 +37,7 @@ import ( const OperatorDiagnoseEnableEnv = "VAULT_DIAGNOSE" -const CoreUninitializedErr = "diagnose cannot attempt this step because core could not be initialized" -const BackendUninitializedErr = "diagnose cannot attempt this step because backend could not be initialized" -const CoreConfigUninitializedErr = "diagnose cannot attempt this step because core config could not be set" +const CoreConfigUninitializedErr = "Diagnose cannot attempt this step because core config could not be set." var ( _ cli.Command = (*OperatorDiagnoseCommand)(nil) @@ -175,7 +173,7 @@ func (c *OperatorDiagnoseCommand) RunWithParsedFlags() int { if c.flagFormat == "json" { resultsJS, err := json.MarshalIndent(results, "", " ") if err != nil { - fmt.Fprintf(os.Stderr, "error marshalling results: %v", err) + fmt.Fprintf(os.Stderr, "Error marshalling results: %v.", err) return 4 } c.UI.Output(string(resultsJS)) @@ -224,7 +222,7 @@ func (c *OperatorDiagnoseCommand) offlineDiagnostics(ctx context.Context) error reloadFuncsLock: new(sync.RWMutex), } - ctx, span := diagnose.StartSpan(ctx, "initialization") + ctx, span := diagnose.StartSpan(ctx, "Vault Diagnose") defer span.End() // OS Specific checks @@ -232,16 +230,17 @@ func (c *OperatorDiagnoseCommand) offlineDiagnostics(ctx context.Context) error var config *cserver.Config - diagnose.Test(ctx, "Parse configuration", func(ctx context.Context) (err error) { + diagnose.Test(ctx, "Parse Configuration", func(ctx context.Context) (err error) { server.flagConfigs = c.flagConfigs var configErrors []configutil.ConfigError config, configErrors, err = server.parseConfig() if err != nil { - return err + return fmt.Errorf("Could not parse configuration: %w.", err) } for _, ce := range configErrors { - diagnose.Warn(ctx, ce.String()) + diagnose.Warn(ctx, diagnose.CapitalizeFirstLetter(ce.String())+".") } + diagnose.Success(ctx, "Vault configuration syntax is ok.") return nil }) if config == nil { @@ -252,20 +251,22 @@ func (c *OperatorDiagnoseCommand) offlineDiagnostics(ctx context.Context) error var metricsHelper *metricsutil.MetricsHelper var backend *physical.Backend - diagnose.Test(ctx, "storage", func(ctx context.Context) error { + diagnose.Test(ctx, "Check Storage", func(ctx context.Context) error { // Ensure that there is a storage stanza if config.Storage == nil { - return fmt.Errorf("no storage stanza found in config") + diagnose.Advise(ctx, "To learn how to specify a storage backend, see the Vault server configuration documentation.") + return fmt.Errorf("No storage stanza in Vault server configuration.") } - diagnose.Test(ctx, "create-storage-backend", func(ctx context.Context) error { + diagnose.Test(ctx, "Create Storage Backend", func(ctx context.Context) error { b, err := server.setupStorage(config) if err != nil { return err } if b == nil { - return fmt.Errorf("storage backend was initialized to nil") + diagnose.Advise(ctx, "To learn how to specify a storage backend, see the Vault server configuration documentation.") + return fmt.Errorf("Storage backend could not be initialized.") } backend = &b return nil @@ -283,20 +284,16 @@ func (c *OperatorDiagnoseCommand) offlineDiagnostics(ctx context.Context) error if path == "" { path, ok := config.Storage.Config["path"] if !ok { - diagnose.SpotError(ctx, "raft folder permission checks", fmt.Errorf("storage file path is required")) + diagnose.SpotError(ctx, "Check Raft Folder Permissions", fmt.Errorf("Storage folder path is required.")) } diagnose.RaftFileChecks(ctx, path) } - if backend != nil { - diagnose.RaftStorageQuorum(ctx, (*backend).(*raft.RaftBackend)) - } else { - diagnose.SpotError(ctx, "raft quorum", fmt.Errorf("could not determine quorum status without initialized backend")) - } + diagnose.RaftStorageQuorum(ctx, (*backend).(*raft.RaftBackend)) } // Consul storage checks if config.Storage != nil && config.Storage.Type == storageTypeConsul { - diagnose.Test(ctx, "test-storage-tls-consul", func(ctx context.Context) error { + diagnose.Test(ctx, "Check Consul TLS", func(ctx context.Context) error { err := physconsul.SetupSecureTLS(ctx, api.DefaultConfig(), config.Storage.Config, server.logger, true) if err != nil { return err @@ -304,18 +301,21 @@ func (c *OperatorDiagnoseCommand) offlineDiagnostics(ctx context.Context) error return nil }) - diagnose.Test(ctx, "test-consul-direct-access-storage", func(ctx context.Context) error { + diagnose.Test(ctx, "Check Consul Direct Storage Access", func(ctx context.Context) error { dirAccess := diagnose.ConsulDirectAccess(config.Storage.Config) if dirAccess != "" { diagnose.Warn(ctx, dirAccess) } + if dirAccess == diagnose.DirAccessErr { + diagnose.Advise(ctx, diagnose.DirAccessAdvice) + } return nil }) } // Attempt to use storage backend if !c.skipEndEnd && config.Storage.Type != storageTypeRaft { - diagnose.Test(ctx, "test-access-storage", diagnose.WithTimeout(30*time.Second, func(ctx context.Context) error { + diagnose.Test(ctx, "Check Storage Access", diagnose.WithTimeout(30*time.Second, func(ctx context.Context) error { maxDurationCrudOperation := "write" maxDuration := time.Duration(0) uuidSuffix, err := uuid.GenerateUUID() @@ -346,7 +346,7 @@ func (c *OperatorDiagnoseCommand) offlineDiagnostics(ctx context.Context) error } if maxDuration > time.Duration(0) { - diagnose.Warn(ctx, diagnose.LatencyWarning+fmt.Sprintf("duration: %s, ", maxDuration)+fmt.Sprintf("operation: %s", maxDurationCrudOperation)) + diagnose.Warn(ctx, diagnose.LatencyWarning+fmt.Sprintf("duration: %s, operation: %s", maxDuration, maxDurationCrudOperation)) } return nil })) @@ -360,14 +360,14 @@ func (c *OperatorDiagnoseCommand) offlineDiagnostics(ctx context.Context) error } var configSR sr.ServiceRegistration - diagnose.Test(ctx, "service-discovery", func(ctx context.Context) error { + diagnose.Test(ctx, "Check Service Discovery", func(ctx context.Context) error { if config.ServiceRegistration == nil || config.ServiceRegistration.Config == nil { - diagnose.Skipped(ctx, "no service registration configured") + diagnose.Skipped(ctx, "No service registration configured.") return nil } srConfig := config.ServiceRegistration.Config - diagnose.Test(ctx, "test-serviceregistration-tls-consul", func(ctx context.Context) error { + diagnose.Test(ctx, "Check Consul Service Discovery TLS", func(ctx context.Context) error { // SetupSecureTLS for service discovery uses the same cert and key to set up physical // storage. See the consul package in physical for details. err := srconsul.SetupSecureTLS(ctx, api.DefaultConfig(), srConfig, server.logger, true) @@ -378,64 +378,21 @@ func (c *OperatorDiagnoseCommand) offlineDiagnostics(ctx context.Context) error }) if config.ServiceRegistration != nil && config.ServiceRegistration.Type == "consul" { - diagnose.Test(ctx, "test-consul-direct-access-service-discovery", func(ctx context.Context) error { + diagnose.Test(ctx, "Check Consul Direct Service Discovery", func(ctx context.Context) error { dirAccess := diagnose.ConsulDirectAccess(config.ServiceRegistration.Config) if dirAccess != "" { diagnose.Warn(ctx, dirAccess) } + if dirAccess == diagnose.DirAccessErr { + diagnose.Advise(ctx, diagnose.DirAccessAdvice) + } return nil }) } return nil }) - diagnose.Test(ctx, "seal-transit-tls-checks", func(ctx context.Context) error { - var checkSealTransit bool - for _, seal := range config.Seals { - if seal.Type == "transit" { - checkSealTransit = true - - tlsSkipVerify, _ := seal.Config["tls_skip_verify"] - if tlsSkipVerify == "true" { - diagnose.Warn(ctx, "TLS verification is Skipped! Using this option is highly discouraged and decreases the security of data transmissions to and from the Vault server.") - return nil - } - - // Checking tls_client_cert and tls_client_key - tlsClientCert, ok := seal.Config["tls_client_cert"] - if !ok { - diagnose.Warn(ctx, "Missing tls_client_cert in the config") - return nil - } - tlsClientKey, ok := seal.Config["tls_client_key"] - if !ok { - diagnose.Warn(ctx, "Missing tls_client_key in the config") - return nil - } - _, err := diagnose.TLSFileChecks(tlsClientCert, tlsClientKey) - if err != nil { - return fmt.Errorf("TLS file check failed for tls_client_cert and tls_client_key with the following error: %w", err) - } - - // checking tls_ca_cert - tlsCACert, ok := seal.Config["tls_ca_cert"] - if !ok { - diagnose.Warn(ctx, "Mising tls_ca_cert in the config") - return nil - } - _, err = diagnose.TLSCAFileCheck(tlsCACert) - if err != nil { - return fmt.Errorf("TLS file check failed for tls_ca_cert with the following error: %w", err) - } - } - } - if !checkSealTransit { - diagnose.Skipped(ctx, "No transit seal found!") - } - return nil - }) - - sealcontext, sealspan := diagnose.StartSpan(ctx, "create-seal") + sealcontext, sealspan := diagnose.StartSpan(ctx, "Create Vault Server Configuration Seals") var seals []vault.Seal var sealConfigError error @@ -443,11 +400,12 @@ func (c *OperatorDiagnoseCommand) offlineDiagnostics(ctx context.Context) error // Check error here if err != nil { - diagnose.Fail(sealcontext, err.Error()) + diagnose.Advise(ctx, "For assistance with the seal stanza, see the Vault configuration documentation.") + diagnose.Fail(sealcontext, fmt.Sprintf("Seal creation resulted in the following error: %s.", err.Error())) goto SEALFAIL } if sealConfigError != nil { - diagnose.Fail(sealcontext, "seal could not be configured: seals may already be initialized") + diagnose.Fail(sealcontext, "Seal could not be configured: seals may already be initialized.") goto SEALFAIL } @@ -455,11 +413,12 @@ func (c *OperatorDiagnoseCommand) offlineDiagnostics(ctx context.Context) error for _, seal := range seals { // Ensure that the seal finalizer is called, even if using verify-only defer func(seal *vault.Seal) { - sealType := (*seal).BarrierType() - finalizeSealContext, finalizeSealSpan := diagnose.StartSpan(ctx, "finalize-seal-"+sealType) + sealType := diagnose.CapitalizeFirstLetter((*seal).BarrierType()) + finalizeSealContext, finalizeSealSpan := diagnose.StartSpan(ctx, "Finalize "+sealType+" Seal") err = (*seal).Finalize(finalizeSealContext) if err != nil { - diagnose.Fail(finalizeSealContext, "error finalizing seal") + diagnose.Fail(finalizeSealContext, "Error finalizing seal.") + diagnose.Advise(finalizeSealContext, "This likely means that the barrier is still in use; therefore, finalizing the seal timed out.") finalizeSealSpan.End() } finalizeSealSpan.End() @@ -468,27 +427,80 @@ func (c *OperatorDiagnoseCommand) offlineDiagnostics(ctx context.Context) error } if barrierSeal == nil { - diagnose.Fail(sealcontext, "could not create barrier seal! Most likely proper Seal configuration information was not set, but no error was generated") + diagnose.Fail(sealcontext, "Could not create barrier seal. No error was generated, but it is likely that the seal stanza is misconfigured. For guidance, see Vault's configuration documentation on the seal stanza.") } SEALFAIL: sealspan.End() + + diagnose.Test(ctx, "Check Transit Seal TLS", func(ctx context.Context) error { + var checkSealTransit bool + for _, seal := range config.Seals { + if seal.Type == "transit" { + checkSealTransit = true + + tlsSkipVerify, _ := seal.Config["tls_skip_verify"] + if tlsSkipVerify == "true" { + diagnose.Warn(ctx, "TLS verification is skipped. This is highly discouraged and decreases the security of data transmissions to and from the Vault server.") + return nil + } + + // Checking tls_client_cert and tls_client_key + tlsClientCert, ok := seal.Config["tls_client_cert"] + if !ok { + diagnose.Warn(ctx, "Missing tls_client_cert in the seal configuration.") + return nil + } + tlsClientKey, ok := seal.Config["tls_client_key"] + if !ok { + diagnose.Warn(ctx, "Missing tls_client_key in the seal configuration.") + return nil + } + _, err := diagnose.TLSFileChecks(tlsClientCert, tlsClientKey) + if err != nil { + return fmt.Errorf("The TLS certificate and key configured through the tls_client_cert and tls_client_key fields of the transit seal configuration are invalid: %w.", err) + } + + // checking tls_ca_cert + tlsCACert, ok := seal.Config["tls_ca_cert"] + if !ok { + diagnose.Warn(ctx, "Missing tls_ca_cert in the seal configuration.") + return nil + } + warnings, err := diagnose.TLSCAFileCheck(tlsCACert) + if len(warnings) != 0 { + for _, warning := range warnings { + diagnose.Warn(ctx, warning) + } + } + if err != nil { + return fmt.Errorf("The TLS CA certificate configured through the tls_ca_cert field of the transit seal configuration is invalid: %w.", err) + } + } + } + if !checkSealTransit { + diagnose.Skipped(ctx, "No transit seal found in seal configuration.") + } + return nil + }) + var coreConfig vault.CoreConfig - diagnose.Test(ctx, "setup-core", func(ctx context.Context) error { + diagnose.Test(ctx, "Create Core Configuration", func(ctx context.Context) error { var secureRandomReader io.Reader // prepare a secure random reader for core + randReaderTestName := "Initialize Randomness for Core" secureRandomReader, err = configutil.CreateSecureRandomReaderFunc(config.SharedConfig, barrierWrapper) if err != nil { - return diagnose.SpotError(ctx, "init-randreader", err) + return diagnose.SpotError(ctx, randReaderTestName, fmt.Errorf("Could not initialize randomness for core: %w.", err)) } - diagnose.SpotOk(ctx, "init-randreader", "") + diagnose.SpotOk(ctx, randReaderTestName, "") coreConfig = createCoreConfig(server, config, *backend, configSR, barrierSeal, unwrapSeal, metricsHelper, metricSink, secureRandomReader) return nil }) var disableClustering bool - diagnose.Test(ctx, "setup-ha-storage", func(ctx context.Context) error { - diagnose.Test(ctx, "create-ha-storage-backend", func(ctx context.Context) error { + diagnose.Test(ctx, "HA Storage", func(ctx context.Context) error { + diagnose.Test(ctx, "Create HA Storage Backend", func(ctx context.Context) error { // Initialize the separate HA storage backend, if it exists disableClustering, err = initHaBackend(server, config, &coreConfig, *backend) if err != nil { @@ -497,19 +509,22 @@ SEALFAIL: return nil }) - diagnose.Test(ctx, "test-consul-direct-access-storage", func(ctx context.Context) error { + diagnose.Test(ctx, "Check HA Consul Direct Storage Access", func(ctx context.Context) error { if config.HAStorage == nil { - diagnose.Skipped(ctx, "no HA storage configured") + diagnose.Skipped(ctx, "No HA storage stanza is configured.") } else { dirAccess := diagnose.ConsulDirectAccess(config.HAStorage.Config) if dirAccess != "" { diagnose.Warn(ctx, dirAccess) } + if dirAccess == diagnose.DirAccessErr { + diagnose.Advise(ctx, diagnose.DirAccessAdvice) + } } return nil }) if config.HAStorage != nil && config.HAStorage.Type == storageTypeConsul { - diagnose.Test(ctx, "test-ha-storage-tls-consul", func(ctx context.Context) error { + diagnose.Test(ctx, "Check Consul TLS", func(ctx context.Context) error { err = physconsul.SetupSecureTLS(ctx, api.DefaultConfig(), config.HAStorage.Config, server.logger, true) if err != nil { return err @@ -523,22 +538,23 @@ SEALFAIL: // Determine the redirect address from environment variables err = determineRedirectAddr(server, &coreConfig, config) if err != nil { - return diagnose.SpotError(ctx, "determine-redirect", err) + return diagnose.SpotError(ctx, "Determine Redirect Address", fmt.Errorf("Redirect Address could not be determined: %w.", err)) } - diagnose.SpotOk(ctx, "determine-redirect", "") + diagnose.SpotOk(ctx, "Determine Redirect Address", "") err = findClusterAddress(server, &coreConfig, config, disableClustering) if err != nil { - return diagnose.SpotError(ctx, "find-cluster-addr", err) + diagnose.Advise(ctx, "Please check that the API and Cluster addresses are different, and that the API, Cluster and Redirect addresses have both a host and port.") + return diagnose.SpotError(ctx, "Check Cluster Address", fmt.Errorf("Cluster Address could not be determined or was invalid: %w.", err)) } - diagnose.SpotOk(ctx, "find-cluster-addr", "") + diagnose.SpotOk(ctx, "Check Cluster Address", "Cluster address is logically valid and can be found.") var vaultCore *vault.Core // Run all the checks that are utilized when initializing a core object // without actually calling core.Init. These are in the init-core section // as they are runtime checks. - diagnose.Test(ctx, "init-core", func(ctx context.Context) error { + diagnose.Test(ctx, "Check Core Creation", func(ctx context.Context) error { var newCoreError error if coreConfig.RawConfig == nil { return fmt.Errorf(CoreConfigUninitializedErr) @@ -546,11 +562,10 @@ SEALFAIL: core, newCoreError := vault.CreateCore(&coreConfig) if newCoreError != nil { if vault.IsFatalError(newCoreError) { - return fmt.Errorf("Error initializing core: %s", newCoreError) + return fmt.Errorf("Error initializing core: %s.", newCoreError) } diagnose.Warn(ctx, wrapAtLength( - "WARNING! A non-fatal error occurred during initialization. Please "+ - "check the logs for more information.")) + "A non-fatal error occurred during initialization. Please check the logs for more information.")) } else { vaultCore = core } @@ -558,10 +573,10 @@ SEALFAIL: }) if vaultCore == nil { - return fmt.Errorf("Diagnose could not initialize the vault core from the vault server configuration.") + return fmt.Errorf("Diagnose could not initialize the Vault core from the Vault server configuration.") } - licenseCtx, licenseSpan := diagnose.StartSpan(ctx, "autoloaded license") + licenseCtx, licenseSpan := diagnose.StartSpan(ctx, "Check For Autoloaded License") // If we are not in enterprise, return from the check if !constants.IsEnterprise { diagnose.Skipped(licenseCtx, "License check will not run on OSS Vault.") @@ -579,7 +594,7 @@ SEALFAIL: licenseSpan.End() var lns []listenerutil.Listener - diagnose.Test(ctx, "init-listeners", func(ctx context.Context) error { + diagnose.Test(ctx, "Start Listeners", func(ctx context.Context) error { disableClustering := config.HAStorage != nil && config.HAStorage.DisableClustering infoKeys := make([]string, 0, 10) info := make(map[string]string) @@ -588,7 +603,7 @@ SEALFAIL: diagnose.ListenerChecks(ctx, config.Listeners) - diagnose.Test(ctx, "create-listeners", func(ctx context.Context) error { + diagnose.Test(ctx, "Create Listeners", func(ctx context.Context) error { status, listeners, _, err = server.InitListeners(config, disableClustering, &infoKeys, &info) if status != 0 { return err @@ -614,18 +629,18 @@ SEALFAIL: // The unseal diagnose check will simply attempt to use the barrier to encrypt and // decrypt a mock value. It will not call runUnseal. - diagnose.Test(ctx, "unseal", diagnose.WithTimeout(30*time.Second, func(ctx context.Context) error { + diagnose.Test(ctx, "Check Barrier Encryption", diagnose.WithTimeout(30*time.Second, func(ctx context.Context) error { if barrierWrapper == nil { - return fmt.Errorf("Diagnose could not create a barrier seal object") + return fmt.Errorf("Diagnose could not create a barrier seal object.") } barrierUUID, err := uuid.GenerateUUID() if err != nil { - return fmt.Errorf("Diagnose could not create unique UUID for unsealing") + return fmt.Errorf("Diagnose could not create unique UUID for unsealing.") } barrierEncValue := "diagnose-" + barrierUUID ciphertext, err := barrierWrapper.Encrypt(ctx, []byte(barrierEncValue), nil) if err != nil { - return fmt.Errorf("Error encrypting with seal barrier: %w", err) + return fmt.Errorf("Error encrypting with seal barrier: %w.", err) } plaintext, err := barrierWrapper.Decrypt(ctx, ciphertext, nil) if err != nil { @@ -633,18 +648,20 @@ SEALFAIL: } if string(plaintext) != barrierEncValue { - return fmt.Errorf("barrier returned incorrect decrypted value for mock data") + return fmt.Errorf("Barrier returned incorrect decrypted value for mock data.") } return nil })) // The following block contains static checks that are run during the // startHttpServers portion of server run. In other words, they are static - // checks during resource creation. - diagnose.Test(ctx, "start-servers", func(ctx context.Context) error { + // checks during resource creation. Currently there is nothing important in this + // diagnose check. For now it is a placeholder for any checks that will be done + // before server run. + diagnose.Test(ctx, "Check Server Before Runtime", func(ctx context.Context) error { for _, ln := range lns { if ln.Config == nil { - return fmt.Errorf("Found nil listener config after parsing") + return fmt.Errorf("Found no listener config after parsing the Vault configuration.") } } return nil diff --git a/command/operator_diagnose_test.go b/command/operator_diagnose_test.go index b49de01b7a..2e59c09dbb 100644 --- a/command/operator_diagnose_test.go +++ b/command/operator_diagnose_test.go @@ -41,130 +41,130 @@ func TestOperatorDiagnoseCommand_Run(t *testing.T) { }, []*diagnose.Result{ { - Name: "Parse configuration", + Name: "Parse Configuration", Status: diagnose.OkStatus, }, { - Name: "init-listeners", + Name: "Start Listeners", Status: diagnose.WarningStatus, Children: []*diagnose.Result{ { - Name: "create-listeners", + Name: "Create Listeners", Status: diagnose.OkStatus, }, { - Name: "check-listener-tls", + Name: "Check Listener TLS", Status: diagnose.WarningStatus, Warnings: []string{ - "TLS is disabled in a Listener config stanza.", + "TLS is disabled in a listener config stanza.", }, }, }, }, { - Name: "storage", + Name: "Check Storage", Status: diagnose.OkStatus, Children: []*diagnose.Result{ { - Name: "create-storage-backend", + Name: "Create Storage Backend", Status: diagnose.OkStatus, }, { - Name: "test-storage-tls-consul", + Name: "Check Consul TLS", Status: diagnose.SkippedStatus, }, { - Name: "test-consul-direct-access-storage", + Name: "Check Consul Direct Storage Access", Status: diagnose.OkStatus, }, }, }, { - Name: "service-discovery", + Name: "Check Service Discovery", Status: diagnose.OkStatus, Children: []*diagnose.Result{ { - Name: "test-serviceregistration-tls-consul", + Name: "Check Consul Service Discovery TLS", Status: diagnose.SkippedStatus, }, { - Name: "test-consul-direct-access-service-discovery", + Name: "Check Consul Direct Service Discovery", Status: diagnose.OkStatus, }, }, }, { - Name: "create-seal", + Name: "Create Vault Server Configuration Seals", Status: diagnose.OkStatus, }, { - Name: "setup-core", + Name: "Create Core Configuration", Status: diagnose.OkStatus, Children: []*diagnose.Result{ { - Name: "init-randreader", + Name: "Initialize Randomness for Core", Status: diagnose.OkStatus, }, }, }, { - Name: "setup-ha-storage", + Name: "HA Storage", Status: diagnose.OkStatus, Children: []*diagnose.Result{ { - Name: "create-ha-storage-backend", + Name: "Create HA Storage Backend", Status: diagnose.OkStatus, }, { - Name: "test-consul-direct-access-storage", + Name: "Check HA Consul Direct Storage Access", Status: diagnose.OkStatus, }, { - Name: "test-ha-storage-tls-consul", + Name: "Check Consul TLS", Status: diagnose.SkippedStatus, }, }, }, { - Name: "determine-redirect", + Name: "Determine Redirect Address", Status: diagnose.OkStatus, }, { - Name: "find-cluster-addr", + Name: "Check Cluster Address", Status: diagnose.OkStatus, }, { - Name: "init-core", + Name: "Check Core Creation", Status: diagnose.OkStatus, }, { - Name: "init-listeners", + Name: "Start Listeners", Status: diagnose.WarningStatus, Children: []*diagnose.Result{ { - Name: "create-listeners", + Name: "Create Listeners", Status: diagnose.OkStatus, }, { - Name: "check-listener-tls", + Name: "Check Listener TLS", Status: diagnose.WarningStatus, Warnings: []string{ - "TLS is disabled in a Listener config stanza.", + "TLS is disabled in a listener config stanza.", }, }, }, }, { - Name: "unseal", + Name: "Check Barrier Encryption", Status: diagnose.ErrorStatus, Message: "Diagnose could not create a barrier seal object", }, { - Name: "start-servers", + Name: "Check Server Before Runtime", Status: diagnose.OkStatus, }, { - Name: "finalize-seal-shamir", + Name: "Finalize Shamir Seal", Status: diagnose.OkStatus, }, }, @@ -176,22 +176,22 @@ func TestOperatorDiagnoseCommand_Run(t *testing.T) { }, []*diagnose.Result{ { - Name: "storage", + Name: "Check Storage", Status: diagnose.WarningStatus, Children: []*diagnose.Result{ { - Name: "create-storage-backend", + Name: "Create Storage Backend", Status: diagnose.OkStatus, }, { - Name: "raft folder permission checks", + Name: "Check Raft Folder Permissions", Status: diagnose.WarningStatus, Message: "too many permissions", }, { - Name: "raft quorum", + Name: "Check For Raft Quorum", Status: diagnose.WarningStatus, - Message: "even number of voters found", + Message: "0 voters found", }, }, }, @@ -204,9 +204,9 @@ func TestOperatorDiagnoseCommand_Run(t *testing.T) { }, []*diagnose.Result{ { - Name: "storage", + Name: "Check Storage", Status: diagnose.ErrorStatus, - Message: "no storage stanza found in config", + Message: "No storage stanza in Vault server configuration.", }, }, }, @@ -217,15 +217,15 @@ func TestOperatorDiagnoseCommand_Run(t *testing.T) { }, []*diagnose.Result{ { - Name: "init-listeners", + Name: "Start Listeners", Status: diagnose.OkStatus, Children: []*diagnose.Result{ { - Name: "create-listeners", + Name: "Create Listeners", Status: diagnose.OkStatus, }, { - Name: "check-listener-tls", + Name: "Check Listener TLS", Status: diagnose.OkStatus, }, }, @@ -239,15 +239,15 @@ func TestOperatorDiagnoseCommand_Run(t *testing.T) { }, []*diagnose.Result{ { - Name: "storage", + Name: "Check Storage", Status: diagnose.ErrorStatus, Children: []*diagnose.Result{ { - Name: "create-storage-backend", + Name: "Create Storage Backend", Status: diagnose.OkStatus, }, { - Name: "test-storage-tls-consul", + Name: "Check Consul TLS", Status: diagnose.ErrorStatus, Message: "certificate has expired or is not yet valid", Warnings: []string{ @@ -255,7 +255,7 @@ func TestOperatorDiagnoseCommand_Run(t *testing.T) { }, }, { - Name: "test-consul-direct-access-storage", + Name: "Check Consul Direct Storage Access", Status: diagnose.OkStatus, }, }, @@ -269,43 +269,45 @@ func TestOperatorDiagnoseCommand_Run(t *testing.T) { }, []*diagnose.Result{ { - Name: "storage", + Name: "Check Storage", Status: diagnose.WarningStatus, Children: []*diagnose.Result{ { - Name: "create-storage-backend", + Name: "Create Storage Backend", Status: diagnose.OkStatus, }, { - Name: "test-storage-tls-consul", + Name: "Check Consul TLS", Status: diagnose.SkippedStatus, }, { - Name: "test-consul-direct-access-storage", + Name: "Check Consul Direct Storage Access", Status: diagnose.WarningStatus, + Advice: "We recommend connecting to a local agent.", Warnings: []string{ - "consul storage does not connect to local agent, but directly to server", + "Vault storage is directly connected to a Consul server.", }, }, }, }, { - Name: "setup-ha-storage", + Name: "HA Storage", Status: diagnose.ErrorStatus, Children: []*diagnose.Result{ { - Name: "create-ha-storage-backend", + Name: "Create HA Storage Backend", Status: diagnose.OkStatus, }, { - Name: "test-consul-direct-access-storage", + Name: "Check HA Consul Direct Storage Access", Status: diagnose.WarningStatus, + Advice: "We recommend connecting to a local agent.", Warnings: []string{ - "consul storage does not connect to local agent, but directly to server", + "Vault storage is directly connected to a Consul server.", }, }, { - Name: "test-ha-storage-tls-consul", + Name: "Check Consul TLS", Status: diagnose.ErrorStatus, Message: "certificate has expired or is not yet valid", Warnings: []string{ @@ -315,7 +317,7 @@ func TestOperatorDiagnoseCommand_Run(t *testing.T) { }, }, { - Name: "find-cluster-addr", + Name: "Check Cluster Address", Status: diagnose.ErrorStatus, }, }, @@ -323,13 +325,15 @@ func TestOperatorDiagnoseCommand_Run(t *testing.T) { { "diagnose_seal_transit_tls_check_fail", []string{ - "-config", "./server/test-fixtures/diagnose_seal_trasit_tls_check.hcl", + "-config", "./server/test-fixtures/diagnose_seal_transit_tls_check.hcl", }, []*diagnose.Result{ { - Name: "seal-transit-tls-checks", - Status: diagnose.ErrorStatus, - Message: "Found at least one intermediate cert in a root CA cert", + Name: "Check Transit Seal TLS", + Status: diagnose.WarningStatus, + Warnings: []string{ + "Found at least one intermediate certificate in the CA certificate file.", + }, }, }, }, @@ -340,11 +344,11 @@ func TestOperatorDiagnoseCommand_Run(t *testing.T) { }, []*diagnose.Result{ { - Name: "service-discovery", + Name: "Check Service Discovery", Status: diagnose.ErrorStatus, Children: []*diagnose.Result{ { - Name: "test-serviceregistration-tls-consul", + Name: "Check Consul Service Discovery TLS", Status: diagnose.ErrorStatus, Message: "certificate has expired or is not yet valid", Warnings: []string{ @@ -352,7 +356,7 @@ func TestOperatorDiagnoseCommand_Run(t *testing.T) { }, }, { - Name: "test-consul-direct-access-service-discovery", + Name: "Check Consul Direct Service Discovery", Status: diagnose.WarningStatus, Warnings: []string{ diagnose.DirAccessErr, @@ -369,19 +373,19 @@ func TestOperatorDiagnoseCommand_Run(t *testing.T) { }, []*diagnose.Result{ { - Name: "storage", + Name: "Check Storage", Status: diagnose.WarningStatus, Children: []*diagnose.Result{ { - Name: "create-storage-backend", + Name: "Create Storage Backend", Status: diagnose.OkStatus, }, { - Name: "test-storage-tls-consul", + Name: "Check Consul TLS", Status: diagnose.SkippedStatus, }, { - Name: "test-consul-direct-access-storage", + Name: "Check Consul Direct Storage Access", Status: diagnose.WarningStatus, Warnings: []string{ diagnose.DirAccessErr, @@ -398,12 +402,12 @@ func TestOperatorDiagnoseCommand_Run(t *testing.T) { }, []*diagnose.Result{ { - Name: "storage", + Name: "Check Storage", Status: diagnose.ErrorStatus, Message: "Diagnose could not initialize storage backend.", Children: []*diagnose.Result{ { - Name: "create-storage-backend", + Name: "Create Storage Backend", Status: diagnose.ErrorStatus, Message: "no such file or directory", }, @@ -472,6 +476,9 @@ func compareResult(exp *diagnose.Result, act *diagnose.Result) error { if exp.Message != "" && exp.Message != act.Message && !strings.Contains(act.Message, exp.Message) { return fmt.Errorf("section %s, message not found: %s in %s", exp.Name, exp.Message, act.Message) } + if exp.Advice != "" && exp.Advice != act.Advice && !strings.Contains(act.Advice, exp.Advice) { + return fmt.Errorf("section %s, advice not found: %s in %s", exp.Name, exp.Advice, act.Advice) + } if len(exp.Warnings) != len(act.Warnings) { return fmt.Errorf("section %s, warning count mismatch: %d vs %d", exp.Name, len(exp.Warnings), len(act.Warnings)) } diff --git a/command/server/test-fixtures/diagnose_seal_trasit_tls_check.hcl b/command/server/test-fixtures/diagnose_seal_transit_tls_check.hcl similarity index 100% rename from command/server/test-fixtures/diagnose_seal_trasit_tls_check.hcl rename to command/server/test-fixtures/diagnose_seal_transit_tls_check.hcl diff --git a/vault/diagnose/constants.go b/vault/diagnose/constants.go index c7fe7d3178..fae2edc65f 100644 --- a/vault/diagnose/constants.go +++ b/vault/diagnose/constants.go @@ -3,7 +3,7 @@ package diagnose const ( AutoLoadedLicenseValidatorError = "Autoloaded license could not be validated: " AutoloadedLicenseValidationError = "Autoloaded license validation failed due to error: " - LicenseAutoloadingError = "license could not be autoloaded: " + LicenseAutoloadingError = "License could not be autoloaded: " StoredLicenseNoAutoloadingWarning = "Vault is using a stored license, which is deprecated! Vault should use autoloaded licenses instead." NoStoredOrAutoloadedLicenseWarning = "No autoloaded or stored license could be detected. If the binary is not a pro/prem binary, this means Vault does not have access to a license at all." LicenseExpiredError = "Autoloaded license is expired." diff --git a/vault/diagnose/file_checks_test.go b/vault/diagnose/file_checks_test.go index 3e4952e84c..ba22f56536 100644 --- a/vault/diagnose/file_checks_test.go +++ b/vault/diagnose/file_checks_test.go @@ -78,19 +78,19 @@ func TestRaftStorageQuorum(t *testing.T) { m.raftServerQuorumType = 0 twoVoterCluster := RaftStorageQuorum(context.Background(), m) - if !strings.Contains(twoVoterCluster, "error") { + if !strings.Contains(twoVoterCluster, "Please ensure that Vault has access to an odd number of voter nodes.") { t.Fatalf("two voter cluster yielded wrong error: %+s", twoVoterCluster) } m.raftServerQuorumType = 1 threeVoterCluster := RaftStorageQuorum(context.Background(), m) - if !strings.Contains(threeVoterCluster, "voter quorum exists") { + if !strings.Contains(threeVoterCluster, "Voter quorum exists") { t.Fatalf("three voter cluster yielded incorrect error: %s", threeVoterCluster) } m.raftServerQuorumType = 2 threeNodeTwoVoterCluster := RaftStorageQuorum(context.Background(), m) - if !strings.Contains(threeNodeTwoVoterCluster, "error") { + if !strings.Contains(threeNodeTwoVoterCluster, "Please ensure that Vault has access to an odd number of voter nodes.") { t.Fatalf("two voter cluster yielded wrong error: %+s", threeNodeTwoVoterCluster) } diff --git a/vault/diagnose/helpers.go b/vault/diagnose/helpers.go index 60e662d7dd..983dccacc9 100644 --- a/vault/diagnose/helpers.go +++ b/vault/diagnose/helpers.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "io" + "strings" "time" "go.opentelemetry.io/otel/attribute" @@ -225,7 +226,7 @@ func WithTimeout(d time.Duration, f testFunction) testFunction { go func() { rch <- f(ctx) }() select { case <-t.C: - return fmt.Errorf("timed out after %s", d.String()) + return fmt.Errorf("Timeout after %s.", d.String()) case err := <-rch: return err } @@ -247,3 +248,15 @@ func Skippable(skipName string, f testFunction) testFunction { return nil } } + +// CapitalizeFirstLetter returns a string with the first letter capitalized +func CapitalizeFirstLetter(msg string) string { + words := strings.Split(msg, " ") + if len(words) == 0 { + return "" + } + if len(words) > 1 { + return strings.Title(words[0]) + " " + strings.Join(words[1:], " ") + } + return strings.Title(words[0]) +} diff --git a/vault/diagnose/helpers_test.go b/vault/diagnose/helpers_test.go index 2b0d1ec9e1..93995d2924 100644 --- a/vault/diagnose/helpers_test.go +++ b/vault/diagnose/helpers_test.go @@ -151,3 +151,18 @@ func disposeGrounds(_ context.Context) error { //Done! return nil } + +func TestCapitalizeFirstLetter(t *testing.T) { + s := "this is a test." + if CapitalizeFirstLetter(s) != "This is a test." { + t.Fatalf("first word of string was not capitalized: got %s", CapitalizeFirstLetter(s)) + } + s = "this" + if CapitalizeFirstLetter(s) != "This" { + t.Fatalf("first word of string was not capitalized: got %s", CapitalizeFirstLetter(s)) + } + s = "." + if CapitalizeFirstLetter(s) != "." { + t.Fatalf("String without letters was not unchanged: got %s", CapitalizeFirstLetter(s)) + } +} diff --git a/vault/diagnose/os_common.go b/vault/diagnose/os_common.go index 51254a0926..292fd7fe53 100644 --- a/vault/diagnose/os_common.go +++ b/vault/diagnose/os_common.go @@ -26,16 +26,18 @@ partLoop: } } usage, err := disk.Usage(partition.Mountpoint) - testName := "disk usage" + testName := "Check Disk Usage" if err != nil { - Warn(ctx, fmt.Sprintf("could not obtain partition usage for %s: %v", partition.Mountpoint, err)) + Warn(ctx, fmt.Sprintf("Could not obtain partition usage for %s: %v.", partition.Mountpoint, err)) } else { if usage.UsedPercent > 95 { - SpotWarn(ctx, testName, partition.Mountpoint+" more than 95% full") + SpotWarn(ctx, testName, fmt.Sprintf(partition.Mountpoint+" is %d percent full.", usage.UsedPercent)) + Advise(ctx, "It is recommended to have more than five percent of the partition free.") } else if usage.Free < 2<<30 { - SpotWarn(ctx, testName, partition.Mountpoint+" less than 1GB free") + SpotWarn(ctx, testName, partition.Mountpoint+" has %d bytes full.") + Advise(ctx, "It is recommended to have at least 1 GB of space free per partition.") } else { - SpotOk(ctx, testName, partition.Mountpoint+" ok") + SpotOk(ctx, testName, partition.Mountpoint+" usage ok.") } } diff --git a/vault/diagnose/os_openbsd_arm.go b/vault/diagnose/os_openbsd_arm.go index 8d82b891af..0a0ffedb70 100644 --- a/vault/diagnose/os_openbsd_arm.go +++ b/vault/diagnose/os_openbsd_arm.go @@ -5,6 +5,6 @@ package diagnose import "context" func diskUsage(ctx context.Context) error { - SpotSkipped(ctx, "disk usage", "unsupported on this platform") + SpotSkipped(ctx, "Check Disk Usage", "Disk Usage diagnostics are unsupported on this platform.") return nil } diff --git a/vault/diagnose/os_unix.go b/vault/diagnose/os_unix.go index 9943dfa2ee..436d611ff1 100644 --- a/vault/diagnose/os_unix.go +++ b/vault/diagnose/os_unix.go @@ -5,25 +5,29 @@ package diagnose import ( "context" "fmt" + "golang.org/x/sys/unix" ) func OSChecks(ctx context.Context) { - ctx, span := StartSpan(ctx, "operating system") + ctx, span := StartSpan(ctx, "Check Operating System") defer span.End() + fileLimitsName := "Check Open File Limits" + var limit unix.Rlimit if err := unix.Getrlimit(unix.RLIMIT_NOFILE, &limit); err != nil { - SpotError(ctx, "open file limits", fmt.Errorf("could not determine open file limit: %w", err)) + SpotError(ctx, fileLimitsName, fmt.Errorf("Could not determine open file limit: %w.", err)) } else { min := limit.Cur if limit.Max < min { min = limit.Max } if min <= 1024 { - SpotWarn(ctx, "open file limits", fmt.Sprintf("set to %d, which may be insufficient.", min)) + SpotWarn(ctx, fileLimitsName, fmt.Sprintf("Open file limits are set to %d", min)) + Advise(ctx, "These limits may be insufficient. We recommend raising the soft and hard limits to 1024768.") } else { - SpotOk(ctx, "open file limits", fmt.Sprintf("set to %d", min)) + SpotOk(ctx, fileLimitsName, fmt.Sprintf("Open file limits are set to %d.", min)) } } diff --git a/vault/diagnose/os_windows.go b/vault/diagnose/os_windows.go index 8e0b4b8b47..b012c4685a 100644 --- a/vault/diagnose/os_windows.go +++ b/vault/diagnose/os_windows.go @@ -7,7 +7,7 @@ import ( ) func OSChecks(ctx context.Context) { - ctx, span := StartSpan(ctx, "operating system") + ctx, span := StartSpan(ctx, "Check Operating System") defer span.End() diskUsage(ctx) } diff --git a/vault/diagnose/output.go b/vault/diagnose/output.go index a626587b44..94035cd164 100644 --- a/vault/diagnose/output.go +++ b/vault/diagnose/output.go @@ -20,9 +20,9 @@ import ( const ( status_unknown = "[ ] " - status_ok = "\u001b[32m[ ok ]\u001b[0m " - status_failed = "\u001b[31m[failed]\u001b[0m " - status_warn = "\u001b[33m[ warn ]\u001b[0m " + status_ok = "\u001b[32m[ success ]\u001b[0m " + status_failed = "\u001b[31m[ failure ]\u001b[0m " + status_warn = "\u001b[33m[ warning ]\u001b[0m " status_skipped = "\u001b[90m[ skip ]\u001b[0m " same_line = "\x0d" ErrorStatus = 2 @@ -352,10 +352,10 @@ func (r *Result) write(sb *strings.Builder, depth int, limit int) { } if r.Advice != "" { - sb.WriteString("\n\n") - indent(sb, depth+1) - writeWrapped(sb, r.Advice, depth+1, limit) + advice := "\u001b[35m" + r.Advice + "\u001b[0m" sb.WriteRune('\n') + indent(sb, depth+1) + writeWrapped(sb, advice, depth+1, limit) } sb.WriteRune('\n') for _, c := range r.Children { diff --git a/vault/diagnose/raft_checks.go b/vault/diagnose/raft_checks.go index b03a481da5..b63228dd49 100644 --- a/vault/diagnose/raft_checks.go +++ b/vault/diagnose/raft_checks.go @@ -16,44 +16,46 @@ const owner = "owner" const group = "group" const other = "other" +const ownershipTestName = "Check Raft Folder Ownership" +const permissionsTestName = "Check Raft Folder Permissions" +const raftQuorumTestName = "Check For Raft Quorum" + func RaftFileChecks(ctx context.Context, path string) { // Note: Stat does not return information about the symlink itself, in the case where we are dealing with one. info, err := os.Stat(path) if err != nil { - SpotError(ctx, "raft folder permission checks", fmt.Errorf("error computing file permissions: %w", err)) + SpotError(ctx, permissionsTestName, fmt.Errorf("Error computing file permissions: %w.", err)) } if !IsDir(info) { - SpotError(ctx, "raft folder ownership checks", fmt.Errorf("error: path does not point to folder")) + SpotError(ctx, ownershipTestName, fmt.Errorf("Error: Raft storage path variable does not point to a folder.")) } if !HasDB(path) { - SpotWarn(ctx, "raft folder ownership checks", "boltDB file has not been created") + SpotWarn(ctx, ownershipTestName, "Raft boltDB file has not been created.") } hasOnlyOwnerRW, errs := CheckFilePerms(info) - if errs != nil { - for _, err := range errs { - switch { - case strings.Contains(err, FileIsSymlinkWarning) || strings.Contains(err, FileTooPermissiveWarning): - SpotWarn(ctx, "raft folder permission checks", err) - case strings.Contains(err, FilePermissionsMissingWarning): - SpotError(ctx, "raft folder permission checks", errors.New(err)) - } + for _, err := range errs { + switch { + case strings.Contains(err, FileIsSymlinkWarning) || strings.Contains(err, FileTooPermissiveWarning): + SpotWarn(ctx, permissionsTestName, err) + case strings.Contains(err, FilePermissionsMissingWarning): + SpotError(ctx, permissionsTestName, errors.New(err)) } } ownedByRoot := IsOwnedByRoot(info) requiresRoot := ownedByRoot && hasOnlyOwnerRW if requiresRoot { - SpotWarn(ctx, "raft folder ownership checks", "raft backend files owned by root and only accessible as root or with overpermissive file perms. This prevents Vault from running as a non-privileged user") + SpotWarn(ctx, ownershipTestName, "Raft backend files are owned by root and are only accessible as root or with overpermissive file permissions. This prevents Vault from running as a non-privileged user.") Advise(ctx, "Please change raft path permissions to allow for non-root access.") } if runtime.GOOS == "windows" { - SpotWarn(ctx, "raft folder permission checks", "Diagnose cannot determine if vault needs to run as root to open boltDB file. Please check these permissions manually.") + SpotWarn(ctx, permissionsTestName, "Diagnose cannot determine if Vault needs to run as root to open boltDB file. Please check these permissions manually.") } else if errs == nil && !requiresRoot { - SpotOk(ctx, "raft folder permission checks", "boltDB file has correct set of permissions") + SpotOk(ctx, permissionsTestName, "Raft BoltDB file has correct set of permissions.") } } @@ -64,8 +66,8 @@ func RaftStorageQuorum(ctx context.Context, b RaftConfigurableStorageBackend) st var err error conf, err = b.GetConfigurationOffline() if err != nil { - SpotError(ctx, "raft quorum", fmt.Errorf("error retrieving server configuration: %w", err)) - return fmt.Sprintf("error retrieving server configuration: %s", err.Error()) + SpotError(ctx, raftQuorumTestName, fmt.Errorf("Error retrieving server configuration: %w.", err)) + return fmt.Sprintf("Error retrieving server configuration: %s.", err.Error()) } voterCount := 0 for _, s := range conf.Servers { @@ -74,23 +76,23 @@ func RaftStorageQuorum(ctx context.Context, b RaftConfigurableStorageBackend) st } } if voterCount == 1 { - nonHAWarning := "warning: only one server node found. Vault is not running in high availability mode" - SpotWarn(ctx, "raft quorum", nonHAWarning) + nonHAWarning := "Only one server node found. Vault is not running in high availability mode." + SpotWarn(ctx, raftQuorumTestName, nonHAWarning) return nonHAWarning } var warnMsg string if voterCount%2 == 0 { - warnMsg = fmt.Sprintf("error: even number of voters found: %d", voterCount) - SpotWarn(ctx, "raft quorum", warnMsg) + warnMsg = fmt.Sprintf("%d voters found. Please ensure that Vault has access to an odd number of voter nodes.", voterCount) + SpotWarn(ctx, raftQuorumTestName, warnMsg) return warnMsg } if voterCount > 7 { - warnMsg = fmt.Sprintf("very large cluster detected: %d voters", voterCount) - SpotWarn(ctx, "raft quorum", warnMsg) + warnMsg = fmt.Sprintf("Very large cluster detected: %d voters found. ", voterCount) + SpotWarn(ctx, raftQuorumTestName, warnMsg) return warnMsg } - okMsg := fmt.Sprintf("voter quorum exists: %d voters", voterCount) - SpotOk(ctx, "raft quorum", okMsg) + okMsg := fmt.Sprintf("Voter quorum exists: %d voters.", voterCount) + SpotOk(ctx, raftQuorumTestName, okMsg) return okMsg } diff --git a/vault/diagnose/storage_checks.go b/vault/diagnose/storage_checks.go index 34494ede03..26fd0395fc 100644 --- a/vault/diagnose/storage_checks.go +++ b/vault/diagnose/storage_checks.go @@ -13,9 +13,10 @@ const ( success string = "success" secretVal string = "diagnoseSecret" - LatencyWarning string = "latency above 100 ms: " - DirAccessErr string = "consul storage does not connect to local agent, but directly to server" - AddrDNExistErr string = "config address does not exist: 127.0.0.1:8500 will be used" + LatencyWarning string = "Latency above 100 ms: " + DirAccessErr string = "Vault storage is directly connected to a Consul server." + DirAccessAdvice string = "We recommend connecting to a local agent." + AddrDNExistErr string = "Storage config address does not exist: 127.0.0.1:8500 will be used." wrongRWValsPrefix string = "Storage get and put gave wrong values: " latencyThreshold time.Duration = time.Millisecond * 100 ) @@ -42,10 +43,10 @@ func EndToEndLatencyCheckRead(ctx context.Context, uuid string, b physical.Backe return time.Duration(0), err } if val == nil { - return time.Duration(0), fmt.Errorf("no value found when reading generated data") + return time.Duration(0), fmt.Errorf("No value found when reading generated data.") } if val.Key != uuid && string(val.Value) != secretVal { - return time.Duration(0), fmt.Errorf(wrongRWValsPrefix+"expecting diagnose, but got %s, %s", val.Key, val.Value) + return time.Duration(0), fmt.Errorf(wrongRWValsPrefix+"expecting %s as key and diagnose for value, but got %s, %s.", uuid, val.Key, val.Value) } if duration > latencyThreshold { return duration, nil diff --git a/vault/diagnose/tls_verification.go b/vault/diagnose/tls_verification.go index 15b2afa9d3..9f815cb593 100644 --- a/vault/diagnose/tls_verification.go +++ b/vault/diagnose/tls_verification.go @@ -21,7 +21,7 @@ const maxVersionError = "'tls_max_version' value %q not supported, please specif // ListenerChecks diagnoses warnings and the first encountered error for the listener // configuration stanzas. func ListenerChecks(ctx context.Context, listeners []*configutil.Listener) ([]string, []error) { - testName := "check-listener-tls" + testName := "Check Listener TLS" ctx, span := StartSpan(ctx, testName) defer span.End() @@ -34,11 +34,11 @@ func ListenerChecks(ctx context.Context, listeners []*configutil.Listener) ([]st listenerID := l.Address if l.TLSDisable { - Warn(ctx, fmt.Sprintf("listener at address: %s has error: TLS is disabled in a Listener config stanza.", listenerID)) + Warn(ctx, fmt.Sprintf("Listener at address %s: TLS is disabled in a listener config stanza.", listenerID)) continue } if l.TLSDisableClientCerts { - Warn(ctx, fmt.Sprintf("listener at address: %s has error: TLS for a listener is turned on without requiring client certs.", listenerID)) + Warn(ctx, fmt.Sprintf("Listener at address %s: TLS for a listener is turned on without requiring client certificates.", listenerID)) } status, warning := TLSMutualExclusionCertCheck(l) @@ -55,13 +55,13 @@ func ListenerChecks(ctx context.Context, listeners []*configutil.Listener) ([]st } _, ok := tlsutil.TLSLookup[l.TLSMinVersion] if !ok { - err := fmt.Errorf("listener at address: %s has error %s: ", listenerID, fmt.Sprintf(minVersionError, l.TLSMinVersion)) + err := fmt.Errorf("Listener at address %s: %s.", listenerID, fmt.Sprintf(minVersionError, l.TLSMinVersion)) listenerErrors = append(listenerErrors, err) Fail(ctx, err.Error()) } _, ok = tlsutil.TLSLookup[l.TLSMaxVersion] if !ok { - err := fmt.Errorf("listener at address: %s has error %s: ", listenerID, fmt.Sprintf(maxVersionError, l.TLSMaxVersion)) + err := fmt.Errorf("Listener at address %s: %s.", listenerID, fmt.Sprintf(maxVersionError, l.TLSMaxVersion)) listenerErrors = append(listenerErrors, err) Fail(ctx, err.Error()) } @@ -133,7 +133,7 @@ func ParseTLSInformation(certFilePath string) ([]*x509.Certificate, []*x509.Cert rootCerts := []*x509.Certificate{} data, err := ioutil.ReadFile(certFilePath) if err != nil { - return leafCerts, interCerts, rootCerts, fmt.Errorf("failed to read certificate file: %w", err) + return leafCerts, interCerts, rootCerts, fmt.Errorf("Failed to read certificate file: %w.", err) } certBlocks := []*pem.Block{} @@ -141,20 +141,20 @@ func ParseTLSInformation(certFilePath string) ([]*x509.Certificate, []*x509.Cert for len(rst) != 0 { block, rest := pem.Decode(rst) if block == nil { - return leafCerts, interCerts, rootCerts, fmt.Errorf("could not decode cert") + return leafCerts, interCerts, rootCerts, fmt.Errorf("Could not decode certificate in certificate file.") } certBlocks = append(certBlocks, block) rst = rest } if len(certBlocks) == 0 { - return leafCerts, interCerts, rootCerts, fmt.Errorf("no certificates found in cert file") + return leafCerts, interCerts, rootCerts, fmt.Errorf("No certificates found in certificate file.") } for _, certBlock := range certBlocks { cert, err := x509.ParseCertificate(certBlock.Bytes) if err != nil { - return leafCerts, interCerts, rootCerts, fmt.Errorf("A pem block does not parse to a certificate: %w", err) + return leafCerts, interCerts, rootCerts, fmt.Errorf("A PEM block does not parse to a certificate: %w.", err) } // Detect if the certificate is a root, leaf, or intermediate @@ -200,7 +200,7 @@ func TLSErrorChecks(leafCerts, interCerts, rootCerts []*x509.Certificate) error KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageAny}, }) if err != nil { - return fmt.Errorf("failed to verify root certificate: %w", err) + return fmt.Errorf("Failed to verify root certificate: %w.", err) } } @@ -211,7 +211,7 @@ func TLSErrorChecks(leafCerts, interCerts, rootCerts []*x509.Certificate) error KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageAny}, }) if err != nil { - return fmt.Errorf("failed to verify intermediate certificate: %w", err) + return fmt.Errorf("Failed to verify intermediate certificate: %w.", err) } } @@ -231,7 +231,7 @@ func TLSErrorChecks(leafCerts, interCerts, rootCerts []*x509.Certificate) error KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageAny}, }) if err != nil { - return fmt.Errorf("failed to verify primary provided leaf certificate: %w", err) + return fmt.Errorf("Failed to verify primary provided leaf certificate: %w.", err) } } @@ -244,22 +244,22 @@ func TLSFileWarningChecks(leafCerts, interCerts, rootCerts []*x509.Certificate) var warnings []string // add a warning for when there are more than one leaf certs if len(leafCerts) > 1 { - warnings = append(warnings, fmt.Sprintf("More than one leaf certificate detected. Please ensure that there is one unique leaf certificate being supplied to vault in the vault server config file.")) + warnings = append(warnings, fmt.Sprintf("More than one leaf certificate detected. Please ensure that there is one unique leaf certificate being supplied to Vault in the Vault server configuration file.")) } for _, c := range leafCerts { if willExpire, timeToExpiry := NearExpiration(c); willExpire { - warnings = append(warnings, fmt.Sprintf("leaf certificate %d is expired or near expiry. Time to expire is: %s", c.SerialNumber, timeToExpiry)) + warnings = append(warnings, fmt.Sprintf("Leaf certificate %d is expired or near expiry. Time to expire is: %s.", c.SerialNumber, timeToExpiry)) } } for _, c := range interCerts { if willExpire, timeToExpiry := NearExpiration(c); willExpire { - warnings = append(warnings, fmt.Sprintf("intermediate certificate %d is expired or near expiry. Time to expire is: %s", c.SerialNumber, timeToExpiry)) + warnings = append(warnings, fmt.Sprintf("Intermediate certificate %d is expired or near expiry. Time to expire is: %s.", c.SerialNumber, timeToExpiry)) } } for _, c := range rootCerts { if willExpire, timeToExpiry := NearExpiration(c); willExpire { - warnings = append(warnings, fmt.Sprintf("root certificate %d is expired or near expiry. Time to expire is: %s", c.SerialNumber, timeToExpiry)) + warnings = append(warnings, fmt.Sprintf("Root certificate %d is expired or near expiry. Time to expire is: %s.", c.SerialNumber, timeToExpiry)) } } @@ -282,7 +282,7 @@ func TLSMutualExclusionCertCheck(l *configutil.Listener) (int, string) { if l.TLSDisableClientCerts { if l.TLSRequireAndVerifyClientCert { - return 1, "the tls_disable_client_certs and tls_require_and_verify_client_cert fields in the listener stanza of the vault server config are mutually exclusive fields. Please ensure they are not both set to true." + return 1, "The tls_disable_client_certs and tls_require_and_verify_client_cert fields in the listener stanza of the Vault server configuration are mutually exclusive fields. Please ensure they are not both set to true." } } return 0, "" @@ -310,33 +310,33 @@ func TLSCAFileCheck(CAFilePath string) ([]string, error) { } if len(rootCerts) == 0 { - return nil, fmt.Errorf("No root cert found!") + return nil, fmt.Errorf("No root certificate found in CA certificate file.") } if len(rootCerts) > 1 { - warningsSlc = append(warningsSlc, fmt.Sprintf("Found Multiple rootCerts instead of just one!")) + warningsSlc = append(warningsSlc, fmt.Sprintf("Found multiple root certificates in CA Certificate file instead of just one.")) } // Checking for Self-Signed cert and return an explicit error about it. // Self-Signed certs are placed in the leafCerts slice when parsed. if len(leafCerts) > 0 && !leafCerts[0].IsCA && bytes.Equal(leafCerts[0].RawIssuer, leafCerts[0].RawSubject) { - return warningsSlc, fmt.Errorf("Found a Self-Signed certificate!") + warningsSlc = append(warningsSlc, "Found a self-signed certificate in the CA certificate file.") } if len(interCerts) > 0 { - return warningsSlc, fmt.Errorf("Found at least one intermediate cert in a root CA cert.") + warningsSlc = append(warningsSlc, "Found at least one intermediate certificate in the CA certificate file.") } if len(leafCerts) > 0 { - return warningsSlc, fmt.Errorf("Found at least one leafCert in a root CA cert.") + warningsSlc = append(warningsSlc, "Found at least one leaf certificate in the CA certificate file.") } var warnings []string // Check for TLS Warnings warnings, err = TLSFileWarningChecks(leafCerts, interCerts, rootCerts) - warningsSlc = append(warningsSlc, warnings...) - for i, warning := range warningsSlc { - warningsSlc[i] = strings.Replace(warning, "leaf", "root", -1) + for i, warning := range warnings { + warnings[i] = strings.Replace(warning, "leaf", "root", -1) } + warningsSlc = append(warningsSlc, warnings...) if err != nil { return warningsSlc, err } diff --git a/vault/diagnose/tls_verification_test.go b/vault/diagnose/tls_verification_test.go index 0017d9b6e2..49a3379b09 100644 --- a/vault/diagnose/tls_verification_test.go +++ b/vault/diagnose/tls_verification_test.go @@ -53,7 +53,7 @@ func TestTLSFakeCert(t *testing.T) { if len(errs) != 1 { t.Fatalf("more than one error returned: %+v", errs) } - if !strings.Contains(errs[0].Error(), "could not decode cert") { + if !strings.Contains(errs[0].Error(), "Could not decode certificate") { t.Fatalf("Bad error message: %s", errs[0]) } } @@ -173,7 +173,7 @@ func TestTLSMultiKeys(t *testing.T) { if errs == nil || len(errs) != 1 { t.Fatalf("TLS Config check on fake certificate should fail") } - if !strings.Contains(errs[0].Error(), "pem block does not parse to a certificate") { + if !strings.Contains(errs[0].Error(), "PEM block does not parse to a certificate") { t.Fatalf("Bad error message: %s", errs[0]) } } @@ -379,8 +379,8 @@ func TestTLSClientCAVerfiyMutualExclusion(t *testing.T) { if status == 0 { t.Fatalf("TLS config check should have failed when both 'tls_disable_client_certs' and 'tls_require_and_verify_client_cert' are true") } - if !strings.Contains(err, "the tls_disable_client_certs and tls_require_and_verify_client_cert fields in the "+ - "listener stanza of the vault server config are mutually exclusive fields") { + if !strings.Contains(err, "The tls_disable_client_certs and tls_require_and_verify_client_cert fields in the "+ + "listener stanza of the Vault server configuration are mutually exclusive fields") { t.Fatalf("Bad error message: %s", err) } } @@ -424,11 +424,18 @@ func TestTLSLeafCertInClientCAFile(t *testing.T) { TLSDisableClientCerts: false, }, } - _, errs := ListenerChecks(context.Background(), listeners) + warnings, errs := ListenerChecks(context.Background(), listeners) + fmt.Println(warnings) if errs == nil || len(errs) != 1 { - t.Fatalf("TLS Config check on bad ClientCAFile certificate should fail") + t.Fatalf("TLS Config check on bad ClientCAFile certificate should fail once") } - if !strings.Contains(errs[0].Error(), "Found at least one leafCert in a root CA cert.") { + if warnings == nil || len(warnings) != 1 { + t.Fatalf("TLS Config check on bad ClientCAFile certificate should warn once") + } + if !strings.Contains(warnings[0], "Found at least one leaf certificate in the CA certificate file.") { + t.Fatalf("Bad error message: %s", warnings[0]) + } + if !strings.Contains(errs[0].Error(), "signed by unknown authority") { t.Fatalf("Bad error message: %s", errs[0]) } } @@ -452,7 +459,7 @@ func TestTLSNoRootInClientCAFile(t *testing.T) { if errs == nil { t.Fatalf("TLS Config check on bad ClientCAFile certificate should fail") } - if !strings.Contains(errs[0].Error(), "No root cert found!") { + if !strings.Contains(errs[0].Error(), " No root certificate found") { t.Fatalf("Bad error message: %s", errs[0]) } } @@ -472,12 +479,12 @@ func TestTLSIntermediateCertInClientCAFile(t *testing.T) { TLSDisableClientCerts: false, }, } - _, errs := ListenerChecks(context.Background(), listeners) - if errs == nil || len(errs) != 1 { + warnings, _ := ListenerChecks(context.Background(), listeners) + if warnings == nil || len(warnings) != 1 { t.Fatalf("TLS Config check on bad ClientCAFile certificate should fail") } - if !strings.Contains(errs[0].Error(), "Found at least one intermediate cert in a root CA cert.") { - t.Fatalf("Bad error message: %s", errs[0]) + if !strings.Contains(warnings[0], "Found at least one intermediate certificate in the CA certificate file.") { + t.Fatalf("Bad error message: %s", warnings[0]) } } @@ -503,7 +510,7 @@ func TestTLSMultipleRootInClietCACert(t *testing.T) { if warnings == nil { t.Fatalf("TLS Config check on valid but bad certificate should warn") } - if !strings.Contains(warnings[0], "Found Multiple rootCerts instead of just one!") { + if !strings.Contains(warnings[0], "Found multiple root certificates in CA Certificate file instead of just one.") { t.Fatalf("Bad warning: %s", warnings[0]) } } @@ -527,7 +534,7 @@ func TestTLSSelfSignedCert(t *testing.T) { if errs == nil { t.Fatalf("Self-signed certificate is insecure") } - if !strings.Contains(errs[0].Error(), "No root cert found!") { + if !strings.Contains(errs[0].Error(), "No root certificate found") { t.Fatalf("Bad error message: %s", errs[0]) } }