From 5cbe4c87136701ab643658044a13a8cdde9580cd Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 17:35:09 +1000 Subject: [PATCH] stop serving another node's factset Also strip a leading BOM and stop reporting a certname mismatch as a parse error. --- internal/pdbmux/pdbmux.go | 52 ++++++++++++----- internal/pdbmux/pdbmux_test.go | 103 ++++++++++++++++++++++++++++++++- 2 files changed, 140 insertions(+), 15 deletions(-) diff --git a/internal/pdbmux/pdbmux.go b/internal/pdbmux/pdbmux.go index ef43d6b..c99b042 100644 --- a/internal/pdbmux/pdbmux.go +++ b/internal/pdbmux/pdbmux.go @@ -83,7 +83,7 @@ func (c *Client) Facts(ctx context.Context, certname string) (map[string]any, er records, err := decodeFactsets(body, certname) if err != nil { - return nil, fmt.Errorf("parsing factset for %s: %w", certname, err) + return nil, err } if len(records) == 0 { return nil, fmt.Errorf("%w: %s", ErrNodeNotFound, certname) @@ -93,34 +93,60 @@ func (c *Client) Facts(ctx context.Context, certname string) (map[string]any, er // decodeFactsets reads the two shapes /factsets/ comes back as: real // PuppetDB returns an array of factsets, pdbmux currently returns a single -// bare object. +// bare object. A factset belonging to another node is never substituted for +// the one that was asked for. func decodeFactsets(body []byte, certname string) ([]factRecord, error) { + body = bytes.TrimPrefix(body, []byte("\xef\xbb\xbf")) + if bytes.HasPrefix(bytes.TrimLeft(body, " \t\r\n"), []byte("[")) { var sets []factset if err := json.Unmarshal(body, &sets); err != nil { - return nil, err - } - for _, fs := range sets { - if fs.Certname == certname { - return fs.Facts.Data, nil - } - } - if len(sets) == 1 { - return sets[0].Facts.Data, nil + return nil, fmt.Errorf("parsing factset for %s: %w", certname, err) } if len(sets) == 0 { return nil, nil } - return nil, fmt.Errorf("%d factsets returned, none for the requested certname", len(sets)) + for _, fs := range sets { + if sameCertname(fs.Certname, certname) { + return fs.Facts.Data, nil + } + } + if len(sets) == 1 && sets[0].Certname == "" { + return sets[0].Facts.Data, nil + } + return nil, fmt.Errorf("no factset for %s: pdbmux returned %s", + certname, strings.Join(certnames(sets), ", ")) } var fs factset if err := json.Unmarshal(body, &fs); err != nil { - return nil, err + return nil, fmt.Errorf("parsing factset for %s: %w", certname, err) + } + if fs.Certname != "" && !sameCertname(fs.Certname, certname) { + return nil, fmt.Errorf("no factset for %s: pdbmux returned %s", certname, fs.Certname) } return fs.Facts.Data, nil } +// sameCertname compares certnames case-insensitively: they are DNS names, and +// PuppetDB matches the per-node route exactly, so folding can only absorb a +// backend that normalised case, never mask a different node. +func sameCertname(got, want string) bool { + return got != "" && strings.EqualFold(got, want) +} + +func certnames(sets []factset) []string { + names := make([]string, 0, len(sets)) + for _, fs := range sets { + if fs.Certname == "" { + names = append(names, "(unnamed)") + continue + } + names = append(names, fs.Certname) + } + return names +} + func (c *Client) get(ctx context.Context, path string) ([]byte, int, error) { req, err := http.NewRequestWithContext(ctx, http.MethodGet, c.BaseURL+path, nil) if err != nil { diff --git a/internal/pdbmux/pdbmux_test.go b/internal/pdbmux/pdbmux_test.go index d8149fe..888315a 100644 --- a/internal/pdbmux/pdbmux_test.go +++ b/internal/pdbmux/pdbmux_test.go @@ -225,8 +225,107 @@ func TestFactsRejectsAFactsetArrayWithNoMatch(t *testing.T) { if err == nil { t.Fatal("expected an error when no factset matches the certname") } - if !strings.Contains(err.Error(), "none for the requested certname") { - t.Errorf("unhelpful error: %v", err) + assertNamesBoth(t, err, "host.example.net", "a.example.net", "b.example.net") +} + +// assertNamesBoth checks the mismatch error names the node asked for and the +// ones that came back, and that it is not dressed up as a parse failure. +func assertNamesBoth(t *testing.T, err error, want string, got ...string) { + t.Helper() + if !strings.Contains(err.Error(), want) { + t.Errorf("error does not name the requested certname %s: %v", want, err) + } + for _, g := range got { + if !strings.Contains(err.Error(), g) { + t.Errorf("error does not name the returned certname %s: %v", g, err) + } + } + if strings.Contains(err.Error(), "parsing factset") { + t.Errorf("a certname mismatch is not a parse failure: %v", err) + } +} + +func TestFactsRejectsASoleFactsetForAnotherNode(t *testing.T) { + srv := factsetServer(t, `[{ + "certname":"totally-different-host.example.net", + "facts":{"data":[ + {"name":"id","value":"nobody"}, + {"name":"os","value":{"family":"Debian"}} + ]} + }]`) + + facts, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net") + if err == nil { + t.Fatalf("another node's facts were served as ours: %#v", facts) + } + assertNamesBoth(t, err, "host.example.net", "totally-different-host.example.net") +} + +func TestFactsRejectsABareFactsetForAnotherNode(t *testing.T) { + srv := factsetServer(t, `{ + "certname":"totally-different-host.example.net", + "facts":{"data":[{"name":"id","value":"nobody"}]} + }`) + + facts, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net") + if err == nil { + t.Fatalf("another node's facts were served as ours: %#v", facts) + } + assertNamesBoth(t, err, "host.example.net", "totally-different-host.example.net") +} + +// The per-node route already names the node, so a factset that asserts no +// certname of its own cannot be contradicting it. +func TestFactsAcceptsAFactsetWithNoCertname(t *testing.T) { + body := `{"facts":{"data":[ + {"name":"id","value":"root"}, + {"name":"os","value":{"family":"RedHat","release":{"major":"9"}}}, + {"name":"processorcount","value":4} + ]}}` + + for name, resp := range map[string]string{"object": body, "array": "[" + body + "]"} { + t.Run(name, func(t *testing.T) { + srv := factsetServer(t, resp) + facts, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net") + if err != nil { + t.Fatalf("Facts: %v", err) + } + assertFacts(t, facts) + }) + } +} + +func TestFactsMatchesCertnameCaseInsensitively(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasPrefix(r.URL.Path, "/pdb/query/v4/factsets/") { + _, _ = w.Write([]byte(factsetBody)) + return + } + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":"unknown node"}`)) + })) + defer srv.Close() + + facts, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "HOST.example.net") + if err != nil { + t.Fatalf("Facts: %v", err) + } + assertFacts(t, facts) +} + +func TestFactsToleratesALeadingBOM(t *testing.T) { + for name, resp := range map[string]string{ + "array": "\ufeff" + factsetBody, + "object": "\ufeff" + bareFactsetBody, + } { + t.Run(name, func(t *testing.T) { + srv := factsetServer(t, resp) + facts, err := New(srv.URL, 5*time.Second).Facts(context.Background(), "host.example.net") + if err != nil { + t.Fatalf("Facts: %v", err) + } + assertFacts(t, facts) + }) } }