feat(secrets): implement native secret management with encryption and… - #2
Merged
Conversation
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
📝 WalkthroughWalkthroughAdds FlowForge native secret management: secret resource model and normalization, validation for create/update (including immutability and typed secrets), redaction helper, AES-256-GCM envelope encryption with MasterKeyProvider, EncryptedMemoryStore and EncryptedFileStore backends, tests, and documentation. ChangesSecret Management Implementation
Sequence DiagramssequenceDiagram
participant Input as SecretResource
participant Cipher as EnvelopeCipher
participant MasterKey as MasterKeyProvider
participant Encrypted as EncryptedSecret
Input->>Cipher: Encrypt(secret)
Cipher->>Cipher: Normalize & Marshal Data JSON
Cipher->>Cipher: Generate random DataKey
Cipher->>MasterKey: MasterKey()
MasterKey-->>Cipher: Master Key (32 bytes)
Cipher->>Cipher: Seal DataKey with Master Key (AES-GCM)
Cipher->>Cipher: Seal plaintext with DataKey (AES-GCM, AAD)
Cipher-->>Encrypted: EncryptedSecret (ciphertext, nonces, meta)
Encrypted->>Cipher: Decrypt(encrypted)
Cipher->>MasterKey: MasterKey()
MasterKey-->>Cipher: Master Key
Cipher->>Cipher: Unseal DataKey
Cipher->>Cipher: Unseal plaintext JSON -> SecretResource
Cipher-->>Input: SecretResource
sequenceDiagram
participant Client
participant Store as EncryptedMemoryStore
participant Cipher as EnvelopeCipher
participant Memory as EncryptedMap
Client->>Store: Create(SecretResource)
Store->>Store: Validate & Normalize
Store->>Cipher: Encrypt(secret)
Cipher-->>Store: EncryptedSecret
Store->>Memory: Store by name
Store-->>Client: ok / error
Client->>Store: Get(name)
Store->>Memory: Retrieve EncryptedSecret
Store->>Cipher: Decrypt(encrypted)
Cipher-->>Store: SecretResource
Store->>Store: Return deep copy
Store-->>Client: SecretResource
Client->>Store: Resolve(SecretRef)
Store->>Store: Get(name) -> return key bytes or ErrSecretKeyMissing
Store-->>Client: []byte or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
internal/secrets/file_store.go (1)
131-138: 💤 Low valueHandle or explicitly discard
Close()errors in error paths.Static analysis flags unchecked
tmp.Close()calls on lines 132 and 136. While these are in error-return paths where the original error takes precedence, explicitly discarding or logging helps satisfy linters and documents intent.Proposed fix
if _, err := tmp.Write(data); err != nil { - tmp.Close() + _ = tmp.Close() return err } if err := tmp.Chmod(0600); err != nil { - tmp.Close() + _ = tmp.Close() return err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/secrets/file_store.go` around lines 131 - 138, The error paths that call tmp.Close() after tmp.Write and tmp.Chmod should explicitly handle or discard the Close() error to satisfy linters and document intent: update the error-return branches in the function using the tmp file (references: tmp.Write, tmp.Chmod, tmp.Close) so that Close() is either deferred earlier (e.g., defer tmp.Close() with proper handling) or its return value is explicitly ignored/logged (e.g., _ = tmp.Close() or processLogger.Warnf("tmp.Close failed: %v", err)). Ensure the original error is still returned but the Close() result is not left unchecked.internal/secrets/validation_test.go (1)
62-78: ⚡ Quick winAdd a regression test for immutable flag downgrade (
true -> false).Current tests cover data mutation but not removing immutability itself, which should also be rejected.
Suggested test case
+func TestValidateUpdateRejectsImmutableFlagRemoval(t *testing.T) { + oldSecret := SecretResource{ + Metadata: Metadata{Name: "service-credential"}, + Data: map[string][]byte{"api-key": []byte("same")}, + Immutable: true, + } + newSecret := SecretResource{ + Metadata: Metadata{Name: "service-credential"}, + Data: map[string][]byte{"api-key": []byte("same")}, + Immutable: false, + } + + err := ValidateUpdate(oldSecret, newSecret) + if !errors.Is(err, ErrSecretImmutable) { + t.Fatalf("ValidateUpdate() error = %v, want ErrSecretImmutable", err) + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/secrets/validation_test.go` around lines 62 - 78, Add a regression test that ensures downgrading the Immutable flag (true -> false) is rejected: create an old SecretResource with Immutable: true and a new SecretResource with the same Metadata/Data but Immutable: false, call ValidateUpdate(oldSecret, newSecret) and assert the returned error satisfies ErrSecretImmutable; place this alongside the existing TestValidateUpdateRejectsImmutableDataChange to cover the immutable-flag-downgrade case using the same ValidateUpdate, SecretResource, Metadata and ErrSecretImmutable symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/secrets.md`:
- Line 25: Replace the example value for the "api-key" entry (currently
"123456:token") with a clearly fake, non-secret placeholder such as
"<api-key-placeholder>" so readers won't accidentally copy a real-looking token;
update the value for the api-key key in docs/secrets.md to an unmistakable
placeholder string.
In `@internal/secrets/crypto.go`:
- Around line 39-41: NewEnvelopeCipher currently allows creating an
EnvelopeCipher with a nil MasterKeyProvider which later causes a panic when
Encrypt/Decrypt call c.provider.MasterKey(); update NewEnvelopeCipher to
validate the provider argument (in NewEnvelopeCipher(provider
MasterKeyProvider)) and fail fast if nil—either return an error by changing the
signature to NewEnvelopeCipher(provider MasterKeyProvider) (*EnvelopeCipher,
error) or, if API change is unacceptable, panic with a clear message like "nil
MasterKeyProvider passed to NewEnvelopeCipher"; ensure the EnvelopeCipher
construction is only performed when provider != nil and update any callers/tests
to handle the new behavior.
In `@internal/secrets/file_store.go`:
- Around line 21-37: The Create method in EncryptedFileStore has a TOCTOU race
between os.Stat(s.path(...)) and s.write(...)—replace the two-step check+write
with an atomic create: either add a writeExclusive helper (or extend s.write)
that opens the target path with os.OpenFile using O_CREATE|O_EXCL and
appropriate file mode, write the already-encrypted data returned by
s.cipher.Encrypt(normalized) to that file, close it, and return ErrSecretExists
if open fails due to existing file; ensure proper error handling/cleanup on
partial writes and propagate other errors unchanged so EncryptedFileStore.Create
no longer relies on the prior os.Stat check.
In `@internal/secrets/validation.go`:
- Around line 40-45: When oldNormalized.Immutable is true, the current logic
allows newSecret.Immutable to be flipped to false if the data and type are
unchanged; update the check in the validation branch that uses oldNormalized and
newNormalized so it also verifies the Immutable flag matches (i.e., require
newNormalized.Immutable == true when oldNormalized.Immutable == true) and return
ErrSecretImmutable if Immutable differs or any of data/type differ; ensure the
comparison uses the existing variables newSecret.Normalized(), oldNormalized,
newNormalized and still returns ErrSecretImmutable on any mismatch.
---
Nitpick comments:
In `@internal/secrets/file_store.go`:
- Around line 131-138: The error paths that call tmp.Close() after tmp.Write and
tmp.Chmod should explicitly handle or discard the Close() error to satisfy
linters and document intent: update the error-return branches in the function
using the tmp file (references: tmp.Write, tmp.Chmod, tmp.Close) so that Close()
is either deferred earlier (e.g., defer tmp.Close() with proper handling) or its
return value is explicitly ignored/logged (e.g., _ = tmp.Close() or
processLogger.Warnf("tmp.Close failed: %v", err)). Ensure the original error is
still returned but the Close() result is not left unchecked.
In `@internal/secrets/validation_test.go`:
- Around line 62-78: Add a regression test that ensures downgrading the
Immutable flag (true -> false) is rejected: create an old SecretResource with
Immutable: true and a new SecretResource with the same Metadata/Data but
Immutable: false, call ValidateUpdate(oldSecret, newSecret) and assert the
returned error satisfies ErrSecretImmutable; place this alongside the existing
TestValidateUpdateRejectsImmutableDataChange to cover the
immutable-flag-downgrade case using the same ValidateUpdate, SecretResource,
Metadata and ErrSecretImmutable symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54563f93-0e02-406b-807c-68331422f861
📒 Files selected for processing (15)
README.mddocs/architecture.mddocs/secrets.mdinternal/secrets/crypto.gointernal/secrets/crypto_test.gointernal/secrets/errors.gointernal/secrets/file_store.gointernal/secrets/file_store_test.gointernal/secrets/model.gointernal/secrets/redact.gointernal/secrets/redact_test.gointernal/secrets/store.gointernal/secrets/store_test.gointernal/secrets/validation.gointernal/secrets/validation_test.go
Owner
Author
|
@coderabbitai autofix stacked pr |
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 4 file(s) based on 4 unresolved review comments. A stacked PR containing fixes has been created.
Time taken: |
Fixed 4 file(s) based on 4 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
fix: CodeRabbit auto-fixes for PR #2
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/secrets/file_store.go (1)
17-19: 💤 Low valueConsider validating that
cipheris non-nil.For consistency with
NewEnvelopeCipherwhich now panics on nil provider, this constructor could similarly guard against a nilcipherto fail fast rather than panicking later inCreate/Get/Updatewhens.cipher.Encryptors.cipher.Decryptis called.♻️ Optional: Add nil guard
func NewEncryptedFileStore(dir string, cipher *EnvelopeCipher) *EncryptedFileStore { + if cipher == nil { + panic("nil EnvelopeCipher passed to NewEncryptedFileStore") + } return &EncryptedFileStore{dir: dir, cipher: cipher} }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/secrets/file_store.go` around lines 17 - 19, NewEncryptedFileStore should validate that the provided cipher is non-nil and fail fast; update the NewEncryptedFileStore constructor to check if cipher == nil and panic (or otherwise return an error consistent with NewEnvelopeCipher) with a clear message, so EncryptedFileStore is never constructed with a nil cipher and later methods like Create/Get/Update won't trigger nil dereference when calling s.cipher.Encrypt or s.cipher.Decrypt.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/secrets/file_store.go`:
- Around line 154-162: The code currently defers file.Close() and also calls
file.Close() explicitly in the same function (variable name file in
internal/secrets/file_store.go), which can cause a redundant/unchecked second
close; remove the deferred close and instead close the file explicitly after
Write (handle the error from file.Write), then call file.Close() once and handle
its error (ensuring os.Remove(path) is called on any write or close error) so
cleanup and error returns are correct and no double-close occurs.
---
Nitpick comments:
In `@internal/secrets/file_store.go`:
- Around line 17-19: NewEncryptedFileStore should validate that the provided
cipher is non-nil and fail fast; update the NewEncryptedFileStore constructor to
check if cipher == nil and panic (or otherwise return an error consistent with
NewEnvelopeCipher) with a clear message, so EncryptedFileStore is never
constructed with a nil cipher and later methods like Create/Get/Update won't
trigger nil dereference when calling s.cipher.Encrypt or s.cipher.Decrypt.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7786a68-1b0b-4125-a73c-67b6b7db9333
📒 Files selected for processing (4)
docs/secrets.mdinternal/secrets/crypto.gointernal/secrets/file_store.gointernal/secrets/validation.go
✅ Files skipped from review due to trivial changes (1)
- docs/secrets.md
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/secrets/validation.go
Comment on lines
+154
to
+162
| defer file.Close() | ||
| if _, err := file.Write(data); err != nil { | ||
| os.Remove(path) | ||
| return err | ||
| } | ||
| if err := file.Close(); err != nil { | ||
| os.Remove(path) | ||
| return err | ||
| } |
Contributor
There was a problem hiding this comment.
Double-close bug: file.Close() is called twice.
The defer file.Close() on line 154 will execute after the explicit file.Close() on line 159, causing a double-close. While Go's os.File.Close is generally safe to call multiple times, this pattern is error-prone and the second close will return an error that goes unchecked.
🐛 Proposed fix: Remove defer and handle close explicitly
func (s *EncryptedFileStore) writeExclusive(path string, secret EncryptedSecret) error {
if err := os.MkdirAll(s.dir, 0700); err != nil {
return err
}
data, err := json.MarshalIndent(secret, "", " ")
if err != nil {
return err
}
file, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0600)
if err != nil {
if errors.Is(err, os.ErrExist) {
return ErrSecretExists
}
return err
}
- defer file.Close()
if _, err := file.Write(data); err != nil {
+ file.Close()
os.Remove(path)
return err
}
if err := file.Close(); err != nil {
os.Remove(path)
return err
}
return nil
}🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 154-154: Error return value of file.Close is not checked
(errcheck)
[error] 156-156: Error return value of os.Remove is not checked
(errcheck)
[error] 160-160: Error return value of os.Remove is not checked
(errcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/secrets/file_store.go` around lines 154 - 162, The code currently
defers file.Close() and also calls file.Close() explicitly in the same function
(variable name file in internal/secrets/file_store.go), which can cause a
redundant/unchecked second close; remove the deferred close and instead close
the file explicitly after Write (handle the error from file.Write), then call
file.Close() once and handle its error (ensuring os.Remove(path) is called on
any write or close error) so cleanup and error returns are correct and no
double-close occurs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
… storage
Summary by CodeRabbit
New Features
Documentation
Tests