-
Notifications
You must be signed in to change notification settings - Fork 5k
Remove fslib and upgrade go-ntfs. Implement low level read ourselves #49763
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| # REQUIRED | ||
| # Kind can be one of: | ||
| # - breaking-change: a change to previously-documented behavior | ||
| # - deprecation: functionality that is being removed in a later release | ||
| # - bug-fix: fixes a problem in a previous version | ||
| # - enhancement: extends functionality but does not break or fix existing behavior | ||
| # - feature: new functionality | ||
| # - known-issue: problems that we are aware of in a given version | ||
| # - security: impacts on the security of a product or a user’s deployment. | ||
| # - upgrade: important information for someone upgrading from a prior version | ||
| # - other: does not fit into any of the other categories | ||
| kind: feature | ||
|
|
||
| # REQUIRED for all kinds | ||
| # Change summary; a 80ish characters long description of the change. | ||
| summary: Removes the dependency on fslib and implements the functionality using go-ntfs instead | ||
|
|
||
| # REQUIRED for breaking-change, deprecation, known-issue | ||
| # Long description; in case the summary is not enough to describe the change | ||
| # this field accommodate a description without length limits. | ||
| # description: | ||
|
|
||
| # REQUIRED for breaking-change, deprecation, known-issue | ||
| # impact: | ||
|
|
||
| # REQUIRED for breaking-change, deprecation, known-issue | ||
| # action: | ||
|
|
||
| # REQUIRED for all kinds | ||
| # Affected component; usually one of "elastic-agent", "fleet-server", "filebeat", "metricbeat", "auditbeat", "all", etc. | ||
| component: osquerybeat | ||
|
|
||
| # AUTOMATED | ||
| # OPTIONAL to manually add other PR URLs | ||
| # PR URL: A link the PR that added the changeset. | ||
| # If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added. | ||
| # NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number. | ||
| # Please provide it if you are adding a fragment for a different PR. | ||
| # pr: https://github.com/owner/repo/1234 | ||
|
|
||
| # AUTOMATED | ||
| # OPTIONAL to manually add other issue URLs | ||
| # Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of). | ||
| # If not present is automatically filled by the tooling with the issue linked to the PR number. | ||
| # issue: https://github.com/owner/repo/1234 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,12 +7,14 @@ | |
| package registry | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "os" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
|
|
||
| "github.com/elastic/beats/v7/x-pack/osquerybeat/ext/osquery-extension/pkg/logger" | ||
| "www.velocidex.com/golang/regparser" | ||
| ) | ||
|
|
||
| func TestRecovery(t *testing.T) { | ||
|
|
@@ -52,6 +54,29 @@ func Test_findTransactionLogs(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| 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") | ||
|
Comment on lines
+57
to
+70
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 Line 64 calls 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 |
||
|
|
||
| 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") | ||
|
|
||
| keyNode := registry.OpenKey("Root\\InventoryApplication") | ||
| assert.NotNil(t, keyNode, "failed to open key Root\\InventoryApplication") | ||
| } | ||
|
|
||
| func TestLoadRegistry(t *testing.T) { | ||
| log := logger.New(os.Stdout, true) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the comment, this sounds like it could be considered a derivative work and we need to satisfy the MIT license requirement of "The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software."
I think we need a comment like
You might want to separate the derivative function into its own file to make it even more clear what code is derivative.
References:
https://github.com/elastic/open-source/blob/main/elastic-product-policy.md#third-party-code-in-an-elastic-repository-eg-copy-paste-vendoring-etc (internal)