-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[management,client] Add anonymize level and upload URL to remote debug bundle jobs #7147
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
Changes from all commits
b08120b
d0bcf82
d325730
a171aa9
b7c2a16
b074013
0bb270c
a3176c9
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 | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -1373,7 +1373,17 @@ func (e *Engine) receiveJobEvents() { | |||||||
| } | ||||||||
|
|
||||||||
| func (e *Engine) handleBundle(params *mgmProto.BundleParameters) (*mgmProto.JobResponse_Bundle, error) { | ||||||||
| log.Infof("handle remote debug bundle request: %s", params.String()) | ||||||||
| // The upload URL can carry a host, credentials, or query tokens, so it is | ||||||||
| // kept out of the info-level line; the full parameters stay available at | ||||||||
| // debug level for troubleshooting. | ||||||||
| log.Infof("handle remote debug bundle request: anonymize=%v anonymize_level=%q log_file_count=%d bundle_for=%v bundle_for_time=%d", | ||||||||
| params.GetAnonymize(), params.GetAnonymizeLevel(), params.GetLogFileCount(), params.GetBundleFor(), params.GetBundleForTime()) | ||||||||
| log.Debugf("remote debug bundle request parameters: %s", params.String()) | ||||||||
|
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. P1: When Prompt for AI agents
Suggested change
|
||||||||
|
|
||||||||
| if err := validateBundleUploadURL(params.GetUploadUrl()); err != nil { | ||||||||
| return nil, err | ||||||||
| } | ||||||||
|
|
||||||||
| syncResponse, err := e.GetLatestSyncResponse() | ||||||||
| if err != nil { | ||||||||
| log.Warnf("get latest sync response: %v", err) | ||||||||
|
|
@@ -1401,7 +1411,7 @@ func (e *Engine) handleBundle(params *mgmProto.BundleParameters) (*mgmProto.JobR | |||||||
|
|
||||||||
| waitFor := time.Duration(params.BundleForTime) * time.Minute | ||||||||
|
|
||||||||
| uploadKey, err := e.jobExecutor.BundleJob(e.ctx, bundleDeps, bundleJobParams, waitFor, e.config.ProfileConfig.ManagementURL.String()) | ||||||||
| uploadKey, err := e.jobExecutor.BundleJob(e.ctx, bundleDeps, bundleJobParams, waitFor, e.config.ProfileConfig.ManagementURL.String(), params.GetUploadUrl()) | ||||||||
|
mlsmaycon marked this conversation as resolved.
|
||||||||
| if err != nil { | ||||||||
| return nil, err | ||||||||
| } | ||||||||
|
|
@@ -1414,6 +1424,26 @@ func (e *Engine) handleBundle(params *mgmProto.BundleParameters) (*mgmProto.JobR | |||||||
| return response, nil | ||||||||
| } | ||||||||
|
|
||||||||
| // validateBundleUploadURL sanity-checks a management-supplied upload URL for a | ||||||||
| // remote debug bundle job. An empty value is accepted — the executor falls back | ||||||||
| // to the default upload service. A non-empty value must be a well-formed https | ||||||||
| // URL with a host; a malformed value or a plaintext scheme is rejected. This | ||||||||
| // deliberately does not constrain which host may receive the bundle; that | ||||||||
| // policy is left open pending a decision on management-directed uploads. | ||||||||
| func validateBundleUploadURL(raw string) error { | ||||||||
| if raw == "" { | ||||||||
| return nil | ||||||||
| } | ||||||||
| parsed, err := url.Parse(raw) | ||||||||
| if err != nil { | ||||||||
| return fmt.Errorf("parse upload URL: %w", err) | ||||||||
| } | ||||||||
| if parsed.Scheme != "https" || parsed.Host == "" { | ||||||||
| return fmt.Errorf("upload URL must be an https URL with a host") | ||||||||
| } | ||||||||
| return nil | ||||||||
| } | ||||||||
|
|
||||||||
| // receiveManagementEvents connects to the Management Service event stream to receive updates from the management service | ||||||||
| // E.g. when a new peer has been registered and we are allowed to connect to it. | ||||||||
| func (e *Engine) receiveManagementEvents() { | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| package internal | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // TestValidateBundleUploadURL covers the sanity check applied to a | ||
| // management-supplied upload URL before a remote debug bundle is generated. | ||
| func TestValidateBundleUploadURL(t *testing.T) { | ||
| for _, tc := range []struct { | ||
| name string | ||
| raw string | ||
| wantErr bool | ||
| }{ | ||
| {name: "empty falls back to default", raw: ""}, | ||
| {name: "https with host", raw: "https://upload.debug.netbird.io/upload"}, | ||
| {name: "https self-hosted host", raw: "https://upload.example.com"}, | ||
| {name: "plaintext rejected", raw: "http://upload.example.com", wantErr: true}, | ||
| {name: "missing host rejected", raw: "https:///upload", wantErr: true}, | ||
| {name: "non-url scheme rejected", raw: "ftp://upload.example.com", wantErr: true}, | ||
| {name: "garbage rejected", raw: "://not a url", wantErr: true}, | ||
| } { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| err := validateBundleUploadURL(tc.raw) | ||
| if tc.wantErr { | ||
| require.Error(t, err, "an invalid upload URL must be rejected") | ||
| return | ||
| } | ||
| assert.NoError(t, err, "a valid or empty upload URL must be accepted") | ||
| }) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,10 +3,12 @@ package types | |
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
| "strings" | ||
| "time" | ||
|
|
||
| "github.com/google/uuid" | ||
|
|
||
| "github.com/netbirdio/netbird/client/anonymize" | ||
|
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. P3: The management server now imports the agent-side Prompt for AI agents |
||
| "github.com/netbirdio/netbird/shared/management/http/api" | ||
| "github.com/netbirdio/netbird/shared/management/proto" | ||
| "github.com/netbirdio/netbird/shared/management/status" | ||
|
|
@@ -150,6 +152,21 @@ func validateAndBuildBundleParams(req api.WorkloadRequest, workload *Workload) e | |
| if bundle.Parameters.LogFileCount < 1 || bundle.Parameters.LogFileCount > 1000 { | ||
| return fmt.Errorf("log-file-count must be between 1 and 1000, got %d", bundle.Parameters.LogFileCount) | ||
| } | ||
| // validate anonymize_level: omitted or empty defaults on the client; | ||
| // otherwise it must name a known level. An unknown value is rejected here | ||
| // rather than silently escalated, so a typo surfaces at job creation. The | ||
| // normalized (trimmed, lowercased) value is persisted so it matches what | ||
| // the client parses — the client only lowercases, so a stored " default " | ||
| // would otherwise resolve to strict. | ||
| if lvl := bundle.Parameters.AnonymizeLevel; lvl != nil { | ||
| normalized := strings.ToLower(strings.TrimSpace(*lvl)) | ||
| switch normalized { | ||
| case "", anonymize.LevelDefaultString, anonymize.LevelStrictString: | ||
| default: | ||
| return fmt.Errorf("anonymize_level must be %q or %q, got %q", anonymize.LevelDefaultString, anonymize.LevelStrictString, *lvl) | ||
| } | ||
| bundle.Parameters.AnonymizeLevel = &normalized | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
Comment on lines
+161
to
+169
Contributor
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. 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 \
'upload_url|UploadUrl|url\.Parse|ParseRequestURI|Scheme|https://' \
management/server/types/job.go \
shared/management/http/api/openapi.yml \
clientRepository: netbirdio/netbird Length of output: 50376 🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- applicable instructions ---'
find .. -name AGENTS.md -print
for f in $(find .. -name AGENTS.md -print); do
printf '\n--- %s ---\n' "$f"
cat "$f"
done
printf '%s\n' '--- job definitions and focused mapping ---'
ast-grep outline management/server/types/job.go
sed -n '120,185p' management/server/types/job.go
sed -n '225,270p' management/server/types/job.go
printf '%s\n' '--- upload URL references outside generated/build-like paths ---'
rg -n -C 6 --glob '!**/*_test.go' --glob '!**/types.gen.go' --glob '!**/*.pb.go' \
'upload_url|UploadUrl|uploadUrl|UploadURL' \
management shared client
printf '%s\n' '--- OpenAPI schema ---'
sed -n '145,175p' shared/management/http/api/openapi.ymlRepository: netbirdio/netbird Length of output: 50376 🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- management job validation and mapping ---'
sed -n '1,40p' management/server/types/job.go
sed -n '130,180p' management/server/types/job.go
sed -n '235,265p' management/server/types/job.go
printf '%s\n' '--- exact upload references ---'
rg -n -C 10 --glob '!**/*_test.go' --glob '!**/types.gen.go' --glob '!**/*.pb.go' \
'upload_url|UploadUrl|UploadURL|uploadUrl' management shared client
printf '%s\n' '--- client request sinks tied to upload URL ---'
rg -n -C 12 --glob '!**/*_test.go' \
'NewRequest|http\.Client|Do\(|Put\(|Post\(|UploadUrl|upload_url' client \
| rg -n -C 6 'UploadUrl|upload_url|NewRequest|Do\(|Put\(|Post\('
printf '%s\n' '--- OpenAPI upload_url schema ---'
rg -n -C 12 'upload_url' shared/management/http/api/openapi.ymlRepository: netbirdio/netbird Length of output: 50376 🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- remote job validator and call ordering ---'
sed -n '1365,1455p' client/internal/engine.go
printf '%s\n' '--- upload request ordering and redirect handling ---'
sed -n '1,155p' client/internal/debug/upload.go
printf '%s\n' '--- management job entry points ---'
rg -n -C 8 --glob '*.go' \
'NewJob\(|ToStreamJobRequest\(|validateAndBuildBundleParams|BundleWorkloadRequest' \
management/server | head -n 240
printf '%s\n' '--- OpenAPI upload_url declaration ---'
rg -n -C 15 '^ upload_url:' shared/management/http/api/openapi.ymlRepository: netbirdio/netbird Length of output: 24185 Reject invalid
📍 Affects 2 files
🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| workload.Parameters, err = json.Marshal(bundle.Parameters) | ||
| if err != nil { | ||
|
|
@@ -209,6 +226,17 @@ func (j *Job) ToStreamJobRequest() (*proto.JobRequest, error) { | |
| } | ||
| } | ||
|
|
||
| // derefString returns the pointed-to string, or "" when the pointer is nil. | ||
| // The bundle parameters carry anonymize_level and upload_url as optional | ||
| // fields; an absent value maps to the empty proto string, which the client | ||
| // resolves to its default. | ||
| func derefString(s *string) string { | ||
| if s == nil { | ||
| return "" | ||
| } | ||
| return *s | ||
| } | ||
|
|
||
| func (j *Job) buildStreamBundleResponse() (*proto.JobRequest, error) { | ||
| var p api.BundleParameters | ||
| if err := json.Unmarshal(j.Workload.Parameters, &p); err != nil { | ||
|
|
@@ -218,10 +246,12 @@ func (j *Job) buildStreamBundleResponse() (*proto.JobRequest, error) { | |
| ID: []byte(j.ID), | ||
| WorkloadParameters: &proto.JobRequest_Bundle{ | ||
| Bundle: &proto.BundleParameters{ | ||
| BundleFor: p.BundleFor, | ||
| BundleForTime: int64(p.BundleForTime), | ||
| LogFileCount: int32(p.LogFileCount), | ||
| Anonymize: p.Anonymize, | ||
| BundleFor: p.BundleFor, | ||
| BundleForTime: int64(p.BundleForTime), | ||
| LogFileCount: int32(p.LogFileCount), | ||
| Anonymize: p.Anonymize, | ||
| AnonymizeLevel: derefString(p.AnonymizeLevel), | ||
| UploadUrl: derefString(p.UploadUrl), | ||
| }, | ||
| }, | ||
| }, nil | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,137 @@ | ||
| package types | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/netbirdio/netbird/shared/management/http/api" | ||
| ) | ||
|
|
||
| func strPtr(s string) *string { return &s } | ||
|
|
||
| // bundleJobFromParams builds a bundle Job whose stored workload parameters are | ||
| // the marshalled REST BundleParameters, mirroring what NewJob persists. | ||
| func bundleJobFromParams(t *testing.T, p api.BundleParameters) *Job { | ||
| t.Helper() | ||
| raw, err := json.Marshal(p) | ||
| require.NoError(t, err, "marshal bundle parameters") | ||
| return &Job{ | ||
| ID: "job-1", | ||
| Workload: Workload{ | ||
| Type: JobTypeBundle, | ||
| Parameters: raw, | ||
| Result: []byte("{}"), | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| // TestBuildStreamBundleResponse_CarriesIdentityAndUploadFields verifies the | ||
| // anonymize_level and upload_url REST fields are mapped onto the proto request | ||
| // the client receives. | ||
| func TestBuildStreamBundleResponse_CarriesIdentityAndUploadFields(t *testing.T) { | ||
| job := bundleJobFromParams(t, api.BundleParameters{ | ||
| BundleFor: true, | ||
| BundleForTime: 2, | ||
| LogFileCount: 100, | ||
| Anonymize: true, | ||
| AnonymizeLevel: strPtr("strict"), | ||
| UploadUrl: strPtr("https://upload.example.com"), | ||
| }) | ||
|
|
||
| req, err := job.ToStreamJobRequest() | ||
| require.NoError(t, err, "ToStreamJobRequest must succeed") | ||
|
|
||
| bundle := req.GetBundle() | ||
| require.NotNil(t, bundle, "the request must carry bundle parameters") | ||
| assert.Equal(t, "strict", bundle.GetAnonymizeLevel(), "anonymize_level must reach the client") | ||
| assert.Equal(t, "https://upload.example.com", bundle.GetUploadUrl(), "upload_url must reach the client") | ||
| assert.True(t, bundle.GetAnonymize(), "existing fields must still map") | ||
| assert.Equal(t, int32(100), bundle.GetLogFileCount(), "existing fields must still map") | ||
| } | ||
|
|
||
| // newBundleJobRequest builds an api.JobRequest carrying a bundle workload with | ||
| // the given parameters, mirroring what the REST handler decodes. | ||
| func newBundleJobRequest(t *testing.T, p api.BundleParameters) *api.JobRequest { | ||
| t.Helper() | ||
| var wr api.WorkloadRequest | ||
| require.NoError(t, wr.FromBundleWorkloadRequest(api.BundleWorkloadRequest{ | ||
| Type: api.WorkloadTypeBundle, | ||
| Parameters: p, | ||
| }), "build bundle workload request") | ||
| return &api.JobRequest{Workload: wr} | ||
| } | ||
|
|
||
| // TestNewJob_AnonymizeLevelValidation verifies the management API accepts only | ||
| // known anonymization levels (empty defaults on the client) and rejects an | ||
| // unknown value instead of silently escalating it. | ||
| func TestNewJob_AnonymizeLevelValidation(t *testing.T) { | ||
| base := api.BundleParameters{BundleFor: false, LogFileCount: 100, Anonymize: true} | ||
|
|
||
| for _, tc := range []struct { | ||
| name string | ||
| level *string | ||
| wantErr bool | ||
| }{ | ||
| {name: "omitted", level: nil}, | ||
| {name: "empty", level: strPtr("")}, | ||
| {name: "default", level: strPtr("default")}, | ||
| {name: "strict", level: strPtr("strict")}, | ||
| {name: "mixed case", level: strPtr("Strict")}, | ||
| {name: "padded", level: strPtr(" default ")}, | ||
| {name: "unknown", level: strPtr("verbose"), wantErr: true}, | ||
| } { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| p := base | ||
| p.AnonymizeLevel = tc.level | ||
| _, err := NewJob("user-1", "acc-1", "peer-1", newBundleJobRequest(t, p)) | ||
| if tc.wantErr { | ||
| require.Error(t, err, "an unknown anonymize_level must be rejected") | ||
| assert.Contains(t, err.Error(), "anonymize_level", "the error must name the offending field") | ||
| return | ||
| } | ||
| require.NoError(t, err, "a known anonymize_level must be accepted") | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // TestNewJob_AnonymizeLevelNormalized verifies an accepted level is persisted | ||
| // trimmed and lowercased, so it reaches the client as a value the client's | ||
| // lowercase-only parser resolves correctly rather than escalating to strict. | ||
| func TestNewJob_AnonymizeLevelNormalized(t *testing.T) { | ||
| job, err := NewJob("user-1", "acc-1", "peer-1", newBundleJobRequest(t, api.BundleParameters{ | ||
| BundleFor: false, | ||
| LogFileCount: 100, | ||
| Anonymize: true, | ||
| AnonymizeLevel: strPtr(" Default "), | ||
| })) | ||
| require.NoError(t, err, "a padded known level must be accepted") | ||
|
|
||
| req, err := job.ToStreamJobRequest() | ||
| require.NoError(t, err, "ToStreamJobRequest must succeed") | ||
| assert.Equal(t, "default", req.GetBundle().GetAnonymizeLevel(), | ||
| "the persisted level must be normalized so the client does not resolve it to strict") | ||
| } | ||
|
|
||
| // TestBuildStreamBundleResponse_OmittedFieldsMapToEmpty verifies that omitted | ||
| // optional fields map to the empty proto string, which the client resolves to | ||
| // its defaults (default anonymization level, default upload server). | ||
| func TestBuildStreamBundleResponse_OmittedFieldsMapToEmpty(t *testing.T) { | ||
| job := bundleJobFromParams(t, api.BundleParameters{ | ||
| BundleFor: false, | ||
| BundleForTime: 1, | ||
| LogFileCount: 50, | ||
| Anonymize: false, | ||
| // AnonymizeLevel and UploadUrl intentionally nil. | ||
| }) | ||
|
|
||
| req, err := job.ToStreamJobRequest() | ||
| require.NoError(t, err, "ToStreamJobRequest must succeed") | ||
|
|
||
| bundle := req.GetBundle() | ||
| require.NotNil(t, bundle, "the request must carry bundle parameters") | ||
| assert.Empty(t, bundle.GetAnonymizeLevel(), "an omitted anonymize_level must map to empty so the client defaults it") | ||
| assert.Empty(t, bundle.GetUploadUrl(), "an omitted upload_url must map to empty so the client defaults it") | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -154,6 +154,14 @@ components: | |||||||||||||||||||
| type: boolean | ||||||||||||||||||||
| description: Whether sensitive data should be anonymized in the bundle. | ||||||||||||||||||||
| example: false | ||||||||||||||||||||
| anonymize_level: | ||||||||||||||||||||
|
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. P2: Clients following this schema can send any string for Prompt for AI agents |
||||||||||||||||||||
| type: string | ||||||||||||||||||||
| description: How much the anonymizer redacts. "default" (or empty) keeps internal IP ranges, "strict" also anonymizes them. | ||||||||||||||||||||
| example: strict | ||||||||||||||||||||
| upload_url: | ||||||||||||||||||||
| type: string | ||||||||||||||||||||
| description: Service URL the client requests an upload URL from before uploading the bundle. Empty selects the default upload server. | ||||||||||||||||||||
| example: https://upload.debug.netbird.io | ||||||||||||||||||||
|
Comment on lines
+161
to
+164
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. P2: Prompt for AI agents
Suggested change
|
||||||||||||||||||||
| required: | ||||||||||||||||||||
| - bundle_for | ||||||||||||||||||||
| - bundle_for_time | ||||||||||||||||||||
|
|
||||||||||||||||||||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.