Conversation
…r token-related requests
… update Redis connection pool settings
|
Caution Review failedThe pull request is closed. WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Config as Pbkdf2Properties
participant SecConf as SecurityConfig
participant Encoder as Pbkdf2PasswordEncoder
App->>Config: Load PBKDF2 properties from security-config.yml
App->>SecConf: Initialize SecurityConfig with Pbkdf2Properties
SecConf->>Encoder: Create Pbkdf2PasswordEncoder(secret, saltLength, iterations, algorithm)
SecConf->>App: Register DelegatingPasswordEncoder with "pbkdf2" id
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @polyglot-k, 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!
I've focused on enhancing the authentication and login process. This includes making our password encoding more robust and configurable, optimizing Redis connection pooling for better performance, and refining logging for clearer operational insights.
Highlights
- Configurable PBKDF2 Password Encoding: I've updated the Pbkdf2PasswordEncoder to use explicitly defined secret, salt length, and iteration parameters, moving away from Spring Security's defaults. This allows for more granular control over password hashing strength.
- Externalized PBKDF2 Properties: I introduced a new configuration class (Pbkdf2Properties) and corresponding entries in security-config.yml to manage the PBKDF2 parameters externally, making them easier to configure and manage.
- Optimized Redis Connection Pool: I've added Lettuce connection pool configurations to application.yml for Redis, which should improve performance and resource management when interacting with Redis, potentially benefiting session or token storage.
- Refactored User Role Check: I simplified the isProfileRegistered method in AuthService to a more concise boolean comparison, improving code readability.
- Adjusted Logging Verbosity: I changed several log.info statements to log.debug within the AccessTokenFilter, reducing log noise for routine authentication flows and making important logs stand out more.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request improves the login process by addressing potential bottlenecks. It externalizes the PBKDF2 password hashing configuration, allowing for performance tuning, and enables Redis connection pooling to handle higher loads. The changes are well-implemented. I've added a couple of suggestions to improve code style and encapsulation.
| String secret; | ||
| Integer saltLength; | ||
| Integer iterations; |
There was a problem hiding this comment.
For better encapsulation and to follow common Java best practices, class fields should be declared as private. Lombok's @Data annotation will correctly generate public getters and setters for private fields.
| String secret; | |
| Integer saltLength; | |
| Integer iterations; | |
| private String secret; | |
| private Integer saltLength; | |
| private Integer iterations; |
| Map<String, PasswordEncoder> encoders = new HashMap<>(); | ||
| encoders.put(idForEncode, Pbkdf2PasswordEncoder.defaultsForSpringSecurity_v5_5()); | ||
| encoders.put(idForEncode, new Pbkdf2PasswordEncoder( | ||
| properties.getSecret(), | ||
| properties.getSaltLength(), | ||
| properties.getIterations(), | ||
| SecretKeyFactoryAlgorithm.PBKDF2WithHmacSHA256 | ||
| )); |
There was a problem hiding this comment.
To create a map with a single, known entry, you can use the static factory method Map.of(). This is more concise than creating a HashMap and adding an entry, and it returns an immutable map, which is generally safer. The DelegatingPasswordEncoder creates a defensive copy of the map, so passing an immutable one is perfectly fine.
Map<String, PasswordEncoder> encoders = Map.of(idForEncode, new Pbkdf2PasswordEncoder(
properties.getSecret(),
properties.getSaltLength(),
properties.getIterations(),
SecretKeyFactoryAlgorithm.PBKDF2WithHmacSHA256
));![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/main/java/life/mosu/mosuserver/application/auth/AuthService.java (1)
38-40: Return primitivebooleaninstead of boxedBoolean
isProfileRegisterednever returnsnull, so boxing adds unnecessary heap churn and potential NPEs downstream.-private Boolean isProfileRegistered(UserJpaEntity user) { - return UserRole.ROLE_USER.equals(user.getUserRole()); -} +private boolean isProfileRegistered(UserJpaEntity user) { + return UserRole.ROLE_USER.equals(user.getUserRole()); +}src/main/resources/security-config.yml (1)
44-47: Ensure property keys match the Java field namesWith relaxed binding, both
saltLengthandsalt-lengthwork, but sticking to kebab-case (salt-length,iterations) is the Spring convention and matches other keys in this file. Consider:pbkdf2: secret: ${PBKDF2_SECRET} salt-length: ${PBKDF2_SALT_LENGTH} iterations: ${PBKDF2_ITERATIONS}Purely stylistic, but aids consistency across configs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/life/mosu/mosuserver/application/auth/AuthService.java(2 hunks)src/main/java/life/mosu/mosuserver/global/config/Pbkdf2Properties.java(1 hunks)src/main/java/life/mosu/mosuserver/global/config/SecurityConfig.java(2 hunks)src/main/java/life/mosu/mosuserver/global/filter/AccessTokenFilter.java(3 hunks)src/main/resources/application.yml(1 hunks)src/main/resources/security-config.yml(1 hunks)
🔇 Additional comments (3)
src/main/resources/application.yml (1)
49-55: Verify Spring-Boot property names for Lettuce poolBoot 3.x expects the prefix
spring.data.redis.lettuce.pool.*.
Your block is correctly nested, but earlier versions (2.x) usedspring.redis.*.
Double-check the Boot version; otherwise the pool may silently remain disabled.src/main/java/life/mosu/mosuserver/global/config/SecurityConfig.java (2)
34-34: LGTM: Import addition for explicit algorithm specification.The import of
SecretKeyFactoryAlgorithmsupports the explicit algorithm specification in the PBKDF2 encoder configuration, which is a good security practice.
87-87: Pbkdf2Properties bean verified and availableThe
Pbkdf2Propertiesclass in
src/main/java/life/mosu/mosuserver/global/config/Pbkdf2Properties.java
is annotated with@ConfigurationProperties(prefix = "pbkdf2")and will be picked up by Spring’s configuration properties binding. No further action is needed.
| @Component | ||
| @ConfigurationProperties(prefix = "pbkdf2") | ||
| @Data | ||
| public class Pbkdf2Properties { | ||
|
|
||
| String secret; | ||
| Integer saltLength; | ||
| Integer iterations; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden the configuration class – make fields private, immutable, and validated
All three fields are package-private mutable objects and accept null.
Exposing mutable, un-validated state undermines configuration safety and thread-safety.
-@Component
-@ConfigurationProperties(prefix = "pbkdf2")
-@Data
-public class Pbkdf2Properties {
-
- String secret;
- Integer saltLength;
- Integer iterations;
-}
+@Component
+@ConfigurationProperties(prefix = "pbkdf2")
+@Validated
+@Getter
+@RequiredArgsConstructor
+public class Pbkdf2Properties {
+
+ @NotBlank private final String secret;
+ @NotNull private final Integer saltLength;
+ @NotNull private final Integer iterations;
+}• @Getter + final removes mutators and keeps the object immutable
• @Validated + JSR-380 annotations guarantees presence of essential values at startup
• Constructor binding eliminates reflective field-injection pitfalls in native-image & modular builds
These changes are minimal yet immediately tighten security and reliability.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/config/Pbkdf2Properties.java lines
7 to 15, the fields are package-private, mutable, and unvalidated, which risks
configuration safety and thread-safety. Change the fields to private and final,
remove setters, and add @Getter for access. Annotate the class with @Validated
and use JSR-380 annotations like @NotNull on fields to enforce validation.
Implement constructor binding by adding a constructor with parameters for all
fields and annotate the class with @ConstructorBinding to ensure immutability
and safer configuration loading.
| encoders.put(idForEncode, new Pbkdf2PasswordEncoder( | ||
| properties.getSecret(), | ||
| properties.getSaltLength(), | ||
| properties.getIterations(), | ||
| SecretKeyFactoryAlgorithm.PBKDF2WithHmacSHA256 | ||
| )); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify PBKDF2 configuration parameters for security compliance.
The explicit configuration of PBKDF2 parameters is a security improvement, but the actual values should be validated to ensure they meet current security standards.
Please verify that the PBKDF2 configuration values in Pbkdf2Properties meet current security recommendations:
Consider adding validation to ensure minimum security standards:
encoders.put(idForEncode, new Pbkdf2PasswordEncoder(
properties.getSecret(),
properties.getSaltLength(),
properties.getIterations(),
SecretKeyFactoryAlgorithm.PBKDF2WithHmacSHA256
));Add parameter validation in Pbkdf2Properties:
- Salt length should be at least 16 bytes (128 bits)
- Iteration count should be at least 100,000 for PBKDF2-SHA256
- Secret should be non-empty and properly configured
🌐 Web query:
What are the current OWASP recommendations for PBKDF2 parameters including salt length and iteration count for 2024-2025?
💡 Result:
The current OWASP recommendations for PBKDF2 parameters (2024–2025) are:
- PBKDF2-HMAC-SHA256: 600,000 iterations
- PBKDF2-HMAC-SHA512: 210,000 iterations
- Salt length: At least 128 bits (16 bytes) is recommended, though some standards (e.g., NIST) require a minimum of 32 bits; OWASP and practical implementations generally prefer 128 bits or more[1][3][4][6].
Context and supporting details:
- The iteration counts above are from the OWASP Password Storage Cheat Sheet Series, last updated in late 2024[4]. Wikipedia and other cybersecurity resources confirm these numbers as the latest official OWASP guidance for PBKDF2[1][6][7].
- Salt length:
- The OWASP Cheat Sheet does not specify an exact salt length, but implementations and other guidance (e.g., .NET examples) use 16 bytes (128 bits)[4][6].
- NIST SP 800-63B (a source cited in OWASP guidance) formally requires at least 32 bits, but recommends 128 bits or more for future proofing and parallel cracking defense[3][6].
| Parameter | 2024–2025 Recommendation | Notes |
|---|---|---|
| PBKDF2-HMAC-SHA1 iterations | 1,300,000 | Nearly obsolete but still referenced for legacy compliance needs[4] |
| PBKDF2-HMAC-SHA256 iterations | 600,000 | Most widely recommended, preferred choice[4][6][7] |
| PBKDF2-HMAC-SHA512 iterations | 210,000 | Higher computational cost per cycle, fewer needed[4][6] |
| Salt length | At least 128 bits (16 bytes) | NIST minimum is 32 bits, but OWASP/practice prefers 128 bits or more[3][6] |
Additional details:
- Both the salt value and the iteration count should be stored securely alongside the derived key (the hashed password)[3][5][6].
- Regularly updating iteration counts is recommended as hardware improves; older values (e.g., 310,000 for SHA-256 in 2021) are no longer sufficient[2][4][6].
- While 600,000 iterations is recommended, implementers should balance security with the risk of denial-of-service on login endpoints if server resources are limited[7].
Summary:
For systems implementing PBKDF2, OWASP 2024–2025 recommendations are 600,000 iterations for SHA256, 210,000 for SHA512, and at least 16 bytes (128 bits) of random salt per password[1][4][6]. These values reflect defense targets against modern hardware attacks and should be revisited regularly as guidance evolves.
Citations:
- 1: https://en.wikipedia.org/wiki/PBKDF2
- 2: https://vaultwarden.discourse.group/t/pbkdf2-default-iterations-acording-to-owasp/2235
- 3: https://pages.nist.gov/800-63-4/sp800-63b.html
- 4: https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html
- 5: https://crypto.stackexchange.com/questions/3484/pbkdf2-and-salt
- 6: https://anthonysimmon.com/evolutive-and-robust-password-hashing-using-pbkdf2-in-dotnet/
- 7: https://forum.djangoproject.com/t/stop-increasing-default-pbkdf2-iteration-count/25539
- 8: https://psono.com/blog/evolution-of-password-hashing
- 9: https://community.bitwarden.com/t/low-kdf-iterations-warning-what-should-i-do/62457
- 10: https://zeropath.com/blog/owasp-2021-vs-2025
🏁 Script executed:
#!/bin/bash
# Display the content of Pbkdf2Properties to review existing validation and defaults
sed -n '1,200p' src/main/java/life/mosu/mosuserver/global/config/Pbkdf2Properties.javaLength of output: 441
Enforce PBKDF2 Parameter Validation in Pbkdf2Properties
To comply with OWASP 2024–2025 guidance (600 000 iterations for PBKDF2‐HMAC‐SHA256, ≥16 bytes salt), add validation and reasonable defaults:
• Annotate your properties class for Bean Validation
• Enforce non-empty secret, minimum salt length (16), and minimum iterations (600 000)
• Supply defaults in case values are omitted
Example diff for Pbkdf2Properties.java:
@Component
@ConfigurationProperties(prefix = "pbkdf2")
-@Data
+@Data
+@Validated
public class Pbkdf2Properties {
-
- String secret;
- Integer saltLength;
- Integer iterations;
+ @NotBlank(message = "PBKDF2 secret must not be empty")
+ private String secret;
+
+ /** Minimum 16 bytes (128 bits) salt recommended by OWASP */
+ @Min(value = 16, message = "Salt length must be at least 16 bytes")
+ private Integer saltLength = 16;
+
+ /** OWASP recommends 600 000 iterations for PBKDF2-SHA256 (2024–2025) */
+ @Min(value = 600000, message = "Iteration count must be at least 600,000")
+ private Integer iterations = 600000;
}And ensure you have the Bean Validation APIs on the classpath:
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-validation</artifactId>
</dependency>With this in place, Spring Boot will fail fast if any property is missing or below the OWASP‐recommended thresholds.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/config/SecurityConfig.java around
lines 91 to 96, the PBKDF2 parameters lack validation against OWASP 2024-2025
recommendations. To fix this, add Bean Validation annotations to the
Pbkdf2Properties class to enforce a non-empty secret, a minimum salt length of
16 bytes, and a minimum of 600,000 iterations. Provide default values for these
properties to ensure safe defaults if omitted. Also, ensure the
spring-boot-starter-validation dependency is included in the project to enable
validation and cause Spring Boot to fail fast on invalid configurations.
| try { | ||
| log.info("액세스 토큰 인증 요청: {}", accessToken); | ||
| log.debug("액세스 토큰 인증 요청: {}", accessToken); | ||
| setAuthentication(accessToken); |
There was a problem hiding this comment.
Stop logging raw access tokens – they are credentials
Even at DEBUG level, access tokens must never be written to logs; they provide full session takeover if leaked via log aggregation or support bundles.
-log.debug("액세스 토큰 인증 요청: {}", accessToken);
+log.debug("액세스 토큰 인증 요청(토큰 해시): {}", accessToken.hashCode());Mask or hash the token (or omit entirely) to avoid credential leakage.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| log.info("액세스 토큰 인증 요청: {}", accessToken); | |
| log.debug("액세스 토큰 인증 요청: {}", accessToken); | |
| setAuthentication(accessToken); | |
| try { | |
| log.debug("액세스 토큰 인증 요청(토큰 해시): {}", accessToken.hashCode()); | |
| setAuthentication(accessToken); |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/filter/AccessTokenFilter.java
around lines 94 to 96, the code logs raw access tokens which is a security risk.
Modify the log statement to either omit the access token entirely or log a
masked or hashed version of the token to prevent credential leakage. Ensure no
raw tokens are written to any logs, even at DEBUG level.
|
|
||
| private void validateSignUpToken(final String signUpToken) { | ||
| log.info("회원가입 토큰 파싱: {}", signUpToken); | ||
| log.debug("회원가입 토큰 파싱: {}", signUpToken); | ||
| signUpTokenService.validateSignUpToken(signUpToken); |
There was a problem hiding this comment.
Remove signup-token from logs as well
Same rationale – the signup token is a bearer credential.
-log.debug("회원가입 토큰 파싱: {}", signUpToken);
+log.debug("회원가입 토큰 파싱(해시): {}", signUpToken.hashCode());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private void validateSignUpToken(final String signUpToken) { | |
| log.info("회원가입 토큰 파싱: {}", signUpToken); | |
| log.debug("회원가입 토큰 파싱: {}", signUpToken); | |
| signUpTokenService.validateSignUpToken(signUpToken); | |
| private void validateSignUpToken(final String signUpToken) { | |
| log.debug("회원가입 토큰 파싱(해시): {}", signUpToken.hashCode()); | |
| signUpTokenService.validateSignUpToken(signUpToken); |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/filter/AccessTokenFilter.java
around lines 128 to 131, remove the signUpToken value from the debug log to
avoid logging sensitive bearer credentials. Update the log statement to exclude
the token or remove the log line entirely to ensure the signup token is not
exposed in logs.
…figuration integrity
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Improvements
Style
Refactor