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: swayne275 <[email protected]> * comments Co-authored-by: swayne275 <[email protected]>
hashicorp · Jun 24, 2021 · bbef373 · bbef373
1 parent 160c409
commit bbef373
Show file tree

Hide file tree

Showing 6 changed files with 311 additions and 326 deletions.
diff --git a/command/operator_diagnose.go b/command/operator_diagnose.go
@@ -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
 	})

diff --git a/vault/diagnose/helpers_test.go b/vault/diagnose/helpers_test.go
@@ -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,27 +85,51 @@ 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)
 
 	Test(ctx, "dispose-grounds", Skippable("dispose-grounds", disposeGrounds))
 	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

diff --git a/vault/diagnose/output.go b/vault/diagnose/output.go
@@ -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')

diff --git a/vault/diagnose/tls_verification.go b/vault/diagnose/tls_verification.go
@@ -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