Unverified Commit bbef373a authored by Hridoy Roy's avatar Hridoy Roy Committed by GitHub
Browse files

TLS Verification Bugfixes (#11910)


* tls verification bugfix

* tls verification bugfix

* allow diagnose fail to report status when there are also warnings

* allow diagnose fail to report status when there are also warnings

* Update vault/diagnose/helpers_test.go
Co-authored-by: default avatarswayne275 <swayne275@gmail.com>

* comments
Co-authored-by: default avatarswayne275 <swayne275@gmail.com>
Showing with 311 additions and 326 deletions
+311 -326
......@@ -426,7 +426,7 @@ func (c *OperatorDiagnoseCommand) offlineDiagnostics(ctx context.Context) error
SEALFAIL:
sealspan.End()
var coreConfig vault.CoreConfig
if err := diagnose.Test(ctx, "setup-core", func(ctx context.Context) error {
diagnose.Test(ctx, "setup-core", func(ctx context.Context) error {
var secureRandomReader io.Reader
// prepare a secure random reader for core
secureRandomReader, err = configutil.CreateSecureRandomReaderFunc(config.SharedConfig, barrierWrapper)
......@@ -436,9 +436,7 @@ SEALFAIL:
diagnose.SpotOk(ctx, "init-randreader", "")
coreConfig = createCoreConfig(server, config, *backend, configSR, barrierSeal, unwrapSeal, metricsHelper, metricSink, secureRandomReader)
return nil
}); err != nil {
diagnose.Error(ctx, err)
}
})
var disableClustering bool
diagnose.Test(ctx, "setup-ha-storage", func(ctx context.Context) error {
......@@ -514,6 +512,9 @@ SEALFAIL:
info := make(map[string]string)
var listeners []listenerutil.Listener
var status int
diagnose.ListenerChecks(ctx, config.Listeners)
diagnose.Test(ctx, "create-listeners", func(ctx context.Context) error {
status, listeners, _, err = server.InitListeners(config, disableClustering, &infoKeys, &info)
if status != 0 {
......@@ -531,32 +532,7 @@ SEALFAIL:
}
}
defer c.cleanupGuard.Do(listenerCloseFunc)
listenerTLSContext, listenerTLSSpan := diagnose.StartSpan(ctx, "check-listener-tls")
sanitizedListeners := make([]listenerutil.Listener, 0, len(config.Listeners))
for _, ln := range lns {
if ln.Config.TLSDisable {
diagnose.Warn(listenerTLSContext, "TLS is disabled in a Listener config stanza.")
continue
}
if ln.Config.TLSDisableClientCerts {
diagnose.Warn(listenerTLSContext, "TLS for a listener is turned on without requiring client certs.")
}
err = diagnose.TLSMutualExclusionCertCheck(ln.Config)
if err != nil {
diagnose.Warn(listenerTLSContext, fmt.Sprintf("TLSDisableClientCerts and TLSRequireAndVerifyClientCert should not both be set. %s", err))
}
sanitizedListeners = append(sanitizedListeners, listenerutil.Listener{
Listener: ln.Listener,
Config: ln.Config,
})
}
diagnose.ListenerChecks(listenerTLSContext, sanitizedListeners)
listenerTLSSpan.End()
c.cleanupGuard.Do(listenerCloseFunc)
return nil
})
......
......@@ -3,11 +3,12 @@ package diagnose
import (
"context"
"errors"
"github.com/go-test/deep"
"os"
"reflect"
"strings"
"testing"
"github.com/go-test/deep"
)
const getMoreCoffee = "You'll find more coffee in the freezer door, or consider buying more for the office."
......@@ -21,6 +22,26 @@ func TestDiagnoseOtelResults(t *testing.T) {
},
Advice: getMoreCoffee,
Children: []*Result{
{
Name: "prepare-kitchen",
Status: ErrorStatus,
Children: []*Result{
{
Name: "build-microwave",
Status: ErrorStatus,
Children: []*Result{
{
Name: "buy-parts",
Status: ErrorStatus,
Message: "no stores sell microwave parts, please buy a microwave instead.",
Warnings: []string{
"warning: you are about to try to build a microwave from scratch.",
},
},
},
},
},
},
{
Name: "warm-milk",
Status: OkStatus,
......@@ -54,6 +75,7 @@ func TestDiagnoseOtelResults(t *testing.T) {
results := sess.Finalize(ctx)
results.ZeroTimes()
if !reflect.DeepEqual(results, expected) {
t.Fatalf("results mismatch: %s", strings.Join(deep.Equal(results, expected), "\n"))
}
......@@ -63,20 +85,25 @@ func TestDiagnoseOtelResults(t *testing.T) {
const coffeeLeft = 3
func makeCoffee(ctx context.Context) error {
if coffeeLeft < 5 {
Warn(ctx, "coffee getting low")
Advise(ctx, getMoreCoffee)
}
err := Test(ctx, "warm-milk", warmMilk)
if err != nil {
return err
}
// To mimic listener TLS checks, we'll see if we can nest a Test and add errors in the function
Test(ctx, "prepare-kitchen", func(ctx context.Context) error {
return Test(ctx, "build-microwave", func(ctx context.Context) error {
buildMicrowave(ctx)
return nil
})
})
err = brewCoffee(ctx)
if err != nil {
return err
}
Test(ctx, "warm-milk", func(ctx context.Context) error {
return warmMilk(ctx)
})
brewCoffee(ctx)
SpotCheck(ctx, "pick-scone", pickScone)
......@@ -84,6 +111,25 @@ func makeCoffee(ctx context.Context) error {
return nil
}
// buildMicrowave will throw an error in the function itself to fail the span,
// but will return nil so the caller test doesn't necessarily throw an error.
// The intended behavior is that the superspan will detect the failed subspan
// and fail regardless. This happens when Fail is used to fail the span, but not
// when Error is used. See the comment in the function itself.
func buildMicrowave(ctx context.Context) error {
ctx, span := StartSpan(ctx, "buy-parts")
Fail(ctx, "no stores sell microwave parts, please buy a microwave instead.")
// The error line here does not actually yield an error in the output.
// TODO: Debug this. In the meantime, always use Fail over Error.
// Error(ctx, errors.New("no stores sell microwave parts, please buy a microwave instead."))
Warn(ctx, "warning: you are about to try to build a microwave from scratch.")
span.End()
return nil
}
func warmMilk(ctx context.Context) error {
// Always succeeds
return nil
......
......@@ -4,13 +4,14 @@ import (
"context"
"errors"
"fmt"
"go.opentelemetry.io/otel/attribute"
"io"
"sort"
"strings"
"sync"
"time"
"go.opentelemetry.io/otel/attribute"
wordwrap "github.com/mitchellh/go-wordwrap"
"go.opentelemetry.io/otel/codes"
sdktrace "go.opentelemetry.io/otel/sdk/trace"
......@@ -318,22 +319,20 @@ func (r *Result) StringWrapped(wrapLimit int) string {
func (r *Result) write(sb *strings.Builder, depth int, limit int) {
indent(sb, depth)
var prelude string
if len(r.Warnings) == 0 {
switch r.Status {
case OkStatus:
prelude = status_ok
case WarningStatus:
prelude = status_warn
case ErrorStatus:
prelude = status_failed
case SkippedStatus:
prelude = status_skipped
}
prelude = prelude + r.Name
switch r.Status {
case OkStatus:
prelude = status_ok
case WarningStatus:
prelude = status_warn
case ErrorStatus:
prelude = status_failed
case SkippedStatus:
prelude = status_skipped
}
prelude = prelude + r.Name
if r.Message != "" {
prelude = prelude + ": " + r.Message
}
if r.Message != "" {
prelude = prelude + ": " + r.Message
}
warnings := r.Warnings
if r.Message == "" && len(warnings) > 0 {
......@@ -343,6 +342,7 @@ func (r *Result) write(sb *strings.Builder, depth int, limit int) {
warnings = warnings[1:]
}
}
writeWrapped(sb, prelude, depth+1, limit)
for _, w := range warnings {
sb.WriteRune('\n')
......
......@@ -12,7 +12,6 @@ import (
"time"
"github.com/hashicorp/vault/internalshared/configutil"
"github.com/hashicorp/vault/internalshared/listenerutil"
"github.com/hashicorp/vault/sdk/helper/tlsutil"
)
......@@ -21,17 +20,32 @@ 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 []listenerutil.Listener) ([]string, []error) {
func ListenerChecks(ctx context.Context, listeners []*configutil.Listener) ([]string, []error) {
testName := "check-listener-tls"
ctx, span := StartSpan(ctx, testName)
defer span.End()
// These aggregated warnings and errors are returned purely for testing purposes.
// The errors and warnings will report in this function itself.
var listenerWarnings []string
var listenerErrors []error
for _, listener := range listeners {
l := listener.Config
for _, l := range listeners {
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))
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))
}
status, warning := TLSMutualExclusionCertCheck(l)
if status == 1 {
Warn(ctx, warning)
}
// Perform the TLS version check for listeners.
if l.TLSMinVersion == "" {
l.TLSMinVersion = "tls12"
......@@ -43,13 +57,13 @@ func ListenerChecks(ctx context.Context, listeners []listenerutil.Listener) ([]s
if !ok {
err := fmt.Errorf("listener at address: %s has error %s: ", listenerID, fmt.Sprintf(minVersionError, l.TLSMinVersion))
listenerErrors = append(listenerErrors, err)
Error(ctx, 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))
listenerErrors = append(listenerErrors, err)
Error(ctx, err)
Fail(ctx, err.Error())
}
// Perform checks on the TLS Cryptographic Information.
......@@ -74,8 +88,7 @@ func outputError(ctx context.Context, newWarnings, listenerWarnings []string, ne
if newErr != nil {
errMsg := listenerID + ": " + newErr.Error()
listenerErrors = append(listenerErrors, fmt.Errorf(errMsg))
Error(ctx, fmt.Errorf(errMsg))
Fail(ctx, errMsg)
}
return listenerWarnings, listenerErrors
}
......@@ -256,15 +269,14 @@ func NearExpiration(c *x509.Certificate) (bool, time.Duration) {
}
// TLSMutualExclusionCertCheck returns error if both TLSDisableClientCerts and TLSRequireAndVerifyClientCert are set
func TLSMutualExclusionCertCheck(l *configutil.Listener) error {
func TLSMutualExclusionCertCheck(l *configutil.Listener) (int, string) {
if l.TLSDisableClientCerts {
if l.TLSRequireAndVerifyClientCert {
return fmt.Errorf("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 config are mutually exclusive fields. Please ensure they are not both set to true."
}
}
return nil
return 0, ""
}
// TLSClientCAFileCheck Checks the validity of a client CA file
......
This diff is collapsed.
......@@ -11,7 +11,6 @@ import (
wrapping "github.com/hashicorp/go-kms-wrapping"
"github.com/hashicorp/vault/physical/raft"
"github.com/hashicorp/vault/vault/diagnose"
"github.com/hashicorp/vault/vault/seal"
aeadwrapper "github.com/hashicorp/go-kms-wrapping/wrappers/aead"
......@@ -467,11 +466,9 @@ func (c *Core) UnsealWithStoredKeys(ctx context.Context) error {
// This usually happens when auto-unseal is configured, but the servers have
// not been initialized yet.
if len(keys) == 0 {
diagnose.Error(ctx, errors.New("stored unseal keys are supported, but none were found"))
return NewNonFatalError(errors.New("stored unseal keys are supported, but none were found"))
}
if len(keys) != 1 {
diagnose.Error(ctx, errors.New("expected exactly one stored key"))
return NewNonFatalError(errors.New("expected exactly one stored key"))
}
......@@ -485,7 +482,6 @@ func (c *Core) UnsealWithStoredKeys(ctx context.Context) error {
// subset of the required threshold of keys. We still consider this a
// "success", since trying again would yield the same result.
c.Logger().Warn("vault still sealed after using stored unseal key")
diagnose.Warn(ctx, "vault still sealed after using stored unseal key")
} else {
c.Logger().Info("unsealed with stored key")
}
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment