The user name and password are printed in the debug level log

[huldarchen]

ClickHouseStatementImpl：L578。

[TTcheng]

I found the same issue occured in my project logs,It seems like that the request uses plain text password, this is extremely dangerous

DEBUG 2020-12-23 15:21:51.732 [hikariPool connection adder] [] [] ru.yandex.clickhouse.ClickHouseDriver              - Creating connection
DEBUG 2020-12-23 15:21:51.732 [hikariPool connection adder] [] [] ru.yandex.clickhouse.ClickHouseConnectionImpl      - Create a new connection to jdbc:clickhouse://10.253.xxx.xx:8123/dw
DEBUG 2020-12-23 15:21:51.734 [hikariPool connection adder] [] [] ru.yandex.clickhouse.ClickHouseStatementImpl       - Executing SQL: select timezone() FORMAT TabSeparatedWithNamesAndTypes;
DEBUG 2020-12-23 15:21:51.734 [hikariPool connection adder] [] [] ru.yandex.clickhouse.ClickHouseStatementImpl       - Request url: http://10.253.xxx.xx:8123/?query_id=3e0c253c-f34b-4d4e-8414-09ec2c7cd6f1&database=dw&user=ck_uat&extre
mes=0&compress=1&password=xxxxxxxxxxx
```

[enqueue]

I see a couple of short-term, i.e. 0.2.5 target, options here. What are your opinions?

1. Hard-coded RegExp filter which can be used in ClickhouseStatementImpl, and perhaps one or five more places
2. Try to find a way to install RegExp filter somehow. I don't think there is a way built-into SLF4J without interfering with users' implementation settings, though. I don't like the idea of introducing something like ClickHouseLogger.getLogger() (which would include such a filter) and forcing all developers to use that.
3. Use HTTP Basic Auth and get rid of these query parameters. I am unsure because original driver authors chose otherwise. Do you see any compatibility issues?

[zhicwu]

I see a couple of short-term, i.e. 0.2.5 target, options here. What are your opinions?

Thanks @enqueue. I agree with you that the first two are not ideal, while the third is better. Actually, in a private version of ClickHouse JDBC driver, I added authMode setting and made basic auth as default. It was not only for security reason but also to ensure the driver works with clickhouse server(s) behind proxy like httpd/nginx/traefik. I think we should go with the third. However, we may still need to keep query parameter for older version. We can add 19.14 into this and add regression test case accordingly. By the way, there'll be one or two cases failed on 19.14 ;)

@alexey-milovidov / @den-crane / @filimonov, do you have impression of why we use query parameter to pass credential instead of basic auth in http header? Any specific version of clickhouse server should be added into regression?

[filimonov]

do you have impression of why we use query parameter to pass credential instead of basic auth in http header?

Have no idea. Basic auth is totally ok (and always was).

[zhicwu]

@enqueue, I hope you don't mind that I merged PR #546 just now. Now 19.14 is added into regression. If you have docker installed, you can run tests against specific version of ClickHouse on develop branch by issue command line below:

mvn -DclickhouseVersion=19.17 clean verify

I think we can proceed with the change by using basic auth instead of query parameters.

[enqueue]

Test plan for user / password combinations versus server settings:

ClickHouse user setting	Property: user	Property: password	Expected result
default			Success
default	default		Success
default	default	bar	Failure
default		bar	Failure
default / bar		bar	Success (hard to test using 1 container instance )
default / bar	default	bar	Success (hard to test using 1 container instance)
foo / bar			Failure
foo / bar	foo		Failure
foo / bar	foo	baz	Failure
foo / bar	foo	bar	Success
foo / bar		bar	Failure
foo / bar	baz	bar	Failure
oof			Failure
oof	oof		Success
oof	oof	bar	Failure
oof	baz		Failure

User / password versus authorization

As mentioned by @zhicwu in the PR #548 we should try not to break behavior for users who want to supply a custom HTTP Authorization header using the parameter authorization.

Property authorization	Property user	Property password	Expected HTTP headers
Digest username="foo",...			Authorization: Digest username="foo",...
Digest username="foo",...	bar		Authorization: Digest username="foo",...
Digest username="foo",...		baz	Authorization: Digest username="foo",...
Digest username="foo",...	bar	baz	Authorization: Digest username="foo",...
	bar	baz	Authorization: Basic YmFyOmJheg== (baz:baz)
	bar		Authorization: Basic YmFyOg== (bar:)
		baz	Authorization: Basic ZGVmYXVsdDpiYXo= (default:baz)

Caveats

Use of authorization and user / password properties will be exclusive, i.e. clients will not be able to supply a user name in addition to a custom HTTP Authorization header.

[zhicwu]

This has been fixed on develop and will be released as part of 0.2.6. With basic auth and a proxy(httpd/nginx etc.) in front of ClickHouse, you can now do lots of stuff like ActiveDirectory authentication :)