feat(agent): make output file permissions configurable - #342
Conversation
Rendered secret templates were written with a bare os.Create, so they landed 0644 under a normal umask with no way to change it. The execute hook is not a workaround for this: it is guarded by `if !firstRun`, so a chmod there is skipped on the very first render and the file stays world-readable until an etag change forces a re-render. Add a `permission` field to the template config and to the file sink config, spelled to match the one certificates already accept. The access token sink had the same hardcoded 0644 on higher-value content, so it is covered too. Values are validated while the agent config is parsed, so a typo aborts startup instead of silently falling back to a looser mode at the first render. Defaults are unchanged. An unset permission keeps the umask-derived mode for templates and 0644 for sinks, so no existing deployment changes behaviour on upgrade. Files are opened with the target mode rather than created at 0666 and chmod'ed afterwards, which would leave a window where the path is world-readable. Permissions are checked at open(2), so a descriptor obtained during that window would survive the chmod. The chmod is still applied, to restore bits the umask stripped and to tighten a file that already existed. The octal parser is promoted out of WriteCertificateFiles so there is one implementation rather than two. It now rejects modes above 0777, which the certificate-only version accepted, and logs a warning on a malformed value instead of silently falling back to 0600. Fixes: Infisical#339 Signed-off-by: Alexander Chernov <alexander@chernov.it>
|
✅ CLA satisfied. All contributors have signed the current CLA. The |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| outputFile, err := os.OpenFile(outputPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, createMode) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer outputFile.Close() | ||
|
|
||
| if mode != nil { | ||
| if err := outputFile.Chmod(*mode); err != nil { | ||
| return fmt.Errorf("unable to set permission %#o on %s: %w", *mode, outputPath, err) |
There was a problem hiding this comment.
When a configured sink or template points to an existing file that the agent can write but does not own, OpenFile truncates the file before Chmod fails, leaving the token or rendered secret empty instead of preserving the last valid content.
Knowledge Base Used: Agent daemon (infisical agent)
There was a problem hiding this comment.
The concern is resolved in the current code. The original diff I reviewed used O_TRUNC on open, which truncated before Chmod. The updated code removes that flag:
outputFile, err := os.OpenFile(outputPath, os.O_WRONLY|os.O_CREATE, createMode)Chmod runs first, and only on success does Truncate(0) execute. The PR even documents this explicitly in the function comment:
"The file is deliberately opened without O_TRUNC and truncated only once the mode is known to be correct. Truncating first would destroy the previous content before a chmod failure could be reported..."
My original comment no longer applies.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
|
| Filename | Overview |
|---|---|
| packages/cmd/agent.go | Adds permission parsing, validation, and application, but can erase an existing output when chmod fails after truncation. |
| packages/cmd/agent_file_permission_test.go | Thoroughly covers normal permission parsing and writes, but omits the writable-yet-not-owned existing-file failure case. |
| agent-config.yaml | Updates the example configuration with valid 0600 permissions for sinks and templates. |
Reviews (1): Last reviewed commit: "feat(agent): make output file permission..." | Re-trigger Greptile
Truncating before the chmod destroyed the old content when the destination was writable but not owned by the agent, so the file is now truncated only once the mode is known to be correct. Refs: Infisical#342 Signed-off-by: Alexander Chernov <alexander@chernov.it>
Signed-off-by: Alexander Chernov <alexander@chernov.it>
1 participant
Description 📣
Fixes #339.
The agent writes rendered secret templates with whatever
os.Creategives it, so on a normal umask they land 0644, andTemplate.Configcarries onlypolling-intervalandexecute, so there is nothing to set. That makes the agent awkward to adopt anywhere it replaces something that wrote 0600.The
executehook is not a workaround. Achmod 600there is valid and runs undersh -c, but the hook is guarded:So it is skipped on the first render and only fires once an etag change causes a re-render. The file is world-readable from the moment it is created until the first rotation.
This adds a
permissionfield to the template config and to the file sink config, spelled to match the onecertificates[].file-output.*already accepts:The sink is included because
WriteTokenToFileshad the same hardcoded 0644 on content more valuable than a rendered template.Implementation notes:
Defaults are unchanged. An unset
permissionkeeps the umask-derived mode for templates and 0644 for sinks, so no existing deployment changes behaviour on upgrade. I did not default templates to 0600 even though the content argues for it, because the agent commonly runs as root while the consuming application reads the file as a different user, and tightening the default breaks those setups with no error from the agent. Glad to change it if you would rather have 0600.Values are validated in
parseAgentConfigWithMode, so a malformed mode stops the agent at startup rather than falling back to a looser one at first render, which would be invisible until somebody stats the file:The octal parser is promoted out of
WriteCertificateFilesinto a package-levelparseFileMode, so there is one implementation rather than two. Two deliberate changes to certificate behaviour come with that: modes above 0777 are now rejected, where the previousstrconv.ParseIntaccepted them and would set setuid on a private key, and a malformed value now logs a warning before falling back to 0600 instead of being silently swallowed. Unset and valid values behave exactly as before.No new dependencies. The agent config reference lives on infisical.com/docs rather than in this repo, so the new option is undocumented there; happy to open a docs PR if you point me at the right place.
Type ✨
Tests 🛠️
Added
packages/cmd/agent_file_permission_test.go, 20 cases, tagged//go:build unixbecause file modes are a POSIX concept andos.Chmodon Windows only toggles the read-only bit. It covers octal parsing including every rejection case,WriteBytesToFileandWriteTemplateToFileagainst both a new and an already-existing file, and config parsing for valid, malformed and omitted values.The already-existing-file cases are the ones that matter. They are umask-independent, so they isolate the chmod as the mechanism instead of passing by accident on whatever umask the runner has. The omitted-permission cases compare against a reference file created with plain
os.Createin the same temp dir, which pins "default unchanged" to something stronger than a hardcoded 0644.To reproduce:
-vet=offis needed becausego vet ./packages/cmd/already fails onmainwith fournon-constant format stringfindings inrun.go, unrelated to this change. I left them alone, and confirmed this change adds none by diffing vet output with and without the patch. Happy to fix those separately if useful.Two things I want to be straight about. The create-with-mode behaviour above is not unit tested and I do not think it can be: once the call returns, create-with-mode and create-then-chmod are indistinguishable by
stat, and observing the intermediate state means racing the writer, so that property rests on the open flags. And I have not run the agent against a live instance, so the verification here is the unit tests plus a build;TestWriteTemplateToFileAppliesConfiguredPermissiondrives the sameWriteTemplateToFilepath the agent uses per render, but not the surrounding fetch-and-render loop. I can add ane2e/agentcase if you want one, though that suite needs a composed instance and currently only covers certificates.