mirror of
https://github.com/netbirdio/netbird.git
synced 2026-08-24 16:41:30 +02:00
[management,client] Normalize anonymize_level and sanity-check the bundle upload URL
Two review follow-ups. The API validated anonymize_level after trimming and lowercasing but persisted the value verbatim, so " default " passed as default yet reached the client — which only lowercases — as an unrecognized value it resolves to strict. Persist the normalized form so what was validated is what the client parses. The remote debug bundle job forwarded the management-supplied upload URL to the uploader unchecked and logged it at info level, where it can leak a host, credentials, or query tokens. Reject a malformed or non-https URL before generating the bundle, and keep the URL out of the info-level line while leaving the full parameters at debug. The accepted host is left unrestricted for now, pending a decision on management-directed uploads.
This commit is contained in:
@@ -1365,7 +1365,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())
|
||||
|
||||
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)
|
||||
@@ -1406,6 +1416,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() {
|
||||
|
||||
35
client/internal/engine_bundle_test.go
Normal file
35
client/internal/engine_bundle_test.go
Normal file
@@ -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")
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -154,13 +154,18 @@ func validateAndBuildBundleParams(req api.WorkloadRequest, workload *Workload) e
|
||||
}
|
||||
// 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.
|
||||
// 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 {
|
||||
switch strings.ToLower(strings.TrimSpace(*lvl)) {
|
||||
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
|
||||
}
|
||||
|
||||
workload.Parameters, err = json.Marshal(bundle.Parameters)
|
||||
|
||||
@@ -80,6 +80,7 @@ func TestNewJob_AnonymizeLevelValidation(t *testing.T) {
|
||||
{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) {
|
||||
@@ -96,6 +97,24 @@ func TestNewJob_AnonymizeLevelValidation(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// 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).
|
||||
|
||||
Reference in New Issue
Block a user