Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not allow to reuse previous credentials in case of inter-server secret #29060

Merged
merged 4 commits into from
Oct 1, 2021
Merged

Do not allow to reuse previous credentials in case of inter-server secret #29060

merged 4 commits into from
Oct 1, 2021

Conversation

azat
Copy link
Collaborator

@azat azat commented Sep 15, 2021 •
edited

Changelog category (leave one):

  • Bug Fix (user-visible misbehaviour in official stable or prestable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Do not allow to reuse previous credentials in case of inter-server secret (Before INSERT via Buffer/Kafka to Distributed table with interserver secret configured for that cluster, may re-use previously set user for that connection)

Fixes: #13156

@robot-clickhouse robot-clickhouse added the pr-bugfix Pull request with bugfix, not backported by default label Sep 15, 2021
@azat
Copy link
Collaborator Author

azat commented Sep 16, 2021

Cc: @vitlibar

@azat azat force-pushed the inter-server-secret-auth-fix branch from 125808b to 7f1160c Compare September 17, 2021 19:07
@azat
Copy link
Collaborator Author

azat commented Sep 18, 2021

Docs check — Found 1 errors

e18af44

Functional stateless tests (debug) — fail: 1, passed: 3363, skipped: 19

2010_lc_native - #29153

@azat
Copy link
Collaborator Author

azat commented Sep 20, 2021

@alexey-milovidov maybe you can take a look? (or ping @vitlibar somehow / or at least assign the PR to him)

@azat
Copy link
Collaborator Author

azat commented Sep 26, 2021 •
edited

@vitlibar can you please take a look?

vitlibar
vitlibar reviewed Sep 27, 2021
View reviewed changes
src/Server/TCPHandler.cpp Outdated Show resolved Hide resolved
@azat azat marked this pull request as draft September 27, 2021 20:37
@azat azat force-pushed the inter-server-secret-auth-fix branch from 7f1160c to 1127bcd Compare September 27, 2021 20:51
@azat azat marked this pull request as ready for review September 27, 2021 20:53
@azat
Copy link
Collaborator Author

azat commented Sep 28, 2021

Fast test — fail: 1, passed: 2912, skipped: 498. Other tests won't be run before fast test will be fixed.

2021-09-28 01:43:49 2024_merge_regexp_assert:                                               [ FAIL ] - return code: 81
2021-09-28 01:43:49 Expected server error code: 47 but got: 81 (query: SELECT a FROM merge(REGEXP('\0'), '^t$'); -- { serverError 47 }).
2021-09-28 01:43:49 Received exception from server (version 21.11.1):
2021-09-28 01:43:49 Code: 81. DB::Exception: Received from localhost:9000. DB::Exception: Database test_xlj25w doesn't exist. (UNKNOWN_DATABASE)
2021-09-28 01:43:49 (query: SELECT a FROM merge(REGEXP('\0'), '^t$'); -- { serverError 47 })

#29461

@azat azat force-pushed the inter-server-secret-auth-fix branch 2 times, most recently from 4bf96c8 to 7599b42 Compare September 28, 2021 06:09
@CLAassistant
Copy link

CLAassistant commented Sep 28, 2021 •
edited

CLA assistant check
All committers have signed the CLA.

@azat azat force-pushed the inter-server-secret-auth-fix branch 2 times, most recently from c78a077 to 770e096 Compare September 28, 2021 19:48
@azat
Copy link
Collaborator Author

azat commented Sep 29, 2021 •
edited

@alesapin can you take a look at CI? Looks like it's hanged?

vitlibar
vitlibar reviewed Sep 29, 2021
View reviewed changes
src/Server/TCPHandler.cpp Outdated Show resolved Hide resolved
@azat azat force-pushed the inter-server-secret-auth-fix branch 4 times, most recently from db3bed5 to c087fc0 Compare September 30, 2021 08:16
@azat
Copy link
Collaborator Author

azat commented Sep 30, 2021

Can someone add force-test label?

@azat azat force-pushed the inter-server-secret-auth-fix branch 2 times, most recently from 7d95e09 to 72ac401 Compare September 30, 2021 21:46
@azat
tests/integration/helpers: add query_id for query()
2822f71
@azat
Add a test for INSERT w/o user in interserver mode
1af02f0 
v2: ensure that the test fails with the version w/o fix
v3: force connect by modifying config and reload it
v4: add comments
@azat
Do not allow to reuse previous credentials in case of inter-server se…
e8a90b8 
…cret

Before this patch INSERT via Buffer/Kafka may re-use previously set user
for that connection, while this is not correct, it should reset the
user, and use global context.

Note, before [1] there was a fallback to default user, but that code had
been removed, and now it got back.

  [1]: 0159c74 ("Secure inter-cluster query execution (with initial_user as current query user) [v3]")

Also note, that context for Buffer table (and others) cannot be changed,
since they don't have any user only profile.

I've tested this patch manually using the following:

    create table dist (key Int) engine=Distributed(test_cluster_two_shards_secure, default, data, key);
    create table buffer (key Int) engine=Buffer(default, dist, 1, 0, 0, 0, 0, 0, 0);
    create table data (key Int) engine=Memory();

    # to start the connection with readonly user
    $ clickhouse-client --user readonly -q 'select * from dist'
    $ clickhouse-client -q 'insert into buffer values (1)'
    # before this patch this produces errors like:
    # 2021.09.27 23:46:48.384920 [ 19474 ] {} <Error> default.dist.DirectoryMonitor: Code: 164. DB::Exception: Received from 127.0.0.2:9000. DB::Exception: readonly: Cannot execute query in readonly mode. Stack trace:

v2: reset the authentication instead of using default user (as suggested by @vitlibar)
v3: reset Session::user and introduce ClientInfo::resetAuthentication (as suggested by @vitlibar)
v4: reset the session every time in interserver mode (suggested by @vitlibar)
@azat
Fix SessionLog::addLoginSuccess() for ClientInfo::Interface::TCP_INTE…
91f6cf4 
…RSERVER

In this case there there can be no user.
CI report [1]:

    Address: 0x1f Access: read. Address not mapped to object.
    Stack trace: 0x24494b2f 0x244784c6 0x260fe1af 0x260ed672 0x260ddffa 0x26104880 0x2d62f3af 0x2d6300a1 0x2d926666 0x2d91fb7a 0x7f8bcea3c609 0x7f8bce963293
    3.1. inlined from ./obj-x86_64-linux-gnu/../contrib/libcxx/include/string:0: std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::operator=(std::__1::basic_string>
    3. ../src/Interpreters/SessionLog.cpp:216: DB::SessionLog::addLoginSuccess(StrongTypedef<wide::integer<128ul, unsigned int>, DB::UUIDTag> const&, std::__1::optional<std::__1::basic_string<char, st>
    4. ./obj-x86_64-linux-gnu/../src/Interpreters/Session.cpp:0: DB::Session::makeQueryContextImpl(DB::ClientInfo const*, DB::ClientInfo*) const @ 0x244784c6 in /usr/bin/clickhouse
    5.1. inlined from ./obj-x86_64-linux-gnu/../contrib/libcxx/include/memory:3299: std::__1::shared_ptr<DB::Context>::swap(std::__1::shared_ptr<DB::Context>&)
    5.2. inlined from ../contrib/libcxx/include/memory:3243: std::__1::shared_ptr<DB::Context>::operator=(std::__1::shared_ptr<DB::Context>&&)
    5. ../src/Server/TCPHandler.cpp:1208: DB::TCPHandler::receiveQuery() @ 0x260fe1af in /usr/bin/clickhouse
    6. ./obj-x86_64-linux-gnu/../src/Server/TCPHandler.cpp:0: DB::TCPHandler::receivePacket() @ 0x260ed672 in /usr/bin/clickhouse
    7. ./obj-x86_64-linux-gnu/../src/Server/TCPHandler.cpp:0: DB::TCPHandler::runImpl() @ 0x260ddffa in /usr/bin/clickhouse
    8. ./obj-x86_64-linux-gnu/../src/Server/TCPHandler.cpp:1643: DB::TCPHandler::run() @ 0x26104880 in /usr/bin/clickhouse

  [1]: https://clickhouse-test-reports.s3.yandex.net/29060/c087fc0ed5fbea133eb3dc3a64b8db93a81d0ece/integration_tests_flaky_check_(asan).html#fail1
@azat azat force-pushed the inter-server-secret-auth-fix branch from 72ac401 to 91f6cf4 Compare September 30, 2021 22:13
@azat
Copy link
Collaborator Author

azat commented Oct 1, 2021

Marker check Pending — Waiting to start Integration tests (debug)

Hunged?

@vitlibar vitlibar merged commit 27f6d58 into ClickHouse:master Oct 1, 2021
@azat azat deleted the inter-server-secret-auth-fix branch October 1, 2021 18:06
This was referenced Oct 1, 2021
Closed
Merged
Merged
Closed
Closed
@robot-clickhouse robot-clickhouse mentioned this pull request Oct 6, 2021
Merged
robot-clickhouse pushed a commit that referenced this pull request Oct 6, 2021
Backport #29060 to 21.10: Do not allow to reuse previous credentials …
c619314 
…in case of inter-server secret
@robot-clickhouse robot-clickhouse mentioned this pull request Oct 6, 2021
Merged
robot-clickhouse pushed a commit that referenced this pull request Oct 6, 2021
Backport #29060 to 21.9: Do not allow to reuse previous credentials i…
0406363 
…n case of inter-server secret
vitlibar pushed a commit that referenced this pull request Oct 12, 2021
Merge pull request #29814 from ClickHouse/backport/21.10/29060
79588d7 
Backport #29060 to 21.10: Do not allow to reuse previous credentials in case of inter-server secret
vitlibar pushed a commit that referenced this pull request Oct 13, 2021
Merge pull request #29815 from ClickHouse/backport/21.9/29060
0af826a 
Backport #29060 to 21.9: Do not allow to reuse previous credentials in case of inter-server secret
@azat azat mentioned this pull request Feb 2, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vitlibar vitlibar vitlibar approved these changes

Assignees

@vitlibar vitlibar

Labels
pr-bugfix Pull request with bugfix, not backported by default
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@azat @CLAassistant @vitlibar @robot-clickhouse