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.

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

Feature/ssl #2584

Merged
merged 26 commits into from
Sep 24, 2021
Merged

Feature/ssl #2584

merged 26 commits into from
Sep 24, 2021

Conversation

Shylock-Hg
Copy link
Contributor

@Shylock-Hg Shylock-Hg commented Aug 26, 2021

See detail in #2556 , the client should be implemented by corresponding maintainer @vesoft-inc/graph-reviewers

The test certificates generating require interactive, and almost other open source projects like even openssl also put the test certificates to source code, so we follow them.

Close #2587

@Shylock-Hg Shylock-Hg added the ready-for-testing PR: ready for the CI test label Aug 26, 2021
@Shylock-Hg Shylock-Hg requested review from a team August 26, 2021 09:58
@Shylock-Hg
Copy link
Contributor Author

Close #2587

@Sophie-Xie Sophie-Xie requested review from CPWstatic and jievince and removed request for a team September 16, 2021 05:11
@Sophie-Xie Sophie-Xie added this to the v2.6.0 milestone Sep 22, 2021
src/daemons/GraphDaemon.cpp Show resolved Hide resolved
src/common/ssl/SSLConfig.cpp Outdated Show resolved Hide resolved
src/common/thrift/ThriftClientManager.h Outdated Show resolved Hide resolved
tests/nebula-test-run.py Outdated Show resolved Hide resolved
Copy link
Contributor

@yixinglu yixinglu left a comment

Choose a reason for hiding this comment

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

Please do not directly commit some private keys and certificates to the repo, you can use scripts or Makefile to generate them as needed

src/common/thrift/ThriftClientManager-inl.h Show resolved Hide resolved
@Shylock-Hg
Copy link
Contributor Author

Please do not directly commit some private keys and certificates to the repo, you can use scripts or Makefile to generate them as needed

The test certificates generating require interaction, and almost other open source projects like even openssl also put the test certificates to source code, so we follow them.

Aiee
Aiee previously approved these changes Sep 24, 2021
src/common/ssl/SSLConfig.cpp Outdated Show resolved Hide resolved
DEFINE_string(ca_path, "", "Path to trusted CA file.");
DEFINE_bool(enable_ssl, false, "Whether enable ssl.");
DEFINE_bool(enable_graph_ssl, false, "Whether enable ssl of graph server.");
DEFINE_bool(enable_meta_ssl, false, "Whether enable ssl of meta server.");
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems that two flags is enough to cover the cases about inside and outside the nebula cluster. Do we really need to add another meta_ssl for users to make a decision?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the suggestion from @sherman-the-tank , which to encrypt the sensitive data in meta.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean whether enable_ssl and enable_graphd_ssl can already cover the enable_meta_ssl scenario, just like below:

* enable_graphd_ssl=true & enable_ssl=false

clients <- ssl -> graphd <- no ssl -> metad <- no ssl -> storaged
                     ^                                    ^
                     |______________ no ssl ______________|


* enable_graphd_ssl=false & enable_ssl=true

clients <- no ssl -> graphd <- ssl -> metad <- ssl -> storaged
                       ^                                 ^
                       |______________ ssl ______________|


* enable_graphd_ssl=true & enable_ssl=true

clients <- ssl -> graphd <- ssl -> metad <- ssl -> storaged
                    ^                                 ^
                    |______________ ssl ______________|

* enable_graphd_ssl=false & enable_ssl=false

clients <- no ssl -> graphd <- no ssl -> metad <- no ssl -> storaged
                        ^                                    ^
                        |______________ no ssl ______________|

Is enable_meta_ssl just to encrypt schema information when communicating with meta? If so, make sense.

.github/workflows/pull_request.yml Show resolved Hide resolved
Copy link
Contributor

@jievince jievince left a comment

Choose a reason for hiding this comment

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

LGTM

@yixinglu yixinglu merged commit 6fa47a4 into vesoft-inc:master Sep 24, 2021
@yixinglu yixinglu added the doc affected PR: improvements or additions to documentation label Sep 24, 2021
@Shylock-Hg Shylock-Hg deleted the feature/ssl branch February 9, 2022 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc affected PR: improvements or additions to documentation ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[QE]Crypto in transportation
6 participants