rpc: split URL generation code to a separate lightweight package (cli refactor sequence 2/n)

pkg/cli/BUILD.bazel

@@ -133,6 +133,8 @@ go_library(
         "//pkg/rpc",
         "//pkg/security",
         "//pkg/security/certnames",
+        "//pkg/security/clientsecopts",
+        "//pkg/security/securityassets",
         "//pkg/security/securitytest",
         "//pkg/security/username",
         "//pkg/server",

pkg/cli/client_url.go

@@ -15,10 +15,8 @@ import (
 	"path/filepath"
 
 	"github.com/cockroachdb/cockroach/pkg/cli/cliflags"
-	"github.com/cockroachdb/cockroach/pkg/roachpb"
-	"github.com/cockroachdb/cockroach/pkg/rpc"
-	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/security/certnames"
+	"github.com/cockroachdb/cockroach/pkg/security/clientsecopts"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/server/pgurl"
 	"github.com/cockroachdb/errors"
@@ -360,8 +358,11 @@ func (cliCtx *cliContext) makeClientConnURL() (*pgurl.URL, error) {
 		userName = username.RootUserName()
 	}
 
-	sCtx := rpc.MakeSecurityContext(cliCtx.Config, security.CommandTLSSettings{}, roachpb.SystemTenantID)
-	if err := sCtx.LoadSecurityOptions(purl, userName); err != nil {
+	ccopts := clientsecopts.ClientOptions{
+		Insecure: cliCtx.Config.Insecure,
+		CertsDir: cliCtx.Config.SSLCertsDir,
+	}
+	if err := clientsecopts.LoadSecurityOptions(ccopts, purl, userName); err != nil {
 		return nil, err
 	}
 

pkg/cli/connect.go

@@ -18,6 +18,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/cli/clierrorplus"
 	"github.com/cockroachdb/cockroach/pkg/cli/cliflags"
 	"github.com/cockroachdb/cockroach/pkg/security/certnames"
+	"github.com/cockroachdb/cockroach/pkg/security/securityassets"
 	"github.com/cockroachdb/cockroach/pkg/server"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/errors"
@@ -66,7 +67,8 @@ func runConnectInit(cmd *cobra.Command, args []string) (retErr error) {
 	// If the node cert already exists, skip all the complexity of setting up
 	// servers, etc.
 	cl := certnames.MakeLocator(baseCfg.SSLCertsDir)
-	if exists, err := cl.HasNodeCert(); err != nil {
+	loader := securityassets.GetLoader()
+	if exists, err := loader.FileExists(cl.NodeCertPath()); err != nil {
 		return err
 	} else if exists {
 		return errors.Newf("node certificate already exists in %s", baseCfg.SSLCertsDir)

pkg/cli/start.go

@@ -37,11 +37,12 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/rpc"
-	"github.com/cockroachdb/cockroach/pkg/security"
+	"github.com/cockroachdb/cockroach/pkg/security/clientsecopts"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/server"
 	"github.com/cockroachdb/cockroach/pkg/server/status"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys"
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/storage/fs"
@@ -518,8 +519,8 @@ func runStart(cmd *cobra.Command, args []string, startSingleNode bool) (returnEr
 			// (Re-)compute the client connection URL. We cannot do this
 			// earlier (e.g. above, in the runStart function) because
 			// at this time the address and port have not been resolved yet.
-			sCtx := rpc.MakeSecurityContext(serverCfg.Config, security.ClusterTLSSettings(serverCfg.Settings), roachpb.SystemTenantID)
-			pgURL, err := sCtx.PGURL(url.User(username.RootUser))
+			clientConnOptions, serverParams := makeServerOptionsForURL(&serverCfg)
+			pgURL, err := clientsecopts.MakeURLForServer(clientConnOptions, serverParams, url.User(username.RootUser))
 			if err != nil {
 				log.Errorf(ctx, "failed computing the URL: %v", err)
 				return
@@ -911,6 +912,24 @@ func waitForShutdown(
 	return returnErr
 }
 
+// makeServerOptionsForURL creates the input for MakeURLForServer().
+// Beware of not calling this too early; the server address
+// is finalized late in the network initialization sequence.
+func makeServerOptionsForURL(
+	serverCfg *server.Config,
+) (clientsecopts.ClientOptions, clientsecopts.ServerParameters) {
+	clientConnOptions := clientsecopts.ClientOptions{
+		Insecure: serverCfg.Config.Insecure,
+		CertsDir: serverCfg.Config.SSLCertsDir,
+	}
+	serverParams := clientsecopts.ServerParameters{
+		ServerAddr:      serverCfg.Config.SQLAdvertiseAddr,
+		DefaultPort:     base.DefaultPort,
+		DefaultDatabase: catalogkeys.DefaultDatabaseName,
+	}
+	return clientConnOptions, serverParams
+}
+
 // reportServerInfo prints out the server version and network details
 // in a standardized format.
 func reportServerInfo(
@@ -936,8 +955,8 @@ func reportServerInfo(
 	// (Re-)compute the client connection URL. We cannot do this
 	// earlier (e.g. above, in the runStart function) because
 	// at this time the address and port have not been resolved yet.
-	sCtx := rpc.MakeSecurityContext(serverCfg.Config, security.ClusterTLSSettings(serverCfg.Settings), roachpb.SystemTenantID)
-	pgURL, err := sCtx.PGURL(url.User(username.RootUser))
+	clientConnOptions, serverParams := makeServerOptionsForURL(serverCfg)
+	pgURL, err := clientsecopts.MakeURLForServer(clientConnOptions, serverParams, url.User(username.RootUser))
 	if err != nil {
 		log.Ops.Errorf(ctx, "failed computing the URL: %v", err)
 		return err

pkg/rpc/BUILD.bazel

@@ -17,7 +17,6 @@ go_library(
         "heartbeat.go",
         "keepalive.go",
         "metrics.go",
-        "pg.go",
         "snappy.go",
         "tls.go",
     ],
@@ -31,11 +30,8 @@ go_library(
         "//pkg/roachpb",
         "//pkg/security",
         "//pkg/security/certnames",
-        "//pkg/security/securityassets",
         "//pkg/security/username",
-        "//pkg/server/pgurl",
         "//pkg/settings/cluster",
-        "//pkg/sql/catalog/catalogkeys",
         "//pkg/util/contextutil",
         "//pkg/util/envutil",
         "//pkg/util/growstack",
@@ -45,7 +41,6 @@ go_library(
         "//pkg/util/log/severity",
         "//pkg/util/metric",
         "//pkg/util/netutil",
-        "//pkg/util/netutil/addr",
         "//pkg/util/stop",
         "//pkg/util/syncutil",
         "//pkg/util/timeutil",

pkg/security/certnames/BUILD.bazel

@@ -9,8 +9,5 @@ go_library(
     ],
     importpath = "github.com/cockroachdb/cockroach/pkg/security/certnames",
     visibility = ["//visibility:public"],
-    deps = [
-        "//pkg/security/username",
-        "@com_github_cockroachdb_errors//oserror",
-    ],
+    deps = ["//pkg/security/username"],
 )

pkg/security/certnames/locator.go

@@ -15,7 +15,6 @@ import (
 	"path/filepath"
 
 	"github.com/cockroachdb/cockroach/pkg/security/username"
-	"github.com/cockroachdb/errors/oserror"
 )
 
 // A Locator provides locations to certificates.
@@ -101,18 +100,6 @@ func (cl Locator) NodeCertPath() string {
 	return filepath.Join(cl.certsDir, NodeCertFilename())
 }
 
-// HasNodeCert returns true iff the node certificate file already exists.
-func (cl Locator) HasNodeCert() (bool, error) {
-	_, err := os.Stat(cl.NodeCertPath())
-	if err != nil {
-		if oserror.IsNotExist(err) {
-			return false, nil
-		}
-		return false, err
-	}
-	return true, nil
-}
-
 // NodeKeyPath returns the expected file path for the node key.
 func (cl Locator) NodeKeyPath() string {
 	return filepath.Join(cl.certsDir, NodeKeyFilename())

pkg/security/clientsecopts/BUILD.bazel

@@ -0,0 +1,18 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+    name = "clientsecopts",
+    srcs = [
+        "client_opts.go",
+        "doc.go",
+    ],
+    importpath = "github.com/cockroachdb/cockroach/pkg/security/clientsecopts",
+    visibility = ["//visibility:public"],
+    deps = [
+        "//pkg/security/certnames",
+        "//pkg/security/securityassets",
+        "//pkg/security/username",
+        "//pkg/server/pgurl",
+        "//pkg/util/netutil/addr",
+    ],
+)

pkg/rpc/pg.go → pkg/security/clientsecopts/client_opts.go

@@ -1,4 +1,4 @@
-// Copyright 2020 The Cockroach Authors.
+// Copyright 2022 The Cockroach Authors.
 //
 // Use of this software is governed by the Business Source License
 // included in the file licenses/BSL.txt.
@@ -8,25 +8,33 @@
 // by the Apache License, Version 2.0, included in the file
 // licenses/APL.txt.
 
-package rpc
+package clientsecopts
 
 import (
 	"net/url"
 
-	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/security/certnames"
 	"github.com/cockroachdb/cockroach/pkg/security/securityassets"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/server/pgurl"
-	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys"
 	"github.com/cockroachdb/cockroach/pkg/util/netutil/addr"
-	"github.com/cockroachdb/errors"
 )
 
+// ClientOptions defines the configurable connection parameters
+// used as input to compute connection URLs.
+type ClientOptions struct {
+	// Insecure corresponds to the --insecure flag.
+	Insecure bool
+
+	// CertsDir corresponds to the --certs-dir flag.
+	CertsDir string
+}
+
 // LoadSecurityOptions extends a url.Values with SSL settings suitable for
 // the given server config.
-func (ctx *SecurityContext) LoadSecurityOptions(u *pgurl.URL, user username.SQLUsername) error {
+func LoadSecurityOptions(opts ClientOptions, u *pgurl.URL, user username.SQLUsername) error {
 	u.WithUsername(user.Normalized())
-	if ctx.config.Insecure {
+	if opts.Insecure {
 		u.WithInsecure()
 	} else if net, _, _ := u.GetNetworking(); net == pgurl.ProtoTCP {
 		tlsUsed, tlsMode, caCertPath := u.GetTLSOptions()
@@ -39,33 +47,24 @@ func (ctx *SecurityContext) LoadSecurityOptions(u *pgurl.URL, user username.SQLU
 			tlsMode = pgurl.TLSVerifyFull
 		}
 
+		loader := securityassets.GetLoader()
+		cl := certnames.MakeLocator(opts.CertsDir)
+
 		// Only verify-full and verify-ca should be doing certificate verification.
 		if tlsMode == pgurl.TLSVerifyFull || tlsMode == pgurl.TLSVerifyCA {
 			if caCertPath == "" {
 				// We need a CA certificate.
-				// Try to use the cert manager to find one, and if that fails,
+				// Try to use the cert locator to find one, and if that fails,
 				// assume that the Go TLS code will fall back to a OS-level
 				// common trust store.
 
-				// First, initialize the cert manager.
-				cm, err := ctx.GetCertificateManager()
+				exists, err := loader.FileExists(cl.CACertPath())
 				if err != nil {
-					// The SecurityContext was unable to get a cert manager. We
-					// can further distinguish between:
-					// - cert manager initialized OK, but contains no certs.
-					// - cert manager did not initialize (bad certs dir, file access error etc).
-					// The former case is legitimate and we will fall back below.
-					// The latter case is a real problem and needs to pop up to the user.
-					if !errors.Is(err, errNoCertificatesFound) {
-						// The certificate manager could not load properly. Let
-						// the user know.
-						return err
-					}
-					// Fall back: cert manager initialized OK, but no certs found.
+					return err
 				}
-				if ourCACert := cm.CACert(); ourCACert != nil {
-					// The CM has a CA cert. Use that.
-					caCertPath = cm.FullPath(ourCACert.Filename)
+				if exists {
+					// The CL has found a CA cert. Use that.
+					caCertPath = cl.CACertPath()
 				}
 			}
 			// Fallback: if caCertPath was not assigned above, either
@@ -79,12 +78,11 @@ func (ctx *SecurityContext) LoadSecurityOptions(u *pgurl.URL, user username.SQLU
 		u.WithTransport(pgurl.TransportTLS(tlsMode, caCertPath))
 
 		var missing bool // certs found on file system?
-		loader := securityassets.GetLoader()
 
 		// Fetch client certs, but don't fail if they're absent, we may be
 		// using a password.
-		certPath := ctx.ClientCertPath(user)
-		keyPath := ctx.ClientKeyPath(user)
+		certPath := cl.ClientCertPath(user)
+		keyPath := cl.ClientKeyPath(user)
 		_, err1 := loader.Stat(certPath)
 		_, err2 := loader.Stat(keyPath)
 		if err1 != nil || err2 != nil {
@@ -94,8 +92,8 @@ func (ctx *SecurityContext) LoadSecurityOptions(u *pgurl.URL, user username.SQLU
 		// client.node.crt, try with just node.crt.
 		if missing && user.IsNodeUser() {
 			missing = false
-			certPath = ctx.NodeCertPath()
-			keyPath = ctx.NodeKeyPath()
+			certPath = cl.NodeCertPath()
+			keyPath = cl.NodeKeyPath()
 			_, err1 = loader.Stat(certPath)
 			_, err2 = loader.Stat(keyPath)
 			if err1 != nil || err2 != nil {
@@ -117,16 +115,33 @@ func (ctx *SecurityContext) LoadSecurityOptions(u *pgurl.URL, user username.SQLU
 	return nil
 }
 
-// PGURL constructs a URL for the postgres endpoint, given a server
-// config. There is no default database set.
-func (ctx *SecurityContext) PGURL(user *url.Userinfo) (*pgurl.URL, error) {
-	host, port, _ := addr.SplitHostPort(ctx.config.SQLAdvertiseAddr, base.DefaultPort)
+// ServerParameters is used to pass a copy of the parameters computed
+// when a SQL server starts up, to the input of PGURL(), to create
+// a connection URL that clients can connect to.
+type ServerParameters struct {
+	// ServerAddr is the configured server address to use if the URL does not contain one.
+	ServerAddr string
+
+	// DefaultPort is the port number to use if ServerAddr does not contain one.
+	DefaultPort string
+
+	// DefaultDatabase is the default database name to use if the connection URL
+	// does not contain one.
+	DefaultDatabase string
+}
+
+// MakeURLForServer constructs a URL for the postgres endpoint, given a server
+// config.
+func MakeURLForServer(
+	copts ClientOptions, sparams ServerParameters, user *url.Userinfo,
+) (*pgurl.URL, error) {
+	host, port, _ := addr.SplitHostPort(sparams.ServerAddr, sparams.DefaultPort)
 	u := pgurl.New().
 		WithNet(pgurl.NetTCP(host, port)).
-		WithDatabase(catalogkeys.DefaultDatabaseName)
+		WithDatabase(sparams.DefaultDatabase)
 
 	username, _ := username.MakeSQLUsernameFromUserInput(user.Username(), username.PurposeValidation)
-	if err := ctx.LoadSecurityOptions(u, username); err != nil {
+	if err := LoadSecurityOptions(copts, u, username); err != nil {
 		return nil, err
 	}
 	return u, nil

pkg/security/clientsecopts/doc.go

@@ -0,0 +1,13 @@
+// Copyright 2022 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+// Package clientsecopts defines the rules to attach TLS
+// authentication options to a client connection URL.
+package clientsecopts

pkg/security/securityassets/BUILD.bazel

@@ -5,4 +5,5 @@ go_library(
     srcs = ["security_assets.go"],
     importpath = "github.com/cockroachdb/cockroach/pkg/security/securityassets",
     visibility = ["//visibility:public"],
+    deps = ["@com_github_cockroachdb_errors//oserror"],
 )

pkg/security/securityassets/security_assets.go

@@ -13,6 +13,8 @@ package securityassets
 import (
 	"io/ioutil"
 	"os"
+
+	"github.com/cockroachdb/errors/oserror"
 )
 
 // Loader describes the functions necessary to read certificate and key files.
@@ -46,3 +48,15 @@ func SetLoader(al Loader) {
 func ResetLoader() {
 	loaderImpl = defaultLoader
 }
+
+// FileExists returns true iff the target file already exists.
+func (al Loader) FileExists(filename string) (bool, error) {
+	_, err := al.Stat(filename)
+	if err != nil {
+		if oserror.IsNotExist(err) {
+			return false, nil
+		}
+		return false, err
+	}
+	return true, nil
+}