diff --git a/pkg/cli/BUILD.bazel b/pkg/cli/BUILD.bazel index da1f9af6516b..7e3f809f5d57 100644 --- a/pkg/cli/BUILD.bazel +++ b/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", diff --git a/pkg/cli/client_url.go b/pkg/cli/client_url.go index 5a935604c621..2423824d880c 100644 --- a/pkg/cli/client_url.go +++ b/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 } diff --git a/pkg/cli/connect.go b/pkg/cli/connect.go index 2dc81c6cb3bc..805c825c8f17 100644 --- a/pkg/cli/connect.go +++ b/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) diff --git a/pkg/cli/start.go b/pkg/cli/start.go index 84208d5560bc..7f07d1c1e111 100644 --- a/pkg/cli/start.go +++ b/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 diff --git a/pkg/rpc/BUILD.bazel b/pkg/rpc/BUILD.bazel index a7cf01006b31..62c58f20501c 100644 --- a/pkg/rpc/BUILD.bazel +++ b/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", diff --git a/pkg/security/certnames/BUILD.bazel b/pkg/security/certnames/BUILD.bazel index 4562eda2110a..62d477b6a939 100644 --- a/pkg/security/certnames/BUILD.bazel +++ b/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"], ) diff --git a/pkg/security/certnames/locator.go b/pkg/security/certnames/locator.go index 16b8b4d2d0f1..c05e18e242b6 100644 --- a/pkg/security/certnames/locator.go +++ b/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()) diff --git a/pkg/security/clientsecopts/BUILD.bazel b/pkg/security/clientsecopts/BUILD.bazel new file mode 100644 index 000000000000..90b2becbc1ed --- /dev/null +++ b/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", + ], +) diff --git a/pkg/rpc/pg.go b/pkg/security/clientsecopts/client_opts.go similarity index 61% rename from pkg/rpc/pg.go rename to pkg/security/clientsecopts/client_opts.go index 2bf08951c069..0154769869a3 100644 --- a/pkg/rpc/pg.go +++ b/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 diff --git a/pkg/security/clientsecopts/doc.go b/pkg/security/clientsecopts/doc.go new file mode 100644 index 000000000000..52d68c6748c2 --- /dev/null +++ b/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 diff --git a/pkg/security/securityassets/BUILD.bazel b/pkg/security/securityassets/BUILD.bazel index d4f19f98c2c5..4389148eef2b 100644 --- a/pkg/security/securityassets/BUILD.bazel +++ b/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"], ) diff --git a/pkg/security/securityassets/security_assets.go b/pkg/security/securityassets/security_assets.go index 2216197670c3..3a7ed7e44d49 100644 --- a/pkg/security/securityassets/security_assets.go +++ b/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 +} diff --git a/pkg/server/BUILD.bazel b/pkg/server/BUILD.bazel index 0cd4e99762ca..dc5157f6d345 100644 --- a/pkg/server/BUILD.bazel +++ b/pkg/server/BUILD.bazel @@ -118,13 +118,16 @@ go_library( "//pkg/scheduledjobs", "//pkg/security", "//pkg/security/certnames", + "//pkg/security/clientsecopts", "//pkg/security/password", + "//pkg/security/securityassets", "//pkg/security/username", "//pkg/server/debug", "//pkg/server/diagnostics", "//pkg/server/diagnostics/diagnosticspb", "//pkg/server/goroutinedumper", "//pkg/server/heapprofiler", + "//pkg/server/pgurl", "//pkg/server/serverpb", "//pkg/server/serverrules", "//pkg/server/settingswatcher", diff --git a/pkg/server/auto_tls_init.go b/pkg/server/auto_tls_init.go index 8829e95e4d68..b51a67e1ac99 100644 --- a/pkg/server/auto_tls_init.go +++ b/pkg/server/auto_tls_init.go @@ -26,6 +26,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/security" "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/util/log" "github.com/cockroachdb/cockroach/pkg/util/netutil/addr" @@ -293,7 +294,8 @@ func (b *CertificateBundle) InitializeFromConfig(ctx context.Context, c base.Con // First check to see if host cert is already present // if it is, we should fail to initialize. - 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.New("inter-node certificate already present") @@ -438,7 +440,8 @@ func (b *CertificateBundle) InitializeNodeFromBundle(ctx context.Context, c base // First check to see if host cert is already present // if it is, we should fail to initialize. - 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.New("inter-node certificate already present") diff --git a/pkg/server/server_sql.go b/pkg/server/server_sql.go index 4f328896e5a8..f23409ef4f07 100644 --- a/pkg/server/server_sql.go +++ b/pkg/server/server_sql.go @@ -14,6 +14,7 @@ import ( "context" "math" "net" + "net/url" "os" "path/filepath" "time" @@ -39,8 +40,10 @@ import ( "github.com/cockroachdb/cockroach/pkg/rpc" "github.com/cockroachdb/cockroach/pkg/rpc/nodedialer" "github.com/cockroachdb/cockroach/pkg/scheduledjobs" + "github.com/cockroachdb/cockroach/pkg/security/clientsecopts" "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/server/diagnostics" + "github.com/cockroachdb/cockroach/pkg/server/pgurl" "github.com/cockroachdb/cockroach/pkg/server/serverpb" "github.com/cockroachdb/cockroach/pkg/server/settingswatcher" "github.com/cockroachdb/cockroach/pkg/server/status" @@ -674,9 +677,21 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*SQLServer, error) { sqlExecutorTestingKnobs = sql.ExecutorTestingKnobs{} } + ccopts := clientsecopts.ClientOptions{ + Insecure: cfg.Config.Insecure, + CertsDir: cfg.Config.SSLCertsDir, + } + sparams := clientsecopts.ServerParameters{ + ServerAddr: cfg.Config.SQLAdvertiseAddr, + DefaultPort: base.DefaultPort, + DefaultDatabase: catalogkeys.DefaultDatabaseName, + } + nodeInfo := sql.NodeInfo{ - AdminURL: cfg.AdminURL, - PGURL: cfg.rpcContext.PGURL, + AdminURL: cfg.AdminURL, + PGURL: func(user *url.Userinfo) (*pgurl.URL, error) { + return clientsecopts.MakeURLForServer(ccopts, sparams, user) + }, LogicalClusterID: cfg.rpcContext.LogicalClusterID.Get, NodeID: cfg.nodeIDContainer, }