Provide a way to rotate wdek and session master key #1209
Conversation
WalkthroughThis PR introduces Key Encryption Key (KEK) management with support for encrypted data key rotation. It adds KEK ID configuration, new command types for key rotation, a REST API for key management (sysadmin scope), and refactors the encryption storage layer with improved session key lifecycle handling and async session management in authentication flows. Changes
Sequence Diagram(s)sequenceDiagram
actor Admin as System Administrator
participant API as KeyManagementService (REST)
participant Executor as StandaloneCommandExecutor
participant Storage as EncryptionStorageManager
participant KeyStorage as SessionKeyStorage
participant Cache as SessionKeyCache
Admin->>API: POST /masterkeys/session/rotate
API->>API: Verify encryption enabled
API->>Storage: getCurrentSessionMasterKey()
Storage->>KeyStorage: getCurrentSessionMasterKey()
KeyStorage->>Cache: fetch current version
KeyStorage-->>Storage: SessionMasterKey(version=N)
Storage-->>API: SessionMasterKey(version=N)
API->>API: New version = N + 1
API->>API: Generate new SessionMasterKey(N+1)
API->>Executor: rotateSessionMasterKey(newKey)
Executor->>Storage: rotateSessionMasterKey(newKey)
Storage->>KeyStorage: Store new version
KeyStorage->>Cache: Update currentSessionKey
KeyStorage-->>Cache: Notify listeners
Cache->>Cache: SessionKey derived from newKey
Storage-->>Executor: ✓ Complete
Executor-->>API: ✓ 200 OK
API-->>Admin: Success
sequenceDiagram
actor User as User
participant Auth as AuthSessionService
participant Handler as SessionCookieHandler
participant KeyStorage as SessionKeyStorage
participant Verifier as JWTVerifier
participant Encrypter as JWEEncrypter
User->>Auth: Login Request
Auth->>Handler: getSessionInfo(ctx)
Handler->>Handler: Check if encryption enabled
Handler->>KeyStorage: getCurrentSessionKey()
KeyStorage->>KeyStorage: Fetch current version key
KeyStorage-->>Handler: SessionKey(version=N, signer, encrypter)
Handler-->>Auth: SessionInfo ready
Auth->>Auth: Create JWT with sessionKey.signer()
Auth->>Auth: Encrypt session cookie with sessionKey.encrypter()
Auth-->>User: JWT + Encrypted Cookie(version=N)
User->>Auth: Later Request with Encrypted Cookie
Auth->>Handler: getSessionInfo(ctx)
Handler->>Handler: Extract version N from JWE header
Handler->>KeyStorage: getSessionKey(N)
KeyStorage->>KeyStorage: Fetch versioned key
KeyStorage-->>Handler: SessionKey(version=N, decrypter, verifier)
Handler->>Verifier: Verify JWT signature
Handler->>Encrypter: Decrypt session cookie
Handler-->>Auth: SessionInfo restored
Auth-->>User: ✓ Authorized
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Motivation: Central Dogma needs to support key rotation for both wrapped data encryption keys (WDEKs) and session master keys to maintain security best practices. Modifications: - Renamed `KeyManagementService` to `KeyWrapper` - Introduced `KeyManagementService` for key APIs - Implemented WDEK rotation logic that generates new wrapped keys with incremented versions - Implemented session master key rotation with version increment and concurrency protection Result: - System administrators can now rotate DEKs and session master key
| /** | ||
| * Re-wraps the given wrapped data encryption key (WDEK). | ||
| */ | ||
| CompletableFuture<String> rewrap(String wdek, String oldKekId, String newKekId); |
There was a problem hiding this comment.
Some KMS providers, such as AWS KMS, embed version information directly within the wrapped key. In contrast, other providers do not support versioning, which requires users to manually specify a different key to perform a key rotation.
There was a problem hiding this comment.
Note) No issue with this API, but it seems like it isn't called from anywhere
There was a problem hiding this comment.
Yeah, let me use it in the follow-up PR. 😉
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (14)
server/src/main/java/com/linecorp/centraldogma/server/internal/admin/service/DefaultLogoutService.java (2)
71-107: Consider adding explicit error handling for async operations.The CompletableFuture chains lack explicit error handling. If any async operation fails (session retrieval, invalidation, or propagation), users may receive generic error responses rather than meaningful logout-specific error messages.
Consider adding
.exceptionally()or.handle()to provide better error responses:return HttpResponse.of(sessionCookieHandler.getSessionInfo(ctx).thenCompose(sessionInfo -> { // ... existing logic ... }).exceptionally(cause -> { logger.warn("Logout failed", cause); return HttpResponse.of(HttpStatus.INTERNAL_SERVER_ERROR, MediaType.PLAIN_TEXT_UTF_8, "Logout failed"); }));
86-88: Consider extracting duplicate success response creation.The same success response with
invalidatingCookieis created in two places. Extracting this to a helper method would improve maintainability.private static HttpResponse logoutSuccessResponse(Cookie invalidatingCookie) { return HttpResponse.of(ResponseHeaders.builder(HttpStatus.OK) .cookie(invalidatingCookie) .build()); }Then replace both occurrences with
logoutSuccessResponse(invalidatingCookie).Also applies to: 102-104
server/src/test/java/com/linecorp/centraldogma/server/storage/encryption/TestKeyWrapper.java (1)
40-42: Remove debug assertion from test utility.The assertion at line 42 verifies the prefix but serves no functional purpose in a test utility. The length check at lines 36-38 already validates the structure. Consider removing this assertion or converting it to a proper validation if needed.
- final byte[] prefix = new byte[prefixLength]; - System.arraycopy(decoded, 0, prefix, 0, prefixLength); - assert "wrapped-".equals(new String(prefix)); - final byte[] dek = new byte[decoded.length - prefixLength]; System.arraycopy(decoded, prefixLength, dek, 0, dek.length);server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java (1)
32-64: Consider adding equals/hashCode for this value-type
WrappedDekDetailsbehaves like a value object (all-final fields, JSON DTO, used across commands), but currently relies on reference equality. This makes comparisons likeRotateWdekCommand.equalsdepend on instance identity rather than logical equality, which is likely not what you want long term.You can make it more predictable by overriding
equals/hashCodehere instead of per-command:@@ -import java.time.Instant; +import java.time.Instant; +import java.util.Objects; @@ public final class WrappedDekDetails { @@ public String repoName() { return repoName; } + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!(o instanceof WrappedDekDetails)) { + return false; + } + final WrappedDekDetails that = (WrappedDekDetails) o; + return dekVersion == that.dekVersion && + wrappedDek.equals(that.wrappedDek) && + kekId.equals(that.kekId) && + projectName.equals(that.projectName) && + repoName.equals(that.repoName) && + creation.equals(that.creation); + } + + @Override + public int hashCode() { + return Objects.hash(wrappedDek, dekVersion, kekId, projectName, repoName, creation); + } + @Override public String toString() { return MoreObjects.toStringHelper(this) .add("wrappedDek", "****") .add("dekVersion", dekVersion)Also applies to: 114-124
webapp/javaTest/java/com/linecorp/centraldogma/webapp/TestKeyWrapper.java (1)
26-54: TestKeyWrapper ignores kekId and relies on assert-only validationFor a test stub this is fine functionally, but two limitations reduce how much rotation logic it exercises:
wrap/unwrap/rewrapignorekekId/oldKekId/newKekId, so rewrapping with a different KEK ID produces the same WDEK and can’t catch KEK-mismatch bugs.- Prefix checking uses
assert, which may be disabled in normal test runs, so malformed inputs can pass silently after the length check.If you want tests to better simulate real key wrapping and KEK rotation, consider:
- Incorporating the KEK ID into the encoded form (and validating it on unwrap/rewrap).
- Replacing the
assertwith a runtime check that throwsIllegalArgumentExceptionon prefix mismatch.server/src/test/java/com/linecorp/centraldogma/server/internal/api/sysadmin/KeyManagementServiceTest.java (1)
277-315: RenameprotoprojectNameinWrappedDekfor clarityThe inner
WrappedDekhelper is clear, but the field nameprois slightly cryptic compared to the other code in this file. Renaming it toprojectNamewould make the helper more self‑documenting and consistent with the rest of the codebase.server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/RepositoryEncryptionStorage.java (1)
443-447: Clarify key‑version extraction frommetadataValueIn the
rev2shaandHEAD/refsbranches you call:final SecretKey dek = getDek(projectName, repoName, Ints.fromByteArray(metadataValue));with
metadataValue.length == 4 + NONCE_SIZE_BYTES.Ints.fromByteArraywill read only the first four bytes (the version), but the fact that the array contains additional nonce bytes is not obvious at first glance.Consider making this intent explicit, e.g.:
final int keyVersion = Ints.fromByteArray(Arrays.copyOfRange(metadataValue, 0, 4)); final SecretKey dek = getDek(projectName, repoName, keyVersion);or by introducing a small helper method. This would reduce confusion for future readers without changing behavior.
Also applies to: 468-472
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/EncryptionStorageManager.java (2)
58-62: TightenkekIdnull-handling in factory methods
EncryptionStorageManager.of(CentralDogmaConfig)effectively enforces non-nullkekIdvia the constructor, but the@VisibleForTesting of(Path, boolean, String kekId)overload only checkspath. Letting a nullkekIdflow into the implementation fails later with a less-obvious NPE.Consider validating
kekIdat the factory boundary for symmetry withpath:@VisibleForTesting static EncryptionStorageManager of(Path path, boolean encryptSessionCookie, String kekId) { - requireNonNull(path, "path"); - return new DefaultEncryptionStorageManager(path.toString(), encryptSessionCookie, kekId); + requireNonNull(path, "path"); + requireNonNull(kekId, "kekId"); + return new DefaultEncryptionStorageManager(path.toString(), encryptSessionCookie, kekId); }Also applies to: 68-71
208-211: Document threading/ordering guarantees foraddSessionKeyListenerThe listener API is a nice addition, but it’s unclear:
- Whether listeners are invoked synchronously or asynchronously.
- From which threads they are called and whether invocations may be concurrent.
- Whether newly added listeners see only future keys or also the current one.
Given that listeners will likely touch security-sensitive state, adding brief Javadoc about these aspects would help prevent misuse.
server/src/main/java/com/linecorp/centraldogma/server/command/Command.java (1)
46-66: Consider whether rotation commands should be force-pushableNew rotation support is wired via:
- JSON subtypes:
ROTATE_WDEKandROTATE_SESSION_MASTER_KEY.- Factory methods:
rotateWdek(...)androtateSessionMasterKey(...).Currently,
forcePushonly allows a fixed set of command types and excludes these new rotation commands. If you expect to perform key rotations while the server is in read-only/maintenance mode, you may want to extend the allowedCommandTypes; if not, the current restriction is fine but worth confirming explicitly.No changes are required for correctness, but please double-check the intended operational behavior.
Also applies to: 412-424, 462-473
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/SessionKeyStorage.java (2)
84-131: Rotation semantics and RocksDB writes look correct; consider logging unwrap failuresThe rotation logic enforces
newVersion == currentVersion + 1whenrotate == trueand rejects duplicate versions using a RocksDB existence check, then atomically:
- Writes the versioned master key entry, and
- Updates the “current version” pointer under
session/master/current.This is a good, simple contract. Two improvements to consider:
- The
get+writesequence is non‑atomic at the RocksDB level; concurrent rotations with the same version could race. In practice admin calls are usually serialized, but if you expect multi‑node or concurrent rotations, you may want a stronger coordination mechanism at the command layer.maybeSetCurrentSessionKey(sessionMasterKey)is invoked but its returnedCompletableFutureis ignored. If unwrapping fails, the failure is only visible to callers ofgetCurrentSessionKey()/getSessionKey. Adding an.exceptionally(...)or logger hook to record unwrap/derivation failures would improve diagnosability without changing behavior.
168-201: SessionKey caching and listener notification are correct; note lock scope
getCurrentSessionKey()+maybeSetCurrentSessionKey(...):
- Use a short lock to return a cached
currentSessionKeyif present, otherwise resolve it lazily by unwrapping the current master key.- Ensure
currentSessionKeyis only updated to a newer version (version()monotonic), preventing regressions if multiple async unwraps race.- Notify all listeners whenever a newer key is installed.
The design is solid. One thing to keep in mind: listeners are called while holding
lock. Current usages (e.g.,SessionCookieHandlerjust assigning a field) are cheap, but if future listeners perform heavier work or call back into this storage, this could lengthen lock hold time or create re‑entrancy hazards. If you expect non‑trivial listeners, consider copying the listener list under the lock and invoking them after releasing it.server/src/main/java/com/linecorp/centraldogma/server/auth/SessionMasterKey.java (1)
18-105: SessionMasterKey refactor is solid; minor Javadoc nitThe class now:
- Validates
wrappedMasterKey,salt,kekId,creationas non‑null andversion > 0.- Exposes base64‑encoded wrapped master key and salt, plus KEK ID and creation timestamp.
- Redacts both wrapped key and salt in
toString(), while keeping non‑sensitive metadata.This is a good security posture. The only minor nit is the incomplete Javadoc on
kekId(“used to wrap the …”); consider finishing that sentence for clarity.server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/NoopEncryptionStorageManager.java (1)
18-172: No‑op implementation aligns with the expanded API; clarify null‑return expectationsThe noop manager cleanly implements the expanded
EncryptionStorageManagersurface by:
- Always returning
falsefromenabled()andencryptSessionCookie().- Returning
UnmodifiableFuture.completedFuture(null)for key‑generation and key‑retrieval methods, andImmutableList.of()/ empty maps for listing methods.- Making all mutation methods (store/rotate/remove WDEK, rotate session master key, listener registration, etc.) true no‑ops.
This is consistent with a disabled‑encryption environment. To avoid accidental null handling bugs, it’s worth documenting at the interface level that these async methods may complete with
nullwhen encryption is disabled, and that callers are expected to guard onenabled()/encryptSessionCookie()and not rely on these futures in that case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (53)
server/src/main/java/com/linecorp/centraldogma/server/CentralDogma.java(2 hunks)server/src/main/java/com/linecorp/centraldogma/server/EncryptionAtRestConfig.java(3 hunks)server/src/main/java/com/linecorp/centraldogma/server/auth/SessionKey.java(5 hunks)server/src/main/java/com/linecorp/centraldogma/server/auth/SessionMasterKey.java(2 hunks)server/src/main/java/com/linecorp/centraldogma/server/command/Command.java(6 hunks)server/src/main/java/com/linecorp/centraldogma/server/command/CommandType.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/command/CreateProjectCommand.java(5 hunks)server/src/main/java/com/linecorp/centraldogma/server/command/CreateRepositoryCommand.java(5 hunks)server/src/main/java/com/linecorp/centraldogma/server/command/MigrateToEncryptedRepositoryCommand.java(4 hunks)server/src/main/java/com/linecorp/centraldogma/server/command/RotateSessionMasterKeyCommand.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/command/RotateWdekCommand.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/command/StandaloneCommandExecutor.java(7 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/AuthSessionService.java(4 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionCookieAuthorizer.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionCookieHandler.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionUtil.java(4 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/admin/service/DefaultLogoutService.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/admin/service/UserService.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceUtil.java(2 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java(3 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/KeyManagementService.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/ServerStatusService.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/SessionMasterKeyDetails.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/storage/project/ProjectApiManager.java(2 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/rocksdb/EncryptionGitStorage.java(9 hunks)server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/DefaultEncryptionStorageManager.java(2 hunks)server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/EncryptionStorageManager.java(6 hunks)server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/KeyWrapper.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/NoopEncryptionStorageManager.java(4 hunks)server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/RepositoryEncryptionStorage.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/RocksDBStorage.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/SessionKeyStorage.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/storage/project/InternalProjectInitializer.java(1 hunks)server/src/test/java/com/linecorp/centraldogma/server/auth/SessionKeyTest.java(2 hunks)server/src/test/java/com/linecorp/centraldogma/server/internal/admin/auth/AuthServerTest.java(2 hunks)server/src/test/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionUtilTest.java(2 hunks)server/src/test/java/com/linecorp/centraldogma/server/internal/api/MigrateToEncryptedRepositoryTest.java(1 hunks)server/src/test/java/com/linecorp/centraldogma/server/internal/api/sysadmin/KeyManagementServiceTest.java(1 hunks)server/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/EncryptedGitRepositoryRemoveTest.java(3 hunks)server/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryTest.java(2 hunks)server/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/rocksdb/EncryptionGitStorageTest.java(2 hunks)server/src/test/java/com/linecorp/centraldogma/server/storage/encryption/DefaultEncryptionStorageManagerTest.java(5 hunks)server/src/test/java/com/linecorp/centraldogma/server/storage/encryption/TestKeyWrapper.java(1 hunks)server/src/test/resources/META-INF/services/com.linecorp.centraldogma.server.storage.encryption.KeyManagementService(0 hunks)server/src/test/resources/META-INF/services/com.linecorp.centraldogma.server.storage.encryption.KeyWrapper(1 hunks)testing/common/src/main/java/com/linecorp/centraldogma/testing/internal/CentralDogmaRuleDelegate.java(2 hunks)testing/junit/src/main/java/com/linecorp/centraldogma/testing/junit/CentralDogmaExtension.java(2 hunks)testing/junit4/src/main/java/com/linecorp/centraldogma/testing/junit4/CentralDogmaRule.java(2 hunks)webapp/javaTest/java/com/linecorp/centraldogma/webapp/SamlCentralDogmaTestServer.java(1 hunks)webapp/javaTest/java/com/linecorp/centraldogma/webapp/TestKeyWrapper.java(1 hunks)webapp/javaTest/resources/META-INF/services/com.linecorp.centraldogma.server.storage.encryption.KeyManagementService(0 hunks)webapp/javaTest/resources/META-INF/services/com.linecorp.centraldogma.server.storage.encryption.KeyWrapper(1 hunks)
💤 Files with no reviewable changes (2)
- server/src/test/resources/META-INF/services/com.linecorp.centraldogma.server.storage.encryption.KeyManagementService
- webapp/javaTest/resources/META-INF/services/com.linecorp.centraldogma.server.storage.encryption.KeyManagementService
🧰 Additional context used
🧬 Code graph analysis (23)
server/src/main/java/com/linecorp/centraldogma/server/command/RotateWdekCommand.java (1)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java (1)
WrappedDekDetails(30-125)
server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/AuthSessionService.java (1)
server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionUtil.java (1)
SessionUtil(59-314)
server/src/test/java/com/linecorp/centraldogma/server/internal/api/sysadmin/KeyManagementServiceTest.java (8)
common/src/main/java/com/linecorp/centraldogma/internal/api/v1/HttpApiV1Constants.java (1)
HttpApiV1Constants(22-48)server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionUtil.java (1)
SessionUtil(59-314)server/src/main/java/com/linecorp/centraldogma/server/internal/storage/MigratingMetaToDogmaRepositoryService.java (1)
MigratingMetaToDogmaRepositoryService(53-226)testing-internal/src/main/java/com/linecorp/centraldogma/testing/internal/auth/TestAuthMessageUtil.java (1)
TestAuthMessageUtil(44-146)server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/rocksdb/EncryptionGitStorage.java (1)
EncryptionGitStorage(58-398)server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/rocksdb/GitObjectMetadata.java (1)
GitObjectMetadata(29-101)server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java (1)
WrappedDekDetails(30-125)server/src/main/java/com/linecorp/centraldogma/server/storage/project/InternalProjectInitializer.java (1)
InternalProjectInitializer(66-314)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/KeyManagementService.java (3)
server/src/main/java/com/linecorp/centraldogma/server/auth/SessionMasterKey.java (1)
SessionMasterKey(28-107)server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/SecretKeyWithVersion.java (1)
SecretKeyWithVersion(28-63)server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java (1)
WrappedDekDetails(30-125)
server/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/EncryptedGitRepositoryRemoveTest.java (1)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java (1)
WrappedDekDetails(30-125)
server/src/main/java/com/linecorp/centraldogma/server/command/StandaloneCommandExecutor.java (1)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java (1)
WrappedDekDetails(30-125)
server/src/main/java/com/linecorp/centraldogma/server/command/RotateSessionMasterKeyCommand.java (1)
server/src/main/java/com/linecorp/centraldogma/server/auth/SessionMasterKey.java (1)
SessionMasterKey(28-107)
server/src/test/java/com/linecorp/centraldogma/server/storage/encryption/TestKeyWrapper.java (1)
webapp/javaTest/java/com/linecorp/centraldogma/webapp/TestKeyManagementService.java (2)
TestKeyManagementService(22-43)Override(24-31)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/SessionKeyStorage.java (5)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/RocksDBStorage.java (1)
RocksDBStorage(47-175)common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java (1)
Jackson(68-390)server/src/main/java/com/linecorp/centraldogma/server/auth/SessionKey.java (1)
SessionKey(41-136)server/src/main/java/com/linecorp/centraldogma/server/auth/SessionMasterKey.java (1)
SessionMasterKey(28-107)server/src/main/java/com/linecorp/centraldogma/server/internal/storage/AesGcmSivCipher.java (1)
AesGcmSivCipher(33-94)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/ServerStatusService.java (1)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/LoggerService.java (1)
RequiresSystemAdministrator(43-147)
server/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryTest.java (1)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java (1)
WrappedDekDetails(30-125)
server/src/main/java/com/linecorp/centraldogma/server/command/CreateProjectCommand.java (1)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java (1)
WrappedDekDetails(30-125)
server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionCookieHandler.java (2)
server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionUtil.java (1)
SessionUtil(59-314)server/src/main/java/com/linecorp/centraldogma/server/auth/SessionKey.java (1)
SessionKey(41-136)
server/src/main/java/com/linecorp/centraldogma/server/internal/storage/project/ProjectApiManager.java (1)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java (1)
WrappedDekDetails(30-125)
server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionUtil.java (1)
server/src/main/java/com/linecorp/centraldogma/server/auth/SessionKey.java (1)
SessionKey(41-136)
server/src/main/java/com/linecorp/centraldogma/server/command/MigrateToEncryptedRepositoryCommand.java (1)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java (1)
WrappedDekDetails(30-125)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceUtil.java (1)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java (1)
WrappedDekDetails(30-125)
server/src/main/java/com/linecorp/centraldogma/server/command/Command.java (1)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java (1)
WrappedDekDetails(30-125)
server/src/test/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionUtilTest.java (1)
server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionUtil.java (1)
SessionUtil(59-314)
server/src/main/java/com/linecorp/centraldogma/server/command/CreateRepositoryCommand.java (1)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java (1)
WrappedDekDetails(30-125)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java (1)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java (1)
WrappedDekDetails(30-125)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/RepositoryEncryptionStorage.java (5)
server/src/main/java/com/linecorp/centraldogma/server/internal/storage/AesGcmSivCipher.java (1)
AesGcmSivCipher(33-94)server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/rocksdb/EncryptionGitStorage.java (1)
EncryptionGitStorage(58-398)server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/RocksDBStorage.java (1)
RocksDBStorage(47-175)common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java (1)
Jackson(68-390)server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/rocksdb/GitObjectMetadata.java (1)
GitObjectMetadata(29-101)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/DefaultEncryptionStorageManager.java (2)
server/src/main/java/com/linecorp/centraldogma/server/auth/SessionKey.java (1)
SessionKey(41-136)server/src/main/java/com/linecorp/centraldogma/server/auth/SessionMasterKey.java (1)
SessionMasterKey(28-107)
⏰ 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). (6)
- GitHub Check: build-ubuntu-latest-jdk-17-coverage
- GitHub Check: build-ubuntu-latest-jdk-17-min-java-11
- GitHub Check: build-macos-latest-jdk-21
- GitHub Check: build-windows-latest-jdk-21
- GitHub Check: build-ubuntu-latest-jdk-11
- GitHub Check: build-ubuntu-latest-jdk-21-snapshot
🔇 Additional comments (60)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceUtil.java (1)
69-76: LGTM!The WDEK wrapping logic correctly constructs a WrappedDekDetails object with version 1 (appropriate for new repository creation) and passes all required metadata (kekId, projectName, repoName) to the Command.createRepository method.
server/src/main/java/com/linecorp/centraldogma/server/CentralDogma.java (1)
1030-1032: LGTM!The conditional binding of KeyManagementService is correctly guarded by the encryption enabled check, ensuring key management endpoints are only exposed when encryption-at-rest is configured.
server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/ServerStatusService.java (1)
36-36: LGTM!The class-level @RequiresSystemAdministrator annotation correctly restricts access to all server status endpoints, consistent with other administrative services like LoggerService.
webapp/javaTest/resources/META-INF/services/com.linecorp.centraldogma.server.storage.encryption.KeyWrapper (1)
1-1: LGTM!Standard Java ServiceLoader registration for the test KeyWrapper implementation.
testing/common/src/main/java/com/linecorp/centraldogma/testing/internal/CentralDogmaRuleDelegate.java (1)
206-211: LGTM!The new encryptionStorageManager() accessor follows the same pattern as other delegate methods like projectManager(), appropriately exposing encryption storage management for test scenarios.
server/src/main/java/com/linecorp/centraldogma/server/internal/admin/service/UserService.java (1)
81-101: LGTM!The refactoring correctly transforms the synchronous session retrieval into an asynchronous composition. The nested CompletableFuture handling is appropriate here—Armeria's HttpResponse.of() correctly handles CompletableFuture at both levels, and all original logic branches are preserved.
webapp/javaTest/java/com/linecorp/centraldogma/webapp/SamlCentralDogmaTestServer.java (1)
55-55: LGTM!The EncryptionAtRestConfig constructor correctly includes the new kekId parameter, aligning with the PR's introduction of KEK (Key Encryption Key) support.
server/src/test/java/com/linecorp/centraldogma/server/internal/api/MigrateToEncryptedRepositoryTest.java (1)
58-58: LGTM!The test correctly updates the EncryptionAtRestConfig constructor to include the new kekId parameter.
server/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/rocksdb/EncryptionGitStorageTest.java (2)
82-85: LGTM!The lenient stubbing approach is appropriate for test setup where stubs may not be invoked in every test method. The addition of the versioned
getDekstub correctly aligns with thegetCurrentDekreturning version 1, supporting the WDEK versioning introduced in this PR.
332-336: Good addition of clarifying comment.The comment and
times(2)verification correctly validates that multiple ref updates are stored properly. The test structure is clear and appropriate.server/src/test/resources/META-INF/services/com.linecorp.centraldogma.server.storage.encryption.KeyWrapper (1)
1-1: LGTM!Standard ServiceLoader configuration for test-time KeyWrapper implementation. This aligns with the PR's refactoring from KeyManagementService to KeyWrapper.
server/src/main/java/com/linecorp/centraldogma/server/command/CommandType.java (1)
40-41: LGTM!The new command types are correctly defined with
Void.classas the result type, and the naming clearly indicates their purpose for key rotation operations.testing/junit4/src/main/java/com/linecorp/centraldogma/testing/junit4/CentralDogmaRule.java (1)
185-190: LGTM!The new accessor method follows the existing pattern (e.g.,
projectManager()) and properly exposes theEncryptionStorageManagerfor test access. The implementation correctly delegates to the underlying delegate.testing/junit/src/main/java/com/linecorp/centraldogma/testing/junit/CentralDogmaExtension.java (1)
199-204: LGTM!The accessor method is consistent with the similar addition in
CentralDogmaRuleand follows the established pattern for exposing server components to tests.server/src/test/java/com/linecorp/centraldogma/server/internal/admin/auth/AuthServerTest.java (2)
81-81: LGTM!The constructor call correctly includes the new
kekIdparameter, aligning with the updatedEncryptionAtRestConfigAPI that supports KEK-based encryption.
93-93: LGTM!Consistent with the previous configuration, this correctly includes the
kekIdparameter.server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionCookieAuthorizer.java (1)
84-112: Well-structured async refactor.The migration to asynchronous session retrieval is properly implemented with appropriate null checks and error handling. The flow correctly handles both JWT-only (read-only) and session-based authorization paths, with CSRF validation in both cases.
server/src/main/java/com/linecorp/centraldogma/server/command/StandaloneCommandExecutor.java (5)
210-216: LGTM!The command dispatch for the new rotation operations is properly integrated into the existing command handling flow.
245-270: LGTM!The refactoring to use
WrappedDekDetailsis consistent and complete. The error handling properly cleans up the stored WDEK using the correct version on failure.
310-336: LGTM!The changes mirror the
createProjectrefactoring with consistent use ofWrappedDekDetailsand proper cleanup logic.
359-382: LGTM!The migration command follows the same
WrappedDekDetailspattern with proper error handling and version-based cleanup.
403-419: LGTM!The rotation methods are properly implemented with appropriate precondition checks and clear error messages. The delegation to
EncryptionStorageManagerand return of completed futures is consistent with other void operations in this class.server/src/main/java/com/linecorp/centraldogma/server/EncryptionAtRestConfig.java (1)
47-50: LGTM! Proper validation for KEK ID when encryption is enabled.The validation correctly enforces that
kekIdmust be provided when encryption at rest is enabled, while allowing it to be null when disabled.server/src/main/java/com/linecorp/centraldogma/server/internal/storage/project/ProjectApiManager.java (1)
121-127: LGTM! Proper WrappedDekDetails construction.The WDEK is correctly wrapped with version 1, KEK ID, and repository metadata before being passed to the command executor.
server/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryTest.java (1)
110-115: LGTM! Test setup correctly updated for new WDEK handling.The test properly constructs
WrappedDekDetailswith version 1 and stores it via the updatedstoreWdekAPI.server/src/main/java/com/linecorp/centraldogma/server/storage/project/InternalProjectInitializer.java (1)
255-255: LGTM! Correct session master key generation with version 1.The updated call properly passes version 1 for the initial session master key generation, aligning with the new API signature.
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/KeyWrapper.java (1)
38-41: LGTM! Essential method for key rotation.The
rewrapmethod enables secure key rotation by allowing a WDEK to be rewrapped with a new KEK without exposing the plaintext DEK to the application layer.server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/SessionMasterKeyDetails.java (1)
29-79: LGTM! Well-designed immutable metadata class.The class correctly:
- Validates version > 0
- Enforces non-null KEK ID and creation timestamp
- Exposes only non-sensitive metadata
- Provides JSON serialization support
server/src/test/java/com/linecorp/centraldogma/server/auth/SessionKeyTest.java (1)
36-38: LGTM! Test correctly updated for new API.The test properly:
- Passes
kekIdtoEncryptionStorageManager.of()- Provides version 1 to
generateSessionMasterKey()- Decodes the Base64-encoded salt before validating its length
server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/rocksdb/EncryptionGitStorage.java (1)
100-100: No performance concerns; internal DEK caching is properly implemented.The
RepositoryEncryptionStorageclass has internal caching via twoConcurrentHashMapfields (currentDeksanddekWithVersion) that cache DEKs after first retrieval. BothgetDek()andgetCurrentDek()usecomputeIfAbsent()for thread-safe lazy initialization and caching. Cache entries are properly invalidated when keys are rotated or removed. The on-demand retrieval pattern is correct—write operations usegetCurrentDek()to get the latest key, while read operations usegetDek(version)to retrieve the versioned key used at encryption time. This ensures correctness after key rotation without performance degradation.server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/RocksDBStorage.java (1)
128-175: LGTM!The CRUD operations and resource management methods are correctly implemented with appropriate error handling and defensive checks.
server/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/EncryptedGitRepositoryRemoveTest.java (2)
66-77: LGTM!The test correctly updates to use the new WrappedDekDetails API, properly constructing the details with version, kekId, project, and repository names.
125-130: LGTM!Consistent usage of WrappedDekDetails for the second repository creation.
server/src/test/java/com/linecorp/centraldogma/server/storage/encryption/DefaultEncryptionStorageManagerTest.java (2)
69-79: LGTM!The test correctly validates the Base64-encoded wrapped DEK structure and length.
82-111: LGTM!The test correctly validates WDEK storage, retrieval, and versioned removal with proper error handling.
server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/AuthSessionService.java (3)
45-69: LGTM!The dynamic session key management via volatile field and listener pattern correctly enables key rotation. The volatile keyword ensures visibility across threads, and the initialization ensures a valid key is available before use.
74-96: LGTM!The read-only mode login correctly requires encryption and uses the current session key for signing and encrypting JWTs.
98-116: LGTM!The writable mode login correctly handles both encrypted and non-encrypted sessions, using the current session key when encryption is enabled.
server/src/main/java/com/linecorp/centraldogma/server/command/RotateSessionMasterKeyCommand.java (1)
31-73: LGTM!The command implementation follows standard patterns with proper field validation, equality semantics, and toString representation. The null timestamp and author in the constructor are appropriate for system-initiated rotation commands.
server/src/test/java/com/linecorp/centraldogma/server/storage/encryption/TestKeyWrapper.java (1)
49-52: LGTM!The rewrap implementation correctly chains unwrap and wrap operations for key rotation.
server/src/main/java/com/linecorp/centraldogma/server/command/MigrateToEncryptedRepositoryCommand.java (1)
32-94: LGTM!The migration to WrappedDekDetails provides better encapsulation and context compared to raw byte arrays, with correctly updated equality and hashing semantics.
server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionUtil.java (2)
152-182: LGTM!The signature verification using the SessionKey is correctly implemented, validating the JWT signature before parsing claims. The error handling appropriately logs and returns null for invalid signatures.
226-239: LGTM!The visibility change to public is appropriate for external callers needing to access session cookies, with proper null handling maintained.
server/src/main/java/com/linecorp/centraldogma/server/command/RotateWdekCommand.java (1)
31-75: RotateWdekCommand structure and JSON binding look consistentCommand type, JSON creator, non-null
wdekDetails, equality, hash code, and redactedtoStringHelperusage are all coherent with the existing command pattern and avoid leaking the wrapped key.server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java (1)
333-342: WrappedDekDetails integration in migration flow is coherent
generateWdek()→WrappedDekDetailsconstruction (with version1,kekId, project, and repo) →migrate(...)wiring preserves the previous control flow while enriching it with the metadata needed for rotation; error handling and status transitions remain unchanged.Also applies to: 404-413
server/src/test/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionUtilTest.java (1)
64-69: SessionUtilTest correctly exercises the new SessionKey-based JWT/JWE flowThe setup using
EncryptionStorageManagerandSessionMasterKey, and the round-trip viaSessionKey(signer, encrypter, decrypter, and verifier passed intogetJwtClaimsSetFromSignedJwt) is consistent and keeps the original behavioral assertions intact.Also applies to: 79-107
server/src/test/java/com/linecorp/centraldogma/server/internal/api/sysadmin/KeyManagementServiceTest.java (1)
114-166: End‑to‑end WDEK rotation coverage looks solidThis test does a nice job validating the full behavior: initial WDEK listing, DEK version used by commits, rotation via the admin API, and that subsequent commits pick up the new version by inspecting
GitObjectMetadata. This gives good confidence that the wiring betweenKeyManagementService,EncryptionGitStorage, and RocksDB metadata is correct.server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/KeyManagementService.java (1)
61-83: Rotation endpoints line up well with storage semanticsThe WDEK and session master key rotation endpoints correctly:
- Guard on encryption/session‑cookie enablement.
- Use the current version from
EncryptionStorageManagerand compute+1for the new version.- Rely on the storage layer’s “version must be exactly one greater” checks to handle concurrent rotations safely.
Given these are admin‑only endpoints, throwing on unsupported
reencrypt=trueand on disabled session cookie encryption is reasonable for now and matches the surrounding style.Also applies to: 101-116
server/src/main/java/com/linecorp/centraldogma/server/auth/SessionKey.java (1)
47-64: ---SessionKey derivation from SessionMasterKey is correct
The refactor to derive
SessionKeyfromSessionMasterKeyis sound:
- Salt is Base64‑decoded from
sessionMasterKey.salt(), matching its documented encoding.- HKDF uses distinct
infostrings for signing vs encryption keys, with 32‑byte outputs for HMAC‑SHA256 and AES, respectively.- The version is taken directly from
sessionMasterKey.version(), so key IDs and cookie headers stay aligned.- MACSigner, MACVerifier, DirectEncrypter, and DirectDecrypter are explicitly documented as thread-safe in Nimbus JOSE+JWT, so caching them as instance fields is both safe and efficient.
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/EncryptionStorageManager.java (1)
84-87: KEK ID accessor looks consistent with configurationExposing
kekId()onEncryptionStorageManagermatches the new constructor wiring andEncryptionAtRestConfig.kekId(). The method and Javadoc are clear and align with the rest of the API.server/src/main/java/com/linecorp/centraldogma/server/command/Command.java (1)
79-90: WDEK-aware factory overloads are consistent with existing patternsThe new overloads:
createProject(Author author, String name, WrappedDekDetails wdekDetails)createRepository(Author author, String projectName, String repositoryName, WrappedDekDetails wdekDetails)migrateToEncryptedRepository(@Nullable Long timestamp, Author author, String projectName, String repositoryName, WrappedDekDetails wdekDetails)mirror the existing command style:
- They reuse existing command implementations (
CreateProjectCommand,CreateRepositoryCommand,MigrateToEncryptedRepositoryCommand).- They validate
authorandwdekDetailswhere appropriate.The signatures and Javadocs look coherent with the rest of the API surface.
Also applies to: 190-203, 296-305
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/DefaultEncryptionStorageManager.java (2)
45-67: Constructor wiring andKeyWrapperloading look reasonableThe new constructor:
- Validates
rocksDbPathandkekId.- Uses
ServiceLoaderwith a strictkeyWrappers.size() == 1check.- Wires
RocksDBStorage,SessionKeyStorage, andRepositoryEncryptionStoragearound the sharedKeyWrapper.Assuming the runtime packaging provides exactly one
KeyWrapperimplementation, this is a clear and fail-fast setup for encryption at rest.
80-198: Delegations toSessionKeyStorage/RepositoryEncryptionStoragelook correctAll updated methods in this class now behave as thin facades:
- Session master key and session key lifecycle:
kekId(),generateSessionMasterKey(int),storeSessionMasterKey(...),
getCurrentSessionMasterKey(),getCurrentSessionKey(),getSessionKey(int),
rotateSessionMasterKey(...), andaddSessionKeyListener(...)all delegate tosessionKeyStorage.- WDEK and DEK operations:
generateWdek(),wdeks(),getDek(...),getCurrentDek(...),
storeWdek(...),rotateWdek(...),removeWdek(...)all delegate torepositoryEncryptionStorage.- Low-level object/metadata APIs and repository cleanup:
getObject(...),getObjectId(...),getMetadata(...),putObject(...),
putObjectId(...),containsMetadata(...),deleteObjectId(...),
deleteRepositoryData(...),getAllData()are all forwarded torepositoryEncryptionStorage.close()correctly closes the sharedrocksDbStorage.The delegations appear 1:1 with no parameter reordering or obvious mistakes, which keeps this class focused on wiring rather than logic.
server/src/main/java/com/linecorp/centraldogma/server/command/CreateRepositoryCommand.java (1)
40-101: WrappedDekDetails integration and logging look goodThe migration from
byte[] wdektoWrappedDekDetails wdekDetailsis consistent across constructor, accessor,equals,hashCode, andtoString. Given thatWrappedDekDetails.toString()already masks the wrapped key, addingwdekDetailsdirectly to theToStringHelperdoes not leak key material, while still exposing useful metadata. No issues from a correctness or security perspective here.server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/SessionKeyStorage.java (4)
63-78: Session master key generation is sound and HKDF‑compatible
generateSessionMasterKey(int version)correctly:
- Generates a 256‑bit random master key and wraps it via
KeyWrapper.- Generates an independent 256‑bit random salt (non‑secret but high entropy) consistent with HKDF recommendations.
- Populates
SessionMasterKeywith wrapped key, version, base64 salt, KEK ID, and ISO‑8601 creation timestamp.This is aligned with
SessionMasterKey’s API and HKDF usage inSessionKey.of(...).
133-166: Current session master key retrieval and deserialization
getCurrentSessionMasterKey()andgetSessionMasterKey(int version)correctly:
- Read the current version pointer, fail fast if it doesn’t exist, and
- Fetch and deserialize the corresponding
SessionMasterKeyJSON, wrapping RocksDB and Jackson errors inEncryptionStorageException.This is straightforward and consistent with the storage format; no issues.
203-209: Per‑version SessionKey cache is reasonable
getSessionKey(int version)uses aConcurrentHashMapofCompletableFuture<SessionKey>keyed by version, lazily populating entries from the storedSessionMasterKey. This is a good fit for infrequently‑rotated session keys, and ensures:
- At most one unwrap/derivation per version, and
- Shared futures across concurrent callers.
Given the expected small number of key versions, the unbounded map is acceptable. No changes needed.
223-234: Listener registration behavior is clear
addSessionKeyListener:
- Enforces non‑null listeners.
- Registers under the same lock used for updates.
- Immediately invokes the listener with the current key if present, which simplifies consumers.
This matches the semantics implied by
SessionCookieHandler. Just ensure listener implementations stay lightweight since they run under the internal lock.server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionCookieHandler.java (2)
76-100: Key‑version aware session lookup is well‑designedThe async
getSessionInfo(ServiceRequestContext)method:
- Short‑circuits to
nullwhen no cookie is present or the key version in the JWE header is invalid.- Uses the cached
sessionKeywhen the version matches, avoiding an extra lookup.- When the version differs, fetches the appropriate
SessionKeyviaencryptionStorageManager.getSessionKey(version)and re‑decodes with that key.- Falls back to plain session ID cookies when encryption is disabled.
This provides a clean migration path across key rotations while honoring per‑version tokens. Behavior on exceptions from
getSessionKey()is to propagate an exceptionally completed future, which is appropriate for callers to handle. No changes needed here.
102-129: JWT parsing and read‑only fallback are consistent with SessionUtilThe private
getSessionInfo(ctx, sessionKey):
- Decrypts the JWE cookie, verifies the inner JWS signature with the
SessionKeyverifier, and validates claims viaDefaultJWTClaimsVerifier.- Preferentially uses a
"sessionId"claim when present.- In read‑only mode (
sessionPropagatorWritableChecker.getAsBoolean()is false), falls back to authenticating purely bysub(username) plus a CSRF token derived from the JWS signature.This matches the utilities in
SessionUtiland maintains redaction and verification of all cryptographic material. Logic is clear and correct.
server/src/main/java/com/linecorp/centraldogma/server/command/CreateProjectCommand.java
Show resolved
Hide resolved
...src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionCookieHandler.java
Show resolved
Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionUtil.java
Outdated
Show resolved
Hide resolved
.../main/java/com/linecorp/centraldogma/server/internal/admin/service/DefaultLogoutService.java
Outdated
Show resolved
Hide resolved
.../main/java/com/linecorp/centraldogma/server/internal/admin/service/DefaultLogoutService.java
Outdated
Show resolved
Hide resolved
.../main/java/com/linecorp/centraldogma/server/storage/encryption/EncryptionStorageManager.java
Show resolved
Hide resolved
.../main/java/com/linecorp/centraldogma/server/storage/encryption/EncryptionStorageManager.java
Show resolved
Hide resolved
...in/java/com/linecorp/centraldogma/server/storage/encryption/RepositoryEncryptionStorage.java
Show resolved
Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/RocksDBStorage.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/RocksDBStorage.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/main/java/com/linecorp/centraldogma/server/internal/admin/service/DefaultLogoutService.java (1)
48-67: Move null check before first usage.The
requireNonNullvalidation on line 56 occurs afterencryptionStorageManageris already used on lines 54-55 and line 60. If null, an NPE will be thrown before the validation check, defeating its purpose.Apply this diff to move the validation to the correct position:
public DefaultLogoutService(Function<String, CompletableFuture<Void>> logoutSessionPropagator, BooleanSupplier sessionPropagatorWritableChecker, SessionManager sessionManager, boolean tlsEnabled, EncryptionStorageManager encryptionStorageManager) { this.logoutSessionPropagator = requireNonNull(logoutSessionPropagator, "logoutSessionPropagator"); this.sessionManager = requireNonNull(sessionManager, "sessionManager"); + requireNonNull(encryptionStorageManager, "encryptionStorageManager"); sessionCookieHandler = new SessionCookieHandler( sessionPropagatorWritableChecker, tlsEnabled, encryptionStorageManager); - requireNonNull(encryptionStorageManager, "encryptionStorageManager");
♻️ Duplicate comments (1)
server/src/main/java/com/linecorp/centraldogma/server/command/CreateProjectCommand.java (1)
44-57: Re-check JSON backward compatibility forwdek→wdekDetails(equality semantics now OK)The switch to
WrappedDekDetails wdekDetailsand theObjects.equals/Objects.hashCodeusage looks correct now thatWrappedDekDetailsimplements structuralequals/hashCode— this addresses the earlier equality semantics concern.However, the JSON field has been renamed from
"wdek"(previousbyte[]field) to"wdekDetails"with a different shape (object instead of the old representation). If anyCreateProjectCommandinstances with awdekfield have been serialized (e.g., in logs, snapshots, or replicated command streams), newer servers will not be able to deserialize them into this constructor:@JsonCreator CreateProjectCommand(..., @JsonProperty("projectName") String projectName, @JsonProperty("wdekDetails") @Nullable WrappedDekDetails wdekDetails)Please confirm one of the following:
- Either no historical commands exist that include the old
"wdek"field (e.g., encryption-at-rest was not previously enabled for project creation), or- You have a migration/deserialization strategy in place (e.g., a custom deserializer or an alternate
@JsonCreatorthat can translate the legacywdekrepresentation into aWrappedDekDetailswith reasonable defaults).If the latter is needed, simple
@JsonAlias("wdek")on this parameter will not be enough by itself because the JSON shape changed (string/bytes vs object). A conversion step from the legacy representation toWrappedDekDetailswould still be required.Also applies to: 88-95
🧹 Nitpick comments (5)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/NoopEncryptionStorageManager.java (1)
54-66: EmptykekId()and null futures are acceptable but could be more explicit.Returning an empty string for
kekId()andnullfromgenerateWdek()/generateSessionMasterKey()futures is fine for a noop implementation, assuming callers always checkenabled()before using these values. If you expect these methods to be surfaced in debugging or metrics, consider returning a sentinel ID such as"noop"instead of""for clarity.server/src/main/java/com/linecorp/centraldogma/server/command/StandaloneCommandExecutor.java (2)
245-269: WDEK handling on create project/repository looks correct; minor reuse improvement.Storing the WDEK before creation and removing it on failure using
dekVersion()gives you a clear lifecycle and versioned cleanup for both projects and repositories. One small improvement would be to reuse the already capturedwdekDetailsinstead of callingc.wdekDetails()again in the error paths, e.g.:- encryptionStorageManager.removeWdek(c.projectName(), Project.REPO_DOGMA, - c.wdekDetails().dekVersion()); + encryptionStorageManager.removeWdek(c.projectName(), Project.REPO_DOGMA, + wdekDetails.dekVersion());and similarly in
createRepository. This avoids any accidental divergence if the command implementation ever changes and makes the code slightly easier to follow.Also applies to: 310-336
403-419: Rotation helpers mirror existing patterns; consider harmonizing error messages.The
rotateWdekandrotateSessionMasterKeyhelpers delegate synchronously toEncryptionStorageManagerand use guards (enabled()vsencryptSessionCookie()) that match their respective concerns, with completed futures for the command pipeline. For consistency with other guards (e.g.,MigrateToEncryptedRepositoryandCreateSessionMasterKeyCommand), you might want to include the command in theIllegalStateExceptionmessage forrotateSessionMasterKeyas well:- if (!encryptionStorageManager.encryptSessionCookie()) { - throw new IllegalStateException("Session cookie encryption is disabled."); - } + if (!encryptionStorageManager.encryptSessionCookie()) { + throw new IllegalStateException("Session cookie encryption is disabled. command: " + c); + }server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/RocksDBStorage.java (1)
71-83: CloseColumnFamilyOptionson the normal path to avoid native leaks.
ColumnFamilyOptionsobjects are created per column family in the constructor and are closed only in the early-error paths. On the normal path, they outlive the constructor but are never closed inclose(), unlikebloomFilter,rocksDb, anddbOptions. SinceColumnFamilyOptionsis a RocksDB native resource, it should also be released:
- Store the
cfNameToOptionsmap (or its values) in a field.- In
close(), iterate and close those options alongside the other resources.Example sketch:
- private final DBOptions dbOptions; + private final DBOptions dbOptions; + private final Map<String, ColumnFamilyOptions> columnFamilyOptionsMap; @@ - final Map<String, ColumnFamilyOptions> cfNameToOptions = new HashMap<>(); + final Map<String, ColumnFamilyOptions> cfNameToOptions = new HashMap<>(); @@ - columnFamilyHandlesMap = handlesMapBuilder.build(); + columnFamilyHandlesMap = handlesMapBuilder.build(); + columnFamilyOptionsMap = ImmutableMap.copyOf(cfNameToOptions); @@ void close() { - bloomFilter.close(); - columnFamilyHandlesMap.values().forEach(RocksDBStorage::closeSilently); - closeSilently(rocksDb); - closeSilently(dbOptions); + bloomFilter.close(); + columnFamilyHandlesMap.values().forEach(RocksDBStorage::closeSilently); + if (columnFamilyOptionsMap != null) { + columnFamilyOptionsMap.values().forEach(RocksDBStorage::closeSilently); + } + closeSilently(rocksDb); + closeSilently(dbOptions); }Adjust visibility and nullability as needed, but the key point is to ensure the options are eventually closed.
Also applies to: 163-168
server/src/test/java/com/linecorp/centraldogma/server/storage/encryption/DefaultEncryptionStorageManagerTest.java (1)
52-79: Tests exercise WDEK generation, storage, and versioned removal well.The updated tests correctly:
- Pass a
kekIdintoDefaultEncryptionStorageManager.- Use
WrappedDekDetailsbuilt fromgenerateWdek().join()andkekId().- Decode the wrapped DEK to assert the expected prefix and AES-256 key length.
- Verify first-time store, duplicate-store failure, successful retrieval, and versioned removal, plus the no-op behavior when removing a non-existent WDEK.
You might optionally extract a small helper like
newWdekDetails()to remove the duplicatednew WrappedDekDetails(...)construction, but functionally the tests look strong.Also applies to: 83-116
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
server/src/main/java/com/linecorp/centraldogma/server/command/CreateProjectCommand.java(5 hunks)server/src/main/java/com/linecorp/centraldogma/server/command/CreateRepositoryCommand.java(5 hunks)server/src/main/java/com/linecorp/centraldogma/server/command/StandaloneCommandExecutor.java(7 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionUtil.java(4 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/admin/service/DefaultLogoutService.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/DefaultEncryptionStorageManager.java(2 hunks)server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/EncryptionStorageManager.java(6 hunks)server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/NoopEncryptionStorageManager.java(4 hunks)server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/RepositoryEncryptionStorage.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/RocksDBStorage.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java(1 hunks)server/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/EncryptedGitRepositoryRemoveTest.java(3 hunks)server/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryTest.java(2 hunks)server/src/test/java/com/linecorp/centraldogma/server/storage/encryption/DefaultEncryptionStorageManagerTest.java(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- server/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/EncryptedGitRepositoryRemoveTest.java
- server/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryTest.java
- server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/RepositoryEncryptionStorage.java
🧰 Additional context used
🧬 Code graph analysis (5)
server/src/main/java/com/linecorp/centraldogma/server/command/StandaloneCommandExecutor.java (1)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java (1)
WrappedDekDetails(30-153)
server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionUtil.java (1)
server/src/main/java/com/linecorp/centraldogma/server/auth/SessionKey.java (1)
SessionKey(41-136)
server/src/main/java/com/linecorp/centraldogma/server/command/CreateProjectCommand.java (1)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java (1)
WrappedDekDetails(30-153)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/DefaultEncryptionStorageManager.java (2)
server/src/main/java/com/linecorp/centraldogma/server/auth/SessionKey.java (1)
SessionKey(41-136)server/src/main/java/com/linecorp/centraldogma/server/auth/SessionMasterKey.java (1)
SessionMasterKey(28-107)
server/src/main/java/com/linecorp/centraldogma/server/command/CreateRepositoryCommand.java (1)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java (1)
WrappedDekDetails(30-153)
⏰ 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). (9)
- GitHub Check: build-ubuntu-latest-jdk-17-min-java-11
- GitHub Check: build-ubuntu-latest-jdk-17-coverage
- GitHub Check: flaky-tests
- GitHub Check: build-ubuntu-latest-jdk-11
- GitHub Check: build-ubuntu-latest-jdk-21-snapshot
- GitHub Check: docker-build
- GitHub Check: build-windows-latest-jdk-21
- GitHub Check: build-macos-latest-jdk-21
- GitHub Check: lint
🔇 Additional comments (12)
server/src/main/java/com/linecorp/centraldogma/server/internal/admin/service/DefaultLogoutService.java (1)
71-111: Async composition is now correct.The previous critical issues have been properly addressed:
- Line 71 correctly uses
thenComposeinstead ofthenApply- Lines 81-85 use proper runtime null checks instead of assertions
- All async branches consistently return
CompletionStage<HttpResponse>via eitherUnmodifiableFuture.completedFuture()or nested composition chainsThe logout flow correctly handles three scenarios: (1) no session, (2) JWT-based session without sessionId, and (3) server-side session with sessionId.
server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionUtil.java (2)
154-164: LGTM! Critical security improvement.The explicit signature verification using
sessionKey.verifier()before extracting claims is essential for preventing signature bypass vulnerabilities. The error handling appropriately returns null for both verification failures and exceptions.
225-225: LGTM!Making
findSessionCookiepublic allows external components to access session cookies, which aligns with the key rotation functionality introduced in this PR.server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/NoopEncryptionStorageManager.java (2)
18-27: No-op WDEK and session-key surface matches the interface.The added imports and the
wdeks(),storeWdek(...),rotateWdek(...),removeWdek(...), andaddSessionKeyListener(...)methods are implemented as true no-ops/empty results, which is appropriate for a disabledEncryptionStorageManagerand keeps the implementation aligned with the interface evolution.Also applies to: 94-97, 112-121, 169-172
73-77: Nullable session master key and session key methods rely on upstream guards.
getCurrentSessionMasterKey()andgetSessionKey(int)returningnull/completednullis consistent withenabled() == falseandencryptSessionCookie() == falsehere. Just ensure all call sites continue to gate on those flags before dereferencing the values so the noop manager can never cause NPEs at runtime.Also applies to: 84-92
server/src/main/java/com/linecorp/centraldogma/server/command/StandaloneCommandExecutor.java (3)
39-40:WrappedDekDetailsimport is consistent with new encryption API.The added import cleanly reflects the move to the richer WDEK metadata type and matches its usage below.
210-216: New rotation commands are correctly dispatched.
RotateWdekCommandandRotateSessionMasterKeyCommandare integrated intodoExecute0in the same style as existing commands, delegating to dedicated helpers and returningCompletableFuture<Void>, so the replication/execution model stays consistent.
359-382: Migration WDEK storage/cleanup is aligned with versioned keys.In
migrateToEncryptedRepository, storing the WDEK viac.wdekDetails()and removing it with the specificdekVersion()on failure mirrors the project/repository creation flow and is safely guarded by the earlierenabled()check indoExecute0. The error handling and logging behavior look appropriate.server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/RocksDBStorage.java (1)
51-61: Column family naming and handle validation are solid.The constants for column family names, the single
ALL_COLUMN_FAMILY_NAMESlist, and the post-open validation that every expected name has a handle give good safety around the RocksDB schema. Building the handle map by decodinghandle.getName()and failing fast if any are missing is a robust pattern.Also applies to: 96-117
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java (1)
30-152: Immutable WDEK details model with redacted logging looks good.
WrappedDekDetailsis a well-structured immutable value object: constructors validate inputs (including positivedekVersion), JSON properties are explicitly annotated, equality/hashCode cover all fields, andtoString()redacts the sensitivewrappedDekwhile still exposing useful metadata. This fits the new WDEK-centric APIs cleanly.server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/DefaultEncryptionStorageManager.java (1)
38-60: Constructor wiring and delegation to storage components look solidThe new constructor wiring (
kekId, singleKeyWrapper, sharedRocksDBStorage) and the added methods that delegate toSessionKeyStorage/RepositoryEncryptionStorageare cohesive and keepDefaultEncryptionStorageManageras a thin coordinator. I don’t see functional or lifecycle issues in these changes from this class’s perspective.Also applies to: 77-196
server/src/main/java/com/linecorp/centraldogma/server/command/CreateRepositoryCommand.java (1)
43-60: Confirm backward compatibility strategy forwdekDetailsproperty changeThe invariant checks and equality semantics are correct. However, the search found no
@JsonAlias,@JsonAnySetter, or custom deserializer in the Command classes, and no evidence of a deserialization bridge for thewdek→wdekDetailsproperty name/shape change.If serialized
CreateRepositoryCommandorCreateProjectCommandinstances with the old"wdek"property exist in any deployment, they will fail to deserialize with the current@JsonProperty("wdekDetails")constructor.Confirm whether:
- This is the first release with WDEK support (no legacy serialized commands in the wild), or
- A migration layer or deserialization compatibility path exists for legacy payloads.
If the latter is needed, a simple annotation change won't suffice; custom logic will be required.
server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionUtil.java
Show resolved
Hide resolved
.../main/java/com/linecorp/centraldogma/server/storage/encryption/EncryptionStorageManager.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/EncryptionStorageManager.java (3)
88-91: Clarify what the returned String represents.The Javadoc doesn't explain what the
StringinCompletableFuture<String>represents. Based on the PR context, this appears to be the version of the newly generated WDEK.Consider updating the Javadoc:
/** - * Generates a new data encryption key (DEK) and wraps it. + * Generates a new data encryption key (DEK), wraps it, and returns the new WDEK version. */ CompletableFuture<String> generateWdek();
93-96: Document the version parameter.The Javadoc doesn't explain the purpose of the
versionparameter.Consider updating the Javadoc:
/** - * Generates a new session master key. + * Generates a new session master key for the specified version. + * + * @param version the version number for the new session master key */ CompletableFuture<SessionMasterKey> generateSessionMasterKey(int version);
151-154: Include the version parameter in the Javadoc.The method signature includes a
versionparameter, but the Javadoc only mentions "project and repository". This was flagged in a previous review.Consider updating the Javadoc:
/** - * Removes the wrapped data encryption key (WDEK) for the specified project and repository. + * Removes the wrapped data encryption key (WDEK) for the specified project, repository, and version. */ void removeWdek(String projectName, String repoName, int version);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/src/main/java/com/linecorp/centraldogma/server/command/CreateProjectCommand.java(5 hunks)server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/EncryptionStorageManager.java(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/src/main/java/com/linecorp/centraldogma/server/command/CreateProjectCommand.java
⏰ 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). (9)
- GitHub Check: docker-build
- GitHub Check: build-ubuntu-latest-jdk-17-min-java-11
- GitHub Check: flaky-tests
- GitHub Check: build-ubuntu-latest-jdk-11
- GitHub Check: build-macos-latest-jdk-21
- GitHub Check: build-ubuntu-latest-jdk-21-snapshot
- GitHub Check: build-ubuntu-latest-jdk-17-coverage
- GitHub Check: build-windows-latest-jdk-21
- GitHub Check: lint
🔇 Additional comments (1)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/EncryptionStorageManager.java (1)
139-149: Great fix! The signature inconsistency has been addressed.The previous review concern about
storeWdekhaving duplicate parameters (explicitprojectName,repoNamealongsideWrappedDekDetails) has been resolved. The signature now only takesWrappedDekDetails, eliminating the risk of mismatched metadata.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/EncryptedGitRepositoryRemoveTest.java (2)
125-135: Clarify the "current" key concept in comments.The WDEK rotation logic is correct. However, the comment on line 132 references a "current" key without explanation. Consider clarifying that "current" is a pointer/metadata entry to the active DEK version, distinct from the versioned keys themselves.
For example:
- wdekSize = 3; // version 1, version 2, and current + wdekSize = 3; // version 1, version 2, and current pointer (points to v2)This helps future maintainers understand that the storage maintains both historical versions (for decrypting old data) and a current pointer (for encrypting new data).
145-145: Consider clarifying the "current" key breakdown.Similar to the comment on line 132, consider clarifying what "current" represents here for improved readability:
- wdekSize = 3 + 2; // first repo has 3 keys (v1, v2, current), second repo has 2 keys (v1, current) + wdekSize = 3 + 2; // first repo: v1, v2, current→v2; second repo: v1, current→v1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/EncryptedGitRepositoryRemoveTest.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/EncryptedGitRepositoryRemoveTest.java (1)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java (1)
WrappedDekDetails(30-153)
🔇 Additional comments (4)
server/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/EncryptedGitRepositoryRemoveTest.java (4)
67-67: LGTM: KEK ID parameter added.The test correctly initializes the EncryptionStorageManager with a kekId, which is then retrieved consistently via
encryptionStorageManager.kekId()when constructing WrappedDekDetails instances.
73-77: LGTM: Initial WDEK setup correctly updated.The test properly generates a WDEK and constructs a WrappedDekDetails object with version 1 for the initial key, consistent with the new API.
137-142: LGTM: Second repository correctly initialized.The test correctly creates a second repository with its own WDEK starting at version 1, confirming that WDEK versions are per-repository rather than global.
109-175: LGTM: Comprehensive test coverage of WDEK lifecycle.The test effectively validates:
- Initial WDEK generation and storage
- WDEK rotation with version increment
- Per-repository WDEK management
- Proper cleanup on repository deletion
- Retention of historical WDEK versions (necessary for decrypting existing data)
The assertions correctly track the expected state at each stage of the test.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionCookieHandler.java (1)
57-71: HardensessionKeylifecycle against unexpected nulls (follow-up on earlier review)The encrypted-cookie path assumes
sessionKeyis never null:
- In the constructor (Lines 63–67),
sessionKeyis set fromencryptionStorageManager.getCurrentSessionKey().join()and updated via a listener, but neither result is checked for null.- In
getSessionInfo(Lines 74–88), you assertsessionKey != null, but asserts are typically disabled in production; a null value here would lead to an NPE when callingsessionKey.version()or later when decrypting.If
getCurrentSessionKey()or the listener ever yieldnull(misconfiguration, partial initialization, or a future refactor), the failure would occur on the first request instead of failing fast at startup.You can make this safer by enforcing non-null at initialization and in the listener, so any violation surfaces immediately:
public SessionCookieHandler(BooleanSupplier sessionPropagatorWritableChecker, boolean tlsEnabled, EncryptionStorageManager encryptionStorageManager) { this.sessionPropagatorWritableChecker = requireNonNull(sessionPropagatorWritableChecker, "sessionPropagatorWritableChecker"); this.encryptionStorageManager = requireNonNull(encryptionStorageManager, "encryptionStorageManager"); sessionCookieName = sessionCookieName(tlsEnabled, encryptionStorageManager.encryptSessionCookie()); if (encryptionStorageManager.encryptSessionCookie()) { - sessionKey = encryptionStorageManager.getCurrentSessionKey().join(); - encryptionStorageManager.addSessionKeyListener(sessionKey -> this.sessionKey = sessionKey); + sessionKey = requireNonNull(encryptionStorageManager.getCurrentSessionKey().join(), + "initial session key must not be null"); + encryptionStorageManager.addSessionKeyListener( + newSessionKey -> this.sessionKey = requireNonNull(newSessionKey, "sessionKey")); jwtClaimsVerifier = new DefaultJWTClaimsVerifier<>(new Builder().issuer("dogma").build(), ImmutableSet.of("exp")); } else { sessionKey = null; jwtClaimsVerifier = null; } } public CompletableFuture<SessionInfo> getSessionInfo(ServiceRequestContext ctx) { if (encryptionStorageManager.encryptSessionCookie()) { final SessionKey sessionKey = this.sessionKey; - assert sessionKey != null; + assert sessionKey != null; final Cookie sessionCookie = findSessionCookie(ctx, sessionCookieName); if (sessionCookie == null) { return UnmodifiableFuture.completedFuture(null); } final int sessionKeyVersion = getSessionKeyVersion(ctx, sessionCookie.value()); if (sessionKeyVersion <= 0) { return UnmodifiableFuture.completedFuture(null); } if (sessionKeyVersion == sessionKey.version()) { return CompletableFuture.completedFuture(getSessionInfo(ctx, sessionKey)); } return encryptionStorageManager.getSessionKey(sessionKeyVersion).thenApply(fetchedSessionKey -> { return getSessionInfo(ctx, fetchedSessionKey); }); } final String sessionId = getSessionIdFromCookie(ctx, sessionCookieName); return sessionId != null ? CompletableFuture.completedFuture(new SessionInfo(sessionId, null, null)) : UnmodifiableFuture.completedFuture(null); }With this change, any unexpected null session key is caught where it originates, and the existing
assertstays as an additional sanity check.Also applies to: 74-98
🧹 Nitpick comments (3)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/KeyManagementService.java (2)
61-83: Clarify behavior forreencrypt=trueand consider user-friendlier failureThe rotation path for
reencrypt=true(Line 67) intentionally throwsUnsupportedOperationException, which will surface as a 5xx. If this flag is exposed to clients, consider failing with a clearer client-facing error (e.g., 400/501) and message so sysadmins don’t misinterpret it as a server bug.Example change:
- if (reencrypt) { - // TODO(minwoox): Implement re-encryption logic. - throw new UnsupportedOperationException(); - } else { + if (reencrypt) { + // TODO(minwoox): Implement re-encryption logic. + throw new IllegalArgumentException("reencrypt=true is not supported yet."); + } else {The concurrency comment around
newVersion = currentDek.version() + 1aligns with the documented storage invariant.
104-116: Avoid blocking withjoin()inrotateSessionMasterKeyfor consistency
rotateSessionMasterKey()currently blocks ongenerateSessionMasterKey(...).join()(Lines 113–114) even though the method is already async and annotated@Blocking. For consistency withrotateWdekand cleaner error propagation, consider composing the returnedCompletableFutureinstead of joining:- final SessionMasterKey newSessionMasterKey = - encryptionStorageManager.generateSessionMasterKey(currentSessionMasterKey.version() + 1).join(); - return execute(Command.rotateSessionMasterKey(newSessionMasterKey)); + final int nextVersion = currentSessionMasterKey.version() + 1; + return encryptionStorageManager.generateSessionMasterKey(nextVersion) + .thenCompose(newSessionMasterKey -> + execute(Command.rotateSessionMasterKey(newSessionMasterKey)));This keeps the method fully asynchronous and avoids wrapping errors in
CompletionExceptionviajoin().server/src/test/java/com/linecorp/centraldogma/server/internal/api/sysadmin/KeyManagementServiceTest.java (1)
69-98: Strong end-to-end tests; a few minor robustness/readability tweaksThe extension setup and both tests provide good coverage of WDEK rotation and session master key rotation flows. A couple of minor, optional refinements could make failures easier to diagnose and the code clearer:
Assert on repository creation response (Lines 105–112)
createRepository(...)currently returns the aggregated response but doesn’t validate it. Adding an assertion ensures setup failures are caught early in the test:private static AggregatedHttpResponse createRepository(WebClient client, String repoName) { final RequestHeaders headers = RequestHeaders.of(HttpMethod.POST, PROJECTS_PREFIX + "/foo" + REPOS, HttpHeaderNames.CONTENT_TYPE, MediaType.JSON); final String body = "{\"name\": \"" + repoName + "\"," + " \"encrypt\": true}";
return client.execute(headers, body).aggregate().join();
final AggregatedHttpResponse response = client.execute(headers, body).aggregate().join();assertThat(response.status()).isEqualTo(HttpStatus.CREATED);return response;(Adjust expected status if the API uses `200 OK` instead of `201 Created`.)
Check creation timestamp ordering, not just inequality (Lines 172–214)
InrotateSessionMasterKey, you assert thecreationstrings differ; asserting ordering asInstantmakes the intent clearer:
final Instant initialCreation = Instant.parse(initialDetails.creation()); // Rotate the session master key final AggregatedHttpResponse rotateResponse = dogma.httpClient().execute(RequestHeaders.of(HttpMethod.POST, API_V1_PATH_PREFIX + "/masterkeys/session/rotate")) .aggregate().join(); assertThat(rotateResponse.status()).isSameAs(HttpStatus.NO_CONTENT); // Get the session master key details after rotation final ResponseEntity<SessionMasterKeyDto> response2 = dogma.httpClient().blocking().prepare().get(API_V1_PATH_PREFIX + "/masterkeys/session") .asJson(SessionMasterKeyDto.class) .execute(); assertThat(response2.status()).isSameAs(HttpStatus.OK); final SessionMasterKeyDto rotatedDetails = response2.content(); // Verify the version has been incremented assertThat(rotatedDetails.version()).isEqualTo(2); assertThat(rotatedDetails.kekId()).isEqualTo("kekId"); assertThat(rotatedDetails.creation()).isNotNull();
// The creation timestamp should be different (later) for the new keyassertThat(rotatedDetails.creation()).isNotEqualTo(initialDetails.creation());
// The creation timestamp should be later for the new keyfinal Instant rotatedCreation = Instant.parse(rotatedDetails.creation());assertThat(rotatedCreation).isAfter(initialCreation);You’d need `import java.time.Instant;` at the top.
Clarify naming in
WrappedDekhelper (Lines 277–295)
For readability, consider renamingprotoprojectName:
private final String pro;
private final String projectName; private final String repoName; private final int dekVersion; private final String kekId;
WrappedDek(String pro, String repoName, int dekVersion, String kekId) {this.pro = pro;
WrappedDek(String projectName, String repoName, int dekVersion, String kekId) {this.projectName = projectName; this.repoName = repoName; this.dekVersion = dekVersion; this.kekId = kekId; } @Override public int hashCode() {
return Objects.hash(pro, repoName, dekVersion, kekId);
return Objects.hash(projectName, repoName, dekVersion, kekId); } @Override public boolean equals(Object obj) { if (this == obj) { return true; } if (obj == null || getClass() != obj.getClass()) { return false; } final WrappedDek other = (WrappedDek) obj;
return pro.equals(other.pro) &&
return projectName.equals(other.projectName) && repoName.equals(other.repoName) && dekVersion == other.dekVersion && kekId.equals(other.kekId); }All of the above are optional; the existing tests are already valuable as-is.
Also applies to: 105-112, 168-248, 277-295
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/src/main/java/com/linecorp/centraldogma/server/command/StandaloneCommandExecutor.java(7 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionCookieHandler.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/KeyManagementService.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/SessionMasterKeyDto.java(1 hunks)server/src/test/java/com/linecorp/centraldogma/server/internal/api/sysadmin/KeyManagementServiceTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
server/src/main/java/com/linecorp/centraldogma/server/command/StandaloneCommandExecutor.java (1)
server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java (1)
WrappedDekDetails(30-153)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/KeyManagementService.java (3)
server/src/main/java/com/linecorp/centraldogma/server/auth/SessionMasterKey.java (1)
SessionMasterKey(28-107)server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/SecretKeyWithVersion.java (1)
SecretKeyWithVersion(28-63)server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java (1)
WrappedDekDetails(30-153)
server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionCookieHandler.java (2)
server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionUtil.java (1)
SessionUtil(59-313)server/src/main/java/com/linecorp/centraldogma/server/auth/SessionKey.java (1)
SessionKey(41-136)
server/src/test/java/com/linecorp/centraldogma/server/internal/api/sysadmin/KeyManagementServiceTest.java (11)
common/src/main/java/com/linecorp/centraldogma/internal/api/v1/HttpApiV1Constants.java (1)
HttpApiV1Constants(22-48)server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/SessionUtil.java (1)
SessionUtil(59-313)server/src/main/java/com/linecorp/centraldogma/server/internal/storage/MigratingMetaToDogmaRepositoryService.java (1)
MigratingMetaToDogmaRepositoryService(53-226)testing-internal/src/main/java/com/linecorp/centraldogma/testing/internal/auth/TestAuthMessageUtil.java (1)
TestAuthMessageUtil(44-146)server/src/main/java/com/linecorp/centraldogma/server/EncryptionAtRestConfig.java (1)
EncryptionAtRestConfig(30-86)server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/rocksdb/EncryptionGitStorage.java (1)
EncryptionGitStorage(58-398)server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/rocksdb/GitObjectMetadata.java (1)
GitObjectMetadata(29-101)server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/rocksdb/RocksDbRepository.java (1)
RocksDbRepository(36-129)server/src/main/java/com/linecorp/centraldogma/server/storage/encryption/WrappedDekDetails.java (1)
WrappedDekDetails(30-153)server/src/main/java/com/linecorp/centraldogma/server/storage/project/InternalProjectInitializer.java (1)
InternalProjectInitializer(66-314)testing/junit/src/main/java/com/linecorp/centraldogma/testing/junit/CentralDogmaExtension.java (1)
CentralDogmaExtension(60-278)
⏰ 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). (9)
- GitHub Check: build-ubuntu-latest-jdk-17-min-java-11
- GitHub Check: docker-build
- GitHub Check: build-ubuntu-latest-jdk-11
- GitHub Check: build-windows-latest-jdk-21
- GitHub Check: build-macos-latest-jdk-21
- GitHub Check: build-ubuntu-latest-jdk-17-coverage
- GitHub Check: build-ubuntu-latest-jdk-21-snapshot
- GitHub Check: flaky-tests
- GitHub Check: lint
🔇 Additional comments (7)
server/src/main/java/com/linecorp/centraldogma/server/command/StandaloneCommandExecutor.java (4)
39-39: LGTM!The import of
WrappedDekDetailssupports the refactoring from raw byte arrays to a structured data model with versioning and KEK ID tracking.
210-217: LGTM!The command dispatch logic follows the established pattern in this executor and properly routes the new rotation commands to their respective handler methods.
247-250: LGTM!The refactoring from raw byte arrays to
WrappedDekDetailsprovides a cleaner API with proper versioning and KEK ID tracking. The changes are applied consistently across project creation, repository creation, and encryption migration flows.Also applies to: 312-316, 367-367
258-259: LGTM!The cleanup paths correctly pass
dekVersiontoremoveWdek, ensuring that the specific WDEK version stored during the operation is removed when failures occur. This maintains storage consistency.Also applies to: 324-325, 372-373
server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/SessionMasterKeyDto.java (1)
29-78: SessionMasterKeyDto validation and surface look solidConstructor enforces positive version and non-null
kekId/creation, and the DTO exposes only non-sensitive metadata.toString()is safe given there’s no key material or salt here. No changes needed.server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/KeyManagementService.java (2)
45-50: Constructor invariants are appropriateRequiring a non-null, enabled
EncryptionStorageManagerat construction time is a good guardrail so this sysadmin API is only exposed when encryption-at-rest is configured.
89-99: Session master key metadata exposure is minimal and consistent
getSessionMasterKeyDetails()(Lines 89–99) returns only version, KEK ID, and creation timestamp, matchingSessionMasterKeyDtoand avoiding exposure of wrapped key material or salt. TheencryptSessionCookie()guard is also appropriate. Looks good.
server/src/main/java/com/linecorp/centraldogma/server/command/StandaloneCommandExecutor.java
Show resolved
Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/command/StandaloneCommandExecutor.java
Show resolved
Hide resolved
jrhee17
left a comment
jrhee17
left a comment
There was a problem hiding this comment.
Understood that with the current changes, the rotated key is used to encrypt new sessions/objects.
Conversely, each session/object is tagged with the master dek version and queries the appropriate dek when unencrypting.
| /** | ||
| * Re-wraps the given wrapped data encryption key (WDEK). | ||
| */ | ||
| CompletableFuture<String> rewrap(String wdek, String oldKekId, String newKekId); |
There was a problem hiding this comment.
Note) No issue with this API, but it seems like it isn't called from anywhere
| * @param wrappedMasterKey the wrapped session master key | ||
| * @param version the version of the session master key | ||
| * @param salt the salt used to derive session keys from the master key. It's encoded in base64. | ||
| * @param kekId the key encryption key (KEK) ID used to wrap the |
There was a problem hiding this comment.
nit: an incomplete sentence?
| private final int version; | ||
| private final String salt; | ||
| private final String kekId; | ||
| private final String creation; |
There was a problem hiding this comment.
How about using Instant for type-safety?
| .add("projectName", projectName); | ||
| if (wdek != null) { | ||
| toStringHelper.add("wdek", "[***]"); | ||
| if (wdekDetails != null) { |
There was a problem hiding this comment.
nit: A null value can be set to .add()
| final String csrfTokenFromSignedJwt = csrfTokenFromSignedJwt(signedJwt); | ||
| return new SessionInfo(null, subject, csrfTokenFromSignedJwt); | ||
|
|
||
| if (sessionKeyVersion == sessionKey.version()) { |
There was a problem hiding this comment.
I understand that a session issued by the new key may not be able to access servers whose key has not been updated yet while the new key is being replicated.
Technically, we could specify the exact time when the sessionMasterKey should take effect, but since key rotation rarely happens and replication completes within a short period, I don’t think it’s a significant issue.
3 participants
Motivation:
Central Dogma needs to support key rotation for both wrapped data encryption keys (WDEKs) and session master keys to maintain security best practices.
Modifications:
KeyManagementServicetoKeyWrapperKeyManagementServicefor key APIsDefaultEncryptionStorageManagerintorocksDbStorage,sessionKeyStorageandrepositoryEncryptionStorage.Result:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes & Improvements