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

Fix service crash caused by connecting using a pre-v2.6.0 client #3942

Merged
merged 5 commits into from
Mar 11, 2022
Merged

Fix service crash caused by connecting using a pre-v2.6.0 client #3942

merged 5 commits into from
Mar 11, 2022

Conversation

Aiee
Copy link
Contributor

@Aiee Aiee commented Feb 24, 2022

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

Fix #3935

Description:

Fix service crash caused by connecting using a pre-v2.6.0 client.

Error message when connecting the v3.0.0 Nebula service with v2.5.0 console:

> ./nebula-console -addr 192.168.8.6 -port 29562 -u root -p nebula
2022/02/24 22:01:21 [INFO] connection pool is initialized successfully
2022/02/24 22:01:21 Fail to create a new session from connection pool, fail to authenticate, error: The version of the client sending request from "::ffff:192.168.8.6":49722 is lower than v2.6.0, please update the client.
panic: Fail to create a new session from connection pool, fail to authenticate, error: The version of the client sending request from "::ffff:192.168.8.6":49722 is lower than v2.6.0, please update the client.

goroutine 1 [running]:
log.Panicf(0x655651, 0x35, 0xc00010de78, 0x1, 0x1)
        /usr/local/go/src/log/log.go:361 +0xc5

How do you solve it?

Add a flag in mataClient to record whether the client triggered verifyClientVersion(). If the client called verifyClientVersion, the client has a version >= v2.6.0. Otherwise the client version is lower than v2.5.0.

When doing auth(), check the IP address of the client to see if the client is later than v2.5.0. Return an error if the client version is lower than v2.5.0.

Special notes for your reviewer, ex. impact of this fix, design document, etc:

Checklist:

Tests:

  • Unit test(positive and negative cases)
  • Function test
  • Performance test
  • N/A

Affects:

  • Documentation affected (Please add the label if documentation needs to be modified.)
  • Incompatibility (If it breaks the compatibility, please describe it and add the label.）
  • If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
  • Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to be reflected in release notes and how to describe:

ex. Fixed the bug .....

@Aiee Aiee added ready-for-testing PR: ready for the CI test type/bug Type: something is unexpected labels Feb 24, 2022
@Sophie-Xie Sophie-Xie added the cherry-pick-v3.0 PR: need cherry-pick to this version label Feb 25, 2022
@Aiee Aiee force-pushed the fix-client-connection branch 2 times, most recently from dd44f0d to d6db366 Compare February 28, 2022 07:56
@Sophie-Xie Sophie-Xie added the doc affected PR: improvements or additions to documentation label Mar 3, 2022
Shylock-Hg
Shylock-Hg reviewed Mar 10, 2022
View reviewed changes
src/graph/service/PasswordAuthenticator.cpp Outdated
@@ -13,7 +13,16 @@ PasswordAuthenticator::PasswordAuthenticator(meta::MetaClient* client) {
}

Status PasswordAuthenticator::auth(const std::string& user, const std::string& password) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't modify auth function, do it outside.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to pass the client addr into MetaClient::authCheckFromCache(), and auth is the middleware

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could add clientIp to meta client in GraphService::auth

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have passed clientIp in src/graph/service/GraphService.cpp line 122
Besides, the GraphService::auth() in the enterprise version also has clientIp as parameter. This should be fine

CPWstatic
CPWstatic previously approved these changes Mar 10, 2022
View reviewed changes
Copy link
Contributor

@CPWstatic CPWstatic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@Aiee Aiee force-pushed the fix-client-connection branch from 1dc8b3d to c3cc849 Compare March 11, 2022 03:38
@Aiee Aiee requested review from CPWstatic and Shylock-Hg March 11, 2022 03:40
@CPWstatic CPWstatic merged commit cd38313 into vesoft-inc:master Mar 11, 2022
@Aiee Aiee deleted the fix-client-connection branch March 11, 2022 05:55
Sophie-Xie pushed a commit that referenced this pull request Mar 11, 2022
@Aiee @Sophie-Xie
Fix service crash caused by connecting using a pre-v2.6.0 client (#3942)
c5c2786 
* Reject clients with a version lower than 2.6.0

* Add TTL for clientAddr_

* Fix tests

* Use client_idle_timeout_secs as the clientAddrTimeout

* Change the param of authCheckFromCache()
@jamieliu1023 jamieliu1023 mentioned this pull request Mar 12, 2022
Closed
@HarrisChu
Copy link
Contributor

depends on client behavior, i.e. client should verify the version and then auth immediately.

prefer to combine these two interfaces in the future.

auth(user, password, version)

CPWstatic pushed a commit that referenced this pull request Mar 14, 2022
@Sophie-Xie @panda-sheep
@Shylock-Hg @jackwener @Aiee
Cherry pick v3.0.1 (#4007)
d523376 
* fix fix 0 file error (#3920)

* Fix crash of null path expression. (#3915)

* Fix crash of null path expression.

* Add test case.

Co-authored-by: jakevin <30525741+jackwener@users.noreply.github.com>

* Change the name of source code zip (#3999)

* Fix service crash caused by connecting using a pre-v2.6.0 client (#3942)

* Reject clients with a version lower than 2.6.0

* Add TTL for clientAddr_

* Fix tests

* Use client_idle_timeout_secs as the clientAddrTimeout

* Change the param of authCheckFromCache()

Co-authored-by: panda-sheep <59197347+panda-sheep@users.noreply.github.com>
Co-authored-by: shylock <33566796+Shylock-Hg@users.noreply.github.com>
Co-authored-by: jakevin <30525741+jackwener@users.noreply.github.com>
Co-authored-by: Yichen Wang <18348405+Aiee@users.noreply.github.com>
This was referenced Mar 23, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Shylock-Hg Shylock-Hg Shylock-Hg approved these changes

@CPWstatic CPWstatic CPWstatic approved these changes

Assignees
No one assigned
Labels
cherry-pick-v3.0 PR: need cherry-pick to this version doc affected PR: improvements or additions to documentation ready for review ready-for-testing PR: ready for the CI test type/bug Type: something is unexpected
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Connect using a pre-v2.6.0 client will cause a service crash
5 participants
@Aiee @HarrisChu @CPWstatic @Shylock-Hg @Sophie-Xie