fix: secure fleetint state permissions#147
Merged
jingxiang-z merged 8 commits intomainfrom Mar 31, 2026
Merged
Conversation
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds SecureStateFilePermissions to enforce owner-only (0600) permissions on SQLite state files and sidecars, integrates it into state DB open paths, tightens default state directory mode to 0700, updates the post-install script, adds tests, and bumps several module dependencies. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/config/config_test.go (1)
211-222: Consider adding edge case tests for SecureStateFilePermissions.The test validates the happy path, but
SecureStateFilePermissionshas several code paths that could benefit from coverage:
- Empty path (should return nil)
:memory:path (should return nil)- Non-existent file (should return nil)
- Directory path (should return error)
🧪 Proposed additional test cases
func TestSecureStateFilePermissions_EdgeCases(t *testing.T) { t.Run("empty path returns nil", func(t *testing.T) { err := SecureStateFilePermissions("") require.NoError(t, err) }) t.Run("memory path returns nil", func(t *testing.T) { err := SecureStateFilePermissions(":memory:") require.NoError(t, err) }) t.Run("non-existent file returns nil", func(t *testing.T) { err := SecureStateFilePermissions(filepath.Join(t.TempDir(), "nonexistent")) require.NoError(t, err) }) t.Run("directory path returns error", func(t *testing.T) { dir := t.TempDir() err := SecureStateFilePermissions(dir) require.Error(t, err) assert.Contains(t, err.Error(), "is a directory") }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config_test.go` around lines 211 - 222, Add a new test function TestSecureStateFilePermissions_EdgeCases that calls SecureStateFilePermissions for the four edge cases described: empty string, ":memory:", a non-existent file path, and a directory path; assert that the first three return nil (no error) and that calling it with a directory returns an error containing text like "is a directory". Use t.TempDir() to create temp dirs/paths and the testing helpers require.NoError/require.Error/assert.Contains consistent with the existing TestSecureStateFilePermissions style.internal/config/default.go (1)
147-166: TOCTOU window between DB creation and permission hardening.
SecureStateFilePermissionsis called aftersqlite.Open()creates/opens the file. This means there's a brief window where the state file exists with default permissions (based on umask, typically 0644 or 0666 masked to 0644) before being hardened to 0600.For defense-in-depth, consider setting a restrictive umask before opening the database:
oldMask := syscall.Umask(0077) defer syscall.Umask(oldMask)However, the parent directory is already 0700, so only root or the owner can access files within it. The current approach provides adequate protection for most scenarios.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/default.go` around lines 147 - 166, There is a TOCTOU window between creating/opening the state DB and hardening permissions; wrap the database open call (where sqlite.Open is invoked) with a temporary restrictive umask to ensure the file is created with 0600-like permissions (e.g., call syscall.Umask(0077) before sqlite.Open and defer restoring the previous umask immediately after), leaving SecureStateFilePermissions(path string) unchanged as a secondary hardening step.deployments/packages/scripts/postinst (1)
7-9: Update .goreleaser.yaml to declare/var/lib/fleetintwithmode: 0700.The postinst script correctly sets
/var/lib/fleetintto mode 700 (line 8). However,.goreleaser.yamldeclares this directory withmode: 0755in both the RPM (lines 88–91) and DEB (lines 127–130) sections. This creates a brief window during package installation where the directory exists with incorrect permissions before postinst runs.Updating both occurrences in
.goreleaser.yamltomode: 0700would ensure consistent permissions from the moment the directory is created.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/packages/scripts/postinst` around lines 7 - 9, The package manifest declares the directory "/var/lib/fleetint" with mode 0755 in both the RPM and DEB archive entries, which conflicts with the postinst script's chmod 700; update the archive declarations for that directory in the release manifest so the mode value is "0700" for both the RPM and DEB package sections (i.e., change the mode field associated with the "/var/lib/fleetint" directory in each package's files/archives configuration).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@deployments/packages/scripts/postinst`:
- Around line 7-9: The package manifest declares the directory
"/var/lib/fleetint" with mode 0755 in both the RPM and DEB archive entries,
which conflicts with the postinst script's chmod 700; update the archive
declarations for that directory in the release manifest so the mode value is
"0700" for both the RPM and DEB package sections (i.e., change the mode field
associated with the "/var/lib/fleetint" directory in each package's
files/archives configuration).
In `@internal/config/config_test.go`:
- Around line 211-222: Add a new test function
TestSecureStateFilePermissions_EdgeCases that calls SecureStateFilePermissions
for the four edge cases described: empty string, ":memory:", a non-existent file
path, and a directory path; assert that the first three return nil (no error)
and that calling it with a directory returns an error containing text like "is a
directory". Use t.TempDir() to create temp dirs/paths and the testing helpers
require.NoError/require.Error/assert.Contains consistent with the existing
TestSecureStateFilePermissions style.
In `@internal/config/default.go`:
- Around line 147-166: There is a TOCTOU window between creating/opening the
state DB and hardening permissions; wrap the database open call (where
sqlite.Open is invoked) with a temporary restrictive umask to ensure the file is
created with 0600-like permissions (e.g., call syscall.Umask(0077) before
sqlite.Open and defer restoring the previous umask immediately after), leaving
SecureStateFilePermissions(path string) unchanged as a secondary hardening step.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ee35f268-cf66-4eb2-8fae-20854f98a60e
📒 Files selected for processing (10)
cmd/fleetint/compact.gocmd/fleetint/enroll.gocmd/fleetint/metadata.gocmd/fleetint/unenroll.godeployments/packages/scripts/postinstdocs/install-deb.mddocs/install-rpm.mdinternal/config/config_test.gointernal/config/default.gointernal/server/server.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/attestation/attestation_test.go (1)
41-43: Prefer a single precomputed missing path in the override.Calling
t.TempDir()inside the resolver returns a different directory per call. Precompute once for deterministic behavior and lower test overhead.♻️ Suggested tweak
func useMissingStateFile(t *testing.T) { t.Helper() orig := defaultStateFileFn + missingStatePath := filepath.Join(t.TempDir(), "missing", "fleetint.state") defaultStateFileFn = func() (string, error) { - return filepath.Join(t.TempDir(), "missing", "fleetint.state"), nil + return missingStatePath, nil } t.Cleanup(func() { defaultStateFileFn = orig }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/attestation/attestation_test.go` around lines 41 - 43, Precompute a single temporary directory and use that constant inside the override instead of calling t.TempDir() per invocation; e.g., call t.TempDir() once into a local variable (e.g., tmp := t.TempDir()) and then set defaultStateFileFn = func() (string, error) { return filepath.Join(tmp, "missing", "fleetint.state"), nil } so defaultStateFileFn uses the same deterministic missing path across calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/attestation/attestation_test.go`:
- Around line 37-47: useMissingStateFile mutates the package-global
defaultStateFileFn causing potential races; change to either protect
defaultStateFileFn with a mutex or (preferably) refactor to inject the
state-file resolver per Manager. To fix: add a sync.RWMutex guarding
reads/writes of defaultStateFileFn (wrap calls that read it, and update tests to
acquire the mutex when swapping) or modify the Manager constructor and type to
accept a stateFileFn parameter (replace uses of defaultStateFileFn inside
NewManager/Manager methods with the injected field) and update
useMissingStateFile to set the resolver on the Manager instance instead of
mutating the package global. Ensure references to defaultStateFileFn,
useMissingStateFile, and Manager/NewManager are updated accordingly.
---
Nitpick comments:
In `@internal/attestation/attestation_test.go`:
- Around line 41-43: Precompute a single temporary directory and use that
constant inside the override instead of calling t.TempDir() per invocation;
e.g., call t.TempDir() once into a local variable (e.g., tmp := t.TempDir()) and
then set defaultStateFileFn = func() (string, error) { return filepath.Join(tmp,
"missing", "fleetint.state"), nil } so defaultStateFileFn uses the same
deterministic missing path across calls.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7f58eca4-43be-4797-81e2-3a09a48ce91c
📒 Files selected for processing (2)
internal/attestation/attestation.gointernal/attestation/attestation_test.go
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/config/default.go (1)
149-171: Consider handling symlinks to avoid unintended permission changes.If
pathor its sidecars are symlinks,os.Chmodfollows the link and modifies the target file's permissions. This could unintentionally change permissions on a file outside the expected state directory. Consider usingos.Lstatto detect symlinks and either error out or skip them.🛡️ Optional: Add symlink detection
func SecureStateFilePermissions(path string) error { if path == "" || path == ":memory:" { return nil } for _, candidate := range []string{path, path + "-wal", path + "-shm"} { - info, err := osStat(candidate) + info, err := stdos.Lstat(candidate) if err != nil { if stdos.IsNotExist(err) { continue } return err } + if info.Mode()&stdos.ModeSymlink != 0 { + return fmt.Errorf("state file path %q is a symlink", candidate) + } if info.IsDir() { return fmt.Errorf("state file path %q is a directory", candidate) } if err := osChmod(candidate, defaultStateFileMode); err != nil { return err } } return nil }Note: If symlinks are intentionally supported (e.g., for flexible deployment), this check may not be necessary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/default.go` around lines 149 - 171, The SecureStateFilePermissions function currently uses osStat and osChmod which follow symlinks and may alter targets outside the state directory; update SecureStateFilePermissions to detect symlinks (use osLstat or equivalent) for each candidate (path, path+"-wal", path+"-shm") and either return an explicit error or skip changing permissions if osLstat indicates a symlink, while preserving existing directory checks and the osChmod(defaultStateFileMode) behavior for regular files; reference SecureStateFilePermissions, osStat, osLstat, osChmod, and defaultStateFileMode when applying the change.internal/config/config_test.go (1)
211-228: Consider adding test cases for edge cases inSecureStateFilePermissions.The test covers the happy path well, but additional coverage would strengthen confidence:
- Empty string path → returns
nil:memory:path → returnsnil- Path that is a directory → returns error
- Only main file exists (no WAL/SHM sidecars) → succeeds
🧪 Suggested additional test cases
func TestSecureStateFilePermissions_EdgeCases(t *testing.T) { t.Run("empty path returns nil", func(t *testing.T) { err := SecureStateFilePermissions("") require.NoError(t, err) }) t.Run("memory path returns nil", func(t *testing.T) { err := SecureStateFilePermissions(":memory:") require.NoError(t, err) }) t.Run("directory path returns error", func(t *testing.T) { dir := t.TempDir() err := SecureStateFilePermissions(dir) require.Error(t, err) assert.Contains(t, err.Error(), "is a directory") }) t.Run("missing sidecars succeeds", func(t *testing.T) { stateFile := filepath.Join(t.TempDir(), "fleetint.state") require.NoError(t, os.WriteFile(stateFile, []byte("test"), 0o644)) err := SecureStateFilePermissions(stateFile) require.NoError(t, err) info, err := os.Stat(stateFile) require.NoError(t, err) assert.Equal(t, os.FileMode(0o600), info.Mode().Perm()) }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config_test.go` around lines 211 - 228, Add edge-case tests for SecureStateFilePermissions: add a new TestSecureStateFilePermissions_EdgeCases that calls SecureStateFilePermissions("") and SecureStateFilePermissions(":memory:") and asserts no error, calls SecureStateFilePermissions(dir) where dir is a temp directory and asserts an error containing "is a directory", and creates only the main state file (no -wal/-shm) then calls SecureStateFilePermissions(stateFile) and asserts no error and that the main file's permission is 0o600; reference the existing TestSecureStateFilePermissions for filename construction and permission assertions and reuse os.WriteFile, os.Stat, t.TempDir, require.NoError/require.Error and assert.Equal/assert.Contains as in that file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/config/config_test.go`:
- Around line 211-228: Add edge-case tests for SecureStateFilePermissions: add a
new TestSecureStateFilePermissions_EdgeCases that calls
SecureStateFilePermissions("") and SecureStateFilePermissions(":memory:") and
asserts no error, calls SecureStateFilePermissions(dir) where dir is a temp
directory and asserts an error containing "is a directory", and creates only the
main state file (no -wal/-shm) then calls SecureStateFilePermissions(stateFile)
and asserts no error and that the main file's permission is 0o600; reference the
existing TestSecureStateFilePermissions for filename construction and permission
assertions and reuse os.WriteFile, os.Stat, t.TempDir,
require.NoError/require.Error and assert.Equal/assert.Contains as in that file.
In `@internal/config/default.go`:
- Around line 149-171: The SecureStateFilePermissions function currently uses
osStat and osChmod which follow symlinks and may alter targets outside the state
directory; update SecureStateFilePermissions to detect symlinks (use osLstat or
equivalent) for each candidate (path, path+"-wal", path+"-shm") and either
return an explicit error or skip changing permissions if osLstat indicates a
symlink, while preserving existing directory checks and the
osChmod(defaultStateFileMode) behavior for regular files; reference
SecureStateFilePermissions, osStat, osLstat, osChmod, and defaultStateFileMode
when applying the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e5425de9-6801-4147-9c96-a840b29398b5
📒 Files selected for processing (2)
internal/config/config_test.gointernal/config/default.go
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/server/server_test.go (1)
214-228: Test may be fragile depending on SQLite sidecar creation timing.This test asserts that all three files (
stateFile,-wal,-shm) exist with0600permissions immediately afterinitializeDatabasesreturns. However:
- The
-waland-shmfiles are created by SQLite based on journal mode and write activity, not guaranteed at any specific point.- If sidecars don't exist, line 225 fails with "no such file or directory" rather than a permission-related failure.
- This differs from
TestSecureStateFilePermissionsininternal/config/config_test.go(lines 211-228), which pre-creates all files before testing.Consider making the test more robust by either:
- Explicitly checking if sidecars exist before asserting permissions (and skipping the permission check if they don't exist), or
- Forcing a checkpoint/sync to ensure sidecars are flushed before the permission check.
🔧 Suggested adjustment to handle missing sidecars
func TestInitializeDatabasesSecuresFreshStateFile(t *testing.T) { ctx := context.Background() stateFile := filepath.Join(t.TempDir(), "fleetint.state") dbRW, dbRO, err := initializeDatabases(ctx, &config.Config{State: stateFile}) require.NoError(t, err) defer dbRW.Close() defer dbRO.Close() + // Main state file must exist and be secured + info, err := os.Stat(stateFile) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm()) + + // Sidecar files may or may not exist depending on SQLite behavior; + // if they exist, they must be secured - for _, candidate := range []string{stateFile, stateFile + "-wal", stateFile + "-shm"} { + for _, candidate := range []string{stateFile + "-wal", stateFile + "-shm"} { info, err := os.Stat(candidate) - require.NoError(t, err) - assert.Equal(t, os.FileMode(0o600), info.Mode().Perm()) + if os.IsNotExist(err) { + continue // Sidecar not yet created by SQLite + } + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm(), "sidecar %s should be secured", candidate) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/server_test.go` around lines 214 - 228, The test TestInitializeDatabasesSecuresFreshStateFile assumes SQLite sidecar files always exist; make it robust by checking existence before asserting permissions: after calling initializeDatabases, iterate over stateFile, stateFile+"-wal", stateFile+"-shm" and call os.Stat; if os.IsNotExist(err) skip the permission assertion for that candidate (optionally t.Logf that it was absent), otherwise assert the file mode is 0o600. Reference initializeDatabases and the stateFile candidates in the loop so the test only checks permissions for files that actually exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/server.go`:
- Around line 95-104: CreateTableMetadata runs only DDL and may not create
SQLite WAL/SHM sidecar files, so SecureStateFilePermissions(stateFile) can miss
them; ensure sidecars exist before securing permissions by performing a
sync/checkpoint on the write DB handle (dbRW) after
pkgmetadata.CreateTableMetadata(ctx, dbRW) — e.g., invoke a checkpoint or
fsync/PRAGMA wal_checkpoint(FULL) via the dbRW connection (or otherwise perform
a small dummy write/flush) and only then call
config.SecureStateFilePermissions(stateFile) so the -wal and -shm files are
present and receive the tightened 0600 permissions.
---
Nitpick comments:
In `@internal/server/server_test.go`:
- Around line 214-228: The test TestInitializeDatabasesSecuresFreshStateFile
assumes SQLite sidecar files always exist; make it robust by checking existence
before asserting permissions: after calling initializeDatabases, iterate over
stateFile, stateFile+"-wal", stateFile+"-shm" and call os.Stat; if
os.IsNotExist(err) skip the permission assertion for that candidate (optionally
t.Logf that it was absent), otherwise assert the file mode is 0o600. Reference
initializeDatabases and the stateFile candidates in the loop so the test only
checks permissions for files that actually exist.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e305ba18-3b46-49b8-a283-26cc881283cb
📒 Files selected for processing (2)
internal/server/server.gointernal/server/server_test.go
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
rsampaio
approved these changes
Mar 31, 2026
jingxiang-z
added a commit
that referenced
this pull request
Apr 14, 2026
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
jingxiang-z
added a commit
that referenced
this pull request
Apr 14, 2026
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
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
2 participants
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.
Summary
07000600on the SQLite state database on writable open pathsTesting
go test ./internal/config/...go test ./cmd/fleetint/...go test ./internal/serverContext
GPUHEALTH-1589go test ./internal/attestationstill shows existing environment-dependent failures unrelated to this patchSummary by CodeRabbit
Bug Fixes
Chores
Tests