chore: refactor complete method to pull out validation and more strictly validate TLS options (#3064)

## Description
If you provided one of the GRPC TLS options (between certpath and
keypath) but not the other, you'd end up with a plaintext configuration.
This makes it so that that's an error state and refactors some other
methods to be able to test them.

## Changes
* Pull out some validation/handling methods
* Test those methods
* Add validation to GRPC TLS options
## Testing
Review.

Author: tstirrat15

CHANGELOG.md

@@ -21,6 +21,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 - Use cgroup-aware memory detection for cache and watch buffer sizing in containerized environments (https://github.com/authzed/spicedb/pull/3000)
 - Upgraded the spanner client, which changed the internal implementation to not use a session pool. This means that the `--datastore-spanner-max-sessions` and `--datastore-spanner-min-sessions` flags are now deprecated and no-op. We also strongly recommend using [Application Default Credentials](https://docs.cloud.google.com/docs/authentication/client-libraries#adc) in favor of a credentials file. (https://github.com/authzed/spicedb/pull/3038)
 - Query Planner: error `"ERROR: index \"pk_relation_tuple\" cannot be used for this query (SQLSTATE 42809)"` returned when using wildcards (https://github.com/authzed/spicedb/pull/3039)
+- Providing one of (`--grpc-tls-cert-path`, `--grpc-tls-key-path`) but not the other is now considered an error state, as both are necessary if you want to use TLS.
 
 ## [1.51.1] - 2026-04-14
 ### Fixed

pkg/cmd/server/server.go

@@ -177,23 +177,9 @@ func (c *Config) Complete(ctx context.Context) (RunnableServer, error) {
 		}
 	}()
 
-	if len(c.PresharedSecureKey) < 1 && c.GRPCAuthFunc == nil {
-		return nil, errors.New("a preshared key must be provided to authenticate API requests")
-	}
-
-	if c.GRPCAuthFunc == nil {
-		log.Ctx(ctx).Trace().Int("preshared-keys-count", len(c.PresharedSecureKey)).Msg("using gRPC auth with preshared key(s)")
-		for index, presharedKey := range c.PresharedSecureKey {
-			if len(presharedKey) == 0 {
-				return nil, fmt.Errorf("preshared key #%d is empty", index+1)
-			}
-
-			log.Ctx(ctx).Trace().Int("preshared-key-"+strconv.Itoa(index+1)+"-length", len(presharedKey)).Msg("preshared key configured")
-		}
-
-		c.GRPCAuthFunc = auth.MustRequirePresharedKey(c.PresharedSecureKey)
-	} else {
-		log.Ctx(ctx).Trace().Msg("using preconfigured auth function")
+	err = c.handleGrpcAuthn(ctx)
+	if err != nil {
+		return nil, err
 	}
 
 	nscc, err := CompleteCache[cache.StringKey, schemacaching.CacheEntry](&c.NamespaceCacheConfig)
@@ -206,7 +192,7 @@ func (c *Config) Complete(ctx context.Context) (RunnableServer, error) {
 	if ds == nil {
 		var err error
 		c.supportOldAndNewReadReplicaConnectionPoolFlags()
-		ds, err = datastorecfg.NewDatastore(context.Background(), c.DatastoreConfig.ToOption(),
+		ds, err = datastorecfg.NewDatastore(ctx, c.DatastoreConfig.ToOption(),
 			// Datastore's filter maximum ID count is set to the max size, since the number of elements to be dispatched
 			// are at most the number of elements returned from a datastore query
 			datastorecfg.WithFilterMaximumIDCount(c.DispatchChunkSize),
@@ -254,6 +240,7 @@ func (c *Config) Complete(ctx context.Context) (RunnableServer, error) {
 		closeables.AddWithoutError(cc.Close)
 		log.Ctx(ctx).Info().EmbedObject(cc).Msg("configured dispatch cache")
 
+		// TODO: is there a circumstance under which this could be empty?
 		dispatchPresharedKey := ""
 		if len(c.PresharedSecureKey) > 0 {
 			dispatchPresharedKey = c.PresharedSecureKey[0]
@@ -352,22 +339,9 @@ func (c *Config) Complete(ctx context.Context) (RunnableServer, error) {
 		serverName = "spicedb"
 	}
 
-	var mismatchZedTokenOption consistency.MismatchingTokenOption
-	switch c.MismatchZedTokenBehavior {
-	case "":
-		fallthrough
-
-	case "full-consistency":
-		mismatchZedTokenOption = consistency.TreatMismatchingTokensAsFullConsistency
-
-	case "min-latency":
-		mismatchZedTokenOption = consistency.TreatMismatchingTokensAsMinLatency
-
-	case "error":
-		mismatchZedTokenOption = consistency.TreatMismatchingTokensAsError
-
-	default:
-		return nil, fmt.Errorf("unknown mismatched zedtoken behavior: %s", c.MismatchZedTokenBehavior)
+	mismatchZedTokenOption, err := c.handleMismatchZedTokenOption()
+	if err != nil {
+		return nil, err
 	}
 
 	memoryUsageProvider := c.BuildMemoryUsageProvider()
@@ -542,6 +516,48 @@ func (c *Config) Complete(ctx context.Context) (RunnableServer, error) {
 	}, nil
 }
 
+func (c *Config) handleGrpcAuthn(ctx context.Context) error {
+	if len(c.PresharedSecureKey) < 1 && c.GRPCAuthFunc == nil {
+		return errors.New("a preshared key must be provided to authenticate API requests")
+	}
+
+	if c.GRPCAuthFunc == nil {
+		log.Ctx(ctx).Trace().Int("preshared-keys-count", len(c.PresharedSecureKey)).Msg("using gRPC auth with preshared key(s)")
+		for index, presharedKey := range c.PresharedSecureKey {
+			if len(presharedKey) == 0 {
+				return fmt.Errorf("preshared key #%d is empty", index+1)
+			}
+
+			log.Ctx(ctx).Trace().Int("preshared-key-"+strconv.Itoa(index+1)+"-length", len(presharedKey)).Msg("preshared key configured")
+		}
+
+		c.GRPCAuthFunc = auth.MustRequirePresharedKey(c.PresharedSecureKey)
+	} else {
+		log.Ctx(ctx).Trace().Msg("using preconfigured auth function")
+	}
+	return nil
+}
+
+func (c *Config) handleMismatchZedTokenOption() (consistency.MismatchingTokenOption, error) {
+	switch c.MismatchZedTokenBehavior {
+	case "":
+		fallthrough
+
+	case "full-consistency":
+		return consistency.TreatMismatchingTokensAsFullConsistency, nil
+
+	case "min-latency":
+		return consistency.TreatMismatchingTokensAsMinLatency, nil
+
+	case "error":
+		return consistency.TreatMismatchingTokensAsError, nil
+
+	default:
+		var zero consistency.MismatchingTokenOption
+		return zero, fmt.Errorf("unknown mismatched zedtoken behavior: %s", c.MismatchZedTokenBehavior)
+	}
+}
+
 func (c *Config) BuildMemoryUsageProvider() memoryprotection.MemoryUsageProvider {
 	if c.EnableMemoryProtectionMiddleware {
 		if DefaultMemoryUsageProvider != nil {

pkg/cmd/server/server_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"reflect"
 	"slices"
 	"testing"
 	"time"
@@ -718,3 +719,30 @@ func TestDebugMapRedactsURI(t *testing.T) {
 	out := fmt.Sprintln(c.FlatDebugMap())
 	require.NotContains(t, out, "postgresql://root@localhost:26257/defaultdb")
 }
+
+func TestHandleGRPCAuthn(t *testing.T) {
+	t.Run("empty configuration errors", func(t *testing.T) {
+		require.ErrorContains(t, (&Config{}).handleGrpcAuthn(t.Context()), "a preshared key must be provided")
+	})
+	t.Run("preshared key list with empty key errors", func(t *testing.T) {
+		require.ErrorContains(t, (&Config{PresharedSecureKey: []string{"foo", ""}}).handleGrpcAuthn(t.Context()), "is empty")
+	})
+	t.Run("if auth fn is provided it is used", func(t *testing.T) {
+		authFn := func(ctx context.Context) (context.Context, error) {
+			return ctx, nil
+		}
+		config := &Config{GRPCAuthFunc: authFn}
+		require.NoError(t, config.handleGrpcAuthn(t.Context()))
+		// Assert that the function objects are the same. this hackaround is necessary
+		// because of the way that golang treats function values.
+		require.Equal(t,
+			reflect.ValueOf(authFn).Pointer(),
+			reflect.ValueOf(config.GRPCAuthFunc).Pointer(),
+		)
+	})
+	t.Run("if auth fn is not provided, one is created", func(t *testing.T) {
+		config := &Config{PresharedSecureKey: []string{"foo"}}
+		require.NoError(t, config.handleGrpcAuthn(t.Context()))
+		require.NotNil(t, config.GRPCAuthFunc)
+	})
+}

pkg/cmd/util/util.go

@@ -145,22 +145,38 @@ func (c *GRPCServerConfig) listenerAndDialer() (net.Listener, DialFunc, NetDialF
 }
 
 func (c *GRPCServerConfig) tlsOpts() ([]grpc.ServerOption, *x509util.CertWatcher, error) {
-	switch {
-	case c.TLSCertPath == "" && c.TLSKeyPath == "":
-		return nil, nil, nil
-	case c.TLSCertPath != "" && c.TLSKeyPath != "":
-		watcher, err := x509util.NewTLSCertWatcher(c.TLSCertPath, c.TLSKeyPath)
-		if err != nil {
-			return nil, nil, err
-		}
-		creds := credentials.NewTLS(&tls.Config{
-			GetCertificate: watcher.GetCertificate,
-			MinVersion:     tls.VersionTLS12,
-		})
-		return []grpc.ServerOption{grpc.Creds(creds)}, watcher, nil
-	default:
+	// Ensure that we've either got both TLS config options or neither
+	if err := c.validateTLSConfig(); err != nil {
+		return nil, nil, err
+	}
+
+	// If no TLS configuration is provided, we're in plaintext mode
+	if c.TLSCertPath == "" {
 		return nil, nil, nil
 	}
+
+	// Else we've got TLS configuration and we'll construct the server options
+	watcher, err := x509util.NewTLSCertWatcher(c.TLSCertPath, c.TLSKeyPath)
+	if err != nil {
+		return nil, nil, err
+	}
+	creds := credentials.NewTLS(&tls.Config{
+		GetCertificate: watcher.GetCertificate,
+		MinVersion:     tls.VersionTLS12,
+	})
+	return []grpc.ServerOption{grpc.Creds(creds)}, watcher, nil
+}
+
+func (c *GRPCServerConfig) validateTLSConfig() error {
+	if (c.TLSCertPath == "") != (c.TLSKeyPath == "") {
+		return fmt.Errorf(
+			"failed to start %s gRPC server: must provide both --%s-tls-cert-path and --%s-tls-key-path",
+			c.flagPrefix,
+			c.flagPrefix,
+			c.flagPrefix,
+		)
+	}
+	return nil
 }
 
 func (c *GRPCServerConfig) clientCreds() (credentials.TransportCredentials, error) {

pkg/cmd/util/util_test.go

@@ -21,3 +21,15 @@ func TestDisabledHTTP(t *testing.T) {
 	require.NoError(t, s.ListenAndServe())
 	s.Close()
 }
+
+func TestValidateTLSConfig(t *testing.T) {
+	t.Run("both options present is fine", func(t *testing.T) {
+		require.NoError(t, (&GRPCServerConfig{TLSCertPath: "some path", TLSKeyPath: "some other path"}).validateTLSConfig())
+	})
+	t.Run("neither option present is fine", func(t *testing.T) {
+		require.NoError(t, (&GRPCServerConfig{}).validateTLSConfig())
+	})
+	t.Run("one present but not the other returns an error", func(t *testing.T) {
+		require.ErrorContains(t, (&GRPCServerConfig{TLSCertPath: "some path"}).validateTLSConfig(), "must provide both")
+	})
+}