efs: add encrypted file system store - #1
Conversation
Add a keystore implementation, that wraps a file system store, but encrypts the files. Fixes: minio#392 Signed-off-by: Sascha Wolke <dersascha@users.noreply.github.com>
WalkthroughAdds an encrypted filesystem-backed keystore (EFS) implemented at internal/keystore/efs/efs.go, integrates EncryptedFS into configuration and connection codepaths, exposes a Dir() accessor on the FS store, and adds tests and YAML testdata exercising EFS behavior and error cases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant EFS as "EFS Store"
participant FS as "FS Store"
participant KeyFile as "Key File"
rect rgb(240,245,255)
note over App,EFS: Initialization
App->>EFS: NewStore(keyPath, keyCipher, dir)
EFS->>KeyFile: loadMasterKey()
KeyFile-->>EFS: master key (validated)
EFS->>FS: init underlying store (ensure dirs)
EFS-->>App: Store
end
rect rgb(245,255,245)
note over App,EFS: Create (encrypt → write)
App->>EFS: Create(name, value)
EFS->>EFS: Encrypt(value, context=name, key)
EFS->>FS: Write(name, ciphertext)
FS-->>EFS: ok/error
EFS-->>App: ok/error
end
rect rgb(255,250,240)
note over App,EFS: Get (read → decrypt)
App->>EFS: Get(name)
EFS->>FS: Read(name)
FS-->>EFS: ciphertext
EFS->>EFS: Decrypt(ciphertext, context=name, key)
EFS-->>App: value/error
end
sequenceDiagram
autonumber
participant App
participant EFS as "EFS Store"
participant FS as "FS Store"
note over App,EFS: List/Delete delegate to FS
App->>EFS: List(prefix, n)
EFS->>FS: List(prefix, n)
FS-->>EFS: names, token
EFS-->>App: names, token
App->>EFS: Delete(name)
EFS->>FS: Delete(name)
FS-->>EFS: ok/error
EFS-->>App: ok/error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
kesconf/testdata/efs.yml (1)
9-10: Fix YAML formatting: remove extra spaces after colons.Lines 9-10 have extra spaces after the colons. While this doesn't break YAML parsing, it violates YAML style conventions.
Apply this diff:
tls: - key: ./server.key - cert: ./server.cert + key: ./server.key + cert: ./server.certinternal/keystore/efs/efs.go (1)
54-85: Redundant file close operation.The file is closed both via
defer file.Close()(line 62) and explicitly at line 69. While harmless (Close is idempotent), the explicit close is unnecessary.Consider removing the explicit close for cleaner code:
if err != nil { return crypto.SecretKey{}, err } - if err = file.Close(); err != nil { - return crypto.SecretKey{}, err - } if len(value) != crypto.SecretKeySize {kesconf/efs_test.go (1)
24-57: Master key file cleanup missing.The master key file created at line 32 is never cleaned up after the test completes. Consider adding a defer statement to remove it.
Apply this diff:
masterKeyPath := filepath.Join(*EncryptedFSPath, "test-master-key") masterKeyCipher := "AES256" if err := os.WriteFile(masterKeyPath, []byte(masterKey), 0o644); err != nil { t.Fatalf("Failed to write master key into test dir") } + defer os.Remove(masterKeyPath) config := kesconf.EncryptedFSKeyStore{
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
internal/keystore/efs/efs.go(1 hunks)internal/keystore/fs/fs.go(1 hunks)kesconf/config.go(2 hunks)kesconf/config_test.go(1 hunks)kesconf/efs_test.go(1 hunks)kesconf/file.go(2 hunks)kesconf/testdata/efs.yml(1 hunks)server-config.yaml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
kesconf/file.go (3)
kesconf/config_test.go (2)
MasterKeyPath(36-36)MasterKeyCipher(37-37)internal/keystore/efs/efs.go (1)
NewStore(35-47)internal/keystore/fs/fs.go (1)
NewStore(34-48)
kesconf/config_test.go (1)
kesconf/file.go (3)
ReadFile(38-50)KeyStore(374-378)EncryptedFSKeyStore(402-413)
kesconf/config.go (2)
kesconf/config_test.go (2)
MasterKeyPath(36-36)MasterKeyCipher(37-37)kesconf/file.go (2)
KeyStore(374-378)EncryptedFSKeyStore(402-413)
internal/keystore/fs/fs.go (1)
internal/keystore/efs/efs.go (1)
Store(92-95)
internal/keystore/efs/efs.go (4)
internal/keystore/fs/fs.go (3)
NewStore(34-48)Store(55-58)MaxSize(105-105)internal/crypto/key.go (4)
SecretKeySize(32-32)ParseSecretKeyType(58-67)ChaCha20(45-45)NewSecretKey(216-226)internal/fips/api.go (1)
Enabled(15-15)keystore.go (1)
KeyStoreState(66-68)
kesconf/efs_test.go (2)
kesconf/file.go (2)
EncryptedFSKeyStore(402-413)KeyStore(374-378)kesconf/edge_test.go (1)
RandString(153-159)
🪛 YAMLlint (1.37.1)
kesconf/testdata/efs.yml
[warning] 9-9: too many spaces after colon
(colons)
[warning] 10-10: too many spaces after colon
(colons)
🔇 Additional comments (18)
internal/keystore/fs/fs.go (1)
62-63: LGTM! Clean accessor for directory inspection.The
Dir()method provides safe read-only access to the underlying directory path, which is useful for introspection by wrapper types like the encrypted filesystem store.kesconf/config_test.go (1)
33-60: LGTM! Test follows established patterns and validates all key fields.The test correctly mirrors the existing
TestReadServerConfigYAML_FSpattern, validates the type assertion, and checks all three configuration fields (MasterKeyPath, MasterKeyCipher, Path) with clear error messages.kesconf/file.go (2)
25-25: LGTM! Import added for encrypted filesystem keystore.The import for the
internal/keystore/efspackage is correctly placed and necessary for the newEncryptedFSKeyStorefunctionality.
415-418: Resolved: validation already handled
fs.NewStorecreates or verifies the directory and errors if it’s not a directory, andloadMasterKeychecks that the key file exists, is readable, matches the expected size, and that the cipher is supported (including FIPS restrictions).kesconf/config.go (1)
79-83: LGTM! Configuration struct follows established patterns.The EncryptedFS configuration struct is well-defined with appropriate YAML tags and follows the same pattern as other keystore configurations in this file.
internal/keystore/efs/efs.go (5)
35-47: LGTM! Constructor follows best practices.The NewStore constructor correctly initializes the filesystem store first, then loads the master key, with proper error propagation throughout.
92-95: LGTM! Store struct is well-designed.The Store structure cleanly encapsulates the encryption key and the underlying filesystem store.
97-105: LGTM! Informational methods correctly delegate.The String and Status methods appropriately delegate to the underlying filesystem store.
111-137: LGTM! Encryption with authenticated context prevents key swapping.The Create and Get methods correctly use an authenticated encryption context (
name=%s) that binds the ciphertext to the key name, preventing attackers from copying encrypted keys between files. This is properly tested in the EncryptionContext test.
142-165: LGTM! Delegation methods are correctly implemented.Delete, List, and Close appropriately delegate to the underlying filesystem store since these operations work with filenames/metadata rather than encrypted content.
kesconf/efs_test.go (8)
60-115: LGTM! Comprehensive CRUD test coverage.The efsTestList function provides thorough testing of all CRUD operations with proper validation of empty states before and after operations.
118-140: LGTM! Backward compatibility test ensures format stability.This test validates that the encryption format remains stable by decrypting a known ciphertext, which is critical for ensuring existing encrypted keys remain readable after code changes.
143-162: LGTM! Basic encryption verification test.This test provides a sanity check that keys are actually encrypted on disk rather than stored in plaintext.
165-191: LGTM! Critical security test for authenticated encryption context.This test validates that the encryption context (key name binding) prevents key swapping attacks by verifying that copying an encrypted key to a different filename makes it unreadable.
194-214: LGTM! Negative test for missing master key.This test correctly validates that initialization fails when the master key file is missing.
217-276: LGTM! Invalid key length tests with proper cleanup.These tests correctly validate that both undersized and oversized master keys are rejected during initialization, with proper resource cleanup.
279-307: LGTM! Unknown cipher validation test.This test correctly validates that an unknown cipher type is rejected during initialization, with proper cleanup.
50-51: Ensure correct package for accessingranStringLength.
ranStringLengthis defined inedge_test.goas an unexported constant, so it’s only visible ifefs_test.godeclarespackage kesconf. Confirm the package declaration at the top ofefs_test.go; if it’spackage kesconf_test, either switch it topackage kesconfor defineranStringLengthlocally.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Eliah Rusin <hello@franchb.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
kesconf/file.go (1)
398-413: Grammar issue resolved; consider refining the description.The grammar issue flagged in the previous review has been correctly fixed (line 401 now uses "An"). However, the struct comment on line 399 describes this as "a simple filesystem keystore," which may be misleading since
EncryptedFSKeyStorewraps the filesystem store with encryption, making it more complex than the baseFSKeyStore.Consider updating line 399 to better distinguish this from
FSKeyStore:-// EncryptedFSKeyStore is a structure containing the configuration -// for a simple filesystem keystore. +// EncryptedFSKeyStore is a structure containing the configuration +// for an encrypted filesystem keystore.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
kesconf/config.go(2 hunks)kesconf/file.go(2 hunks)server-config.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- kesconf/config.go
- server-config.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
kesconf/file.go (3)
kesconf/config_test.go (2)
MasterKeyPath(36-36)MasterKeyCipher(37-37)internal/keystore/efs/efs.go (1)
NewStore(35-47)internal/keystore/fs/fs.go (1)
NewStore(34-48)
🔇 Additional comments (1)
kesconf/file.go (1)
25-25: LGTM!The import is necessary for the new
EncryptedFSKeyStorefunctionality and follows the existing import organization pattern.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Eliah Rusin <hello@franchb.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
kesconf/file.go (1)
398-399: Consider clarifying the comment to distinguish from FSKeyStore.The comment on line 399 says "for a simple filesystem keystore" which is identical to the FSKeyStore comment above (line 381). Consider updating it to "for an encrypted filesystem keystore" to clearly distinguish this from the unencrypted FSKeyStore.
Apply this diff:
// EncryptedFSKeyStore is a structure containing the configuration -// for a simple filesystem keystore. +// for an encrypted filesystem keystore.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
kesconf/file.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
kesconf/file.go (4)
kesconf/config_test.go (2)
MasterKeyPath(36-36)MasterKeyCipher(37-37)keystore.go (1)
KeyStore(30-62)internal/keystore/fs/fs.go (1)
NewStore(34-48)internal/keystore/efs/efs.go (1)
NewStore(35-47)
🔇 Additional comments (1)
kesconf/file.go (1)
25-25: LGTM!The import is correctly placed and necessary for the new EncryptedFSKeyStore functionality.
2 participants
Add a keystore implementation, that wraps a file system store, but encrypts the files.
Fixes: minio#392
Cherry-pick: minio#489
Summary by CodeRabbit
New Features
Documentation
Tests