ClickHouse / clickhouse-js

Official JS client for ClickHouse DB
https://clickhouse.com
Apache License 2.0
189 stars 22 forks source link

Colon ':' character in username causes username to be parsed incorrectly #254

Closed Antlion12 closed 3 months ago

Antlion12 commented 3 months ago

Describe the bug

I have a ClickHouse username abc:def. Performing createClient({ username: 'abc:def', ...}) returns an error of the form.

    abc: Authentication failed: password is incorrect, or there is no user with such name.
      at parseError (node_modules/packages/client-common/src/error/parse_error.ts:29:12)
      at ClientRequest.onResponse (node_modules/packages/client-node/src/connection/node_base_connection.ts:133:28)

As if the first half of the username (abc) is treated as the full username.

Oddly, username abc:def works for the clickhouse client CLI tool.

slvrtrn commented 3 months ago

@Antlion12, can you make it work via curl? I tried a few variations of escaping, but it does not seem to work even there:

CREATE USER 'foo:bar' IDENTIFIED BY ''

This is the reported issue itself:

➜  ~ curl -u "foo:bar:" --data-binary "SELECT 1" "http://localhost:8123"                     
Code: 516. DB::Exception: foo: Authentication failed: password is incorrect, or there is no user with such name. (AUTHENTICATION_FAILED) (version 24.3.1.2672 (official build))

Manual encoding with %3A does not work:

➜  ~ curl -u "foo%3Abar:" --data-binary "SELECT 1" "http://localhost:8123"
Code: 516. DB::Exception: foo%3Abar: Authentication failed: password is incorrect, or there is no user with such name. (AUTHENTICATION_FAILED) (version 24.3.1.2672 (official build))

Adding backslashes in various representations does not help, too:

➜  ~ curl -u "foo\:bar:" --data-binary "SELECT 1" "http://localhost:8123" 
Code: 516. DB::Exception: foo\: Authentication failed: password is incorrect, or there is no user with such name. (AUTHENTICATION_FAILED) (version 24.3.1.2672 (official build))
➜  ~ curl -u "foo\\:bar:" --data-binary "SELECT 1" "http://localhost:8123"
Code: 516. DB::Exception: foo\: Authentication failed: password is incorrect, or there is no user with such name. (AUTHENTICATION_FAILED) (version 24.3.1.2672 (official build))
➜  ~ curl -u "foo%5C:bar:" --data-binary "SELECT 1" "http://localhost:8123"
Code: 516. DB::Exception: foo%5C: Authentication failed: password is incorrect, or there is no user with such name. (AUTHENTICATION_FAILED) (version 24.3.1.2672 (official build))

Wrapping either the username or the password in extra double quotes does not work:

➜  ~ curl -u "foo:bar:""" --data-binary "SELECT 1" "http://localhost:8123"
Code: 516. DB::Exception: foo: Authentication failed: password is incorrect, or there is no user with such name. (AUTHENTICATION_FAILED) (version 24.3.1.2672 (official build))
➜  ~ curl -u ""foo:bar":" --data-binary "SELECT 1" "http://localhost:8123"
Code: 516. DB::Exception: foo: Authentication failed: password is incorrect, or there is no user with such name. (AUTHENTICATION_FAILED) (version 24.3.1.2672 (official build))

NB: clickhouse-client uses TCP/Native, which, unlike the HTTP interface, does not send the username + password as the basic auth header, which is separated by a colon (and this is causing this issue).

NB2: JS URL is not happy about this, too:

new URL('http://foo:bar:qaz@localhost:8123/').username
'foo'
new URL('http://foo\:bar:qaz@localhost:8123/').username
'foo'
new URL('http://foo\\:bar:qaz@localhost:8123/').username
''
new URL('http://foo%3Abar:qaz@localhost:8123/').username
'foo%3Abar'
new URL('http://"foo:bar":qaz@localhost:8123/').username
'%22foo'
new URL('http://\'foo:bar\':qaz@localhost:8123/').username
'%27foo'
new URL('http://\"foo:bar\":qaz@localhost:8123/').username
'%22foo'
slvrtrn commented 3 months ago

The web auth spec is pretty clear: https://www.rfc-editor.org/rfc/rfc2617#section-2

user-pass   = userid ":" password
userid      = *<TEXT excluding ":">
password    = *TEXT

Even if we fix it by sending different headers on the connection level (and adjust a fair bit of tests accordingly), it will still not work with an upcoming URL configuration, and it will require different hacks there (and more testing), as it is essentially working with the JS URL object (which behaves differently than cURL, which still does not work).

I'd say that, for now, we won't fix it. Please use a username compatible with HTTP auth standards.

Antlion12 commented 3 months ago

Appreciate you digging into this @slvrtrn. Those are the exact errors i ran into. I'll ask our admin to make the change to the username to exclude :.

Meanwhile, where can I file an feature request for clickhouse-client to add validation to disallow :-containing usernames? Or does that require a deeper change to the SQL interpreter?

slvrtrn commented 3 months ago

We could only do that if the username is provided via the configuration object.

However, this use case (with URL configuration) is tricky:

> const u = new URL('http://foo:bar:qaz@localhost:8123/db')
> u.username
'foo'
> u.password
'bar%3Aqaz'

Currently, it does not look like we can reasonably assume anything in the client config parser. Multiple colons by the HTTP auth standard are disambiguated with all colons going into the password part, as it is allowed only there. As you can see in the example above, it always goes into the password part (with an automatic URI component encoding), effectively cutting off the rest of the username, which we think was intended to be foo:bar (but we have no way to validate that with the URL, as it could've been username foo + password bar:qaz, and it is perfectly valid).

slvrtrn commented 3 months ago

@Antlion12, regarding disallowing usernames with colons in clickhouse-client (CLI client), as I mentioned, it's only the limitation of the HTTP interface, which is used by many language clients, including this one, but it is not the main one for ClickHouse. The "main" one is TCP, and that is used by the CLI client.

For now, the JS client is tied to the HTTP interface and its related protocol limitations. However, it should not be disallowed in ClickHouse because of specific HTTP standards such as basic auth. And I hope that this client will support TCP in the future, too.

Antlion12 commented 3 months ago

Totally understood. Will just be mindful of that in the future and leave it be. Thanks again!