refactor(auth): simplify authentication logic and remove hardcoded credentials#73
Conversation
Upgraded CodeQL GitHub Actions to v3 for improved security and features. Enhanced .gitignore and pom.xml to exclude and clean up dist.sh files during build and packaging. Removed unused hugegraph-struct dependency from hugegraph-store. Updated NOTICE copyright year to 2025.
dosubot
bot
added
the
size:L
|
@codecov-ai-reviewer review |
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough该 PR 简化 PD 认证逻辑：移除外部令牌/缓存与若干工具接口，新增内置服务白名单并改为仅校验服务名，新增 ServiceConstant 常量替代硬编码，调整测试与配置默认值以配合新认证方式。 Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Auth as Authentication\nFilter
participant White as innerModules\n(Set)
participant Service as Service\nHandler
Client->>Auth: authenticate(authority, token, tokenCall, call)
Auth->>Auth: 解析 authority -> name
Auth->>White: contains(name)?
alt 白名单内 (hg/store/hubble/vermeer)
White-->>Auth: true
Auth->>Service: call.get()
Service-->>Auth: result
Auth-->>Client: result
else 非白名单
White-->>Auth: false
Auth-->>Client: AccessDeniedException("invalid service name")
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 分钟
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
On it! We are reviewing the PR and will provide feedback shortly. |
Summary of ChangesHello @koi2000, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the authentication system by simplifying the internal authentication logic and removing all hardcoded credentials. The primary goal is to enhance the security posture of the system by eliminating sensitive information from the codebase and streamlining the authentication process for internal services. This change makes the system more robust against credential exposure and easier to manage. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
This PR removes hardcoded authentication credentials from the codebase and simplifies the authentication logic in the PD (Placement Driver) service. The changes are part of addressing incomplete auth management, with the understanding that PD's REST and gRPC ports must be protected via network IP whitelisting in production.
Key changes include:
- Replacing hardcoded credentials with constants and empty strings
- Removing complex token validation logic in favor of simple username validation
- Eliminating hardcoded credential arrays and token generation methods
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| SampleRegister.java | Replaces hardcoded service name and authority with constants from ServiceConstant |
| PdRegister.java | Updates getInstance() to use ServiceConstant instead of hardcoded credentials |
| ServiceConstant.java | Introduces new constants for SERVICE_NAME and AUTHORITY (empty string) |
| ServerOptions.java | Changes default service access token from hardcoded bcrypt hash to empty string |
| PDPulseTest.java | Refactors test to use constants instead of hardcoded credentials |
| BaseTest.java | Updates test password field from bcrypt hash to empty string |
| TokenUtil.java | Removes hardcoded credential arrays, token generation method, and main method |
| Authentication.java | Simplifies authentication to only validate username against whitelist |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public class ServiceConstant { | ||
| // FIXME: Strictly prohibited from external exposure; network IP whitelisting must be configured in production environments. | ||
| public static final String SERVICE_NAME = "hg"; | ||
| public static final String AUTHORITY = ""; |
There was a problem hiding this comment.
The AUTHORITY constant is an empty string, which effectively disables authentication. While the PR description mentions IP whitelisting is required, having no fallback authentication mechanism creates a security risk if network controls are misconfigured or bypassed. Consider implementing a configuration-based approach where AUTHORITY must be explicitly set, rather than defaulting to an empty string.
| public class ServiceConstant { | |
| // FIXME: Strictly prohibited from external exposure; network IP whitelisting must be configured in production environments. | |
| public static final String SERVICE_NAME = "hg"; | |
| public static final String AUTHORITY = ""; | |
| // Added for AUTHORITY initialization | |
| import java.lang.IllegalStateException; | |
| public class ServiceConstant { | |
| // FIXME: Strictly prohibited from external exposure; network IP whitelisting must be configured in production environments. | |
| public static final String SERVICE_NAME = "hg"; | |
| public static final String AUTHORITY; | |
| static { | |
| String authority = System.getProperty("hugegraph.authority"); | |
| if (authority == null || authority.isEmpty()) { | |
| authority = System.getenv("HUGEGRAPH_AUTHORITY"); | |
| } | |
| if (authority == null || authority.isEmpty()) { | |
| throw new IllegalStateException("AUTHORITY must be explicitly set via the 'hugegraph.authority' system property or 'HUGEGRAPH_AUTHORITY' environment variable."); | |
| } | |
| AUTHORITY = authority; | |
| } |
| if (innerUsers.contains(name)) { | ||
| return call.get(); | ||
| } else { | ||
| throw new AccessDeniedException("invalid service name"); | ||
| } |
There was a problem hiding this comment.
Authentication now only validates the username without verifying credentials (password/token). Any request with a whitelisted username ('hg', 'store', 'hubble', 'vermeer') will be granted access regardless of the password provided. This creates a critical security vulnerability if the service is accessible beyond the intended network boundaries.
| private static final Set<String> innerUsers = new HashSet<>() { | ||
| { | ||
| add("hg"); | ||
| add("store"); | ||
| add("hubble"); | ||
| add("vermeer"); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The hardcoded set of valid usernames should be externalized to configuration. This would allow adding/removing services without code changes and redeployment. Consider using a ConfigOption or loading from a configuration file.
| private PDConfig pdConfig; | ||
|
|
||
| private static final Cache<String> TOKEN_CACHE = new Cache<>(); | ||
| private static volatile TokenUtil util; |
There was a problem hiding this comment.
The 'util' field is no longer used after removing the token validation logic. This field should be removed along with the TokenUtil dependency in this class.
![codecov-ai[bot]](https://avatars.githubusercontent.com/in/797565?s=60&v=4){kind=small}
| String name = info.substring(0, delim); | ||
| String pwd = info.substring(delim + 1); | ||
| if (!"store".equals(name)) { | ||
| if (util == null) { | ||
| synchronized (this) { | ||
| if (util == null) { | ||
| util = new TokenUtil(pdConfig.getSecretKey()); | ||
| } | ||
| } | ||
| } | ||
| String[] i = util.getInfo(name); | ||
| if (i == null) { | ||
| throw new AccessDeniedException("invalid service name"); | ||
| } | ||
| if (!StringUtils.isEmpty(token)) { | ||
| String value = TOKEN_CACHE.get(name); | ||
| if (StringUtils.isEmpty(value)) { | ||
| synchronized (i) { | ||
| value = kvService.get(getTokenKey(name)); | ||
| } | ||
| } | ||
| if (!StringUtils.isEmpty(value) && token.equals(value)) { | ||
| return call.get(); | ||
| } | ||
| } | ||
| if (StringUtils.isEmpty(pwd) || !StringEncoding.checkPassword(i[2], pwd)) { | ||
| throw new AccessDeniedException(invalidMsg); | ||
| } | ||
| token = util.getToken(name); | ||
| String tokenKey = getTokenKey(name); | ||
| String dbToken = kvService.get(tokenKey); | ||
| if (StringUtils.isEmpty(dbToken)) { | ||
| synchronized (i) { | ||
| dbToken = kvService.get(tokenKey); | ||
| if (StringUtils.isEmpty(dbToken)) { | ||
| kvService.put(tokenKey, token, | ||
| TokenUtil.AUTH_TOKEN_EXPIRE); | ||
| TOKEN_CACHE.put(name, token, | ||
| TokenUtil.AUTH_TOKEN_EXPIRE); | ||
| return tokenCall.apply(token); | ||
| } | ||
| } | ||
| } | ||
| if (innerUsers.contains(name)) { | ||
| return call.get(); | ||
| } else { | ||
| throw new AccessDeniedException("invalid service name"); | ||
| } | ||
| return call.get(); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
CRITICAL SECURITY ISSUE: The authentication logic has been drastically simplified to only check if a username exists in the innerUsers whitelist, completely removing password validation. The original code validated passwords using StringEncoding.checkPassword(), which provided essential security. This new implementation accepts any request where the username matches a whitelisted service name, regardless of the password or token provided. This represents a severe security regression that removes multi-factor authentication validation. Either proper password validation must be restored, or a clear architectural decision to rely solely on network-level security (as hinted in ServiceConstant.java comments) must be documented and enforced.
Severity: CRITICAL
🤖 Prompt for AI Agent
Fix this code. In
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/interceptor/Authentication.java#L65-L72:
CRITICAL SECURITY ISSUE: The authentication logic has been drastically simplified to
only check if a username exists in the `innerUsers` whitelist, completely removing
password validation. The original code validated passwords using
`StringEncoding.checkPassword()`, which provided essential security. This new
implementation accepts any request where the username matches a whitelisted service
name, regardless of the password or token provided. This represents a severe security
regression that removes multi-factor authentication validation. Either proper password
validation must be restored, or a clear architectural decision to rely solely on
network-level security (as hinted in ServiceConstant.java comments) must be documented
and enforced.
Did we get this right? 👍 / 👎 to inform future reviews.
| private static volatile TokenUtil util; | ||
| private static String invalidMsg = | ||
| "invalid token and invalid user name or password, access denied"; | ||
|
|
||
| private static final Set<String> innerUsers = new HashSet<>() { | ||
| { | ||
| add("hg"); | ||
| add("store"); | ||
| add("hubble"); | ||
| add("vermeer"); | ||
| } | ||
| }; | ||
| private static String invalidBasicInfo = "invalid basic authentication info"; |
There was a problem hiding this comment.
SECURITY CONCERN: The innerUsers set is hardcoded directly in the class. Whitelisted service names should be externalized to configuration files or environment variables to support different deployment environments (development, staging, production). Consider moving this to pdConfig or another configuration mechanism to avoid recompilation for different environments.
Severity: HIGH
🤖 Prompt for AI Agent
Fix this code. In
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/interceptor/Authentication.java#L40-L50:
SECURITY CONCERN: The `innerUsers` set is hardcoded directly in the class. Whitelisted
service names should be externalized to configuration files or environment variables to
support different deployment environments (development, staging, production). Consider
moving this to `pdConfig` or another configuration mechanism to avoid recompilation for
different environments.
Did we get this right? 👍 / 👎 to inform future reviews.
|
|
||
| @Component | ||
| public class Authentication { | ||
|
|
||
| @Autowired | ||
| private KvService kvService; | ||
| @Autowired | ||
| private PDConfig pdConfig; | ||
|
|
||
| private static final Cache<String> TOKEN_CACHE = new Cache<>(); | ||
| private static volatile TokenUtil util; | ||
| private static String invalidMsg = | ||
| "invalid token and invalid user name or password, access denied"; | ||
|
|
||
| private static final Set<String> innerUsers = new HashSet<>() { | ||
| { | ||
| add("hg"); | ||
| add("store"); | ||
| add("hubble"); | ||
| add("vermeer"); | ||
| } | ||
| }; | ||
| private static String invalidBasicInfo = "invalid basic authentication info"; | ||
|
|
There was a problem hiding this comment.
ARCHITECTURAL ISSUE: The removal of @Autowired fields for kvService and pdConfig eliminates all token caching and database persistence mechanisms. This removes the ability to revoke or invalidate tokens, making the authentication system susceptible to token reuse attacks. If token-based authentication is no longer needed, this should be clearly documented. If it's still needed elsewhere in the system, this change may introduce inconsistencies.
Severity: HIGH
🤖 Prompt for AI Agent
Fix this code. In
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/interceptor/Authentication.java#L37-L51:
ARCHITECTURAL ISSUE: The removal of `@Autowired` fields for `kvService` and `pdConfig`
eliminates all token caching and database persistence mechanisms. This removes the
ability to revoke or invalidate tokens, making the authentication system susceptible to
token reuse attacks. If token-based authentication is no longer needed, this should be
clearly documented. If it's still needed elsewhere in the system, this change may
introduce inconsistencies.
Did we get this right? 👍 / 👎 to inform future reviews.
| String name = info.substring(0, delim); | ||
| String pwd = info.substring(delim + 1); | ||
| if (!"store".equals(name)) { | ||
| if (util == null) { | ||
| synchronized (this) { | ||
| if (util == null) { | ||
| util = new TokenUtil(pdConfig.getSecretKey()); | ||
| } | ||
| } | ||
| } | ||
| String[] i = util.getInfo(name); | ||
| if (i == null) { | ||
| throw new AccessDeniedException("invalid service name"); | ||
| } | ||
| if (!StringUtils.isEmpty(token)) { | ||
| String value = TOKEN_CACHE.get(name); | ||
| if (StringUtils.isEmpty(value)) { | ||
| synchronized (i) { | ||
| value = kvService.get(getTokenKey(name)); | ||
| } | ||
| } | ||
| if (!StringUtils.isEmpty(value) && token.equals(value)) { | ||
| return call.get(); | ||
| } | ||
| } | ||
| if (StringUtils.isEmpty(pwd) || !StringEncoding.checkPassword(i[2], pwd)) { | ||
| throw new AccessDeniedException(invalidMsg); | ||
| } | ||
| token = util.getToken(name); | ||
| String tokenKey = getTokenKey(name); | ||
| String dbToken = kvService.get(tokenKey); | ||
| if (StringUtils.isEmpty(dbToken)) { | ||
| synchronized (i) { | ||
| dbToken = kvService.get(tokenKey); | ||
| if (StringUtils.isEmpty(dbToken)) { | ||
| kvService.put(tokenKey, token, | ||
| TokenUtil.AUTH_TOKEN_EXPIRE); | ||
| TOKEN_CACHE.put(name, token, | ||
| TokenUtil.AUTH_TOKEN_EXPIRE); | ||
| return tokenCall.apply(token); | ||
| } | ||
| } | ||
| } | ||
| if (innerUsers.contains(name)) { | ||
| return call.get(); | ||
| } else { | ||
| throw new AccessDeniedException("invalid service name"); | ||
| } |
There was a problem hiding this comment.
CODE QUALITY: The simplified logic always returns call.get() for whitelisted users. Consider adding logging to track authentication attempts (both successful and failed) for security audit trails. This is important for detecting unauthorized access attempts.
Severity: MEDIUM
🤖 Prompt for AI Agent
Fix this code. In
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/interceptor/Authentication.java#L65-L71:
CODE QUALITY: The simplified logic always returns `call.get()` for whitelisted users.
Consider adding logging to track authentication attempts (both successful and failed)
for security audit trails. This is important for detecting unauthorized access attempts.
Did we get this right? 👍 / 👎 to inform future reviews.
|
|
||
| private TokenGenerator generator; | ||
| public static final long AUTH_TOKEN_EXPIRE = 3600 * 24L * 1000; | ||
| private static String[] storeInfo = {"store", | ||
| "$2a$04$9ZGBULe2vc73DMj7r" + | ||
| "/iBKeQB1SagtUXPrDbMmNswRkTwlWQURE/Jy", | ||
| "E3UnnQa605go"}; | ||
| private static String[] serverInfo = {"hg", | ||
| "$2a$04$i10KooNg6wLvIPVDh909n" + | ||
| ".RBYlZ/4pJo978nFK86nrqQiGIKV4UGS", | ||
| "qRyYhxVAWDb5"}; | ||
| private static String[] hubbleInfo = {"hubble", | ||
| "$2a$04$pSGkohaywGgFrJLr6VOPm" + | ||
| ".IK2WtOjlNLcZN8gct5uIKEDO1I61DGa", | ||
| "iMjHnUl5Pprx"}; | ||
| private static String[] vermeer = {"vermeer", | ||
| "$2a$04$N89qHe0v5jqNJKhQZHnTdOFSGmiNoiA2B2fdWpV2BwrtJK72dXYD.", | ||
| "FqU8BOvTpteT"}; | ||
| private static Map<String, String[]> apps = new HashMap<>() {{ | ||
| put(storeInfo[0], storeInfo); | ||
| put(serverInfo[0], serverInfo); | ||
| put(hubbleInfo[0], hubbleInfo); | ||
| put(vermeer[0], vermeer); |
There was a problem hiding this comment.
INCOMPLETE REFACTORING: The hardcoded credential data (storeInfo, serverInfo, hubbleInfo, vermeer) and the apps HashMap have been removed, but the getToken(String[] info) method and verify() method are still present. This creates unused code paths. If token generation is no longer needed, these methods should also be removed. If they are still used elsewhere, consider whether this class should be deprecated in favor of a new authentication mechanism.
Severity: MEDIUM
🤖 Prompt for AI Agent
Fix this code. In
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/util/TokenUtil.java#L33-L55:
INCOMPLETE REFACTORING: The hardcoded credential data (storeInfo, serverInfo,
hubbleInfo, vermeer) and the `apps` HashMap have been removed, but the
`getToken(String[] info)` method and `verify()` method are still present. This creates
unused code paths. If token generation is no longer needed, these methods should also be
removed. If they are still used elsewhere, consider whether this class should be
deprecated in favor of a new authentication mechanism.
Did we get this right? 👍 / 👎 to inform future reviews.
| package org.apache.hugegraph.constant; | ||
|
|
||
| public class ServiceConstant { | ||
| // FIXME: Strictly prohibited from external exposure; network IP whitelisting must be configured in production environments. | ||
| public static final String SERVICE_NAME = "hg"; | ||
| public static final String AUTHORITY = ""; | ||
| } |
There was a problem hiding this comment.
CRITICAL SECURITY ISSUE: The AUTHORITY field is set to an empty string. This indicates that authentication/authorization tokens are being emptied out. The FIXME comment states 'Strictly prohibited from external exposure; network IP whitelisting must be configured in production environments', which suggests the authentication mechanism has been completely removed in favor of network-level security. This is extremely dangerous if exposed and must not be committed to a public repository or used in any environment accessible over the internet. Either proper credentials must be used, or this must be clearly marked as development-only code with strong CI/CD guards preventing production deployment.
Severity: CRITICAL
🤖 Prompt for AI Agent
Fix this code. In
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/constant/ServiceConstant.java#L1-L7:
CRITICAL SECURITY ISSUE: The `AUTHORITY` field is set to an empty string. This indicates
that authentication/authorization tokens are being emptied out. The FIXME comment states
'Strictly prohibited from external exposure; network IP whitelisting must be configured
in production environments', which suggests the authentication mechanism has been
completely removed in favor of network-level security. This is extremely dangerous if
exposed and must not be committed to a public repository or used in any environment
accessible over the internet. Either proper credentials must be used, or this must be
clearly marked as development-only code with strong CI/CD guards preventing production
deployment.
Did we get this right? 👍 / 👎 to inform future reviews.
| protected static String user = "store"; | ||
| protected static String pwd = "$2a$04$9ZGBULe2vc73DMj7r/iBKeQB1SagtUXPrDbMmNswRkTwlWQURE/Jy"; | ||
| protected static String pwd = ""; |
There was a problem hiding this comment.
SECURITY ISSUE: The password has been changed to an empty string. This appears to be a temporary placeholder for testing purposes. Ensure this is not accidentally committed or merged to main branches. If authentication is disabled for testing, this should be clearly documented with a comment explaining the test-only nature and the security implications.
Severity: HIGH
🤖 Prompt for AI Agent
Fix this code. In
hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/BaseTest.java#L26-L27:
SECURITY ISSUE: The password has been changed to an empty string. This appears to be a
temporary placeholder for testing purposes. Ensure this is not accidentally committed or
merged to main branches. If authentication is disabled for testing, this should be
clearly documented with a comment explaining the test-only nature and the security
implications.
Did we get this right? 👍 / 👎 to inform future reviews.
| private static final String SERVICE_NAME = "store"; | ||
| private static final String AUTHORITY = ""; |
There was a problem hiding this comment.
SECURITY ISSUE: The AUTHORITY constant is set to an empty string, which disables authentication for this test. While appropriate for test code, ensure this configuration is not reused for production scenarios. The constants should have visibility or documentation indicating their test-only purpose.
Severity: HIGH
🤖 Prompt for AI Agent
Fix this code. In
hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/client/PDPulseTest.java#L43-L44:
SECURITY ISSUE: The `AUTHORITY` constant is set to an empty string, which disables
authentication for this test. While appropriate for test code, ensure this configuration
is not reused for production scenarios. The constants should have visibility or
documentation indicating their test-only purpose.
Did we get this right? 👍 / 👎 to inform future reviews.
| disallowEmpty(), | ||
| "$2a$04$i10KooNg6wLvIPVDh909n.RBYlZ/4pJo978nFK86nrqQiGIKV4UGS" | ||
| "" |
There was a problem hiding this comment.
CRITICAL SECURITY ISSUE: The default SERVICE_ACCESS_PD_TOKEN has been changed from a hash value to an empty string. This effectively disables authentication for server-to-PD communication. This should be a strong, securely generated token or a reference to a secure credential store (e.g., environment variable, secrets vault). Empty credentials leave the system vulnerable to unauthorized access. If this is intentional for development, add a prominent comment and ensure the configuration is overridden in production.
Severity: CRITICAL
🤖 Prompt for AI Agent
Fix this code. In
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/config/ServerOptions.java#L263-L264:
CRITICAL SECURITY ISSUE: The default `SERVICE_ACCESS_PD_TOKEN` has been changed from a
hash value to an empty string. This effectively disables authentication for server-to-PD
communication. This should be a strong, securely generated token or a reference to a
secure credential store (e.g., environment variable, secrets vault). Empty credentials
leave the system vulnerable to unauthorized access. If this is intentional for
development, add a prominent comment and ensure the configuration is overridden in
production.
Did we get this right? 👍 / 👎 to inform future reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the authentication logic to remove hardcoded credentials, which is a significant improvement for security and maintainability. The changes simplify the authentication process by relying on a predefined set of internal users, with the assumption that the service is protected by network-level security like IP whitelisting. The removal of hardcoded tokens and passwords from the codebase is a positive step.
My review focuses on improving the implementation details of the new authentication logic. I've suggested using a more standard way to initialize the set of inner users, improving code clarity by removing unused variables and simplifying control flow, and refining the exception handling to be more specific. These changes will make the new code more robust and easier to maintain.
| private static final Set<String> innerUsers = new HashSet<>() { | ||
| { | ||
| add("hg"); | ||
| add("store"); | ||
| add("hubble"); | ||
| add("vermeer"); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The use of double-brace initialization is generally considered an anti-pattern because it creates an anonymous inner class, which can have performance implications and may cause issues like memory leaks. A more modern and efficient approach is to use a static factory method. For Java 9+, Set.of() is a great choice. Given that this project uses Guava, ImmutableSet.of() from Guava would also be an excellent alternative.
Additionally, static final fields, which are constants, should be named in UPPER_SNAKE_CASE according to Java conventions. innerUsers should be renamed to INNER_USERS.1
private static final Set<String> INNER_USERS = Set.of("hg", "store", "hubble", "vermeer");Style Guide References
Footnotes
-
Following the Google Java Style Guide, constant names (static final fields) should use
UPPER_SNAKE_CASE: all uppercase letters, with words separated by underscores. ↩
| String pwd = info.substring(delim + 1); | ||
| if (!"store".equals(name)) { | ||
| if (util == null) { | ||
| synchronized (this) { | ||
| if (util == null) { | ||
| util = new TokenUtil(pdConfig.getSecretKey()); | ||
| } | ||
| } | ||
| } | ||
| String[] i = util.getInfo(name); | ||
| if (i == null) { | ||
| throw new AccessDeniedException("invalid service name"); | ||
| } | ||
| if (!StringUtils.isEmpty(token)) { | ||
| String value = TOKEN_CACHE.get(name); | ||
| if (StringUtils.isEmpty(value)) { | ||
| synchronized (i) { | ||
| value = kvService.get(getTokenKey(name)); | ||
| } | ||
| } | ||
| if (!StringUtils.isEmpty(value) && token.equals(value)) { | ||
| return call.get(); | ||
| } | ||
| } | ||
| if (StringUtils.isEmpty(pwd) || !StringEncoding.checkPassword(i[2], pwd)) { | ||
| throw new AccessDeniedException(invalidMsg); | ||
| } | ||
| token = util.getToken(name); | ||
| String tokenKey = getTokenKey(name); | ||
| String dbToken = kvService.get(tokenKey); | ||
| if (StringUtils.isEmpty(dbToken)) { | ||
| synchronized (i) { | ||
| dbToken = kvService.get(tokenKey); | ||
| if (StringUtils.isEmpty(dbToken)) { | ||
| kvService.put(tokenKey, token, | ||
| TokenUtil.AUTH_TOKEN_EXPIRE); | ||
| TOKEN_CACHE.put(name, token, | ||
| TokenUtil.AUTH_TOKEN_EXPIRE); | ||
| return tokenCall.apply(token); | ||
| } | ||
| } | ||
| } | ||
| if (innerUsers.contains(name)) { | ||
| return call.get(); | ||
| } else { | ||
| throw new AccessDeniedException("invalid service name"); | ||
| } |
There was a problem hiding this comment.
The pwd variable is extracted but never used, which can be confusing for future readers. Since the password is no longer checked, it's best to remove its extraction. The if-else block can also be simplified by inverting the condition.
As suggested in my other comment, innerUsers should be renamed to INNER_USERS.
if (!INNER_USERS.contains(name)) {
throw new AccessDeniedException("invalid service name");
}
return call.get();| } catch (Exception e) { | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
Catching a generic Exception and wrapping it in a RuntimeException can obscure the original cause of the error, especially for existing RuntimeExceptions. It's better to either catch more specific checked exceptions if any are expected, or to let RuntimeExceptions propagate up. In this try block, most potential exceptions like IllegalArgumentException from Base64.decode or IndexOutOfBoundsException from substring are already RuntimeExceptions. Consider refining the exception handling to be more specific, for example by catching IllegalArgumentException and re-throwing it as a BadCredentialsException.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/interceptor/Authentication.java (3)
72-75: 异常处理丢失上下文信息将所有异常（包括
BadCredentialsException和AccessDeniedException）包装在RuntimeException中会丢失重要的安全上下文。这使得调用方无法区分不同的认证失败原因，也使调试变得困难。建议直接抛出原始异常，或至少保留异常类型和消息。
应用此修改以改进异常处理：
- } catch (Exception e) { - throw new RuntimeException(e); - } + } catch (BadCredentialsException | AccessDeniedException e) { + throw e; + } catch (Exception e) { + throw new BadCredentialsException("Authentication failed: " + e.getMessage(), e); + }
77-79: 移除未使用的getTokenKey方法验证确认该方法在整个代码库中完全未被使用。由于已移除 token 验证逻辑，此方法已成为死代码，应予以移除。
位置：
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/interceptor/Authentication.java第 77-79 行
52-53: 移除未使用的参数tokenCall和token，并补充缺失的密码验证
authenticate方法声明的tokenCall和token参数均未在方法体中使用，表明存在不完整的重构。此外，解码的密码变量pwd也未被验证，仅检查服务名称是否在允许列表中，这是设计缺陷。建议：
- 移除未使用的
tokenCall参数（如不需要），或补充实现其逻辑- 移除或实现
token参数的验证逻辑- 添加密码验证逻辑，或若密码不需要验证，在代码中明确说明理由
- 保留原始异常类型，避免包装在
RuntimeException中丢失上下文信息hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/config/ServerOptions.java (1)
259-265: SERVICE_ACCESS_PD_TOKEN 的默认值与验证器冲突 - 已确认验证确认存在严重问题：
disallowEmpty()验证器使用StringUtils.isNotBlank((String) o)，它会拒绝空字符串。而 SERVICE_ACCESS_PD_TOKEN 的默认值为""（第 264 行），这会导致配置验证失败。需要修复其中一项：
- 移除
disallowEmpty()验证器（第 263 行），或- 将默认值改为非空字符串
此外需要评估对现有部署的影响：如果依赖此配置的服务在启动时进行验证，空默认值会导致启动失败。
🧹 Nitpick comments (1)
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/constant/ServiceConstant.java (1)
3-7: 考虑改进类设计以防止实例化作为常量类，考虑添加私有构造函数防止实例化，并根据使用范围评估是否需要 public 可见性。
可选的改进：
public class ServiceConstant { // FIXME: Strictly prohibited from external exposure; network IP whitelisting must be configured in production environments. + + private ServiceConstant() { + throw new AssertionError("Utility class should not be instantiated"); + } + public static final String SERVICE_NAME = "hg"; public static final String AUTHORITY = ""; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/interceptor/Authentication.java(3 hunks)hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/util/TokenUtil.java(0 hunks)hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/BaseTest.java(1 hunks)hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/client/PDPulseTest.java(1 hunks)hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/config/ServerOptions.java(1 hunks)hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/constant/ServiceConstant.java(1 hunks)hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/space/register/registerImpl/PdRegister.java(2 hunks)hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/space/register/registerImpl/SampleRegister.java(2 hunks)
💤 Files with no reviewable changes (1)
- hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/util/TokenUtil.java
🧰 Additional context used
🧬 Code graph analysis (3)
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/space/register/registerImpl/PdRegister.java (1)
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/constant/ServiceConstant.java (1)
ServiceConstant(3-7)
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/space/register/registerImpl/SampleRegister.java (1)
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/constant/ServiceConstant.java (1)
ServiceConstant(3-7)
hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/client/PDPulseTest.java (1)
hugegraph-pd/hg-pd-client/src/main/java/org/apache/hugegraph/pd/client/PDConfig.java (1)
PDConfig(28-136)
🪛 GitHub Actions: License Checker
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/constant/ServiceConstant.java
[error] License header check failed: missing or invalid license header.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build-server (memory, 11)
- GitHub Check: build-server (rocksdb, 11)
- GitHub Check: build-server (hbase, 11)
- GitHub Check: dependency-check
- GitHub Check: struct
- GitHub Check: build-commons (11)
- GitHub Check: Analyze (java)
🔇 Additional comments (8)
hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/BaseTest.java (1)
27-27: 验证表明原审查意见有误——空密码在测试中不会导致认证逻辑失效。HTTP Basic认证规范允许用户名和密码用冒号组合编码，空密码的格式为
username:是有效的。在BaseTest中的改动已符合此标准。更重要的是，代码库中已有空密码的使用案例：
hugegraph-pd/hg-pd-cli/src/main/java/org/apache/hugegraph/pd/cli/cmd/Command.java第35行同样使用setAuthority("store", "")，说明此模式已是既定的测试实践。从脚本输出可见，BaseTest继承的五个测试类（BaseServerTest、BaseCliToolsTest、BaseCoreTest、BaseClientTest、DiscoveryClientTest）及多个具体测试都调用
getPdConfig()方法，该方法中setAuthority(user, pwd)接受空字符串参数完全有效。不存在测试覆盖中断的风险。第29行的硬编码凭据
value = "Basic c3RvcmU6YWRtaW4="是独立的静态字段，不被getPdConfig()方法使用，不影响此次改动的有效性。Likely an incorrect or invalid review comment.
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/interceptor/Authentication.java (4)
22-23: 导入语句正确新增的
HashSet和Set导入用于支持innerUsers集合，使用恰当。
42-49: 服务名称应移至配置中以提高灵活性
innerUsers集合中的服务名称虽然是硬编码的标识符（而非凭据），但仍存在可维护性问题。当需要添加或移除服务时，必须修改代码并重新部署应用程序。建议将此允许列表移至外部配置（如 PDConfig），以避免频繁的代码变更。
28-32: 修正导入清理建议：仅 4 个导入可以安全移除，TokenUtil 仍在使用验证结果显示，Authentication.java 文件中 TokenUtil 在第 45 行被使用（
private static volatile TokenUtil util;），因此不应移除。可以安全移除的导入：
KvService(line 28)Cache(line 29)PDConfig(line 30)StringEncoding(line 32)TokenUtil 导入必须保留。
Likely an incorrect or invalid review comment.
40-40: 删除第 40 行未使用的util字段第 40 行声明的
private static volatile TokenUtil util;字段在整个 Authentication 类中从未被使用，属于死代码，应删除。同时建议检查第 28-32 行的导入中，
KvService、Cache和PDConfig是否也未被使用，如果确实未使用，也应一并清理。hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/space/register/registerImpl/PdRegister.java (1)
56-56: 重构方式正确，成功消除硬编码凭据使用
ServiceConstant集中管理服务名称和认证信息是一个好的实践，消除了硬编码值，提高了可维护性。Also applies to: 91-93
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/space/register/registerImpl/SampleRegister.java (1)
25-25: 重构模式一致，良好实践与
PdRegister保持一致的重构方式，使用ServiceConstant替换硬编码凭据。Also applies to: 87-89
hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/client/PDPulseTest.java (1)
43-55: 测试代码更新合理测试使用本地常量定义
SERVICE_NAME和AUTHORITY，这允许测试场景使用不同的值。启用PDClient.create()也使测试更加完整。注意：测试使用的
SERVICE_NAME = "store"与生产代码中的"hg"不同，这是合理的测试隔离。
| String name = info.substring(0, delim); | ||
| String pwd = info.substring(delim + 1); | ||
| if (!"store".equals(name)) { | ||
| if (util == null) { | ||
| synchronized (this) { | ||
| if (util == null) { | ||
| util = new TokenUtil(pdConfig.getSecretKey()); | ||
| } | ||
| } | ||
| } | ||
| String[] i = util.getInfo(name); | ||
| if (i == null) { | ||
| throw new AccessDeniedException("invalid service name"); | ||
| } | ||
| if (!StringUtils.isEmpty(token)) { | ||
| String value = TOKEN_CACHE.get(name); | ||
| if (StringUtils.isEmpty(value)) { | ||
| synchronized (i) { | ||
| value = kvService.get(getTokenKey(name)); | ||
| } | ||
| } | ||
| if (!StringUtils.isEmpty(value) && token.equals(value)) { | ||
| return call.get(); | ||
| } | ||
| } | ||
| if (StringUtils.isEmpty(pwd) || !StringEncoding.checkPassword(i[2], pwd)) { | ||
| throw new AccessDeniedException(invalidMsg); | ||
| } | ||
| token = util.getToken(name); | ||
| String tokenKey = getTokenKey(name); | ||
| String dbToken = kvService.get(tokenKey); | ||
| if (StringUtils.isEmpty(dbToken)) { | ||
| synchronized (i) { | ||
| dbToken = kvService.get(tokenKey); | ||
| if (StringUtils.isEmpty(dbToken)) { | ||
| kvService.put(tokenKey, token, | ||
| TokenUtil.AUTH_TOKEN_EXPIRE); | ||
| TOKEN_CACHE.put(name, token, | ||
| TokenUtil.AUTH_TOKEN_EXPIRE); | ||
| return tokenCall.apply(token); | ||
| } | ||
| } | ||
| } | ||
| if (innerUsers.contains(name)) { | ||
| return call.get(); | ||
| } else { | ||
| throw new AccessDeniedException("invalid service name"); | ||
| } |
There was a problem hiding this comment.
严重安全漏洞：密码未验证
代码在第 66 行提取了密码 (pwd)，但从未进行验证。认证逻辑仅检查服务名称是否在 innerUsers 列表中，完全忽略密码。这意味着任何知道有效服务名称的客户端都可以使用任意密码进行认证。
这是一个严重的安全漏洞，与典型的认证模型不符。如果密码验证已移至其他地方，请添加注释说明；否则必须恢复密码验证逻辑。
🤖 Prompt for AI Agents
In
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/interceptor/Authentication.java
around lines 65 to 71, the code extracts the password into variable `pwd` but
never validates it — it only checks that the service name is in `innerUsers`,
allowing authentication with any password. Fix by validating `pwd` against the
expected password for `name` (e.g., look up the stored credential for `name` and
compare), and if the password does not match throw AccessDeniedException; if
authentication is delegated elsewhere, add a clear comment explaining why
password is not verified here. Ensure the comparison is performed securely (use
constant-time comparison or appropriate credential-checking API) and return
call.get() only on successful name+password verification.
| protected static String user = "store"; | ||
| protected static String pwd = "$2a$04$9ZGBULe2vc73DMj7r/iBKeQB1SagtUXPrDbMmNswRkTwlWQURE/Jy"; | ||
| protected static String pwd = ""; | ||
| protected static String key = "Authorization"; | ||
| protected static String value = "Basic c3RvcmU6YWRtaW4="; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
认证凭据配置不一致。
代码中存在凭据处理的不一致性：
- 第 26-27 行：
user = "store"和pwd = ""（空密码） - 第 29 行：
value = "Basic c3RvcmU6YWRtaW4="（编码的store:admin）
这种不一致可能导致混淆，并暗示重构不完整。如果 key 和 value 字段用于不同的认证机制，请添加注释说明其用途；如果不再需要，请将其删除。
🤖 Prompt for AI Agents
In hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/BaseTest.java
around lines 26 to 29, the hardcoded credential fields are inconsistent
(user="store", pwd="" but value="Basic c3RvcmU6YWRtaW4=" which decodes to
"store:admin"); fix by either (A) aligning them — set pwd="admin" so user/pwd
match the Base64 value, or (B) remove the redundant key/value pair if only
user/pwd are used, or (C) retain both but add a clear comment explaining that
key/value are a pre-encoded header for a different auth mechanism and must stay
in sync with user/pwd; implement one consistent choice across the file.
| protected static String pwd = "$2a$04$9ZGBULe2vc73DMj7r/iBKeQB1SagtUXPrDbMmNswRkTwlWQURE/Jy"; | ||
| protected static String pwd = ""; | ||
| protected static String key = "Authorization"; | ||
| protected static String value = "Basic c3RvcmU6YWRtaW4="; |
There was a problem hiding this comment.
移除硬编码的认证凭据。
此行包含硬编码的 Base64 编码凭据（Basic c3RvcmU6YWRtaW4= 解码为 store:admin），这与 PR 的目标"移除硬编码凭据"相矛盾。即使在测试代码中，也应避免硬编码凭据以符合安全最佳实践。
考虑以下解决方案：
- 如果该字段未使用，请将其删除
- 如果需要，从配置文件或环境变量中读取
- 使用测试专用的配置管理
运行以下脚本验证该字段的使用情况：
#!/bin/bash
# 描述：查找 value 字段和 "Basic c3RvcmU6YWRtaW4=" 的使用情况
# 查找 value 字段的引用
echo "=== 查找 BaseTest.value 的使用 ==="
rg -n "\.value\b" --type java -C 2
# 查找硬编码认证值的使用
echo "=== 查找 Basic c3RvcmU6YWRtaW4= 的使用 ==="
rg -n "c3RvcmU6YWRtaW4=" --type java -C 2
# 查找所有 "Basic" 认证头的使用
echo "=== 查找所有 Basic 认证的使用 ==="
rg -n "Basic\s+" --type java -C 2🤖 Prompt for AI Agents
In hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/BaseTest.java
around line 29, the field protected static String value = "Basic
c3RvcmU6YWRtaW4=" hardcodes credentials; remove or replace it: first search the
repo for usages of BaseTest.value and the literal "c3RvcmU6YWRtaW4=" to confirm
whether it is used; if unused, delete the field; if used, change the code to
load the credential from a test configuration source (e.g., environment variable
or test-specific config property) via System.getenv or test config utility and
ensure a safe default or fail fast when not provided; update any tests or
fixtures to supply the credential from the new config and remove the hardcoded
literal from the codebase.
...graph-server/hugegraph-core/src/main/java/org/apache/hugegraph/constant/ServiceConstant.java
Show resolved
Hide resolved
| // FIXME: Strictly prohibited from external exposure; network IP whitelisting must be configured in production environments. | ||
| public static final String SERVICE_NAME = "hg"; | ||
| public static final String AUTHORITY = ""; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
验证空认证凭据的安全影响
AUTHORITY 设置为空字符串实际上禁用了认证。虽然 FIXME 注释警告"严禁对外暴露"并要求在生产环境配置网络 IP 白名单，但这仍然带来重大安全风险：
- 认证绕过：空凭据意味着没有认证保护
- 依赖网络隔离：完全依赖网络层保护是危险的，应采用深度防御策略
- 配置错误风险：如果网络配置错误，服务将完全暴露
请确认：
- 是否有其他认证层（如 mTLS、API Gateway 等）提供保护？
- 空凭据是否是临时方案？FIXME 注释暗示这需要修复
- 是否有自动化检查确保 PD 端口不会对外暴露？
- 是否应该至少生成一个随机 token 作为默认值？
运行以下脚本验证认证流程和 AUTHORITY 的使用：
🏁 Script executed:
#!/bin/bash
# 检查认证实现和 AUTHORITY 的使用
# 查找 AUTHORITY 在认证流程中的使用
echo "=== AUTHORITY usage in authentication ==="
rg -n -C5 "AUTHORITY" --type=java -g "**/auth/**" -g "**/pd/**"
# 查找 PDConfig.setAuthority 的所有调用
echo "=== All setAuthority calls ==="
rg -n -C3 "setAuthority" --type=java
# 查找认证拦截器或验证逻辑
echo "=== Authentication interceptors ==="
rg -n -C5 "Interceptor|authenticate|checkAuth" --type=java -g "**/pd/**"Length of output: 35935
AUTHORITY 空值会导致 PD 认证失败，需要生产环境配置
虽然注释警告要求网络隔离，但实际上这个空 AUTHORITY 会导致更严重的问题。服务端认证拦截器（Authentication.java 第 55 行）明确拒绝空凭据，抛出 BadCredentialsException。
实际情况：
AUTHORITY=""被 Base64 编码为"aGc6"（即 "hg:" 的编码）并发送给 PD 服务- PD 服务显式检查并拒绝空凭据，导致认证失败
- 这表明
ServiceConstant.AUTHORITY仅适用于开发/测试环境
问题：
SampleRegister.java中的示例代码使用了此常量，容易被误用于生产- 需要明确这是临时配置，而不是生产就绪状态
- 生产环境应使用
PdMetaDriver.PDAuthConfig或配置注入的凭据（如PdRegister中的做法）
建议：
- 删除或更新
SampleRegister.java中的这个使用，使用正确的凭据配置 - 或添加启动时的验证，确保生产环境中 AUTHORITY 不为空
🧰 Tools
🪛 GitHub Actions: License Checker
[error] License header check failed: missing or invalid license header.
🤖 Prompt for AI Agents
In
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/constant/ServiceConstant.java
around lines 4-6, the empty AUTHORITY constant causes PD authentication failures
in non-dev environments; update the code and usage so this value is never
treated as a production credential: replace the empty AUTHORITY with an explicit
dev-only marker (or null) and document it as development-only, remove/update
SampleRegister.java examples to stop using this constant and instead demonstrate
PdMetaDriver.PDAuthConfig or injected credentials, and add a startup validation
check (or fail-fast) that prevents running in production with an
empty/placeholder AUTHORITY so misconfiguration is detected early.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 1.7-rebase #73 +/- ##
=============================================
Coverage ? 29.49%
Complexity ? 264
=============================================
Files ? 801
Lines ? 67314
Branches ? 8728
=============================================
Hits ? 19857
Misses ? 45188
Partials ? 2269 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| private static String invalidMsg = | ||
| "invalid token and invalid user name or password, access denied"; | ||
|
|
||
| private static final Set<String> innerUsers = new HashSet<>() { |
There was a problem hiding this comment.
使用 Sets.of 或者 ImmutableSet 这种 (不可变对象优先)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/interceptor/Authentication.java (3)
40-40: 移除未使用的字段
util字段在移除令牌验证逻辑后不再使用，应该删除。应用此差异移除未使用的字段：
- private static volatile TokenUtil util; - private static final Set<String> innerUsers = Set.of("hg", "store", "hubble", "vermeer");
42-42: 常量命名规范和配置外部化该字段存在两个问题：
- 命名规范：静态常量应使用
UPPER_SNAKE_CASE命名，应改为INNER_USERS- 硬编码白名单：服务名称列表应外部化到配置文件（如
PDConfig），以便在不同部署环境中灵活调整，避免每次修改都需要重新编译建议将服务名称移至配置：
- private static final Set<String> innerUsers = Set.of("hg", "store", "hubble", "vermeer"); + private static final Set<String> INNER_USERS = pdConfig.getInnerServiceNames();并在
PDConfig中添加相应的配置项。
58-67: 严重安全漏洞：密码验证缺失该认证方法存在多个严重问题：
关键安全漏洞：第 59 行提取了密码
pwd，但在第 60-64 行的认证逻辑中完全未使用。任何知道有效服务名称（hg、store、hubble、vermeer）的客户端都可以使用任意密码通过认证。这是一个严重的安全退化，移除了原有的密码验证机制。代码质量问题：
- 提取的
pwd变量完全未使用，应该移除或用于密码验证- 第 65-67 行捕获通用
Exception并包装为RuntimeException会掩盖原始错误，应该捕获更具体的异常类型- 缺少审计日志记录认证尝试（成功和失败）
如果密码验证已移至网络层安全控制（如 PR 描述中提到的 IP 白名单），则必须：
- 在代码中添加清晰的注释说明为何不在此处验证密码
- 在文档中明确说明 PD 端口不得对外暴露的强制要求
- 移除未使用的
pwd变量以避免误导否则，必须恢复密码验证逻辑：
String name = info.substring(0, delim); String pwd = info.substring(delim + 1); - if (innerUsers.contains(name)) { + if (!INNER_USERS.contains(name)) { + throw new AccessDeniedException("invalid service name"); + } + // 验证密码（需要实现密码验证逻辑） + if (!validatePassword(name, pwd)) { + throw new AccessDeniedException("invalid credentials"); + } - return call.get(); - } else { - throw new AccessDeniedException("invalid service name"); - } + return call.get();
🧹 Nitpick comments (1)
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/interceptor/Authentication.java (1)
22-22: 移除未使用的导入
HashSet导入未被使用，因为第 42 行使用的是Set.of()工厂方法。应用此差异移除未使用的导入：
-import java.util.HashSet; import java.util.Set;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/interceptor/Authentication.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: pd
- GitHub Check: store
- GitHub Check: hstore
- GitHub Check: build-server (rocksdb, 11)
- GitHub Check: build-server (hbase, 11)
- GitHub Check: build-server (memory, 11)
- GitHub Check: cluster-test
- GitHub Check: build-commons (11)
- GitHub Check: Analyze (java)
- GitHub Check: dependency-check
Enhanced internal authentication logic and documentation in Authentication.java, emphasizing production security best practices. Refactored TokenUtil for clarity and immutability. Improved code formatting in PDPulseTest and SampleRegister, and updated ServiceConstant with stricter external exposure warnings.
Enhanced internal authentication logic and documentation in Authentication.java, emphasizing production security best practices. Refactored TokenUtil for clarity and immutability. Improved code formatting in PDPulseTest and SampleRegister, and updated ServiceConstant with stricter external exposure warnings. --------- Co-authored-by: imbajin <[email protected]>
Purpose of the PR
Main Changes
Currently, the auth management system in PD is incomplete. PD's REST and gRPC ports must never be exposed externally; network IP whitelisting must be configured in production environments.
Verifying these changes
Does this PR potentially affect the following parts?
Documentation Status
Doc - TODODoc - DoneDoc - No NeedSummary by CodeRabbit
发布说明
重构
移除/API 变更
修复