Merge #102079 #111424 #114681 #114709

102079: kv: unit test `txnMetricRecorder` r=nvanbenschoten a=nvanbenschoten

Addresses a TODO to wrap unit tests directly around this struct, like we do for the other txnInterceptor. I plan to add a new metric to `txnMetricRecorder`, so it makes sense to improve testing first.

Epic: None
Release note: None

111424: kvcoord: keep refresh spans after savepoint rollback r=nvanbenschoten a=miraradeva

Previously, when a read occurred between SAVEPOINT and ROLLBACK TO
SAVEPOINT, upon rollback the read was removed from the transaction's
refresh spans. If the transaction's read and write timestamps diverged,
this read would not be refreshed, which could lead to a serializability
violation. Moreover, this behavior diverged from the Postgres behavior,
which considers reads in a subtransaction to belong to the parent
transaction
(https://github.com/postgres/postgres/blob/master/src/backend/storage/lmgr/README-SSI#L461-L467).

The previous behavior was an intentional design choice, which made sense
at the time with the intention of using savepoints to recover from
serialization errors; however, this was never implemented. After some
discussions, we have decided to match the Postgres behavior instead.

This patch adresses the issue by ensuring that all refresh spans
accumulated since a savepoint was created are kept after the savepoint
is rolled back. We don't expect this new behavior to impact customers
because they should already be able to handle serialization errors; in
case any unforeseen customer issues arise, this patch also includes a
private cluster setting to revert to the old behavior.

Fixes: #111228

Release note (sql change): Reads rolled back by savepoints are now
refreshable, matching the Postgres behavior and avoiding potential
serializability violations.

114681: stress: tweak args for `deadlock` and `race` nightlies r=jlinder a=rickystewart

Increase timeouts, reduce the number of test runs, unset
GITHUB_API_TOKEN for nightlies that aren't working fully yet.

Also set `GOTRACEBACK=all` for all EngFlow tests.

Epic: [CRDB-8308](https://cockroachlabs.atlassian.net/browse/CRDB-8308)
Release note: None

114709: fipsccl: Add interfaces to expose FIPS-readiness status r=bdarnell a=bdarnell

- CLI tool `cockroach debug enterprise-check-fips` for detailed diagnostics
- Global CLI flag `--enterprise-require-fips-ready` to abort if FIPS checks fail
- SQL function `crdb_internal.fips_ready()` to check at runtime

Closes #114344

Even though FIPS is security-related (and I added a `securityccl` package to be owned by `@cockroachdb/prodsec),` this PR is really more build wrangling than anything significant from a security perspective. 

Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Mira Radeva <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
Co-authored-by: Ben Darnell <[email protected]>

.bazelrc

@@ -45,7 +45,7 @@ query --ui_event_filters=-DEBUG
 clean --ui_event_filters=-WARNING
 info --ui_event_filters=-WARNING
 
-build:race --@io_bazel_rules_go//go/config:race "--test_env=GORACE=halt_on_error=1 log_path=stdout" --test_sharding_strategy=disabled
+build:race --@io_bazel_rules_go//go/config:race "--test_env=GORACE=halt_on_error=1 log_path=stdout"
 test:test --test_env=TZ=
 # Note: these timeout values are used indirectly in `build/teamcity/cockroach/ci/tests/testrace_impl.sh`.
 # If those values are updated, the script should be updated accordingly.
@@ -127,6 +127,7 @@ build:engflowbase --extra_execution_platforms=//build/toolchains:cross_linux
 build:engflowbase --remote_upload_local_results=false
 build:engflowbase --remote_download_toplevel
 test:engflowbase --test_env=REMOTE_EXEC=1
+test:engflowbase --test_env=GOTRACEBACK=all
 build:engflow --config=engflowbase
 build:engflow --remote_cache=grpcs://tanzanite.cluster.engflow.com
 build:engflow --remote_executor=grpcs://tanzanite.cluster.engflow.com

.github/CODEOWNERS

@@ -530,6 +530,7 @@
 /pkg/scheduledjobs/          @cockroachdb/jobs-prs @cockroachdb/disaster-recovery
 /pkg/security/               @cockroachdb/prodsec @cockroachdb/server-prs
 /pkg/security/clientsecopts/ @cockroachdb/sql-foundations @cockroachdb/prodsec
+/pkg/ccl/securityccl/        @cockroachdb/prodsec
 #!/pkg/settings/             @cockroachdb/unowned
 /pkg/spanconfig/                         @cockroachdb/kv-prs @cockroachdb/sql-foundations
 /pkg/spanconfig/spanconfigbounds/        @cockroachdb/sql-foundations

WORKSPACE

@@ -645,4 +645,14 @@ go_download_sdk(
     },
     urls = ["https://storage.googleapis.com/public-bazel-artifacts/go/20231019-214851/{}"],
     version = "1.21.3fips",
+    # In the golang-fips toolchain, FIPS-ready crypto packages are used by default, regardless of build tags.
+    # The boringcrypto experiment does almost nothing in this toolchain, but it does enable the use of the
+    # crypto/boring.Enabled() method which is the only application-visible way to inspect whether FIPS mode
+    # is working correctly.
+    #
+    # The golang-fips toolchain also supports an experiment `strictfipsruntime` which causes a panic at startup
+    # if the kernel is in FIPS mode but OpenSSL cannot be loaded. We do not currently use this experiment
+    # because A) we also want to detect the case when the kernel is not in FIPS mode and B) we want to be
+    # able to provide additional diagnostic information such as the expected version of OpenSSL.
+    experiments = ["boringcrypto"],
 )

build/teamcity/cockroach/ci/tests/testrace_impl.sh

@@ -32,6 +32,7 @@ do
             $(bazel info bazel-bin --config=ci)/pkg/cmd/bazci/bazci_/bazci -- test --config=ci --config=race "$test" \
                                 --test_env=COCKROACH_LOGIC_TESTS_SKIP=true \
                                 --test_timeout $timeout \
+                                --test_sharding_strategy=disabled \
                                 --test_env=GOMAXPROCS=8
         done
     done

build/teamcity/cockroach/nightlies/pebble_nightly_metamorphic_race_impl.sh

@@ -20,6 +20,7 @@ $BAZEL_BIN/pkg/cmd/bazci/bazci_/bazci --formatter=pebble-metamorphic -- test --c
                                       @com_github_cockroachdb_pebble//internal/metamorphic:metamorphic_test \
                                       --test_env TC_SERVER_URL=$TC_SERVER_URL \
                                       --test_timeout=14400 '--test_filter=TestMeta$' \
+                                      --test_sharding_strategy=disabled \
                                       --define gotags=bazel,invariants \
                                       --run_under "@com_github_cockroachdb_stress//:stress -bazel -shardable-artifacts 'XML_OUTPUT_FILE=$BAZEL_BIN/pkg/cmd/bazci/bazci_/bazci merge-test-xmls' -maxtime 3h -maxfails 1 -timeout 30m -stderr -p 1" \
                                       --test_arg -dir --test_arg $ARTIFACTS_DIR \

build/teamcity/cockroach/nightlies/stress_engflow.sh

@@ -6,4 +6,6 @@ export EXTRA_TEST_ARGS="--config use_ci_timeouts"
 
 THIS_DIR=$(cd "$(dirname "$0")" && pwd)
 
+unset GITHUB_API_TOKEN
+
 $THIS_DIR/stress_engflow_impl.sh

build/teamcity/cockroach/nightlies/stress_engflow_deadlock.sh

@@ -2,8 +2,11 @@
 
 set -euo pipefail
 
-export EXTRA_TEST_ARGS="--define gotags=bazel,gss,deadlock"
+export RUNS_PER_TEST=3
+export EXTRA_TEST_ARGS="--define gotags=bazel,gss,deadlock --test_timeout=1800,3600,5395,5395"
 
 THIS_DIR=$(cd "$(dirname "$0")" && pwd)
 
+unset GITHUB_API_TOKEN
+
 $THIS_DIR/stress_engflow_impl.sh

build/teamcity/cockroach/nightlies/stress_engflow_race.sh

@@ -2,9 +2,11 @@
 
 set -euo pipefail
 
-export RUNS_PER_TEST=5
-export EXTRA_TEST_ARGS="--@io_bazel_rules_go//go/config:race --test_env=GORACE=halt_on_error=1"
+export RUNS_PER_TEST=3
+export EXTRA_TEST_ARGS="--config=race --test_timeout=1800,3600,5395,5395"
 
 THIS_DIR=$(cd "$(dirname "$0")" && pwd)
 
+unset GITHUB_API_TOKEN
+
 $THIS_DIR/stress_engflow_impl.sh

dev

@@ -8,7 +8,7 @@ fi
 set -euo pipefail
 
 # Bump this counter to force rebuilding `dev` on all machines.
-DEV_VERSION=89
+DEV_VERSION=90
 
 THIS_DIR=$(cd "$(dirname "$0")" && pwd)
 BINARY_DIR=$THIS_DIR/bin/dev-versions

pkg/BUILD.bazel

@@ -895,6 +895,7 @@ GO_TARGETS = [
     "//pkg/ccl/pgcryptoccl:pgcryptoccl_test",
     "//pkg/ccl/schemachangerccl:schemachangerccl",
     "//pkg/ccl/schemachangerccl:schemachangerccl_test",
+    "//pkg/ccl/securityccl/fipsccl:fipsccl",
     "//pkg/ccl/serverccl/adminccl:adminccl_test",
     "//pkg/ccl/serverccl/diagnosticsccl:diagnosticsccl_test",
     "//pkg/ccl/serverccl/statusccl:statusccl_test",

pkg/ccl/BUILD.bazel

@@ -21,6 +21,7 @@ go_library(
         "//pkg/ccl/oidcccl",
         "//pkg/ccl/partitionccl",
         "//pkg/ccl/pgcryptoccl",
+        "//pkg/ccl/securityccl/fipsccl",
         "//pkg/ccl/storageccl",
         "//pkg/ccl/storageccl/engineccl",
         "//pkg/ccl/streamingccl/streamingest",

pkg/ccl/ccl_init.go

@@ -27,6 +27,7 @@ import (
 	_ "github.com/cockroachdb/cockroach/pkg/ccl/oidcccl"
 	_ "github.com/cockroachdb/cockroach/pkg/ccl/partitionccl"
 	_ "github.com/cockroachdb/cockroach/pkg/ccl/pgcryptoccl"
+	_ "github.com/cockroachdb/cockroach/pkg/ccl/securityccl/fipsccl"
 	_ "github.com/cockroachdb/cockroach/pkg/ccl/storageccl"
 	_ "github.com/cockroachdb/cockroach/pkg/ccl/storageccl/engineccl"
 	_ "github.com/cockroachdb/cockroach/pkg/ccl/streamingccl/streamingest"

pkg/ccl/cliccl/BUILD.bazel

@@ -20,16 +20,19 @@ go_library(
         "//pkg/base",
         "//pkg/ccl/baseccl",
         "//pkg/ccl/cliccl/cliflagsccl",
+        "//pkg/ccl/securityccl/fipsccl",
         "//pkg/ccl/sqlproxyccl",
         "//pkg/ccl/sqlproxyccl/tenantdirsvr",
         "//pkg/ccl/storageccl/engineccl/enginepbccl",
         "//pkg/ccl/utilccl",
         "//pkg/ccl/workloadccl/cliccl",
         "//pkg/cli",
+        "//pkg/cli/clierror",
         "//pkg/cli/clierrorplus",
         "//pkg/cli/cliflagcfg",
         "//pkg/cli/cliflags",
         "//pkg/cli/democluster",
+        "//pkg/cli/exit",
         "//pkg/storage",
         "//pkg/storage/enginepb",
         "//pkg/util/log",
@@ -41,7 +44,9 @@ go_library(
         "@com_github_cockroachdb_errors//oserror",
         "@com_github_cockroachdb_pebble//vfs",
         "@com_github_cockroachdb_redact//:redact",
+        "@com_github_olekukonko_tablewriter//:tablewriter",
         "@com_github_spf13_cobra//:cobra",
+        "@com_github_spf13_pflag//:pflag",
     ],
 )
 

pkg/ccl/cliccl/debug.go

@@ -14,12 +14,14 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"runtime"
 	"sort"
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/ccl/baseccl"
 	"github.com/cockroachdb/cockroach/pkg/ccl/cliccl/cliflagsccl"
+	"github.com/cockroachdb/cockroach/pkg/ccl/securityccl/fipsccl"
 	"github.com/cockroachdb/cockroach/pkg/ccl/storageccl/engineccl/enginepbccl"
 	"github.com/cockroachdb/cockroach/pkg/cli"
 	"github.com/cockroachdb/cockroach/pkg/cli/clierrorplus"
@@ -31,6 +33,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/errors"
 	"github.com/cockroachdb/errors/oserror"
+	"github.com/olekukonko/tablewriter"
 	"github.com/spf13/cobra"
 )
 
@@ -102,12 +105,24 @@ with their env type and encryption settings (if applicable).
 		RunE: clierrorplus.MaybeDecorateError(runList),
 	}
 
+	checkFipsCmd := &cobra.Command{
+		Use:   "enterprise-check-fips",
+		Short: "print diagnostics for FIPS-ready configuration",
+		Long: `
+Performs various tests of this binary's ability to operate in FIPS-ready
+mode in the current environment.
+`,
+
+		RunE: clierrorplus.MaybeDecorateError(runCheckFips),
+	}
+
 	// Add commands to the root debug command.
 	// We can't add them to the lists of commands (eg: DebugCmdsForPebble) as cli init() is called before us.
 	cli.DebugCmd.AddCommand(encryptionStatusCmd)
 	cli.DebugCmd.AddCommand(encryptionActiveKeyCmd)
 	cli.DebugCmd.AddCommand(encryptionDecryptCmd)
 	cli.DebugCmd.AddCommand(encryptionRegistryList)
+	cli.DebugCmd.AddCommand(checkFipsCmd)
 
 	// Add the encryption flag to commands that need it.
 	// For the encryption-status command.
@@ -376,3 +391,44 @@ func getActiveEncryptionkey(dir string) (string, string, error) {
 
 	return setting.EncryptionType.String(), setting.KeyId, nil
 }
+
+func runCheckFips(cmd *cobra.Command, args []string) error {
+	if runtime.GOOS != "linux" {
+		return errors.New("FIPS-ready mode is only supported on linux")
+	}
+	// Our FIPS-ready deployments have three major requirements:
+	// 1. This binary is built with the golang-fips toolchain and running on linux
+	// 2. FIPS mode is enabled in the kernel.
+	// 3. We can dynamically load the OpenSSL library (which must be the same major version that was present at
+	//    build time). Verifying that the OpenSSL library is FIPS-compliant is outside the scope of this command.
+	table := tablewriter.NewWriter(os.Stdout)
+	table.SetBorder(false)
+	table.SetAlignment(tablewriter.ALIGN_LEFT)
+	emit := func(label string, status bool, detail string) {
+		statusSymbol := "❌"
+		if status {
+			statusSymbol = "✅"
+		}
+		table.Append([]string{label, statusSymbol, detail})
+	}
+
+	emit("FIPS-ready build", fipsccl.IsCompileTimeFIPSReady(), "")
+	buildOpenSSLVersion, soname, err := fipsccl.BuildOpenSSLVersion()
+	if err == nil {
+		table.Append([]string{"Build-time OpenSSL Version", "", buildOpenSSLVersion})
+		table.Append([]string{"OpenSSL library filename", "", soname})
+	}
+
+	isKernelEnabled, err := fipsccl.IsKernelEnabled()
+	detail := ""
+	if err != nil {
+		detail = err.Error()
+	}
+	emit("Kernel FIPS mode enabled", isKernelEnabled, detail)
+
+	emit("OpenSSL loaded", fipsccl.IsOpenSSLLoaded(), "")
+	emit("FIPS ready", fipsccl.IsFIPSReady(), "")
+
+	table.Render()
+	return nil
+}

pkg/ccl/cliccl/flags.go

@@ -9,11 +9,59 @@
 package cliccl
 
 import (
+	"os"
+	"strconv"
+
+	"github.com/cockroachdb/cockroach/pkg/ccl/securityccl/fipsccl"
 	"github.com/cockroachdb/cockroach/pkg/cli"
+	"github.com/cockroachdb/cockroach/pkg/cli/clierror"
 	"github.com/cockroachdb/cockroach/pkg/cli/cliflagcfg"
 	"github.com/cockroachdb/cockroach/pkg/cli/cliflags"
+	"github.com/cockroachdb/cockroach/pkg/cli/exit"
+	"github.com/cockroachdb/errors"
+	"github.com/spf13/pflag"
 )
 
+type requireFipsFlag bool
+
+// Type implements the pflag.Value interface.
+func (f *requireFipsFlag) Type() string {
+	return "bool"
+}
+
+// String implements the pflag.Value interface.
+func (f *requireFipsFlag) String() string {
+	return strconv.FormatBool(bool(*f))
+}
+
+// Set implements the pflag.Value interface.
+func (f *requireFipsFlag) Set(s string) error {
+	v, err := strconv.ParseBool(s)
+	if err != nil {
+		return err
+	}
+	// We implement the logic of this check in the flag setter itself because it
+	// applies to all commands and we do not have another good way to inject
+	// this behavior globally (PersistentPreRun functions don't help because
+	// they are inherited across different levels of the command hierarchy only
+	// if that level does not have its own hook).
+	if v && !fipsccl.IsFIPSReady() {
+		err := errors.WithHint(errors.New("FIPS readiness checks failed"), "Run `cockroach debug enterprise-check-fips` for details")
+		clierror.OutputError(os.Stderr, err, true, false)
+		exit.WithCode(exit.UnspecifiedError())
+	}
+	*f = requireFipsFlag(v)
+	return nil
+}
+
+var _ pflag.Value = (*requireFipsFlag)(nil)
+
+// IsBoolFlag implements a non-public pflag interface to indicate that this
+// flag is used without an explicit value.
+func (*requireFipsFlag) IsBoolFlag() bool {
+	return true
+}
+
 func init() {
 	// Multi-tenancy proxy command flags.
 	{
@@ -44,4 +92,12 @@ func init() {
 		// Use StringFlagDepth to avoid conflicting with the already registered KVAddrs env var.
 		cliflagcfg.StringFlagDepth(1, f, &testDirectorySvrContext.kvAddrs, cliflags.KVAddrs)
 	})
+
+	// FIPS verification flags.
+	cli.RegisterFlags(func() {
+		cmd := cli.CockroachCmd()
+		var requireFips = requireFipsFlag(false)
+		flag := cmd.PersistentFlags().VarPF(&requireFips, "enterprise-require-fips-ready", "", "abort if FIPS readiness checks fail")
+		flag.NoOptDefVal = "true"
+	})
 }

pkg/ccl/logictestccl/testdata/logic_test/fips_ready

@@ -0,0 +1,14 @@
+subtest fips_ready
+
+# We do not have the plumbing that would let test cases know whether they are
+# running in a fips environment or not so this is just a very basic test to
+# make sure that all the registration, oids, etc work properly.
+query _
+SELECT crdb_internal.fips_ready()
+----
+_
+
+user testuser
+
+statement error pq: crdb_internal\.fips_ready\(\): user testuser does not have VIEWCLUSTERSETTING system privilege
+SELECT crdb_internal.fips_ready()

pkg/ccl/logictestccl/tests/fakedist-disk/BUILD.bazel

@@ -9,7 +9,7 @@ go_test(
         "//pkg/ccl/logictestccl:testdata",  # keep
     ],
     exec_properties = {"Pool": "large"},
-    shard_count = 6,
+    shard_count = 7,
     tags = [
         "ccl_test",
         "cpu:2",

pkg/ccl/logictestccl/tests/fakedist-vec-off/BUILD.bazel

@@ -9,7 +9,7 @@ go_test(
         "//pkg/ccl/logictestccl:testdata",  # keep
     ],
     exec_properties = {"Pool": "large"},
-    shard_count = 6,
+    shard_count = 7,
     tags = [
         "ccl_test",
         "cpu:2",