Skip to content

Commit 85e01e1

Browse files
committed
security(operator): validate LLM base URL and audit changes
A RoleOperator could redirect the Anthropic base URL to any host via POST /operator/api/llm/settings, exfiltrating the bearer credential on the next ping or /suggest-rule call. Address per review feedback: - validateLLMBaseURL enforces https for Anthropic (+ any non-Ollama provider), rejects userinfo / opaque URIs, and honours an optional host allowlist via LLM_BASE_URL_ALLOWED_HOSTS. - handleLLMSettings now persists a config_change AuditEvent (actor = GitHub login, old/new base URL + model) so changes are detectable after the fact. - AuditEvent gains an Actor field; AuditLogger gains LogConfigChangeEvent.
1 parent fc05f7d commit 85e01e1

3 files changed

Lines changed: 590 additions & 20 deletions

File tree

services/audit_logger.go

Lines changed: 36 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -14,27 +14,31 @@ import (
1414
type AuditEventType string
1515

1616
const (
17-
AuditEventCopy AuditEventType = "copy"
18-
AuditEventDeprecation AuditEventType = "deprecation"
19-
AuditEventError AuditEventType = "error"
17+
AuditEventCopy AuditEventType = "copy"
18+
AuditEventDeprecation AuditEventType = "deprecation"
19+
AuditEventError AuditEventType = "error"
20+
AuditEventConfigChange AuditEventType = "config_change"
2021
)
2122

2223
// AuditEvent represents an audit log entry
2324
type AuditEvent struct {
24-
ID string `bson:"_id,omitempty" json:"id,omitempty"`
25-
Timestamp time.Time `bson:"timestamp" json:"timestamp"`
26-
EventType AuditEventType `bson:"event_type" json:"event_type"`
27-
RuleName string `bson:"rule_name,omitempty" json:"rule_name,omitempty"`
28-
SourceRepo string `bson:"source_repo" json:"source_repo"`
29-
SourcePath string `bson:"source_path" json:"source_path"`
30-
TargetRepo string `bson:"target_repo,omitempty" json:"target_repo,omitempty"`
31-
TargetPath string `bson:"target_path,omitempty" json:"target_path,omitempty"`
32-
CommitSHA string `bson:"commit_sha,omitempty" json:"commit_sha,omitempty"`
33-
PRNumber int `bson:"pr_number,omitempty" json:"pr_number,omitempty"`
34-
Success bool `bson:"success" json:"success"`
35-
ErrorMessage string `bson:"error_message,omitempty" json:"error_message,omitempty"`
36-
DurationMs int64 `bson:"duration_ms,omitempty" json:"duration_ms,omitempty"`
37-
FileSize int64 `bson:"file_size,omitempty" json:"file_size,omitempty"`
25+
ID string `bson:"_id,omitempty" json:"id,omitempty"`
26+
Timestamp time.Time `bson:"timestamp" json:"timestamp"`
27+
EventType AuditEventType `bson:"event_type" json:"event_type"`
28+
RuleName string `bson:"rule_name,omitempty" json:"rule_name,omitempty"`
29+
SourceRepo string `bson:"source_repo" json:"source_repo"`
30+
SourcePath string `bson:"source_path" json:"source_path"`
31+
TargetRepo string `bson:"target_repo,omitempty" json:"target_repo,omitempty"`
32+
TargetPath string `bson:"target_path,omitempty" json:"target_path,omitempty"`
33+
CommitSHA string `bson:"commit_sha,omitempty" json:"commit_sha,omitempty"`
34+
PRNumber int `bson:"pr_number,omitempty" json:"pr_number,omitempty"`
35+
Success bool `bson:"success" json:"success"`
36+
ErrorMessage string `bson:"error_message,omitempty" json:"error_message,omitempty"`
37+
DurationMs int64 `bson:"duration_ms,omitempty" json:"duration_ms,omitempty"`
38+
FileSize int64 `bson:"file_size,omitempty" json:"file_size,omitempty"`
39+
// Actor identifies the GitHub login responsible for an operator-initiated
40+
// event (e.g. an LLM settings change). Empty for webhook-driven copies.
41+
Actor string `bson:"actor,omitempty" json:"actor,omitempty"`
3842
AdditionalData map[string]any `bson:"additional_data,omitempty" json:"additional_data,omitempty"`
3943
}
4044

@@ -43,6 +47,10 @@ type AuditLogger interface {
4347
LogCopyEvent(ctx context.Context, event *AuditEvent) error
4448
LogDeprecationEvent(ctx context.Context, event *AuditEvent) error
4549
LogErrorEvent(ctx context.Context, event *AuditEvent) error
50+
// LogConfigChangeEvent records an operator-initiated configuration change
51+
// (e.g. LLM base URL or active model). Used for after-the-fact detection
52+
// of suspicious changes — the event captures who, what, and old→new value.
53+
LogConfigChangeEvent(ctx context.Context, event *AuditEvent) error
4654
GetRecentEvents(ctx context.Context, limit int) ([]AuditEvent, error)
4755
GetFailedEvents(ctx context.Context, limit int) ([]AuditEvent, error)
4856
GetEventsByRule(ctx context.Context, ruleName string, limit int) ([]AuditEvent, error)
@@ -183,6 +191,14 @@ func (mal *MongoAuditLogger) LogErrorEvent(ctx context.Context, event *AuditEven
183191
return err
184192
}
185193

194+
// LogConfigChangeEvent logs an operator-initiated configuration change.
195+
func (mal *MongoAuditLogger) LogConfigChangeEvent(ctx context.Context, event *AuditEvent) error {
196+
event.EventType = AuditEventConfigChange
197+
event.Timestamp = time.Now()
198+
_, err := mal.collection.InsertOne(ctx, event)
199+
return err
200+
}
201+
186202
// QueryAuditEvents retrieves audit events matching the given filter criteria.
187203
func (mal *MongoAuditLogger) QueryAuditEvents(ctx context.Context, q AuditListQuery) ([]AuditEvent, error) {
188204
limit := q.Limit
@@ -362,6 +378,9 @@ func (nal *NoOpAuditLogger) LogDeprecationEvent(ctx context.Context, event *Audi
362378
return nil
363379
}
364380
func (nal *NoOpAuditLogger) LogErrorEvent(ctx context.Context, event *AuditEvent) error { return nil }
381+
func (nal *NoOpAuditLogger) LogConfigChangeEvent(ctx context.Context, event *AuditEvent) error {
382+
return nil
383+
}
365384
func (nal *NoOpAuditLogger) GetRecentEvents(ctx context.Context, limit int) ([]AuditEvent, error) {
366385
return []AuditEvent{}, nil
367386
}

services/operator_llm_admin.go

Lines changed: 153 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,96 @@ import (
77
"fmt"
88
"io"
99
"net/http"
10+
"net/url"
11+
"os"
1012
"strings"
1113
"time"
1214
)
1315

16+
// llmBaseURLAllowedHostsEnv lets deployments pin the set of hosts an operator
17+
// can route the LLM client at (comma-separated, host[:port], case-insensitive).
18+
// Unset = no host pinning; scheme rules below still apply.
19+
const llmBaseURLAllowedHostsEnv = "LLM_BASE_URL_ALLOWED_HOSTS"
20+
21+
// validateLLMBaseURL enforces scheme + host rules on operator-supplied LLM base
22+
// URLs. Hosted providers (anthropic) ship a bearer credential to whatever host
23+
// the client points at, so we require https and reject userinfo / opaque URIs;
24+
// otherwise a malicious operator could exfiltrate the API key by setting the
25+
// base URL to a host they control. Ollama is exempt from the https requirement
26+
// because the legitimate default is http://localhost:11434 for local dev.
27+
//
28+
// Returns the cleaned URL (trailing slash trimmed) or an error suitable for a
29+
// 400 response. allowedHosts may be empty to skip host pinning.
30+
func validateLLMBaseURL(provider, raw string, allowedHosts []string) (string, error) {
31+
raw = strings.TrimSpace(raw)
32+
if raw == "" {
33+
return "", fmt.Errorf("base_url is required")
34+
}
35+
u, err := url.Parse(raw)
36+
if err != nil {
37+
return "", fmt.Errorf("base_url is not a valid URL: %w", err)
38+
}
39+
if u.Scheme == "" || u.Host == "" {
40+
return "", fmt.Errorf("base_url must be an absolute URL with scheme and host")
41+
}
42+
// Reject userinfo (https://attacker@victim.com/) and opaque forms — both
43+
// confuse host-based allowlisting and have no legitimate use here.
44+
if u.User != nil {
45+
return "", fmt.Errorf("base_url must not contain userinfo")
46+
}
47+
if u.Opaque != "" {
48+
return "", fmt.Errorf("base_url must not be opaque")
49+
}
50+
scheme := strings.ToLower(u.Scheme)
51+
prov := strings.ToLower(strings.TrimSpace(provider))
52+
switch prov {
53+
case "anthropic":
54+
if scheme != "https" {
55+
return "", fmt.Errorf("base_url for anthropic must use https (got %q)", scheme)
56+
}
57+
case "", "ollama":
58+
// Local dev commonly uses http://localhost:11434.
59+
if scheme != "http" && scheme != "https" {
60+
return "", fmt.Errorf("base_url must use http or https (got %q)", scheme)
61+
}
62+
default:
63+
if scheme != "https" {
64+
return "", fmt.Errorf("base_url must use https (got %q)", scheme)
65+
}
66+
}
67+
if len(allowedHosts) > 0 {
68+
host := strings.ToLower(u.Host)
69+
ok := false
70+
for _, h := range allowedHosts {
71+
if strings.EqualFold(strings.TrimSpace(h), host) {
72+
ok = true
73+
break
74+
}
75+
}
76+
if !ok {
77+
return "", fmt.Errorf("base_url host %q is not in the allowlist", u.Host)
78+
}
79+
}
80+
return strings.TrimSuffix(raw, "/"), nil
81+
}
82+
83+
// llmBaseURLAllowedHosts reads and parses LLM_BASE_URL_ALLOWED_HOSTS. Returns
84+
// nil when unset so callers know to skip host pinning.
85+
func llmBaseURLAllowedHosts() []string {
86+
raw := strings.TrimSpace(os.Getenv(llmBaseURLAllowedHostsEnv))
87+
if raw == "" {
88+
return nil
89+
}
90+
parts := strings.Split(raw, ",")
91+
out := make([]string, 0, len(parts))
92+
for _, p := range parts {
93+
if p = strings.TrimSpace(p); p != "" {
94+
out = append(out, p)
95+
}
96+
}
97+
return out
98+
}
99+
14100
// handleLLMStatus returns the current LLM settings, reachability, and installed models.
15101
func (o *operatorUI) handleLLMStatus(w http.ResponseWriter, r *http.Request) {
16102
if r.Method != http.MethodGet {
@@ -91,27 +177,91 @@ func (o *operatorUI) handleLLMSettings(w http.ResponseWriter, r *http.Request) {
91177
_ = json.NewEncoder(w).Encode(map[string]string{"error": "invalid json"})
92178
return
93179
}
180+
// Capture pre-change state for the audit record. This must happen before
181+
// any setter call so the "before" value reflects what the operator changed
182+
// from, not what they changed to.
183+
oldModel := o.llm.GetActiveModel()
184+
oldBaseURL := o.llm.GetBaseURL()
94185
changed := false
95-
if m := strings.TrimSpace(req.ActiveModel); m != "" {
186+
newModel := oldModel
187+
newBaseURL := oldBaseURL
188+
if m := strings.TrimSpace(req.ActiveModel); m != "" && m != oldModel {
96189
o.llm.SetActiveModel(m)
190+
newModel = o.llm.GetActiveModel()
97191
changed = true
98192
}
99193
if u := strings.TrimSpace(req.BaseURL); u != "" {
100-
o.llm.SetBaseURL(u)
101-
changed = true
194+
// Validate before applying. The Anthropic client ships the bearer
195+
// credential to whatever host the base URL points at — without
196+
// scheme/host validation, an operator could redirect the credential
197+
// to a host they control.
198+
cleaned, err := validateLLMBaseURL(o.cfg.LLMProvider, u, llmBaseURLAllowedHosts())
199+
if err != nil {
200+
w.WriteHeader(http.StatusBadRequest)
201+
_ = json.NewEncoder(w).Encode(map[string]string{"error": err.Error()})
202+
return
203+
}
204+
if cleaned != oldBaseURL {
205+
o.llm.SetBaseURL(cleaned)
206+
newBaseURL = o.llm.GetBaseURL()
207+
changed = true
208+
}
102209
}
103210
// Invalidate the ping cache on mutation so the next /llm/status call
104211
// re-checks liveness against the new config — otherwise an operator
105212
// flipping the URL sees a stale "connected" line for up to 30s.
106213
if changed {
107214
o.llmPing.invalidate()
215+
o.recordLLMSettingsAudit(r, oldBaseURL, newBaseURL, oldModel, newModel)
108216
}
109217
_ = json.NewEncoder(w).Encode(map[string]any{
110218
"active_model": o.llm.GetActiveModel(),
111219
"base_url": o.llm.GetBaseURL(),
112220
})
113221
}
114222

223+
// recordLLMSettingsAudit emits a structured log line and (when MongoDB audit
224+
// logging is enabled) persists a config_change event capturing who changed
225+
// what. This is the detection backstop for the SetBaseURL credential-exfil
226+
// risk: scheme/host validation blocks the obvious cases, but a persisted
227+
// trail of every successful change lets responders spot abuse after the fact.
228+
func (o *operatorUI) recordLLMSettingsAudit(r *http.Request, oldBaseURL, newBaseURL, oldModel, newModel string) {
229+
actor := ""
230+
if u := operatorUserFromCtx(r); u != nil {
231+
actor = u.Login
232+
}
233+
LogInfo("operator changed LLM settings",
234+
"actor", actor,
235+
"provider", o.cfg.LLMProvider,
236+
"old_base_url", oldBaseURL,
237+
"new_base_url", newBaseURL,
238+
"old_model", oldModel,
239+
"new_model", newModel,
240+
)
241+
if o.container == nil || o.container.AuditLogger == nil {
242+
return
243+
}
244+
// Use a detached short-timeout context: writing the audit row must not
245+
// fail just because the client disconnected after receiving the 200.
246+
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
247+
defer cancel()
248+
ev := &AuditEvent{
249+
Actor: actor,
250+
Success: true,
251+
AdditionalData: map[string]any{
252+
"setting": "llm",
253+
"provider": o.cfg.LLMProvider,
254+
"old_base_url": oldBaseURL,
255+
"new_base_url": newBaseURL,
256+
"old_model": oldModel,
257+
"new_model": newModel,
258+
},
259+
}
260+
if err := o.container.AuditLogger.LogConfigChangeEvent(ctx, ev); err != nil {
261+
LogWarning("audit LogConfigChangeEvent failed", "error", err)
262+
}
263+
}
264+
115265
// handleLLMDeleteModel deletes a model from the LLM server.
116266
func (o *operatorUI) handleLLMDeleteModel(w http.ResponseWriter, r *http.Request) {
117267
if r.Method != http.MethodDelete {

0 commit comments

Comments
 (0)