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

TLS/SSL Security Framework #349

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

TLS/SSL Security Framework #349

wants to merge 24 commits into from

Conversation

huerni
Copy link
Collaborator

@huerni huerni commented Nov 6, 2024

No description provided.

Nativu5
Nativu5 reviewed Nov 6, 2024
View reviewed changes
.gitignore Outdated
@@ -61,13 +61,15 @@ dependencies/.extracted

# CMake
cmake-build-*/
build/
Copy link
Collaborator

Choose a reason for hiding this comment

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

gitignore 有一些重复的条目

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

这个commit暂时的，后面不会推上来

Nativu5
Nativu5 requested changes Nov 6, 2024
View reviewed changes
dependencies/cmake/CMakeLists.txt Outdated
add_subdirectory(BSThreadPool)
add_subdirectory(nlohmann_json)
# add_subdirectory(nlohmann_json)
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个依赖不能去掉

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

这个也在想怎么解决

protos/PublicDefs.proto
repeated AllowedPartitionQos allowed_partition_qos_list = 5;
repeated string coordinator_accounts = 6;
AdminLevel admin_level = 7;
string password = 3;
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔 明文？

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

有SSL的话，前端加密应该没必要了吧

Copy link
Collaborator

Choose a reason for hiding this comment

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

SSL不一定强制启用 可能还是考虑Hash 存储再加一个Salt

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@RileyWen @Nativu5 @L-Xiafeng 密码这块感觉还得对一下 今天马老师说集群已经认证过账号密码了，所以鹤思本身不需要密码，直接clogin获取token即可。现在的实现是clogin通过uid和密码登录（密码可为空），所以目前的安全框架是否需要对鹤思系统加入密码，保留现在的实现？还是说现在不需要密码，只依赖集群认证登录？

Copy link
Collaborator

Choose a reason for hiding this comment

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

这个应该还是需要的：

  1. 之前的规划中有提到前端在客户主机而不只是在登录节点上，那么自然需要密码；
  2. 从 JWT 的原理来讲，如果调用 clogin 传递 UID 就能使服务端签发 JWT，那么冒充者可以直接仿造一个 clogin 请求，传递一个受害者的 UID 来拿到 JWT。此时 JWT 防冒充的作用就完全失效了。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

确实是的

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@RileyWen @Nativu5 @L-Xiafeng 马老师认为鹤思不能有输入密码这个步骤，但要保证获取token是安全的。感觉有点麻烦，目前我只能想到AddUser时传回密码保存文件用于登录自动读取，但文件权限保证不了，感觉不太行，或者手动在登录节点配置类似于公私钥这种，你们有没有什么更好的想法

src/CraneCtld/CraneCtld.cpp Outdated Show resolved Hide resolved
src/CraneCtld/CraneCtld.cpp Outdated Show resolved Hide resolved
L-Xiafeng
L-Xiafeng reviewed Nov 6, 2024
View reviewed changes
etc/config.yaml

DomainSuffix: crane.com

JwtCertFilePath: /etc/crane/jwt.pem
Copy link
Collaborator

Choose a reason for hiding this comment

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

这一堆config新建一个SSL层级吧

SSL:
- CranedCertPath
- Ctld....
- xxxx

dependencies/cmake/jwt-cpp/CMakeLists.txt
URL_HASH SHA256=b9eb270e3ba8221e4b2bc38723c9a1cb4fa6c241a42908b9a334daff31137406
INACTIVITY_TIMEOUT 5
)
set(JWT_BUILD_EXAMPLES OFF CACHE BOOL "disable building examples" FORCE)
Copy link
Collaborator

@L-Xiafeng L-Xiafeng Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

这个需要cache吗？
Crane动态链接需不需要修改参数？需要确认一下

@L-Xiafeng L-Xiafeng added enhancement New feature or request build Build system change labels Nov 6, 2024
RileyWen
RileyWen reviewed Nov 19, 2024
View reviewed changes
src/CraneCtld/CtldForCforedServer.cpp Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

*Server和CranedKeeper 统一移到RpcService的子文件夹下吧

RileyWen
RileyWen reviewed Nov 19, 2024
View reviewed changes
src/Utilities/PublicHeader/GrpcHelper.cpp Outdated
info_->server_context()->TryCancel();
return;
}
char token[iter->second.size()];
Copy link
Collaborator

Choose a reason for hiding this comment

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

C++里面就别写C系风格了吧

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

只能这样，直接转为string的话会出bug

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

不对，好像也是可以用string操作的

huerni added 21 commits December 4, 2024 11:24
@huerni
feat: external tls
3c576b1
@huerni
feat: Separate the CraneCtldForCrane gRPC service and connect Crane t…
8557142 
…o CraneCtld.
@huerni
feat: Separate the CraneCtldForCfored gRPC service and connect Cfored…
b5eb611 
… to CraneCtld.
@huerni
feat: Move the SubmitTaskToScheduler function to TaskScheduler.
71ecb99
@huerni
feat: update config.yaml
7b66c6a
@huerni
feat: jwt util
ac9f8d0
@huerni
feat: jwt config
f1eae39
@huerni
feat: User adds password information.
bf1b015
@huerni
refactor: jwt-cpp uses in-project json dependency
04f2882
@huerni
feat: token
9073d12
@huerni
refactor
75ec4c8
@huerni
refactor
b41d4dc
@huerni
feat: login
ab5b36b
@huerni
feat: login allowlist
f83c3d2
@huerni
fix: The backend retrieves the token with control characters present.
77ab683
@huerni
refactor: config
2e317b6
@huerni
feat: grpc shutdown
67e21f1
@huerni
refactor
6f02465
@huerni
feat: refactor grpc server
418fbee
@huerni
refactor: login
45023b1
@huerni
refactor
6a06ba7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@L-Xiafeng L-Xiafeng L-Xiafeng left review comments

@Nativu5 Nativu5 Nativu5 requested changes

@RileyWen RileyWen RileyWen left review comments

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
build Build system change enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@huerni @RileyWen @Nativu5 @L-Xiafeng