Remove fslib and upgrade go-ntfs. Implement low level read ourselves#49763
Open
brian-mckinney wants to merge 3 commits intoelastic:mainfrom
Open
Remove fslib and upgrade go-ntfs. Implement low level read ourselves#49763brian-mckinney wants to merge 3 commits intoelastic:mainfrom
brian-mckinney wants to merge 3 commits intoelastic:mainfrom
Conversation
botelastic
bot
added
the
needs_team
Contributor
🤖 GitHub commentsJust comment with:
|
Contributor
|
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
brian-mckinney
added
the
Team:Security-Windows Platform
botelastic
bot
removed
the
needs_team
Contributor
|
Pinging @elastic/sec-windows-platform (Team:Security-Windows Platform) |
📝 WalkthroughWalkthroughThis PR removes the fslib dependency and replaces its NTFS read logic with go-ntfs. go.mod is updated to add go-ntfs and related Velocidex modules and drop fslib. The amcache registry code replaces the fslib-based fallback with readFileViaNTFS, which opens the raw volume (\.<drive>:), parses NTFS structures, locates files via the MFT, and reads the $DATA attribute. A Windows-only test calls readFileViaNTFS against C:\Windows\AppCompat\Programs\Amcache.hve and validates the returned registry data. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@x-pack/osquerybeat/ext/osquery-extension/pkg/amcache/registry/registry_test.go`:
- Around line 57-70: The test Test_readFileViaNTFS should be adjusted to match
the current readFileViaNTFS signature and to avoid panics and CI failures: call
readFileViaNTFS with the single parameter expected by the implementation (use
readFileViaNTFS(amcachePath) and remove the logger argument or update to the
function signature if you prefer changing the implementation), handle the
returned error by skipping the test when the error indicates lack of
raw-volume/permission (use t.Skipf with the error) instead of failing the run,
and guard any slice access by checking len(data) >= len(magic) before using
data[:len(magic)] (or skip/assert with a clear message) — reference
Test_readFileViaNTFS, readFileViaNTFS, magic, and data to locate the affected
code.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 695a93aa-94dd-451a-971e-52f373a364e3
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
changelog/fragments/1774802490-remove_fslib_dep.yamlgo.modx-pack/osquerybeat/ext/osquery-extension/pkg/amcache/registry/registry.gox-pack/osquerybeat/ext/osquery-extension/pkg/amcache/registry/registry_test.go
Comment on lines
+57
to
+70
| func Test_readFileViaNTFS(t *testing.T) { | ||
| log := logger.New(os.Stdout, true) | ||
|
|
||
| // Read the actual amcache hive using the readFileViaNTFS function to ensure it can read the file and return valid registry data. | ||
| amcachePath := "C:\\Windows\\AppCompat\\Programs\\Amcache.hve" | ||
|
|
||
| // Read the file using | ||
| data, err := readFileViaNTFS(amcachePath, log) | ||
| assert.NoError(t, err, "readFileViaNTFS() failed: %v", err) | ||
| assert.NotEmpty(t, data, "readFileViaNTFS() returned empty data") | ||
|
|
||
| magic := []byte{0x72, 0x65, 0x67, 0x66} // "regf" | ||
| log.Infof("readFileViaNTFS() returned data with magic: %x", data[:5]) | ||
| assert.True(t, bytes.HasPrefix(data, magic), "readFileViaNTFS() returned data with incorrect magic") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 '\bfunc readFileViaNTFS\s*\(|\breadFileViaNTFS\s*\(|data\[:5\]|Test_readFileViaNTFS|\\\\\.\\C:' \
x-pack/osquerybeat/ext/osquery-extension/pkg/amcache/registry/registry.go \
x-pack/osquerybeat/ext/osquery-extension/pkg/amcache/registry/registry_test.goRepository: elastic/beats
Length of output: 3137
Fix compile error and panic risk in Test_readFileViaNTFS before merge.
Line 64 calls readFileViaNTFS(amcachePath, log) with two arguments, but the function is declared with one parameter in registry.go (compile failure). Line 69 accesses data[:5] without bounds checking, causing panic on short reads. The test also hard-fails on runners without raw-volume access, blocking CI.
Proposed fix
import (
"bytes"
+ "errors"
"os"
"testing"
@@
- data, err := readFileViaNTFS(amcachePath, log)
+ data, err := readFileViaNTFS(amcachePath)
+ if err != nil && (errors.Is(err, os.ErrPermission) || errors.Is(err, os.ErrNotExist)) {
+ t.Skipf("skipping NTFS raw-read test on this runner: %v", err)
+ }
assert.NoError(t, err, "readFileViaNTFS() failed: %v", err)
assert.NotEmpty(t, data, "readFileViaNTFS() returned empty data")
@@
magic := []byte{0x72, 0x65, 0x67, 0x66} // "regf"
- log.Infof("readFileViaNTFS() returned data with magic: %x", data[:5])
+ assert.GreaterOrEqual(t, len(data), len(magic), "readFileViaNTFS() returned truncated data")
+ log.Infof("readFileViaNTFS() returned data with magic: %x", data[:len(magic)])
assert.True(t, bytes.HasPrefix(data, magic), "readFileViaNTFS() returned data with incorrect magic")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@x-pack/osquerybeat/ext/osquery-extension/pkg/amcache/registry/registry_test.go`
around lines 57 - 70, The test Test_readFileViaNTFS should be adjusted to match
the current readFileViaNTFS signature and to avoid panics and CI failures: call
readFileViaNTFS with the single parameter expected by the implementation (use
readFileViaNTFS(amcachePath) and remove the logger argument or update to the
function signature if you prefer changing the implementation), handle the
returned error by skipping the test when the error indicates lack of
raw-volume/permission (use t.Skipf with the error) instead of failing the run,
and guard any slice access by checking len(data) >= len(magic) before using
data[:len(magic)] (or skip/assert with a clear message) — reference
Test_readFileViaNTFS, readFileViaNTFS, magic, and data to locate the affected
code.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
x-pack/osquerybeat/ext/osquery-extension/pkg/amcache/registry/registry_test.go (1)
10-12:⚠️ Potential issue | 🔴 CriticalMake this raw-volume test fail cleanly on unsupported runners.
It still assumes a
C:Amcache hive plus raw NTFS access, so it can fail on ordinary Windows runners. More importantly, read/parse failures still flow intodata[:5]andregistry.OpenKey(...), which can panic instead of skip/fail cleanly.Possible fix
import ( "bytes" + "errors" "os" "testing" @@ // Read the file using data, err := readFileViaNTFS(amcachePath) - assert.NoError(t, err, "readFileViaNTFS() failed: %v", err) - assert.NotEmpty(t, data, "readFileViaNTFS() returned empty data") + if err != nil { + if errors.Is(err, os.ErrPermission) || errors.Is(err, os.ErrNotExist) { + t.Skipf("skipping NTFS raw-read test on this runner: %v", err) + } + t.Fatalf("readFileViaNTFS() failed: %v", err) + } + if len(data) == 0 { + t.Fatal("readFileViaNTFS() returned empty data") + } magic := []byte{0x72, 0x65, 0x67, 0x66} // "regf" - log.Infof("readFileViaNTFS() returned data with magic: %x", data[:5]) - assert.True(t, bytes.HasPrefix(data, magic), "readFileViaNTFS() returned data with incorrect magic") + if len(data) < len(magic) { + t.Fatalf("readFileViaNTFS() returned truncated data: %d bytes", len(data)) + } + log.Infof("readFileViaNTFS() returned data with magic: %x", data[:len(magic)]) + if !bytes.HasPrefix(data, magic) { + t.Fatal("readFileViaNTFS() returned data with incorrect magic") + } registry, err := regparser.NewRegistry(bytes.NewReader(data)) - assert.NoError(t, err, "failed to create registry from NTFS data") - assert.NotNil(t, registry, "registry is nil") + if err != nil { + t.Fatalf("failed to create registry from NTFS data: %v", err) + } + if registry == nil { + t.Fatal("registry is nil") + } keyNode := registry.OpenKey("Root\\InventoryApplication") - assert.NotNil(t, keyNode, "failed to open key Root\\InventoryApplication") + if keyNode == nil { + t.Fatal("failed to open key Root\\InventoryApplication") + } }Also applies to: 57-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x-pack/osquerybeat/ext/osquery-extension/pkg/amcache/registry/registry_test.go` around lines 10 - 12, The raw-volume test must be made to skip cleanly on unsupported runners and avoid panics on read/parse failures: at the start of the test (e.g., TestRawVolume or similarly named test) detect unsupported environments (non-Windows, missing C: volume, or lack of raw NTFS access) and call t.Skipf with an explanatory message; before taking data[:5] or calling registry.OpenKey ensure the file read/parse succeeded (check for nil/len bounds and error return values) and return or t.Fatalf/t.Skip on error instead of slicing or calling registry.OpenKey on invalid data; update any helper that opens the hive to return an error which the test checks, and reference the registry.OpenKey call and the data variable so the guards are added immediately before those operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@x-pack/osquerybeat/ext/osquery-extension/pkg/amcache/registry/registry_test.go`:
- Around line 10-12: The raw-volume test must be made to skip cleanly on
unsupported runners and avoid panics on read/parse failures: at the start of the
test (e.g., TestRawVolume or similarly named test) detect unsupported
environments (non-Windows, missing C: volume, or lack of raw NTFS access) and
call t.Skipf with an explanatory message; before taking data[:5] or calling
registry.OpenKey ensure the file read/parse succeeded (check for nil/len bounds
and error return values) and return or t.Fatalf/t.Skip on error instead of
slicing or calling registry.OpenKey on invalid data; update any helper that
opens the hive to return an error which the test checks, and reference the
registry.OpenKey call and the data variable so the guards are added immediately
before those operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7440aa5c-a047-43b6-a5d7-9350965a3b2a
📒 Files selected for processing (2)
x-pack/osquerybeat/ext/osquery-extension/pkg/amcache/registry/registry.gox-pack/osquerybeat/ext/osquery-extension/pkg/amcache/registry/registry_test.go
✅ Files skipped from review due to trivial changes (1)
- x-pack/osquerybeat/ext/osquery-extension/pkg/amcache/registry/registry.go
4 tasks
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.
Proposed commit message
Removes the dependency on the
fsliblibrary.fslibis pinned to a much older version ofgo-ntfs, and if we kept thefslibdependency, we could not use the newer version ofgo-ntfsfor the upcoming MFT table.fslibwas used for a single purpose, to perform low level forensic read of a system locked file (the Amcache registry hive). With a bit of help from Claude to identify how it worked, I was able to pretty easily extract the functionality we were using and implement it ourselves withgo-ntfs.Summary:
fslibdependencygo-ntfsdependency tov0.2.1fslibregistry.goto use the new function when reading the amcache hiveChecklist
stresstest.shscript to run them under stress conditions and race detector to verify their stability../changelog/fragmentsusing the changelog tool.