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

optimize: add secure authentication to interfaces in ClusterController #6042

Merged
merged 35 commits into from
Nov 29, 2023

Conversation

ggbocoder
Copy link
Contributor

@ggbocoder ggbocoder commented Nov 17, 2023

  • I have registered the PR changes.

Ⅰ. Describe what this PR did

Add secure authentication to interfaces in ClusterController,all requesets in seata-discovery-raft will take token to visit clusterController interface.

Ⅱ. Does this pull request fix one issue?

fixes #6012

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

TODO: It is necessary to implement the JWT renewal mechanism first.

Copy link

codecov bot commented Nov 17, 2023

Codecov Report

Merging #6042 (7db947e) into 2.x (03e88d5) will decrease coverage by 0.37%.
The diff coverage is 25.19%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #6042      +/-   ##
============================================
- Coverage     49.57%   49.20%   -0.37%     
+ Complexity     4789     4787       -2     
============================================
  Files           909      913       +4     
  Lines         31416    31685     +269     
  Branches       3778     3824      +46     
============================================
+ Hits          15573    15592      +19     
- Misses        14305    14546     +241     
- Partials       1538     1547       +9     
Files Coverage Δ
...ommon/exception/AuthenticationFailedException.java 0.00% <0.00%> (ø)
.../io/seata/common/exception/RetryableException.java 0.00% <0.00%> (ø)
...re/properties/registry/RegistryRaftProperties.java 16.66% <10.00%> (-8.34%) ⬇️
...main/java/io/seata/common/util/HttpClientUtil.java 0.00% <0.00%> (ø)
...scovery/registry/raft/RaftRegistryServiceImpl.java 16.73% <36.47%> (ø)

... and 7 files with indirect coverage changes

@funky-eyes funky-eyes added this to the 2.x Backlog milestone Nov 17, 2023
@ggbocoder
Copy link
Contributor Author

此pr依赖jwt的续期机制,由于jwt的续期机制还没有实现,因此考虑基于现有的jwt接口进行修改,基于目前的jwt验证有两个方案:
方案一:seata-discovery-raft在初始化时开启定时任务调用login接口,刷新token(这样需要新增定时任务的执行周期这一配置项,同时需要保证定时任务的执行间隔小于server端配置的token失效时间)
方案二:seata-discovery-raft保存token和token的失效时间,在每次调用clusterController的接口前判断当前时间是否大于失效时间,是的话就调用login接口刷新token(这样做的话需要修改目前的jwt的鉴权接口,目前的login接口只会返回一个token,而不会返回token的失效时间)
This PR relies on the JWT renewal mechanism. Due to the absence of the JWT renewal mechanism, we are considering modifying it based on the existing JWT interface. There are two proposed solutions for the current JWT verification:

Option 1: Seata-discovery-raft initializes and activates a scheduled task to call the login interface for token refresh. This requires adding a configuration item for the scheduled task execution period, ensuring that the task interval is shorter than the token expiration time configured on the server.

Option 2: Seata-discovery-raft saves the token and its expiration time. Before each call to the clusterController's interface, it checks whether the current time is greater than the expiration time. If true, it calls the login interface to refresh the token. This approach requires modifying the existing JWT authentication interface, as it currently only returns a token and does not include the token expiration time.

@ggbocoder
Copy link
Contributor Author

i will try to add some test later.

# Conflicts:
#	discovery/seata-discovery-raft/src/main/java/io/seata/discovery/registry/raft/RaftRegistryServiceImpl.java
@funky-eyes funky-eyes modified the milestones: 2.x Backlog, 2.1.0 Nov 24, 2023
…y/registry/raft/RaftRegistryServiceImpl.java

Co-authored-by: funkye <[email protected]>
@funky-eyes funky-eyes closed this Nov 28, 2023
@funky-eyes funky-eyes reopened this Nov 28, 2023
Copy link
Contributor

@funky-eyes funky-eyes left a comment

Choose a reason for hiding this comment

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

LGTM

@funky-eyes funky-eyes merged commit 1921b2d into apache:2.x Nov 29, 2023
8 checks passed
// get token and set it in cache
if (StringUtils.isNotBlank(raftClusterAddress)) {
String[] tcAddressList = raftClusterAddress.split(",");
String tcAddress = tcAddressList[0];
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the original ip1 goes offline?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add secure authentication to interfaces in ClusterController
3 participants