Repository navigation
Conversation
… with the legacy `s3queue_` prefix
…3114 Backport ClickHouse#123114 to 26.8: S3 client check initial url in backup client
…use_variant_as_common_type=0` for jemalloc fragmentation dashboard
`Storages/getEffectiveRowPolicyFilter.h` does not exist on 26.8. The call site always passes the data part's MergeTree, which has no underlying storages, so the walk it does on master reduces to the table's own policy; the local helper here checks exactly that. The `Alias` case still holds: on 26.8 the alias pushes its policy into `query_info.row_level_filter`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…gles only for columns the filter uses
… indexHint() that used aliases
…1621 Backport ClickHouse#121621 to 26.8: Fix `NO_COMMON_TYPE` in case of `use_variant_as_common_type=0` for jemalloc fragmentation dashboard
…ht table has a row policy
…2035 Backport ClickHouse#122035 to 26.8: Disable statistics pruning in case of row level policy
…2681 Backport ClickHouse#122681 to 26.8: Build Iceberg min/max hyperrectangles only for columns the filter uses
…and URL settings
Six call sites still masked a credential embedded in a URL-shaped value with
maskURIPassword, an unanchored scan that reproduces the old regular expression
([^:]+://[^:]*):([^@]*)@(.*) exactly: it masks from the first ':' after any
'://' to the first '@'. The masked extent has to cover whatever the value's
consumer treats as a credential, and that scan matches no consumer in the tree,
so it is at once too narrow (a password containing '@' keeps its tail, and a
userinfo with no password is not masked at all) and too wide (a credential-free
URL whose query string contains '@' is rewritten into a host that is not the
real one). All three show up in SHOW CREATE, system.tables and
system.query_log.
Swapping in maskURIUserinfo at every site, which is what the issue asks for,
would replace a partial leak with a full one at five of the six. That function
implements the RFC 3986 bound, which is Poco::URI's bound, and three of these
consumers are not Poco::URI:
- libnats trims the value, takes the scheme as optional and ends the userinfo
at the LAST '@' of the whole remainder, unbounded by the '/?#' that closes
an authority (contrib/nats-io/src/url.c, natsUrl_Create; both halves go
into the CONNECT frame in conn.c). So nats://u:pa/ss@h:4222 authenticates
with password pa/ss, which maskURIUserinfo leaves untouched, and
user:pass@h:4222 authenticates while neither masker touches it.
- AMQP-CPP ends the login at the FIRST '@' after the scheme, again unbounded
(contrib/AMQP-CPP/include/amqpcpp/address.h).
- an XDBC connection string is never parsed by ClickHouse: it is read
verbatim in ITableFunctionXDBC and forwarded to the bridge as one opaque
parameter, so its grammar is the driver's. This repository's own examples
put the password in a query parameter, and the majority spelling is
DSN=...;Pwd=... . No URI scan can bound a credential in that.
A password containing '/' is an ordinary shape, so this is not a corner case.
Apply one rule instead: a value is echoed with only its credential hidden
where ClickHouse owns its grammar, and a value whose grammar belongs to
someone else is not echoed. The second half is already the policy in
DatabaseURL.cpp and in the old XDBC fallback. Concretely, the two broker
addresses and the XDBC connection string are hidden whole (for the brokers,
exactly when the value carries an '@', which is a superset of anything either
parser can read as a credential and is empty precisely when there is nothing
to hide, so the documented forms such as localhost:4222 and amqp://h:5672/v
stay fully visible).
The three query-level URL settings are read through Poco::URI, so there the
partial mask is right and is kept: it moves from maskURIPassword to
maskURIUserinfo, which is what fixes the reporter's three cases. They gain a
fail-closed leg, because nothing validates them at SET time, resolveURLBase
accepts a base whose '://' is anywhere rather than at the start, and a
statement is masked for logging before its settings are validated: a value
with no scheme in front of it therefore reaches a log, and no parser can say
which part of it is the credential. url_base additionally reaches the Azure
URL parser, where abfs and abfss put a non-secret container in front of the
'@', so it gets its own rule that skips the userinfo leg for those two
schemes; az and azure have no '@' in their grammar and need no exception.
Two holes in the XDBC named-collection branch close as a side effect of
dropping its bespoke helper for findSecretNamedArgument: only the first
occurrence of a duplicated datasource key used to be masked, and a key written
as a constant expression (concat('data', 'source') = ...) was applied by the
named-collection parser while being invisible to this finder, so the whole
connection string was echoed. The fail-closed scan that already covered that
hazard for the TLS keys is extracted and reused, leaving one carrier for it.
The anchored-scheme test is likewise reduced to one carrier, findURIAuthority,
which also replaces a private copy in DatabaseURL.cpp that used the
locale-dependent std::isalpha and std::isalnum.
Deliberate consequences: a broker address carrying a stray '@' and any
URI-shaped connection string are now hidden whole, and
format_display_secrets_in_show_and_select still shows the real value to a
privileged reader. maskURIUserinfo itself is unchanged, which the re2
equivalence test proves by passing unedited.
Closes: ClickHouse#121257
…eption Three defects found reviewing the previous commit. The abfs/abfss exception in CoreSettings::SETTINGS_TO_HIDE masked strictly LESS than the code it replaced. maskURLBaseCredentials returned maskPresignedURLParameters(value) alone for those two schemes, so url_base = 'abfss://c:pw@a.dfs.core.windows.net/d/' logged pw verbatim where maskURIPassword had logged abfss://c:[HIDDEN]@A... . That is a masking regression in one of the settings this change set names, and it also contradicted the rule the rest of the work applies: the exception's own comment said the credential of such a base is the SAS in the query string, and then delegated to a helper that carries only the AWS and GCS parameter names and so cannot mask a SAS. A value whose credential ClickHouse cannot bound is not echoed with a partial mask. The exception is therefore deleted rather than widened, url_base goes back to maskURLCredentials, all three URL settings share one rule for every scheme, and Core no longer mirrors a copy of the Azure grammar that had to be kept in sync with parseAzureURL. This supersedes the paragraph of the previous commit message that described the container as staying visible. findTableEngineSecretArguments dispatched on the engine name and had branches for NATS and XDBC but none for RabbitMQ or Kafka, although exactly three engines read named-collection overrides out of their engine arguments (tryGetNamedCollectionWithOverrides(args.engine_args, ...) in StorageKafkaUtils.cpp, StorageNATS.cpp and StorageRabbitMQ.cpp) and only NATS had a finder. So ENGINE = RabbitMQ(nc, rabbitmq_address = 'amqp://u:secret@h/') ENGINE = RabbitMQ(nc, rabbitmq_password = 'secret') ENGINE = Kafka(nc, kafka_sasl_password = 'secret') reached SHOW CREATE, system.tables.create_table_query and system.query_log.query in cleartext, while the same settings written in a SETTINGS clause were hidden by the engine's own SETTINGS_TO_HIDE. Rather than copy the NATS loop, its body becomes findBrokerTableEngineSecretArguments(secret_keys, address_key), so the requirement that the finder and SETTINGS_TO_HIDE agree has one carrier; NATS and RabbitMQ are two thin wrappers over it. Kafka deliberately does not use it, because Kafka still accepts the legacy positional form Kafka('brokers', 'topics', 'group', 'format', ...) whose arguments are all non-secret, so failing closed on a positional argument there would hide information a user is entitled to see. NATS and RabbitMQ accept no positional argument other than the collection name, so for those two a positional is hidden. In the XDBC named-collection branch every helper walks equals arguments only, so a positional argument after the collection name was never masked and jdbc(nc, 'jdbc://u:pw@h/db') echoed the connection string. Such a statement is invalid and XDBC rejects it, but rejection happens after the statement has been formatted for logging. The branch now hides any argument after the collection name that is not a readable key = value pair, which is the treatment the NATS sibling already gave that shape. Tests: seven arms added to 05233_broker_and_xdbc_credential_masking, each positive paired with the control that has to stay fully visible, in particular an '@'-free rabbitmq_address and Kafka's legacy positional form. Three gtests added next to the NATS ones, each asserting the engine-argument spelling, the SETTINGS spelling and that control. Every existing NATS assertion, the re2 equivalence test and the TLS expectations pass unedited.
Two defects found reviewing the previous commit, both the same shape one argument index further out: a scan that starts at index 1 is only correct when index 0 is proven to be a named-collection name. The Kafka branch the previous commit added scanned from index 1, but Kafka is the one engine here whose collection name is optional: registerStorageKafka falls through to the legacy positional path when there is no identifier at index 0, and rejects a malformed call in checkAndGetLiteralArgument only after the statement has been formatted for logging. So ENGINE = Kafka(kafka_sasl_password = 'secret', 'clickhouse') was masked nowhere and reached system.query_log.query and the server log in cleartext. The shape is one users write; 03305_fix_kafka_table_with_kw_arguments exists for exactly ENGINE = Kafka(a = '1', 'clickhouse'), and kafka_sasl_password is the engine's only secret setting. Both scans now start at 0. That cannot widen the mask, because findNamedArgument and markNamedArgumentsWithUnreadableKeys skip any argument that is not an equals function with two arguments, which is what keeps the legacy positional form fully visible. findXDBCSecretArguments had the mirror image in its other branch. The else branch is taken whenever index 0 is not an identifier, and it hid only index 0, so in a keyword-argument call with no collection the connection string sits at another index and was echoed: jdbc(external_table = 't', datasource = 'DSN=x;Uid=u;Pwd=secret') jdbc and odbc have no validity precondition and ITableFunctionXDBC validates after formatting, so the call is logged. Whether the credential leaked depended only on the order the keys were typed in, while the named-argument style without a collection is what a user coming from s3(url = ..., access_key_id = ...) writes. The two aliases and the unreadable-key scan now run from index 1 in that branch too, on top of the unchanged markSecretArgument(0, false). For a valid positional call all three additions are no-ops, since no equals argument exists, so the schema and table arguments stay visible. The class is now closed rather than the two examples: every other named-argument scan in the file that starts at a non-zero index sits inside an isNamedCollectionName test, so index 0 is proven to be an identifier there, and these two branches were the only ones without that property. Two arms are added to 05233_broker_and_xdbc_credential_masking, each keeping its non-secret neighbour visible so it doubles as the over-masking control, plus one gtest for the XDBC engine spelling and one assertion block for the Kafka one.
findMySQLFunctionSecretArguments and findMySQLDatabaseSecretArguments scanned for TLS credentials from index 5 and index 4 in their positional branches, one past the last positional slot. The keys those scans hide are PEM material (ssl_ca_pem, ssl_cert_pem, ssl_key_pem and the three PostgreSQL spellings), the contents of a certificate or a private key, so a credential written before the positional arguments was masked nowhere: mysql(ssl_key_pem = '<key>', '127.0.0.1:3306', 'db', 't', 'u') CREATE DATABASE d ENGINE = MySQL(ssl_ca_pem = '<cert>', '127.0.0.1:3306', 'db', 'u') Such a call is invalid, but ASTFunction::formatImpl runs the finder whenever show_secrets is false, which is before any argument is validated, so the statement reaches system.query_log.query and the server log in cleartext first. The first function serves the mysql and postgresql table functions and the MySQL, PostgreSQL and MaterializedPostgreSQL table engines; the second serves the MySQL, PostgreSQL, MaterializedPostgreSQL, Remote and RemoteSecure database engines. Both scans now start at 0. That cannot widen the mask, because findNamedArgument and markNamedArgumentsWithUnreadableKeys skip any argument that is not an equals function with exactly two arguments, so every positional literal stays visible. The pre-existing arms of 04648_mysql_tls_credentials keep asserting that byte for byte. Two corrections to the previous commit's message. It claimed the class was closed, on the grounds that every other named-argument scan in the file starting at a non-zero index sits inside an isNamedCollectionName test and that the two branches it fixed were the only ones without that property. That was wrong. These two positional MySQL branches have the same shape and sit in the negative branch of the same test, where index 0 is proven not to be a collection name, and they are closed here. The statement that holds now is that no named-argument scan in the file starts past index 0 unless index 0 is proven to be a collection name. It also said the two arms it added each keep their non-secret neighbour visible, "so it doubles as the over-masking control". That holds for the Kafka arm, whose positional neighbour 'clickhouse' stays visible. It does not hold for the jdbc one: that neighbour sits at index 0 and is hidden whole by the untouched markSecretArgument(0, false). The over-masking control for that branch is d1 to d3, which keep their schema and table arguments visible. Two arms are added to 04648_mysql_tls_credentials, the test that already owns these keys, one per code path, through the formatter that both share.
The positional branches of findMySQLFunctionSecretArguments and
findMySQLDatabaseSecretArguments got the password wrong in one way with two
faces: a positional slot is not a raw argument index, and a positional branch
owes the same named-argument scan its named-collection sibling runs.
A `key = value` argument written before the positional block shifts every
positional one index along, so markSecretArgument(4) and markSecretArgument(3)
hid the wrong argument and echoed the password; and because the branch scanned
named arguments only for the TLS keys, a `password =` override written there was
masked nowhere at all:
mysql(ssl_key_pem = '<key>', '127.0.0.1:3306', 'db', 't', 'u', '<password>')
mysql('127.0.0.1:3306', 'db', 't', password = '<password>', 'u')
CREATE DATABASE d ENGINE = MySQL(ssl_ca_pem = '<cert>', '127.0.0.1:3306', 'db', 'u', '<password>')
CREATE DATABASE d ENGINE = MySQL('127.0.0.1:3306', 'db', password = '<password>', 'u')
Such calls are invalid, but ASTFunction::formatImpl runs the finder whenever
show_secrets is false, which is before any argument is validated, so the
statement reaches system.query_log.query and the server log in cleartext first.
Both faces are fixed together. classifyPositionalArguments returns the raw
indexes of the arguments that are not `key = value` pairs, so the branch reads
its slot out of that list instead of counting raw arguments, and a positional
argument written after the first named one is hidden rather than listed: the
parsers reject that mix, the statement is formatted before they do, and the slot
it was meant to fill is then unknowable. classifyS3Arguments already applies that
rule to the s3 signatures, and the helper reuses its shape rather than its body,
which carries S3-specific key handling. The branch then runs
findSecretNamedArgument("password", 0), the call its named-collection sibling
already makes.
A well-formed call is unchanged byte for byte: its named arguments come after the
positional block, so the returned list is [0, 1, 2, ...] and the slot lookup
yields the same index as before. Twenty of the twenty-two formatting assertions
of 04648_mysql_tls_credentials are untouched; the two that move are the arms that
previously recorded a credential as visible, and they now carry the password so
the file's own leak oracle can see it.
The invariant this closes is narrow, and only the part I enumerated is claimed. A
named-argument scan in this file starts past index 0 in twenty-four places. It is
safe where index 0 is proven to be a collection name, and also where index 0 is
hidden whole: findXDBCSecretArguments's else branch scans from 1 with
isNamedCollectionName(0) false, and is safe because markSecretArgument(0, false)
above it hides that argument entirely. No claim is made for the file as a whole.
A scan that starts past index 0 behind isNamedCollectionName(N) for N greater
than zero still skips a secret named argument written first, which
ExternalDistributed, azureBlobStorage and remote were measured to echo. Those are
untouched here.
StorageURL::resolveURLBase rejects a base with no "://" by interpolating the raw
value into its BAD_ARGUMENTS message. The value is a setting that can carry a
credential, so the credential is disclosed in full through the client error, the
exception column of the query log and the server log:
SELECT * FROM url('data.csv', CSV, 'c String')
SETTINGS url_base = 'user:<password>@example.invalid/def/'
Code: 36. The `url_base` setting must contain a scheme (e.g. https://),
got: user:<password>@example.invalid/def/
Nothing gates that. url_base is settable in a settings profile, so the value
belongs to whoever wrote the profile while the error goes to whichever user trips
it, and the message is not behind format_display_secrets_in_show_and_select. The
same message carries s3_base and format_avro_schema_registry_url: the callers
that reach it with a raw user value are TableFunctionURL.cpp for url_base and
ObjectStorage/S3/Configuration.cpp in two places for s3_base.
The value is dropped rather than masked. A masker only finds a credential it can
locate: maskURLCredentials fires on a value containing '@', so
s3_base = 'DSN=x;Pwd=<password>' would still be echoed whole. The setting name
stays in the message, which is what a user needs in order to find their own typo.
The sibling carrier already has this treatment. DatabaseURL refuses a scheme-less
base without echoing it, and 04627_url_database_engine asserts that with a leak
count. The url_base and s3_base arms of the same invariant grep only the message
prefix, which is why nothing in tree caught this. They now carry the same pair of
assertions, so all three carriers are pinned the same way. The text before the
removed ", got:" is byte identical, so those pre-existing prefix arms and their
reference lines do not move.
The new arms run clickhouse-local rather than clickhouse-client: the client
appends the query it was given to an exception, and that echo is the caller's own
input rather than anything the message disclosed, so a leak count taken through
the client can never reach zero.
04648_mysql_tls_credentials gains two arms as well. The two
findSecretNamedArgument("password", 0) calls added to the positional MySQL
branches in the previous commit were exercised by no test: every existing named
password arm goes through the collection branch, and the arms that carry a named
credential beside a positional block use a TLS key. The new arms are the
collection-less named password shape for each branch, and deleting either call
reddens exactly one of them.
Delete comments narrating this change's own design, keep the third-party masker semantics (libnats, AMQP-CPP, XDBC driver grammar) and the cross-file sync invariants as standalone properties.
…0516 Backport ClickHouse#120516 to 26.8: Fix access checks in PREWHERE and indexHint() that used aliases
…an `Array(Tuple(...))`
…onary sources
A right table with a row policy is joined through its query plan instead of its special storage, so the `FilterStep` carrying the policy is kept. The `ACCESS_DENIED` for `JOIN` and `joinGet` on a `Join` table and its docs are not backported. This also drops the `registerStatementRowPolicy` block that the conflict resolution pulled into the parser: `StatementFactory` does not exist in 26.8. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…3060 Backport ClickHouse#123060 to 26.8: Hide queue engine secrets written with the legacy `s3queue_` prefix
…2345 Backport ClickHouse#122345 to 26.8: Fix LOGICAL_ERROR on lambda over an `Array(Tuple(...))`
…2673 Backport ClickHouse#122673 to 26.8: Hide HTTP headers of `HTTP` dictionary sources
…1398 Backport ClickHouse#121398 to 26.8: Skip the direct join when the right table has a row policy
… of the indexed column and of constants in the text index
… the drop privilege of the replaced table via `Replicated` databases and `ON CLUSTER`
…n bypass via embedded NUL byte in certificate CN
…3566 Backport ClickHouse#123566 to 26.8: Fix parallel replicas over a Distributed table over a Distributed table
…4931 Backport ClickHouse#124931 to 26.8: Fix slow planning of queries with skip indexes on tables with pending patch parts
…rialized view reads its source table from a subquery in an expression
…4740 Backport ClickHouse#124740 to 26.8: Fix a crash on INSERT when a materialized view reads its source table from a subquery in an expression
…expression into the join graph
…` JOIN ON condition evaluated during the join
…7250 Backport ClickHouse#117250 to 26.8: Do not merge a non-deterministic expression into the join graph
…4325 Backport ClickHouse#114325 to 26.8: Reject `arrayJoin` in an `ie_join` JOIN ON condition evaluated during the join
… that cannot be widened inside its type
…earch_lightweight_delete_limit` Reading `SELECT ... FINAL PREWHERE _row_exists` with `apply_prewhere_after_final = 1` fails with `THERE_IS_NO_COLUMN` in 26.8 regardless of the vector index (reproduced without any index). The bug is fixed on master by ClickHouse#114578, which is not in 26.8. Keep the `EXPLAIN` check that the index is not used in this case, which is what this backport changes. CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=125108&sha=76bfddc8e3fe65c82d6272e7107fb6cb46e658ca&name_0=BackportPR&name_1=Stateless%20tests%20%28amd_asan_ubsan%2C%20distributed%20plan%2C%20parallel%29 PR: ClickHouse#125108 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…he Native protocol
…9712 Backport ClickHouse#119712 to 26.8: Saturate an Iceberg decimal bound that cannot be widened inside its type
…9371 Backport ClickHouse#119371 to 26.8: Do not restrict a vector search to index rows when a delete is pending
…4233 Backport ClickHouse#124233 to 26.8: Do not derive conjuncts in outer JOIN ON
…7015 Backport ClickHouse#117015 to 26.8: Bound the collections read from the Native protocol
…ounts in columns_substreams.txt
…4157 Backport ClickHouse#124157 to 26.8: Do not allocate memory from the counts in columns_substreams.txt
… integration harness decodes captured output
…ks with checkStackSize
…4280 Backport ClickHouse#124280 to 26.8: Bump `thrift` from v0.24.0 to v0.25.0
…4282 Backport ClickHouse#124282 to 26.8: Bump `mongo-c-driver` from 2.5.4 to 2.5.5
…8195 Backport ClickHouse#118195 to 26.8: Tolerate non-UTF-8 bytes when the integration harness decodes captured output
…4303 Backport ClickHouse#124303 to 26.8: Fix wrong results when skip indexes evaluate a predicate with OR
…0885 Backport ClickHouse#120885 to 26.8: Guard the constant-to-literal walks with checkStackSize
Release v26.8.25.14-lts
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
13 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changelog category (leave one):
CI/CD Options
Exclude tests:
Regression jobs to run: