NATS: add inline credentials setting - #110733
Conversation
|
Workflow [PR], commit [10838a5] Summary: ✅
AI ReviewSummaryThis PR adds inline Final Verdict✅ No remaining review findings in the current PR head. LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 151/157 (96.18%) · Uncovered code |
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
|
I do have a potential patch for the AI/ CI reported potential trial issue, but I haven't pushed it as I was not yet find time to rebuild everything to validate it... |
A table-level `nats_credential_file` or `nats_credentials` setting now overrides both config-level sources, so a table with `nats_credential_file` no longer silently authenticates with a server-level `nats.credentials`. When both credential sources are set in the server configuration and none in the table settings, throw `BAD_ARGUMENTS` instead of silently preferring the inline credentials. Addresses the AI review finding on ClickHouse#110733 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…amed-collection overrides `NATS` takes its arguments as overrides of a named collection (`ENGINE = NATS(collection, nats_credentials = '...')`), so the credentials can appear as engine arguments and not only in the `SETTINGS` clause: * `FunctionSecretArgumentsFinder` had no `NATS` branch, so `nats_password`, `nats_token`, `nats_credential_file` and `nats_credentials` passed as named overrides leaked verbatim into `SHOW CREATE TABLE` and the query log. Mask them there too, next to the `SETTINGS` clause masking done by `NATS::SETTINGS_TO_HIDE`. * The mutual-exclusion check of `nats_credential_file` and `nats_credentials` ran on the raw `changed` bits after `loadFromNamedCollection`, so a collection providing one source plus a query override providing the other threw `BAD_ARGUMENTS` instead of letting the more specific query value replace the collection value. Resolve the effective source instead: a source specified in the query (`SETTINGS` clause or named-collection override) replaces the collection source, while two sources specified at the same level stay ambiguous. Also drop the hand edit of `docs/reference/engines/table-engines/integrations/nats.mdx`: that page is generated from the `Documentation` block of `registerStorageNATS`, which is updated in this pull request, and a direct edit is rejected by the autogenerated-region guard of `Docs check (Mintlify)`.
|
🕵 Pushed
On the remaining reds of the previous run, none of which are related to this change:
|
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 99/104 (95.19%) · Uncovered code |
…berParser`
`Poco::strToInt` accumulated digits guarding only against `result > max / base`. With
`result == max / base` there is still room for only `max % base` in the last digit, so appending
a larger digit overflowed the accumulator. For a signed type that is undefined behavior, and the
wrapped-around value was returned as a successfully parsed number.
This is reachable from user input through `Poco::JSON`, which parses every integer with
`NumberParser::tryParse64`: a JSON number above the `UInt64` maximum was silently parsed as a
wrong value instead of being rejected, so
`SELECT formatQueryFromJSON('{"type":"Literal","value":{"field_type":"UInt64","value":18446744073709551617}}')`
returned `1`. The same reason made a `UInt64` literal above the `Int64` maximum fail to survive a
`parseQueryToJSON` / `formatQueryFromJSON` round trip: it wrapped to a negative number, and
`Poco::JSON` never reached its `parseUnsigned64` fallback.
The magnitude is now accumulated in the unsigned counterpart of the target type, both because the
carry check needs the exact remaining room for the last digit and because the magnitude of the most
negative value of a signed type does not fit into that type (`-9223372036854775808` parsed only by
accident of the overflow before).
Found by the AST fuzzer:
`runtime error: signed integer overflow: 9223372036854775800 + 9 cannot be represented in type 'long'`
in `Poco::strToInt<long>` via `Poco::JSON::ParserImpl::handle` and `DB::IAST::createFromJSON`.
https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=110733&sha=b94fe95fe4bb45b4c9f48dc6241e7ff0dd623581&name_0=PR&name_1=AST%20fuzzer%20%28arm_asan_ubsan%29
ClickHouse#110733
`findNATSTableEngineSecretArguments` only hid the named overrides listed in `nats_secret_keys`, so `ENGINE = NATS(collection, nats_url = 'nats://user:password@host:4222')` leaked the password in `SHOW CREATE TABLE` and in the query log, even though the `SETTINGS` clause form already masks `nats_url` through `NATS::SETTINGS_TO_HIDE`. The named overrides are now walked once: a `nats_url` value is masked with `maskURIPassword`, keeping everything but the userinfo password visible, and a key or a `nats_url` value that cannot be read as a plain literal is hidden whole (fail closed) - an unevaluable key can name a secret setting, and a url built from an expression can embed the credentials in its pieces. Unit tests `ParserCreateQuery.MaskNATSTableEngineURLPassword` and `ParserCreateQuery.MaskNATSTableEngineNonLiteralArguments`. Also tag `04665_nats_credentials_named_collection` as `no-parallel, no-replicated-database`: named collections are server-global, and the flaky check runs the same test concurrently, so the first finishing repetition drops the collections while the others still use them (`NAMED_COLLECTION_DOESNT_EXIST`).
|
🕵 Pushed
The remaining red of the previous run is unrelated to this change: |
`formatForLogging` runs before the engine rejects the argument list, so a malformed query such as `ENGINE = NATS(nc, '/var/nats.creds')` logged the literal secret even though the engine later throws `BAD_ARGUMENTS`. The only positional argument the engine accepts is the named-collection name in the first position; any other non-`key = value` argument is now hidden whole (fail closed). Unit test `ParserCreateQuery.MaskNATSTableEnginePositionalArguments`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🕵 Pushed
The only red of the previous run is unrelated to this change: |
The legacy Docusaurus tree docs/en was removed on master; the PR's edit of docs/en/engines/table-engines/integrations/nats.md is dropped because the content already lives in the embedded Documentation block of registerStorageNATS.
…ential source The `credential_file_from_collection` / `credentials_from_collection` flags were captured before `loadFromQuery`, so a `SETTINGS` assignment of the key the named collection already provides was still classified as collection-sourced. As a result `ENGINE = NATS(collection) SETTINGS nats_credential_file = '...', nats_credentials = '...'` was accepted and `resolveCredentialSource` silently kept one source instead of throwing `BAD_ARGUMENTS`. Clear the collection provenance for keys assigned in the `SETTINGS` clause after `loadFromQuery`, and cover both mixed directions in 04665_nats_credentials_named_collection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🕵 Pushed
The two CI reds on |
robot-ch-test-poll3
added
the
pr-synced-to-cloud
| -- A `SETTINGS` assignment of the key the collection already has is query-level as well, | ||
| -- so providing both sources through the `SETTINGS` clause is ambiguous in both directions. | ||
| CREATE TABLE nats_both_in_settings_over_file (key UInt64) ENGINE = NATS(04665_nats_credential_file) | ||
| SETTINGS nats_credential_file = '/var/other.creds', nats_credentials = 'user JWT and seed'; -- { serverError BAD_ARGUMENTS } |
There was a problem hiding this comment.
@groeneai if there is a user_files_path server setting limiting where files can be read from, is this going to throw an error? It shouldn't be allowed, I think
There was a problem hiding this comment.
No, it does not throw, and I agree it should not be allowed.
I checked master (604017b) and confirmed it empirically on a server with user_files_path set. nats_credential_file reaches libnats unvalidated: StorageNATS.cpp:119 expands the setting and stores it in the connection configuration, and NATSConnection.cpp:28 hands it straight to natsOptions_SetUserCredentialsFromFiles, which fopens the path (util.c nats_ReadFile). There is no fileOrSymlinkPathStartsWith / getUserFilesPath check anywhere on that path, unlike StorageFileLog.cpp:193 or StorageEmbeddedRocksDB.cpp:264.
Two observable consequences with user_files_path configured:
-
Filesystem probe. The error text distinguishes the three cases, so
CREATE TABLEbecomes an existence/readability oracle for arbitrary absolute paths:/tmp/does_not_exist->error opening file '...': No such file or directory/etc/shadow->error opening file '/etc/shadow': Permission denied/etc/passwd->no nkey user seed found in '/etc/passwd'
-
Content exfiltration to an attacker-chosen endpoint, since
nats_urlis also user-supplied. Against a listener of mine that advertisesauth_requiredplus a nonce, a file outsideuser_files_pathwhose last line is a valid seed had its entire preceding content sent in theCONNECTframe'sjwtfield:
"jwt":"TOPSECRET_DB_PASSWORD=hunter2\nTOPSECRET_TOKEN=ghp_abcdef\nSUACSSL3..."
That comes from_getJwtOrSeed(conn.c) falling back toNATS_STRDUPof the whole buffer when no JWT header is found. A file with no seed at all fails before the CONNECT is sent, so the leak needs a seed line in the file, but the path itself is unrestricted.
Mitigation that does exist: NATS declares .source_access_type = AccessTypeObjects::Source::NATS (StorageNATS.cpp:941), so a user without the grant gets Code: 497 ... necessary to have the grant TABLE ENGINE ON NATS. So this needs the NATS source grant, which is why I read it as a privilege-boundary gap rather than an unauthenticated hole.
Note this is not from #110733. nats_credential_file was added in e822ba3 (Feb 2024) and b597a8597054fd3 (master before this merge) has the same unguarded SetUserCredentialsFromFiles call. #110733 added the inline nats_credentials alternative, which reads no file at all.
Unless you would rather own it, I will open a PR adding the ctor-time fileOrSymlinkPathStartsWith(..., getUserFilesPath()) guard, matching FileLog, with a stateless test asserting a path outside user_files_path is refused.
There was a problem hiding this comment.
Understood.
One thing to flag before this closes: the nats_credential_file path gap is not from #110733, and the revert does not close it. The setting came in e822ba3a47dea82 (2024-02-02) and the merge parent b597a8597054fd3 has the same unguarded natsOptions_SetUserCredentialsFromFiles call. After the revert it is also the only way to pass NATS credentials, so restricting SQL-supplied paths the way StorageMySQL::getSSLParams does (e700bbec4c84c585) would leave no SQL alternative.
Should I restrict it anyway, or leave it as is? I am not opening anything until you say.
This reverts ClickHouse#110733 (merge commit 48ebef1): the `nats_credentials` setting of the `NATS` table engine, the mutual-exclusion check against `nats_credential_file`, the masking of the `NATS` table-engine arguments in `FunctionSecretArgumentsFinder`, and the tests `04665_nats_credentials_named_collection` and `ParserCreateQuery.MaskNATS*`. `src/Parsers/FunctionSecretArgumentsFinder.h` was resolved by hand: only the `NATS` declarations are removed, while the TLS-credentials and BigQuery argument finders added to the same lists afterwards are kept. `docs/reference/engines/table-engines/integrations/nats.mdx` still mentions `nats_credentials` inside its `{/*AUTOGENERATED_START*/}` region. It is generated from the `Documentation` block of `registerStorageNATS`, which this commit updates, and direct edits of a generated region are rejected by the docs check, so the page is left to the nightly documentation autogeneration.
…n file `nats_credential_file` is a path on the server filesystem: the server opens it with its own privileges, and during authentication the credentials are sent to `nats_url`, which comes from the same query. So a path taken from SQL lets anyone who can define a `NATS` source probe the local filesystem and exfiltrate files the server can read to a NATS server they control: #110733 (comment) The path is now accepted only from a named collection defined in the server configuration file, or as `nats.credential_file` in the server configuration itself; every SQL spelling throws `BAD_ARGUMENTS` and points to the inline `nats_credentials` setting instead. Loading from previously-validated metadata (server startup, force-restore, and short-syntax `ATTACH`) is exempt, so tables created before this restriction keep working after an upgrade. This fixes the reason of the revert #114178, allowing this revert of the revert: #114644 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
6 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Add
nats_credentialssetting to the NATS table engine, allowing users to specify NATS credentials inline as a string (matching the payload of a.credsfile).Detailed Description
This pull request adds support for specifying NATS credentials directly as a string via a new
nats_credentialssetting, in addition to the existing file-based option. It also ensures proper validation and updates the documentation and codebase accordingly.New NATS credentials support:
nats_credentialssetting to allow specifying NATS credentials content directly as a string (matching the payload of a.credsfile with user JWT and seed).NATSConnection.cppto usenats_credentialsfrom memory if provided, falling back tonats_credential_fileotherwise.Validation and security:
StorageNATS.cppto ensure only one ofnats_credential_fileornats_credentialscan be specified at a time, throwing aBAD_ARGUMENTSexception if both are set.SETTINGS_TO_HIDEinNATS_fwd.h) to includenats_credentials, ensuring sensitive data is not exposed in logs or system tables.Documentation updates:
integrations/nats.mdandnats.mdx) to describe the newnats_credentialsparameter and its usage syntax.Related issues
At least somewhat related to #85213
Validation
I noticed in #69396
So I have tried to provide validation that can be reviewed alongside this change in this commit on my fork addshore@0649f97
It checks happy and sad paths, and validates the existing behavior of using a file as well as then passing that file content in directly as a string.
The "evidence" is in https://github.com/addshore/ClickHouse/blob/0649f97219a7d27c0398cc99dbbff034cf47e3bd/v/VALIDATION.md
Version info
26.8.1.788(included in26.8and later)