Skip to content

Commit 5374696

Browse files
craig[bot]angles-n-daemonsrickystewartspilchenarulajmani
committed
144181: security: cleanup clientcert expiration cache functionality r=angles-n-daemons a=angles-n-daemons security: cleanup clientcert expiration cache functionality The work done for #142686 in #143081 was truncated to make the changeset more backportable. Some of the modifications held are ported instead to this PR, so that extra functions are removed, and the `clientcert/cache.go` is simpler in terms of its responsibility. The changes include: - A simplified interface for creating an managing the client certificate cache. - Removes some of the extra code used for getting and checking times. - Removes the (now unused) cluster setting for limiting the size of the cache. - Removes the (unused and unsafe) getters from the cache, ports the tests to reading the values from the metrics. Fixes: #144163 Epic: none Release note (ops change): cluster setting `server.client_cert_expiration_cache.capacity` has been deprecated. The client certificate cache now evicts client certificates based on time. 144714: builder: install a more recent version of `patchelf` r=rail a=rickystewart The version previously in use produces buggy results on s390x. Epic: CRDB-21133 Release note: None 144843: sql: honour RLS policies during query-based backfill r=spilchen a=spilchen Previously, schema changes that performed backfill operations from a query—such as those for materialized views (MQTs) or CREATE TABLE ... AS (CTAS)—executed the query as the node user. This user has administrative privileges and bypasses all Row-Level Security (RLS) policies, unintentionally exposing rows the originator of the change should not have been able to access. This change ensures that such query-based backfills run under the privileges of the user who initiated the schema change. As a result, RLS policies are correctly enforced, and only the rows visible to the initiating user are included in the result. Fixes #144816 Fixes #144776 Epic: CRDB-11724 Release note: none 144873: sql: enable TestUpsertFastPath with buffered writes r=arulajmani a=arulajmani We're able to hit 1PC in more cases when buffered writes are enabled. Teach TestUpsertFastPath about it. Epic: none Release note: None Co-authored-by: Brian Dillmann <[email protected]> Co-authored-by: Ricky Stewart <[email protected]> Co-authored-by: Matt Spilchen <[email protected]> Co-authored-by: Arul Ajmani <[email protected]>
5 parents 4ca3bfe + e51de55 + 74ee053 + 28662a5 + 223fdb4 commit 5374696

File tree

14 files changed

+503
-467
lines changed

14 files changed

+503
-467
lines changed

build/.bazelbuilderversion

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
us-east1-docker.pkg.dev/crl-ci-images/cockroach/bazel:20250409-061148
1+
us-east1-docker.pkg.dev/crl-ci-images/cockroach/bazel:20250418-212710

build/bazelbuilder/Dockerfile

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ RUN apt-get update \
2424
netbase \
2525
openjdk-8-jre \
2626
openssh-client \
27-
patchelf \
2827
python-is-python3 \
2928
python3 \
3029
python3.8-venv \
@@ -92,6 +91,15 @@ RUN case ${TARGETPLATFORM} in \
9291
./aws/install && \
9392
rm -rf aws awscliv2.zip
9493

94+
RUN case ${TARGETPLATFORM} in \
95+
"linux/amd64") ARCH=x86_64; SHASUM=ce84f2447fb7a8679e58bc54a20dc2b01b37b5802e12c57eece772a6f14bf3f0 ;; \
96+
"linux/arm64") ARCH=aarch64; SHASUM=ae13e2effe077e829be759182396b931d8f85cfb9cfe9d49385516ea367ef7b2 ;; \
97+
esac && \
98+
curl -fsSL "https://github.com/NixOS/patchelf/releases/download/0.18.0/patchelf-0.18.0-$ARCH.tar.gz" -o "patchelf.tar.gz" && \
99+
echo "$SHASUM patchelf.tar.gz" | sha256sum -c - && \
100+
tar --strip-components=1 -C /usr -xzf patchelf.tar.gz && \
101+
rm -rf patchelf.tar.gz
102+
95103
# Install Bazelisk as Bazel.
96104
# NOTE: you should keep this in sync with build/packer/teamcity-agent.sh and
97105
# build/bootstrap/bootstrap-debian.sh -- if an update is necessary here, it's probably

build/toolchains/BUILD.bazel

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ platform(
3232
"@platforms//cpu:x86_64",
3333
],
3434
exec_properties = {
35-
"container-image": "docker://us-east1-docker.pkg.dev/crl-ci-images/cockroach/bazel@sha256:dd13f3b97a0baac1dc311cc1d81c39b97071907efb3c6669fa659667a4df5d9b",
35+
"container-image": "docker://us-east1-docker.pkg.dev/crl-ci-images/cockroach/bazel@sha256:97228ff5e3ccfed242b49b37fa427d03203bb69ee74c5129f29b6a6fa7662ad1",
3636
"dockerReuse": "True",
3737
"Pool": "default",
3838
},
@@ -137,7 +137,7 @@ platform(
137137
"@platforms//cpu:arm64",
138138
],
139139
exec_properties = {
140-
"container-image": "docker://us-east1-docker.pkg.dev/crl-ci-images/cockroach/bazel@sha256:3c43c89d875fe1d1c05ce40313fad76a928b8dd29af1554e9cb765e9aaaab054",
140+
"container-image": "docker://us-east1-docker.pkg.dev/crl-ci-images/cockroach/bazel@sha256:57c597624ed396b33be80548acfef221e5c340b09ed36a0d826c1798a414b10d",
141141
"dockerReuse": "True",
142142
"Pool": "default",
143143
},

docs/generated/settings/settings-for-tenants.txt

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,6 @@ server.auth_log.sql_sessions.enabled boolean false if set, log verbose SQL sessi
115115
server.authentication_cache.enabled boolean true enables a cache used during authentication to avoid lookups to system tables when retrieving per-user authentication-related information application
116116
server.child_metrics.enabled boolean false enables the exporting of child metrics, additional prometheus time series with extra labels application
117117
server.child_metrics.include_aggregate.enabled boolean true include the reporting of the aggregate time series when child metrics are enabled. This cluster setting has no effect if child metrics are disabled. application
118-
server.client_cert_expiration_cache.capacity integer 1000 the maximum number of client cert expirations stored application
119118
server.clock.forward_jump_check.enabled (alias: server.clock.forward_jump_check_enabled) boolean false if enabled, forward clock jumps > max_offset/2 will cause a panic application
120119
server.clock.persist_upper_bound_interval duration 0s the interval between persisting the wall time upper bound of the clock. The clock does not generate a wall time greater than the persisted timestamp and will panic if it sees a wall time greater than this value. When cockroach starts, it waits for the wall time to catch-up till this persisted timestamp. This guarantees monotonic wall time across server restarts. Not setting this or setting a value of 0 disables this feature. application
121120
server.eventlog.enabled boolean true if set, logged notable events are also stored in the table system.eventlog application

docs/generated/settings/settings.html

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,6 @@
146146
<tr><td><div id="setting-server-authentication-cache-enabled" class="anchored"><code>server.authentication_cache.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>enables a cache used during authentication to avoid lookups to system tables when retrieving per-user authentication-related information</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
147147
<tr><td><div id="setting-server-child-metrics-enabled" class="anchored"><code>server.child_metrics.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>enables the exporting of child metrics, additional prometheus time series with extra labels</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
148148
<tr><td><div id="setting-server-child-metrics-include-aggregate-enabled" class="anchored"><code>server.child_metrics.include_aggregate.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>include the reporting of the aggregate time series when child metrics are enabled. This cluster setting has no effect if child metrics are disabled.</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
149-
<tr><td><div id="setting-server-client-cert-expiration-cache-capacity" class="anchored"><code>server.client_cert_expiration_cache.capacity</code></div></td><td>integer</td><td><code>1000</code></td><td>the maximum number of client cert expirations stored</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
150149
<tr><td><div id="setting-server-clock-forward-jump-check-enabled" class="anchored"><code>server.clock.forward_jump_check.enabled<br />(alias: server.clock.forward_jump_check_enabled)</code></div></td><td>boolean</td><td><code>false</code></td><td>if enabled, forward clock jumps &gt; max_offset/2 will cause a panic</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
151150
<tr><td><div id="setting-server-clock-persist-upper-bound-interval" class="anchored"><code>server.clock.persist_upper_bound_interval</code></div></td><td>duration</td><td><code>0s</code></td><td>the interval between persisting the wall time upper bound of the clock. The clock does not generate a wall time greater than the persisted timestamp and will panic if it sees a wall time greater than this value. When cockroach starts, it waits for the wall time to catch-up till this persisted timestamp. This guarantees monotonic wall time across server restarts. Not setting this or setting a value of 0 disables this feature.</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
152151
<tr><td><div id="setting-server-consistency-check-max-rate" class="anchored"><code>server.consistency_check.max_rate</code></div></td><td>byte size</td><td><code>8.0 MiB</code></td><td>the rate limit (bytes/sec) to use for consistency checks; used in conjunction with server.consistency_check.interval to control the frequency of consistency checks. Note that setting this too high can negatively impact performance.</td><td>Dedicated/Self-Hosted</td></tr>

pkg/security/certificate_manager.go

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import (
1313
"github.com/cockroachdb/cockroach/pkg/security/certnames"
1414
"github.com/cockroachdb/cockroach/pkg/security/clientcert"
1515
"github.com/cockroachdb/cockroach/pkg/security/username"
16-
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
1716
"github.com/cockroachdb/cockroach/pkg/util/log"
1817
"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
1918
"github.com/cockroachdb/cockroach/pkg/util/log/severity"
@@ -57,7 +56,7 @@ type CertificateManager struct {
5756
certMetrics *Metrics
5857

5958
// Client cert expiration cache.
60-
clientCertExpirationCache *clientcert.ClientCertExpirationCache
59+
clientCertExpirationCache *clientcert.Cache
6160

6261
// mu protects all remaining fields.
6362
mu syncutil.RWMutex
@@ -201,12 +200,16 @@ func (cm *CertificateManager) RegisterSignalHandler(
201200
// It is called during server startup.
202201
func (cm *CertificateManager) RegisterExpirationCache(
203202
ctx context.Context,
204-
st *cluster.Settings,
205203
stopper *stop.Stopper,
206204
timeSrc timeutil.TimeSource,
207205
parentMon *mon.BytesMonitor,
208-
) {
209-
cm.clientCertExpirationCache = clientcert.NewClientCertExpirationCache(ctx, st, stopper, timeSrc, parentMon, cm.certMetrics.ClientExpiration, cm.certMetrics.ClientTTL)
206+
) error {
207+
m := mon.NewMonitorInheritWithLimit(mon.MakeName("client-expiration-caches"), 0 /* limit */, parentMon, true /* longLiving */)
208+
acc := m.MakeConcurrentBoundAccount()
209+
m.StartNoReserved(ctx, parentMon)
210+
211+
cm.clientCertExpirationCache = clientcert.NewCache(timeSrc, acc, cm.certMetrics.ClientExpiration, cm.certMetrics.ClientTTL)
212+
return cm.clientCertExpirationCache.StartPurgeJob(ctx, stopper)
210213
}
211214

212215
// MaybeUpsertClientExpiration updates or inserts the expiration time for the
@@ -216,7 +219,7 @@ func (cm *CertificateManager) MaybeUpsertClientExpiration(
216219
ctx context.Context, identity string, serial string, expiration int64,
217220
) {
218221
if cache := cm.clientCertExpirationCache; cache != nil {
219-
cache.MaybeUpsert(ctx,
222+
cache.Upsert(ctx,
220223
identity,
221224
serial,
222225
expiration,

pkg/security/clientcert/BUILD.bazel

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,8 @@ go_library(
66
importpath = "github.com/cockroachdb/cockroach/pkg/security/clientcert",
77
visibility = ["//visibility:public"],
88
deps = [
9-
"//pkg/settings",
10-
"//pkg/settings/cluster",
119
"//pkg/util/log",
1210
"//pkg/util/metric/aggmetric",
13-
"//pkg/util/mon",
1411
"//pkg/util/stop",
1512
"//pkg/util/syncutil",
1613
"//pkg/util/timeutil",
@@ -22,7 +19,6 @@ go_test(
2219
srcs = ["cert_expiry_cache_test.go"],
2320
deps = [
2421
":clientcert",
25-
"//pkg/settings/cluster",
2622
"//pkg/util/leaktest",
2723
"//pkg/util/metric",
2824
"//pkg/util/metric/aggmetric",

0 commit comments

Comments
 (0)