Merge cockroachdb#72394 cockroachdb#73776

72394: build: build the docker image in teamcity r=jlinder a=ZhouXing19

This commit builds the docker image based on script from `build/deploy`, and 
saves the docker image in the artifacts directory.

Note: the associated build configuration is at `Cockroach >> Scratch Project >> 
Build Docker Image` ([here](https://teamcity.cockroachdb.com/viewType.html?buildTypeId=Cockroach_ScratchProjectPutTcExperimentsInHere_JaneDockerImage&tab=buildTypeStatusDiv&branch_Cockroach_ScratchProjectPutTcExperimentsInHere=72394)). 
Once this PR is merged, we will move this config to `Cockroach > CI > Builds`.
Please run this config upon this branch (cockroachdb#72394).


Release note: None

73776: rpc: use system certificates when certs dir not specified or empty r=yecs1999 a=yecs1999

Previously, we always required either a certs dir path to be specified
as an option or a root cert path in the connection url.

This was incorrect because when sslmode was set to "require"
or any other mode which did not require certificate checking,
not specifying a certs dir would cause an error asking for one.
Another problem was that in modes that did need certificate checking,
like "verify-full" and "verify-ca", we did not first check the system
trust store for any available certificates.

To address this, this patch removes the requirement that certificate
paths need to be checked for require or disable sslmodes. Also removed
is the error when certificates were required, we now check the system
trust store for the CA. This only applies to deployments which use certs
ultimately signed by a public CA, such as CockroachCloud serverless
(which uses LetsEncrypt) or when using a publicly rooted CA chain.
It does not apply to certificates derived from a CA generated via
"cockroach cert create-ca". This also allows us to reduce
the number of steps required to connect to CockroachCloud serverless
clusters by not having the user download a certificate they already have.

Fixes cockroachdb#70946.

Release note (cli change): Not finding the right certs in the certs dir
or not specifying a certs dir or certificate path will now fall back on
checking server CA using Go's TLS code to find the certificates in
the OS trust store. If no matching certificate is found, then an x509
error will occur announcing that the certificate is signed by an
unknown authority.

Release note (bug fix): Setting sslmode=require would check for local
certificates, so omitting a certs path would cause an error even though
"require" does not verify server certificates. This has been fixed by
bypassing certificate path checking for sslmode=require. This bug has
been present since 21.2.0.

Co-authored-by: Jane Xing <[email protected]>
Co-authored-by: yecs1999 <[email protected]>

build/teamcity/cockroach/ci/builds/build_docker_image.sh

@@ -0,0 +1,50 @@
+#!/usr/bin/env bash
+set -euo pipefail
+
+dir="$(dirname $(dirname $(dirname $(dirname $(dirname "${0}")))))"
+source "$dir/teamcity-support.sh"  # For $root
+
+# The artifacts dir should match up with that supplied by TC.
+artifacts=$PWD/artifacts
+mkdir -p "${artifacts}"
+chmod o+rwx "${artifacts}"
+
+tc_start_block "Copy cockroach binary and dependency files to build/deploy"
+
+# Get the cockroach binary from Build (Linux x86_64)
+# Artifacts rules:
+# bazel-bin/pkg/cmd/cockroach/cockroach_/cockroach=>upstream_artifacts
+# bazel-bin/c-deps/libgeos/lib/libgeos.so=>upstream_artifacts
+# bazel-bin/c-deps/libgeos/lib/libgeos_c.so=>upstream_artifacts
+cp upstream_artifacts/cockroach\
+   upstream_artifacts/libgeos.so \
+   upstream_artifacts/libgeos_c.so \
+   build/deploy
+
+cp -r licenses build/deploy/
+
+chmod 755 build/deploy/cockroach
+
+tc_end_block "Copy cockroach binary and dependency files to build/deploy"
+
+tc_start_block "Build and save docker image to artifacts"
+
+docker_image_tar_name="cockroach-docker-image.tar"
+
+docker_tag="cockroachdb/cockroach-ci"
+
+docker build \
+  --no-cache \
+  --tag="$docker_tag" \
+  build/deploy
+
+docker save "$docker_tag" | gzip > "${artifacts}/${docker_image_tar_name}".gz
+
+cp upstream_artifacts/cockroach "${artifacts}"/cockroach
+
+docker_fsnotify_dir="$(dirname "${0}")/docker-fsnotify"
+cd $docker_fsnotify_dir && go build
+cp ./docker-fsnotify "${artifacts}"/docker-fsnotify
+
+tc_end_block "Build and save docker image to artifacts"
+

build/teamcity/cockroach/ci/builds/docker-fsnotify/BUILD.bazel

@@ -0,0 +1,15 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library")
+
+go_library(
+    name = "docker-fsnotify_lib",
+    srcs = ["ListenFileCreation.go"],
+    importpath = "github.com/cockroachdb/cockroach/build/teamcity/cockroach/ci/builds/docker-fsnotify",
+    visibility = ["//visibility:private"],
+    deps = ["@com_github_fsnotify_fsnotify//:fsnotify"],
+)
+
+go_binary(
+    name = "docker-fsnotify",
+    embed = [":docker-fsnotify_lib"],
+    visibility = ["//visibility:public"],
+)

build/teamcity/cockroach/ci/builds/docker-fsnotify/ListenFileCreation.go

@@ -0,0 +1,113 @@
+// Copyright 2021 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.
+
+// Usage: go run ./ListenFileChange.go parent_folder_path file_name [timeout_duration]
+
+package main
+
+import (
+	"errors"
+	"fmt"
+	"log"
+	"os"
+	"path/filepath"
+	"strconv"
+	"strings"
+	"time"
+
+	"github.com/fsnotify/fsnotify"
+)
+
+type result struct {
+	finished bool
+	err      error
+}
+
+const defaultTimeout = 30
+
+func main() {
+
+	if len(os.Args) < 2 {
+		panic(fmt.Errorf("must provide the folder to watch and the file to listen to"))
+	}
+
+	var err error
+
+	folderPath := os.Args[1]
+	wantedFileName := os.Args[2]
+
+	timeout := defaultTimeout
+
+	if len(os.Args) > 3 {
+		timeoutArg := os.Args[3]
+		timeout, err = strconv.Atoi(timeoutArg)
+		if err != nil {
+			panic(fmt.Errorf("timeout argument must be an integer: %v", err))
+		}
+	}
+
+	watcher, err := fsnotify.NewWatcher()
+	if err != nil {
+		log.Fatal(err)
+	}
+	defer watcher.Close()
+
+	done := make(chan result)
+
+	go func() {
+		for {
+			if _, err := os.Stat(filepath.Join(folderPath, wantedFileName)); errors.Is(err, os.ErrNotExist) {
+			} else {
+				done <- result{finished: true, err: nil}
+			}
+			time.Sleep(time.Second * 1)
+		}
+	}()
+
+	go func() {
+		for {
+			select {
+			case event, ok := <-watcher.Events:
+				if !ok {
+					return
+				}
+				fileName := event.Name[strings.LastIndex(event.Name, "/")+1:]
+				if event.Op&fsnotify.Write == fsnotify.Write && fileName == wantedFileName {
+					done <- result{finished: true, err: nil}
+				}
+			case err, ok := <-watcher.Errors:
+				if !ok {
+					return
+				}
+				done <- result{finished: false, err: err}
+			}
+		}
+	}()
+
+	err = watcher.Add(folderPath)
+	if err != nil {
+		fmt.Printf("error: %v", err)
+		return
+	}
+
+	select {
+	case res := <-done:
+		if res.finished && res.err == nil {
+			fmt.Println("finished")
+		} else {
+			fmt.Printf("error: %v", res.err)
+		}
+
+	case <-time.After(time.Duration(timeout) * time.Second):
+		fmt.Printf("timeout for %d second", timeout)
+	}
+
+	return
+}

go.mod

@@ -160,6 +160,8 @@ require (
 	vitess.io/vitess v0.0.0-00010101000000-000000000000
 )
 
+require github.com/fsnotify/fsnotify v1.5.1
+
 require (
 	cloud.google.com/go v0.99.0 // indirect
 	cloud.google.com/go/kms v1.1.0 // indirect
@@ -208,7 +210,6 @@ require (
 	github.com/envoyproxy/go-control-plane v0.10.1 // indirect
 	github.com/envoyproxy/protoc-gen-validate v0.6.2 // indirect
 	github.com/felixge/httpsnoop v1.0.1 // indirect
-	github.com/fsnotify/fsnotify v1.5.1 // indirect
 	github.com/ghodss/yaml v1.0.0 // indirect
 	github.com/go-kit/log v0.1.0 // indirect
 	github.com/go-logfmt/logfmt v0.5.1 // indirect

pkg/cli/flags_test.go

@@ -333,6 +333,11 @@ func TestClientURLFlagEquivalence(t *testing.T) {
 		{anyNonSQL, []string{"--url=postgresql://foo?sslmode=verify-full&sslrootcert=blih/loh.crt"}, nil, `invalid file name for "sslrootcert": expected .* got .*`, ""},
 		{anyNonSQL, []string{"--url=postgresql://foo?sslmode=verify-full&sslcert=blih/loh.crt"}, nil, `invalid file name for "sslcert": expected .* got .*`, ""},
 		{anyNonSQL, []string{"--url=postgresql://foo?sslmode=verify-full&sslkey=blih/loh.crt"}, nil, `invalid file name for "sslkey": expected .* got .*`, ""},
+
+		// Check that not specifying a certs dir will cause Go to use root trust store.
+		{anyCmd, []string{"--url=postgresql://foo?sslmode=verify-full"}, []string{"--host=foo"}, "", ""},
+		{anySQL, []string{"--url=postgresql://foo?sslmode=verify-ca"}, []string{"--host=foo"}, "", ""},
+		{anySQL, []string{"--url=postgresql://foo?sslmode=require"}, []string{"--host=foo"}, "", ""},
 	}
 
 	type capturedFlags struct {

pkg/cli/interactive_tests/test_error_hints.tcl

@@ -30,8 +30,8 @@ end_test
 
 start_test "Connecting a SQL client to a non-started server"
 send "$argv sql -e 'select 1'\r"
-eexpect "ERROR: cannot load certificates.\r\nCheck your certificate settings"
-eexpect "or use --insecure"
+eexpect "ERROR: cannot dial server.\r\nIs the server running?"
+eexpect "connection refused"
 eexpect ":/# "
 
 send "$argv sql -e 'select 1' --certs-dir=$certs_dir\r"

pkg/cli/interactive_tests/test_url_db_override.tcl

@@ -15,7 +15,8 @@ start_test "Check that the SSL settings come from flags is URL does not set them
 set ::env(COCKROACH_INSECURE) "false"
 
 spawn $argv sql --url "postgresql://test@localhost:26257" -e "select 1"
-eexpect "cannot load certificates"
+eexpect "ERROR: cannot establish secure connection to insecure server."
+eexpect "Maybe use --insecure?"
 eexpect eof
 
 spawn $argv sql --url "postgresql://test@localhost:26257" --insecure -e "select 1"

pkg/rpc/pg.go

@@ -18,6 +18,7 @@ import (
 	"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"
 )
 
 // LoadSecurityOptions extends a url.Values with SSL settings suitable for
@@ -37,15 +38,40 @@ func (ctx *SecurityContext) LoadSecurityOptions(u *pgurl.URL, username security.
 			tlsMode = pgurl.TLSVerifyFull
 		}
 
-		if caCertPath == "" {
-			// Fetch CA cert. This is required to exist, so try to load it. We use
-			// the fact that GetCertificateManager checks that "some certs" exist
-			// and want to return "its error" here since we test it in
-			// test_url_db_override.tcl.
-			if _, err := ctx.GetCertificateManager(); err != nil {
-				return wrapError(err)
+		// 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,
+				// 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()
+				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.
+				}
+				if ourCACert := cm.CACert(); ourCACert != nil {
+					// The CM has a CA cert. Use that.
+					caCertPath = cm.FullPath(ourCACert)
+				}
 			}
-			caCertPath = ctx.CACertPath()
+			// Fallback: if caCertPath was not assigned above, either
+			// we did not have a certs dir, or it did not contain
+			// a CA cert. In that case, we rely on the OS trust store.
+			// Documentation of tls.Config:
+			//     https://pkg.go.dev/crypto/tls#Config
 		}
 
 		// (Re)populate the transport information.

pkg/rpc/tls.go

@@ -104,13 +104,15 @@ func (ctx *SecurityContext) GetCertificateManager() (*security.CertificateManage
 				// If we know there should be certificates (we're in secure mode)
 				// but there aren't any, this likely indicates that the certs dir
 				// was misconfigured.
-				ctx.lazy.certificateManager.err = errors.New("no certificates found; does certs dir exist?")
+				ctx.lazy.certificateManager.err = errNoCertificatesFound
 			}
 		}
 	})
 	return ctx.lazy.certificateManager.cm, ctx.lazy.certificateManager.err
 }
 
+var errNoCertificatesFound = errors.New("no certificates found; does certs dir exist?")
+
 // GetServerTLSConfig returns the server TLS config, initializing it if needed.
 // If Insecure is true, return a nil config, otherwise ask the certificate
 // manager for a server TLS config.

pkg/security/certificate_manager.go

@@ -269,6 +269,11 @@ func (cl CertsLocator) CACertPath() string {
 	return filepath.Join(cl.certsDir, CACertFilename())
 }
 
+// FullPath takes a CertInfo and returns the full path for it.
+func (cl CertsLocator) FullPath(ci *CertInfo) string {
+	return filepath.Join(cl.certsDir, ci.Filename)
+}
+
 // EnsureCertsDirectory ensures that the certs directory exists by
 // creating it if does not exist yet.
 func (cl CertsLocator) EnsureCertsDirectory() error {