Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

roachtest: make secure/non-root the default for new tests #63145

Closed
solongordon opened this issue Apr 6, 2021 · 7 comments · Fixed by #119310
Closed

roachtest: make secure/non-root the default for new tests #63145

solongordon opened this issue Apr 6, 2021 · 7 comments · Fixed by #119310
Assignees
Labels
A-testing Testing tools and infrastructure C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-testeng TestEng Team

Comments

@solongordon
Copy link
Contributor

solongordon commented Apr 6, 2021

Most of our roachtests use the root user on insecure clusters, which is not realistic. Now that we have one roachtest which does the proper thing, let's look at making that the default behavior for future tests. This should also make it easier to switch over existing tests.

Jira issue: CRDB-6444

@solongordon solongordon added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-testing Testing tools and infrastructure labels Apr 6, 2021
@rafiss
Copy link
Collaborator

rafiss commented Apr 8, 2021

Might relate to this roachprod issue: #38539

@rafiss rafiss added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label May 12, 2021
craig bot pushed a commit that referenced this issue Nov 16, 2023
114354: sql: ALTER PK carries over comments from old to new primary index r=Xiang-Gu a=Xiang-Gu

Fixes #114081

Release note (bug fix): Previously, when session variable
`use_declarative_schema_changer=off`, ALTER PK would delete any comments
associated with the old primary index and with the old primary key
constraint. This is inconsistent with the behavior with
when `use_declarative_schema_changer=on`, which is the default setting,
where those comments would be carried over to the new primary index.
Furthermore, the old behavior also caused a bug that could prevent
command `SHOW CREATE t` from working (see #114081).

114531: roachtest: use secure cluster and non-root user for kv workload r=rafiss a=rafiss

A recent performance regression (#114472) flew under the radar because we lack performance tests that use a non-root user.

In advance of a full-throated effort to improve our tests across the board, this commit changes only the basic KV workload roachtests to use a non-root user.

informs #63145
Release note: None

114583: changefeedccl: Avoid logging when context may be canceled r=miretskiy a=miretskiy

Calling log  methods on a context that's already canceled may produce "use of Span after Finish" under tests. Avoid doing that when pacing CPU usage.

Fixes #114130

Release note: None

Co-authored-by: Xiang Gu <xiang@cockroachlabs.com>
Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
Co-authored-by: Yevgeniy Miretskiy <yevgeniy@cockroachlabs.com>
@blathers-crl blathers-crl bot added the T-testeng TestEng Team label Dec 3, 2023
Copy link

blathers-crl bot commented Dec 3, 2023

cc @cockroachdb/test-eng

@srosenberg
Copy link
Member

srosenberg commented Dec 3, 2023

Current State

At this time only a small subset of roachtests is executed in secure mode,

find pkg/cmd/roachtest -name "*.go" |xargs grep "install.SecureOption"|grep true|sort -u -k1,1 -t':'
pkg/cmd/roachtest/roachtestutil/mixedversion/mixedversion.go:		install.SecureOption(true),
pkg/cmd/roachtest/tests/admission_control_multitenant_fairness.go:		install.MakeClusterSettings(install.SecureOption(true)),
pkg/cmd/roachtest/tests/backup_restore_roundtrip.go:	c.Start(ctx, t.L(), maybeUseMemoryBudget(t, 50), install.MakeClusterSettings(install.SecureOption(true), envOption), roachNodes)
pkg/cmd/roachtest/tests/cluster_to_cluster.go:	srcClusterSetting := install.MakeClusterSettings(install.SecureOption(true))
pkg/cmd/roachtest/tests/connection_latency.go:	settings := install.MakeClusterSettings(install.SecureOption(true))
pkg/cmd/roachtest/tests/kv.go:		settings := install.MakeClusterSettings(install.SecureOption(true))
pkg/cmd/roachtest/tests/mixed_version_backup.go:		install.BinaryOption(cockroachPath), install.SecureOption(true),
pkg/cmd/roachtest/tests/multitenant.go:	c.Start(ctx, t.L(), option.DefaultStartOpts(), install.MakeClusterSettings(install.SecureOption(true)), c.All())
pkg/cmd/roachtest/tests/multitenant_distsql.go:	settings := install.MakeClusterSettings(install.SecureOption(true))
pkg/cmd/roachtest/tests/multitenant_shared_process.go:			clusterSettings := install.MakeClusterSettings(install.SecureOption(true))
pkg/cmd/roachtest/tests/multitenant_upgrade.go:	settings := install.MakeClusterSettings(install.BinaryOption(predecessorBinary), install.SecureOption(true))
pkg/cmd/roachtest/tests/network.go:	settings := install.MakeClusterSettings(install.SecureOption(true))
pkg/cmd/roachtest/tests/network_logging.go:		c.Start(ctx, t.L(), startOpts, install.MakeClusterSettings(install.SecureOption(true)), crdbNodes)
pkg/cmd/roachtest/tests/nodejs_postgres.go:		settings := install.MakeClusterSettings(install.SecureOption(true))
pkg/cmd/roachtest/tests/npgsql.go:		c.Start(ctx, t.L(), option.DefaultStartOptsInMemory(), install.MakeClusterSettings(install.SecureOption(true)), c.All())
pkg/cmd/roachtest/tests/pgjdbc.go:		c.Start(ctx, t.L(), option.DefaultStartOptsInMemory(), install.MakeClusterSettings(install.SecureOption(true)), c.All())
pkg/cmd/roachtest/tests/smoketest_secure.go:				settings := install.MakeClusterSettings(install.SecureOption(true))

By default, roachprod creates an admin user roach (see createAdminUserForSecureCluster) if the cluster is using secure mode. The password of the user is logged but not persisted. Thus, tests which require password-based authentication via programatic access are effectively broken (in secure mode); e.g., see [1].

The new mixed-version API defaults to secure mode [2]. The existing corpus of mixed-version tests doesn't require programmatic access to the cluster adminURL with the exception of [3]. Thus, it's effectively the same issue as above. In order to maintain secure mode as the default, we need to enable an API for password-based authentication; see more below.

The other issue is the generated certs are not signed by a recognized root CA. Thus, by default http clients will fail to verify the node cert. This is essentially another blocker for a number of existing roachtests. The immediate workaround is to skip this verification step; e.g., the following can be added as an option to pkg/util/httputil/client.go,

TLSClientConfig:   &tls.Config{InsecureSkipVerify: insecure},

NOTE: postgres:// urls will need to either have sslmode=allow to skip verification or use the specified root cert via sslrootcert=%s/ca.crt&sslmode=require (see examples below).

Authentication API

Host-based

Default host-based rules (HBA) permit root (admin) access via loopback as well as remote host access via client certs [4]. Thus, we can run sql as root programmatically via roachprod API (SyncedCluster.ExecSQL) or CLI,

roachprod sql $CLUSTER:$NODE --secure --  -e "..."
Cert-based

Similarly, we can enable (client) cert-based authentication [5],

./cockroach cert create-client testuser --certs-dir %s --ca-key=%s/ca.key

The testuser can now be authenticated via the client cert by using the postgres URL of the form,

postgres://%s@%s:%s?sslcert=%s/client.%s.crt&sslkey=%s/client.%s.key&sslrootcert=%s/ca.crt&sslmode=require
Password-based

We can also execute SQL via password-based authentication, e.g.,

./cockroach sql --url 'postgres://user:pass@$HOST:26257?sslmode=allow'
HTTP API

Recall [1] requires programmatic authentication to adminURL via HTTP. It appears we may have to build such an API. FindAndDecodeSessionCookie does the heavy lifting of decoding the session cookie, but I couldn't find existing code to do the full authentication end-to-end. E.g., the following curl statements authenticate and extract the cookie session,

curl -v -L --insecure https://roach:system@$HOST:26258/login -X POST -d '{"username": "roach", "password": "system"}'
Set-Cookie: session=CIGA6rSrxsnmDBIQhVcPIVKsiLWZq6yqEXZMtQ==
curl -v -H "Cookie: session=CIGA6rSrxsnmDBIQhVcPIVKsiLWZq6yqEXZMtQ==" --insecure "https://$HOST:26258/ts/query" -X POST

Programmatically (via roachprod API), we can also extract the most recent session id, but it's best not to rely on these implementation details; i.e., create and use an HTTP api instead.

roachprod sql $CLUSTER:2 --secure --  -e "select id from system.web_sessions where username='roach' order by \"createdAt\" DESC LIMIT 1"

[1]

url := "http://" + adminURLs[0] + "/ts/query"

[2]
defaultClusterSettings = []install.ClusterSettingOption{
install.SecureOption(true),
}

[3] #115317
[4]
var DefaultHBAConfig = func() *hba.Conf {
loadDefaultMethods()
conf, err := ParseAndNormalize(`
loopback all all all trust # built-in CockroachDB default
host all all all cert-password # built-in CockroachDB default
local all all password # built-in CockroachDB default

[5] https://www.cockroachlabs.com/docs/v22.2/cockroach-cert#create-the-certificate-and-key-pair-for-a-client

@renatolabs
Copy link
Contributor

The password of the user is logged but not persisted

Not sure what is meant here, but the passwords should be persisted? We use them to log in to the DB console after the cluster is setup.

@srosenberg
Copy link
Member

Not sure what is meant here, but the passwords should be persisted? We use them to log in to the DB console after the cluster is setup.

I just meant that the user/password is not currently persisted with the rest of the (roachprod) cluster metadata, which makes it hard to reuse programmatically, e.g., invoking a console endpoint via http client.

@exalate-issue-sync exalate-issue-sync bot added T-testeng TestEng Team and removed T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) T-testeng TestEng Team labels Dec 5, 2023
Copy link

blathers-crl bot commented Dec 5, 2023

cc @cockroachdb/test-eng

@srosenberg
Copy link
Member

Linking another authentication example from Renato [1] which retrieves the session id via (cockroach) CLI,

./cockroach auth-session login root --certs-dir ./certs --format raw

[1] https://github.com/cockroachdb/cockroach/blob/release-23.1/pkg/cmd/roachtest/tests/mixed_version_tenant_span_stats.go#L140

craig bot pushed a commit that referenced this issue Feb 9, 2024
117677: server: wait for all KV nodes to observe tenant stop request r=stevendanna a=stevendanna

In order to make it possible for a caller to know that no new SQL
requests will be served by a tenant after a STOP command, this PR adds
a new STOPPING state. While nodes are in the STOPPING state, KV
requests will be rejected. We then wait for all nodes to observe the
STOPPING state before transitioning the cluster to NONE.

Epic: none

Release note: None

118504: roachtest: create secure clusters by default r=renatolabs a=DarrylWong

Previously, the default for roachtests was to create insecure clusters. This change now switches the default to create secure clusters, allowing tests to opt out as needed. To support this, the following changes were made:

1. Refactor PGUrl functions to use PGURLOptions instead of tenant/SQLInstance, which were usually not specified anyway. This change allows up to add an authMode option, which we currently only use to specify root user authentication but in the future can use to allow for non root authentication.
2. Add helpers to extract session ID and create an HTTP Client with the extracted session ID. This lets us create HTTP Requests to secure clusters.
3. Explicitly specify a connection string with a root certificate or a certs directory when connecting to a cluster. If not specified, the connection will be rejected as the default behavior is to authenticate with root and no certificates. Many tests already did this through specifying `{pgurl}`.

Follow up work would be to start using non root user everywhere now that we are running on secure clusters and have the ability to easily generate non root urls.

Release note: None
Epic: None
Informs: #63145

119007: catalog: fix tracking of id and name state r=rafiss a=rafiss

This allows us to re-enable the COMMENT ON statements in the
schemachange workload.

The previous code only updated the entries that were already in the
byIDState map. However, some descriptor IDs may not be in that map, so
instead we should add everything we just read into the map.

fixes #116795
Release note (bug fix): Fixed a bug where COMMENT statements could
fail with an "unexpected value" error if multiple COMMENT statements
were running concurrently.


119012: technotes: add tech notes for sql statistics r=maryliag a=maryliag

Add the tech notes of the values generated for statement and transaction statistics.

Part Of CRDB-35839

Release note: None

Co-authored-by: Steven Danna <danna@cockroachlabs.com>
Co-authored-by: DarrylWong <darryl@cockroachlabs.com>
Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
Co-authored-by: maryliag <marylia@cockroachlabs.com>
@craig craig bot closed this as completed in 463c52a Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testing Testing tools and infrastructure C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-testeng TestEng Team
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

7 participants