feat(agent): make output file permissions configurable - #342
Open
alekc wants to merge 3 commits into
Open
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 |
Contributor
|
| Filename | Overview |
|---|---|
| packages/cmd/agent.go | Adds validated file-mode configuration and safely applies it before truncating and writing agent outputs. |
| packages/cmd/agent_file_permission_test.go | Covers mode parsing, existing-file handling, chmod failures, unchanged defaults, and config validation. |
| agent-config.yaml | Demonstrates optional 0600 permissions for token sinks and rendered templates. |
Reviews (2): Last reviewed commit: "slight change to the documentation comme..." | 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>
Author
|
@greptile the issue has been solved, update summary |
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
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.
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.