diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 106b68106..3a757f1c4 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -137,6 +137,7 @@ Request controls are composed at route registration: - `cmd/api/auth.go`: `handlerAuthCheck(...)` accepts a bearer JWT or the SPA's JWT cookie, verifies it against the user's currently stored token, enforces CSRF on cookie-authenticated mutations, and attaches the username to request context. - Login, pre-auth discovery, service-config apply, and TLS enrollment have configurable in-memory per-IP rate limits. +- `osctrld` routes in `osctrl-tls` (`rateLimits.osctrldAuth`, `osctrld`, `osctrldDownload`; `cmd/tls/handlers/osctrld_limits.go`): every request is limited per client IP (default 300/min, extension downloads also 30/min), and wrong environment secrets are charged to a per-IP-and-environment budget (default 10/hour). Once that budget is spent the IP gets 429 with `Retry-After` on that environment's `osctrld` routes, even with the right secret: checked before the body is read, and again atomically with the secret comparison (`ratelimit.Limiter.Attempt`), so concurrent requests cannot exceed it. Correct secrets never charge it, so hosts sharing a NAT are unaffected. The client IP is `utils.GetIP`: behind a reverse proxy or load balancer, `service.trustedProxies` must list it, otherwise all clients share the proxy's budgets and one bad client can block the fleet. Past `maxBuckets` tracked keys, new keys share one overflow budget and are blocked together. Every wrong secret is audited (while blocked, one rejection per budget period per IP and environment), capped at 60 audit entries per hour per environment; drops are counted. Metrics: `osctrld_auth_failures_total{environment}`, `osctrld_rate_limited_total{route,limit}`, `osctrld_auth_audit_dropped_total{environment}`, and request duration/status by route pattern. Limits are per process. - Forwarded client IP headers are ignored unless the peer matches configured trusted-proxy CIDRs. - `cmd/tls/handlers/handlers.go`: `PrometheusMiddleware(...)` records request duration/status for enabled osquery-facing endpoints; handlers also apply endpoint-specific body-size caps. diff --git a/cmd/tls/config_test.go b/cmd/tls/config_test.go index a2018859d..5e9f24596 100644 --- a/cmd/tls/config_test.go +++ b/cmd/tls/config_test.go @@ -8,6 +8,7 @@ import ( "time" "github.com/jmpsec/osctrl/pkg/config" + "github.com/spf13/viper" "github.com/urfave/cli/v3" ) @@ -151,6 +152,20 @@ func TestSampleTLSConfigLoads(t *testing.T) { if params.Osquery == nil { t.Fatal("sample tls.yml did not load osquery") } + // The sample documents every rate limit with its built-in default. + v := viper.New() + v.SetConfigFile(filepath.Join("..", "..", "deploy", "config", "tls.yml")) + if err := v.ReadInConfig(); err != nil { + t.Fatal(err) + } + for _, name := range config.TLSRateLimits { + if !v.IsSet("rateLimits." + name + ".burst") { + t.Errorf("sample tls.yml does not document rateLimits.%s", name) + } + } + if *params.RateLimits != config.DefaultRateLimits() { + t.Fatalf("sample tls.yml rateLimits = %+v, want the defaults", *params.RateLimits) + } } func TestConfigVersionMismatchDoesNotFailLoad(t *testing.T) { @@ -175,3 +190,37 @@ redis: t.Fatalf("cfg.Version = %d, want 999", cfg.Version) } } + +// A tls.yml written before the osctrld limits existed has a rateLimits block +// without them; it must still load, with the built-in osctrld defaults. +func TestLoadedYAMLKeepsOsctrldRateLimitDefaults(t *testing.T) { + const body = ` +service: + auth: none +rateLimits: + enroll: + burst: 9 + period: 2m + evictAfter: 12m + retryAfter: 45 + osctrld: + burst: 50 +` + cfg, err := loadYAMLConfiguration(writeTempTLSConfig(t, body)) + if err != nil { + t.Fatalf("loadYAMLConfiguration: %v", err) + } + params := loadedYAMLToServiceParams(cfg, "tls.yml") + d := config.DefaultRateLimits() + if params.RateLimits.OsctrldAuth != d.OsctrldAuth || params.RateLimits.OsctrldDownload != d.OsctrldDownload { + t.Fatalf("osctrld limits = %+v / %+v, want defaults", params.RateLimits.OsctrldAuth, params.RateLimits.OsctrldDownload) + } + want := d.Osctrld + want.Burst = 50 + if params.RateLimits.Osctrld != want { + t.Fatalf("osctrld = %+v, want %+v", params.RateLimits.Osctrld, want) + } + if params.RateLimits.Enroll.Burst != 9 { + t.Fatalf("enroll burst = %d, want 9", params.RateLimits.Enroll.Burst) + } +} diff --git a/cmd/tls/handlers/extensions.go b/cmd/tls/handlers/extensions.go index d220bb249..0e7f4c5cc 100644 --- a/cmd/tls/handlers/extensions.go +++ b/cmd/tls/handlers/extensions.go @@ -50,8 +50,7 @@ func (h *HandlersTLS) osctrldRequest(w http.ResponseWriter, r *http.Request, max utils.HTTPResponse(w, "", http.StatusBadRequest, []byte("")) return env, false } - if !h.checkValidSecret(secret(), env) { - utils.HTTPResponse(w, "", http.StatusForbidden, []byte("")) + if !h.osctrldSecret(w, r, secret(), env) { return env, false } return env, true diff --git a/cmd/tls/handlers/handlers.go b/cmd/tls/handlers/handlers.go index ca7e629fc..b8aea507d 100644 --- a/cmd/tls/handlers/handlers.go +++ b/cmd/tls/handlers/handlers.go @@ -83,6 +83,7 @@ type HandlersTLS struct { NodeConfig *nodeconfig.Manager // nil: per-node config overrides are inert FlagsOverrides *nodeconfig.FlagsManager // nil: per-node flags overrides are inert Extensions *extensions.Manager // nil: osctrld extension routes are not registered + OsctrldLimits *OsctrldLimits // nil: osctrld routes are not rate-limited // DBHealth, when wired via WithDBHealth, lets HealthHandler // report backend degradation. nil means "no monitor" and the // health endpoint reports healthy as before. @@ -438,6 +439,11 @@ func NewResponseWriter(w http.ResponseWriter) *responseWriter { return &responseWriter{w, http.StatusOK} } +// Unwrap lets http.ResponseController reach the underlying writer. +func (rw *responseWriter) Unwrap() http.ResponseWriter { + return rw.ResponseWriter +} + func (rw *responseWriter) WriteHeader(code int) { rw.statusCode = code rw.ResponseWriter.WriteHeader(code) diff --git a/cmd/tls/handlers/metrics.go b/cmd/tls/handlers/metrics.go index d9be6735b..9897b22c7 100644 --- a/cmd/tls/handlers/metrics.go +++ b/cmd/tls/handlers/metrics.go @@ -37,6 +37,24 @@ var ( Help: "The duration of batch data flushing to backend", Buckets: []float64{0.001, 0.005, 0.01, 0.05, 0.1, 0.5, 1, 2, 5}, }, []string{"operation"}) + // osctrldAuthFailures counts wrong osctrld secrets per environment name + // (only resolved environments, so cardinality stays bounded). + osctrldAuthFailures = prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "osctrld_auth_failures_total", + Help: "osctrld requests rejected for a wrong environment secret", + }, []string{"environment"}) + // osctrldRateLimited counts osctrld requests rejected with 429, by + // route pattern and rate limit (osctrldAuth, osctrld, osctrldDownload). + osctrldRateLimited = prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "osctrld_rate_limited_total", + Help: "osctrld requests rejected by a rate limit", + }, []string{"route", "limit"}) + // osctrldAuthAuditDropped counts failed-secret audit entries dropped by + // the per-environment audit cap. + osctrldAuthAuditDropped = prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "osctrld_auth_audit_dropped_total", + Help: "osctrld failed-secret audit entries dropped by the per-environment cap", + }, []string{"environment"}) // DBDegraded is 1 when the DB health monitor reports the database // as degraded (caches are in stale-serve mode), 0 when healthy. dbDegraded = prometheus.NewGauge(prometheus.GaugeOpts{ @@ -52,6 +70,9 @@ func RegisterMetrics(reg prometheus.Registerer) { reg.MustRegister(distributedQueryProcessingDuration) reg.MustRegister(batchFlushDuration) reg.MustRegister(dbDegraded) + reg.MustRegister(osctrldAuthFailures) + reg.MustRegister(osctrldRateLimited) + reg.MustRegister(osctrldAuthAuditDropped) } // SetDBDegraded updates the db_degraded gauge. Called by the DB diff --git a/cmd/tls/handlers/osctrld_limits.go b/cmd/tls/handlers/osctrld_limits.go new file mode 100644 index 000000000..b856c5f1a --- /dev/null +++ b/cmd/tls/handlers/osctrld_limits.go @@ -0,0 +1,168 @@ +package handlers + +import ( + "net/http" + "strconv" + "strings" + "time" + + "github.com/jmpsec/osctrl/pkg/config" + "github.com/jmpsec/osctrl/pkg/environments" + "github.com/jmpsec/osctrl/pkg/ratelimit" + "github.com/jmpsec/osctrl/pkg/utils" +) + +// osctrldAuditPerHour caps failed-secret audit writes per environment, so +// a spray from many addresses cannot flood audit_logs. +const osctrldAuditPerHour = 60 + +// OsctrldLimits holds the per-process rate limiters of the osctrld routes. +// They key on utils.GetIP: the TCP peer, or the forwarded client when the +// peer is a configured trusted proxy (otherwise every client behind a proxy +// shares the proxy's budgets). +type OsctrldLimits struct { + // Auth is charged only by wrong secrets, per address and environment. + Auth *ratelimit.Limiter + // Traffic is charged by every osctrld request, per address. + Traffic *ratelimit.Limiter + // Download is charged by every extension download, per address. + Download *ratelimit.Limiter + // authAudit lets one blocked rejection per Auth period reach the + // audit log, per address and environment. + authAudit *ratelimit.Limiter + // auditCap bounds failed-secret audit writes per environment. + auditCap *ratelimit.Limiter +} + +// NewOsctrldLimits builds the osctrld limiters from the rate-limit config. +func NewOsctrldLimits(cfg config.YAMLConfigurationRateLimits) *OsctrldLimits { + audit := cfg.OsctrldAuth + audit.Burst = 1 + return &OsctrldLimits{ + Auth: ratelimit.NewFromConfig(cfg.OsctrldAuth), + Traffic: ratelimit.NewFromConfig(cfg.Osctrld), + Download: ratelimit.NewFromConfig(cfg.OsctrldDownload), + authAudit: ratelimit.NewFromConfig(audit), + auditCap: ratelimit.New(osctrldAuditPerHour, time.Hour, 2*time.Hour), + } +} + +// WithOsctrldLimits rate-limits the osctrld routes; nil leaves them unlimited. +func WithOsctrldLimits(l *OsctrldLimits) Option { + return func(h *HandlersTLS) { + h.OsctrldLimits = l + } +} + +// osctrldAuthKey keys the failed-secret budget by address and environment +// UUID, case-folded so spellings of one environment share a budget. +func osctrldAuthKey(ip, envUUID string) string { + return ip + "|" + strings.ToLower(envUUID) +} + +// OsctrldMiddleware wraps an osctrld route: the per-address traffic limit +// (and the download limit when download is set), then the failed-secret +// block, which turns a blocked address away before its body is read. +// Requests and latency are recorded by route pattern. +func (h *HandlersTLS) OsctrldMiddleware(download bool, next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + start := time.Now() + rw := NewResponseWriter(w) + h.osctrldLimit(rw, r, download, next) + requestDuration.WithLabelValues(r.Method, r.Pattern, strconv.Itoa(rw.statusCode)).Observe(time.Since(start).Seconds()) + }) +} + +func (h *HandlersTLS) osctrldLimit(w http.ResponseWriter, r *http.Request, download bool, next http.Handler) { + l := h.OsctrldLimits + if l == nil { + next.ServeHTTP(w, r) + return + } + reject := func(lim *ratelimit.Limiter, name string) { + osctrldRateLimited.WithLabelValues(r.Pattern, name).Inc() + lim.Reject(w) + } + ip := utils.GetIP(r) + if !l.Traffic.Allow(ip) { + reject(l.Traffic, "osctrld") + return + } + if download && !l.Download.Allow(ip) { + reject(l.Download, "osctrldDownload") + return + } + // Cheap early rejection; osctrldSecret re-checks atomically. + envVar := r.PathValue("env") + if l.Auth.Exhausted(osctrldAuthKey(ip, envVar)) { + h.osctrldBlocked(w, r, ip, envVar, nil) + return + } + next.ServeHTTP(w, r) +} + +// osctrldBlocked answers 429 for an address whose failed-secret budget for +// the environment is spent. The first rejection per Auth period is audited; +// env is resolved from envVar when nil. +func (h *HandlersTLS) osctrldBlocked(w http.ResponseWriter, r *http.Request, ip, envVar string, env *environments.TLSEnvironment) { + l := h.OsctrldLimits + if l.authAudit.Allow(osctrldAuthKey(ip, envVar)) { + if env == nil { + resolved := h.osctrldAuditEnv(r, envVar) + env = &resolved + } + h.osctrldAudit(ip, *env, "blocked after too many failed secrets") + } + osctrldRateLimited.WithLabelValues(r.Pattern, "osctrldAuth").Inc() + l.Auth.Reject(w) +} + +// osctrldAudit writes a failed-secret audit entry unless the environment's +// audit cap is spent, in which case the drop is counted. +func (h *HandlersTLS) osctrldAudit(ip string, env environments.TLSEnvironment, reason string) { + if l := h.OsctrldLimits; l != nil && !l.auditCap.Allow(strings.ToLower(env.UUID)) { + osctrldAuthAuditDropped.WithLabelValues(env.Name).Inc() + return + } + h.AuditLog.FailedOsctrld(ip, env.Name, reason, env.ID) +} + +// osctrldAuditEnv resolves the environment in the path for an audit entry, +// falling back to the (UUID-validated) path value with no ID. +func (h *HandlersTLS) osctrldAuditEnv(r *http.Request, envVar string) environments.TLSEnvironment { + if h.EnvCache != nil { + if env, err := h.EnvCache.GetByUUID(r.Context(), strings.ToLower(envVar)); err == nil { + return env + } + } + if !utils.CheckUUID(envVar) { + envVar = "" + } + return environments.TLSEnvironment{UUID: envVar, Name: envVar} +} + +// osctrldSecret checks the secret of an osctrld request. The budget check, +// the comparison and the charge for a wrong secret happen atomically, so +// requests that passed the middleware together cannot exceed the budget: +// once it is spent the request gets 429 without a comparison. A wrong +// secret is counted, audited (capped per environment) and answered 403. +func (h *HandlersTLS) osctrldSecret(w http.ResponseWriter, r *http.Request, secret string, env environments.TLSEnvironment) bool { + valid := func() bool { return h.checkValidSecret(secret, env) } + ip := utils.GetIP(r) + if h.OsctrldLimits != nil { + allowed, ok := h.OsctrldLimits.Auth.Attempt(osctrldAuthKey(ip, env.UUID), valid) + if !allowed { + h.osctrldBlocked(w, r, ip, env.UUID, &env) + return false + } + if ok { + return true + } + } else if valid() { + return true + } + osctrldAuthFailures.WithLabelValues(env.Name).Inc() + h.osctrldAudit(ip, env, "invalid secret") + utils.HTTPResponse(w, "", http.StatusForbidden, []byte("")) + return false +} diff --git a/cmd/tls/handlers/osctrld_limits_test.go b/cmd/tls/handlers/osctrld_limits_test.go new file mode 100644 index 000000000..2567108ae --- /dev/null +++ b/cmd/tls/handlers/osctrld_limits_test.go @@ -0,0 +1,370 @@ +package handlers + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "strconv" + "strings" + "sync" + "testing" + "time" + + "github.com/jmpsec/osctrl/pkg/auditlog" + "github.com/jmpsec/osctrl/pkg/config" + "github.com/jmpsec/osctrl/pkg/environments" + "github.com/jmpsec/osctrl/pkg/utils" + "github.com/prometheus/client_golang/prometheus" + "github.com/stretchr/testify/require" + "gorm.io/driver/sqlite" + "gorm.io/gorm" +) + +const ( + osctrldCertRoute = "POST /{env}/" + environments.DefaultCertPath + osctrldDownloadRoute = "POST /{env}/" + environments.DefaultExtensionPath +) + +type osctrldFixture struct { + h *HandlersTLS + db *gorm.DB + mux *http.ServeMux + env, other environments.TLSEnvironment +} + +// newOsctrldFixture serves the cert route and the extension download route +// through OsctrldMiddleware, with limits built from cfg. +func newOsctrldFixture(t *testing.T, cfg config.YAMLConfigurationRateLimits) *osctrldFixture { + t.Helper() + dsn := "file:" + strings.NewReplacer("/", "_", " ", "_").Replace(t.Name()) + "?mode=memory&cache=shared" + db, err := gorm.Open(sqlite.Open(dsn), &gorm.Config{}) + require.NoError(t, err) + sqlDB, err := db.DB() + require.NoError(t, err) + t.Cleanup(func() { _ = sqlDB.Close() }) + sqlDB.SetMaxOpenConns(1) + envs := environments.CreateEnvironment(db) + f := &osctrldFixture{ + db: db, + env: environments.TLSEnvironment{UUID: "abcdef01-1111-4111-8111-111111111111", Name: "limits-env", Secret: "osctrld-secret", Certificate: "CERT"}, + other: environments.TLSEnvironment{UUID: "22222222-2222-4222-8222-222222222222", Name: "limits-other", Secret: "other-secret", Certificate: "CERT"}, + } + require.NoError(t, db.Create(&f.env).Error) + require.NoError(t, db.Create(&f.other).Error) + audit, err := auditlog.CreateAuditLogManager(db, config.ServiceTLS, true) + require.NoError(t, err) + f.h = CreateHandlersTLS(WithEnvs(envs), WithAuditLog(audit), WithOsctrldLimits(NewOsctrldLimits(cfg))) + f.mux = http.NewServeMux() + f.mux.Handle(osctrldCertRoute, f.h.OsctrldMiddleware(false, http.HandlerFunc(f.h.CertHandler))) + f.mux.Handle(osctrldDownloadRoute, f.h.OsctrldMiddleware(true, http.HandlerFunc(f.h.ExtensionDownloadHandler))) + return f +} + +func (f *osctrldFixture) post(ip, path string, env environments.TLSEnvironment, secret string) *httptest.ResponseRecorder { + return f.postXFF(ip, "", path, env, secret) +} + +func (f *osctrldFixture) postXFF(ip, xff, path string, env environments.TLSEnvironment, secret string) *httptest.ResponseRecorder { + body, _ := json.Marshal(map[string]string{"secret": secret}) + r := httptest.NewRequest(http.MethodPost, "/"+env.UUID+"/"+path, bytes.NewReader(body)) + r.RemoteAddr = ip + ":40000" + if xff != "" { + r.Header.Set(utils.XForwardedFor, xff) + } + w := httptest.NewRecorder() + f.mux.ServeHTTP(w, r) + return w +} + +func (f *osctrldFixture) cert(ip string, env environments.TLSEnvironment, secret string) int { + return f.post(ip, environments.DefaultCertPath, env, secret).Code +} + +func (f *osctrldFixture) audits(t *testing.T) int64 { + t.Helper() + var n int64 + require.NoError(t, f.db.Model(&auditlog.AuditLog{}).Where("line LIKE ?", "failed osctrld secret%").Count(&n).Error) + return n +} + +func TestOsctrldAuthLimitBlocksOnlyFailingAddressAndEnv(t *testing.T) { + f := newOsctrldFixture(t, config.DefaultRateLimits()) + const attacker, nat, neighbour = "198.51.100.7", "198.51.100.8", "198.51.100.9" + // A shared NAT with many healthy hosts: correct secrets never charge the + // failure budget. + for i := 0; i < 100; i++ { + require.Equal(t, http.StatusOK, f.cert(nat, f.env, f.env.Secret), "correct secret #%d", i+1) + } + for i := 0; i < 10; i++ { + require.Equal(t, http.StatusForbidden, f.cert(attacker, f.env, "wrong"), "failure #%d", i+1) + } + // Budget spent: blocked even with the right secret, before the check. + w := f.post(attacker, environments.DefaultCertPath, f.env, f.env.Secret) + require.Equal(t, http.StatusTooManyRequests, w.Code) + require.Equal(t, "360", w.Header().Get("Retry-After")) + // The block is per address and environment. + require.Equal(t, http.StatusOK, f.cert(attacker, f.other, f.other.Secret)) + require.Equal(t, http.StatusOK, f.cert(neighbour, f.env, f.env.Secret)) + require.Equal(t, http.StatusOK, f.cert(nat, f.env, f.env.Secret)) + // The upper-case spelling of the environment shares the same budget. + upper := f.env + upper.UUID = strings.ToUpper(upper.UUID) + require.Equal(t, http.StatusTooManyRequests, f.cert(attacker, upper, f.env.Secret)) +} + +func TestOsctrldAuthBlockRefills(t *testing.T) { + cfg := config.DefaultRateLimits() + cfg.OsctrldAuth = config.YAMLConfigurationRateLimit{Burst: 1, Period: 50 * time.Millisecond, EvictAfter: time.Minute, RetryAfter: 1} + f := newOsctrldFixture(t, cfg) + require.Equal(t, http.StatusForbidden, f.cert("198.51.100.7", f.env, "wrong")) + require.Equal(t, http.StatusTooManyRequests, f.cert("198.51.100.7", f.env, f.env.Secret)) + time.Sleep(60 * time.Millisecond) + require.Equal(t, http.StatusOK, f.cert("198.51.100.7", f.env, f.env.Secret)) +} + +func TestOsctrldFailedSecretAuditThrottled(t *testing.T) { + f := newOsctrldFixture(t, config.DefaultRateLimits()) + const ip = "198.51.100.7" + for i := 0; i < 10; i++ { + f.cert(ip, f.env, "wrong") + } + require.EqualValues(t, 10, f.audits(t), "every failed secret is audited") + for i := 0; i < 20; i++ { + require.Equal(t, http.StatusTooManyRequests, f.cert(ip, f.env, "wrong")) + } + require.EqualValues(t, 11, f.audits(t), "only the first rejection while blocked is audited") + var row auditlog.AuditLog + require.NoError(t, f.db.Order("id desc").First(&row).Error) + require.Equal(t, ip, row.SourceIP) + require.Equal(t, f.env.ID, row.EnvironmentID) + require.Contains(t, row.Line, f.env.Name) +} + +func TestOsctrldTrafficLimit(t *testing.T) { + cfg := config.DefaultRateLimits() + cfg.Osctrld.Burst = 3 + cfg.Osctrld.RetryAfter = 11 + f := newOsctrldFixture(t, cfg) + limited := osctrldRateLimited.WithLabelValues(osctrldCertRoute, "osctrld") + before := metricValue(t, limited) + for i := 0; i < 3; i++ { + require.Equal(t, http.StatusOK, f.cert("198.51.100.7", f.env, f.env.Secret)) + } + w := f.post("198.51.100.7", environments.DefaultCertPath, f.env, f.env.Secret) + require.Equal(t, http.StatusTooManyRequests, w.Code) + require.Equal(t, "11", w.Header().Get("Retry-After")) + require.Equal(t, before+1, metricValue(t, limited)) + require.Equal(t, http.StatusOK, f.cert("198.51.100.8", f.env, f.env.Secret)) +} + +func TestOsctrldDownloadLimit(t *testing.T) { + cfg := config.DefaultRateLimits() + cfg.OsctrldDownload.Burst = 2 + f := newOsctrldFixture(t, cfg) + limited := osctrldRateLimited.WithLabelValues(osctrldDownloadRoute, "osctrldDownload") + before := metricValue(t, limited) + for i := 0; i < 2; i++ { + // No extension with this SHA: past the limiter the handler answers 400. + require.NotEqual(t, http.StatusTooManyRequests, f.post("198.51.100.7", environments.DefaultExtensionPath, f.env, f.env.Secret).Code) + } + w := f.post("198.51.100.7", environments.DefaultExtensionPath, f.env, f.env.Secret) + require.Equal(t, http.StatusTooManyRequests, w.Code) + require.Equal(t, "60", w.Header().Get("Retry-After")) + require.Equal(t, before+1, metricValue(t, limited)) + // Downloads have their own budget: other osctrld routes still pass. + require.Equal(t, http.StatusOK, f.cert("198.51.100.7", f.env, f.env.Secret)) +} + +func TestOsctrldMetrics(t *testing.T) { + f := newOsctrldFixture(t, config.DefaultRateLimits()) + failures := osctrldAuthFailures.WithLabelValues(f.env.Name) + authLimited := osctrldRateLimited.WithLabelValues(osctrldCertRoute, "osctrldAuth") + failBefore, limitedBefore := metricValue(t, failures), metricValue(t, authLimited) + requestDuration.Reset() + f.cert("198.51.100.7", f.env, f.env.Secret) + for i := 0; i < 11; i++ { + f.cert("198.51.100.7", f.env, "wrong") + } + require.Equal(t, failBefore+10, metricValue(t, failures)) + require.Equal(t, limitedBefore+1, metricValue(t, authLimited)) + // Latency is labelled by route pattern, never by the raw URL path. + for _, code := range []string{"200", "403", "429"} { + require.True(t, requestDuration.DeleteLabelValues(http.MethodPost, osctrldCertRoute, code), code) + } + require.Zero(t, metricSeries(t, requestDuration)) +} + +// Every osctrld handler charges the failure budget, counts and audits a +// wrong secret, and answers 403. +func TestOsctrldHandlersChargeFailedSecret(t *testing.T) { + for name, call := range map[string]func(h *HandlersTLS) http.HandlerFunc{ + "flags": func(h *HandlersTLS) http.HandlerFunc { return h.FlagsHandler }, + "cert": func(h *HandlersTLS) http.HandlerFunc { return h.CertHandler }, + "verify": func(h *HandlersTLS) http.HandlerFunc { return h.VerifyHandler }, + "script": func(h *HandlersTLS) http.HandlerFunc { return h.ScriptHandler }, + "extensions": func(h *HandlersTLS) http.HandlerFunc { return h.ExtensionsHandler }, + "extension": func(h *HandlersTLS) http.HandlerFunc { return h.ExtensionDownloadHandler }, + } { + t.Run(name, func(t *testing.T) { + cfg := config.DefaultRateLimits() + cfg.OsctrldAuth.Burst = 1 + f := newOsctrldFixture(t, cfg) + failures := osctrldAuthFailures.WithLabelValues(f.env.Name) + before := metricValue(t, failures) + r := httptest.NewRequest(http.MethodPost, "/osctrld", strings.NewReader(`{"secret":"wrong"}`)) + r.RemoteAddr = "198.51.100.7:40000" + r.SetPathValue("env", f.env.UUID) + r.SetPathValue("action", "enroll") + r.SetPathValue("platform", "linux") + w := httptest.NewRecorder() + call(f.h)(w, r) + require.Equal(t, http.StatusForbidden, w.Code) + require.True(t, f.h.OsctrldLimits.Auth.Exhausted(osctrldAuthKey("198.51.100.7", f.env.UUID))) + require.Equal(t, before+1, metricValue(t, failures)) + require.EqualValues(t, 1, f.audits(t)) + }) + } +} + +// Without limits (OsctrldLimits nil) the middleware only records metrics. +func TestOsctrldMiddlewareWithoutLimits(t *testing.T) { + h := &HandlersTLS{} + called := 0 + mw := h.OsctrldMiddleware(true, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { called++ })) + for i := 0; i < 50; i++ { + mw.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(http.MethodPost, "/x", nil)) + } + require.Equal(t, 50, called) +} + +// metricValue reads a counter through a throwaway registry. +func metricValue(t *testing.T, c prometheus.Counter) float64 { + t.Helper() + reg := prometheus.NewRegistry() + require.NoError(t, reg.Register(c)) + mfs, err := reg.Gather() + require.NoError(t, err) + require.Len(t, mfs, 1) + return mfs[0].GetMetric()[0].GetCounter().GetValue() +} + +// metricSeries counts the label sets a collector currently exports. +func metricSeries(t *testing.T, c prometheus.Collector) int { + t.Helper() + reg := prometheus.NewRegistry() + require.NoError(t, reg.Register(c)) + mfs, err := reg.Gather() + require.NoError(t, err) + n := 0 + for _, mf := range mfs { + n += len(mf.GetMetric()) + } + return n +} + +// Behind a trusted proxy the limits key on the forwarded client address, so +// one client's failures do not block the rest of the proxied fleet; the +// client cannot change its key by prepending a forged hop. +func TestOsctrldLimitsKeyOnTrustedProxyClient(t *testing.T) { + utils.SetTrustedProxies([]string{"10.0.0.0/8"}) + t.Cleanup(func() { utils.SetTrustedProxies(nil) }) + f := newOsctrldFixture(t, config.DefaultRateLimits()) + const proxy = "10.0.0.1" + cert := func(xff, secret string) int { + return f.postXFF(proxy, xff, environments.DefaultCertPath, f.env, secret).Code + } + for i := 0; i < 10; i++ { + // The appending proxy adds the real client last; the forged + // left-most entry changes on every request. + require.Equal(t, http.StatusForbidden, cert("203.0.113."+strconv.Itoa(i)+", 198.51.100.7", "wrong")) + } + require.Equal(t, http.StatusTooManyRequests, cert("192.0.2.99, 198.51.100.7", f.env.Secret)) + require.Equal(t, http.StatusOK, cert("198.51.100.8", f.env.Secret), "another client behind the same proxy") + var row auditlog.AuditLog + require.NoError(t, f.db.Order("id").First(&row).Error) + require.Equal(t, "198.51.100.7", row.SourceIP) +} + +// Without trusted proxies the TCP peer is the key; forwarding headers are +// ignored. +func TestOsctrldLimitsIgnoreForwardingWithoutTrustedProxies(t *testing.T) { + utils.SetTrustedProxies(nil) + f := newOsctrldFixture(t, config.DefaultRateLimits()) + for i := 0; i < 10; i++ { + f.postXFF("198.51.100.7", "203.0.113."+strconv.Itoa(i), environments.DefaultCertPath, f.env, "wrong") + } + require.Equal(t, http.StatusTooManyRequests, f.postXFF("198.51.100.7", "203.0.113.200", environments.DefaultCertPath, f.env, f.env.Secret).Code) +} + +// Requests that all passed the middleware before any body arrived are still +// charged atomically at the comparison: at most burst wrong secrets are +// compared and answered 403, the rest get 429. +func TestOsctrldConcurrentFailuresBounded(t *testing.T) { + f := newOsctrldFixture(t, config.DefaultRateLimits()) + var mu sync.Mutex + codes := map[int]int{} + var wg sync.WaitGroup + for i := 0; i < 40; i++ { + wg.Add(1) + go func() { + defer wg.Done() + r := httptest.NewRequest(http.MethodPost, "/osctrld", strings.NewReader(`{"secret":"wrong"}`)) + r.RemoteAddr = "198.51.100.7:40000" + r.SetPathValue("env", f.env.UUID) + w := httptest.NewRecorder() + f.h.CertHandler(w, r) // as if every request had already passed OsctrldMiddleware + mu.Lock() + codes[w.Code]++ + mu.Unlock() + }() + } + wg.Wait() + require.Equal(t, map[int]int{http.StatusForbidden: 10, http.StatusTooManyRequests: 30}, codes) + require.EqualValues(t, 11, f.audits(t), "10 failures and one blocked rejection") +} + +// Past maxBuckets, addresses without their own bucket share the overflow +// budget, so a spray is blocked collectively. +func TestOsctrldOverflowBlocksSpray(t *testing.T) { + cfg := config.DefaultRateLimits() + cfg.OsctrldAuth.MaxBuckets = 1 + cfg.OsctrldAuth.Burst = 2 + f := newOsctrldFixture(t, cfg) + require.Equal(t, http.StatusForbidden, f.cert("198.51.100.1", f.env, "wrong")) // own bucket; map full + for i := 2; i <= 3; i++ { + require.Equal(t, http.StatusForbidden, f.cert("198.51.100."+strconv.Itoa(i), f.env, "wrong")) + } + require.Equal(t, http.StatusTooManyRequests, f.cert("198.51.100.4", f.env, "wrong")) +} + +// Failed-secret audit writes are capped per environment; drops are counted. +func TestOsctrldAuditCapPerEnvironment(t *testing.T) { + f := newOsctrldFixture(t, config.DefaultRateLimits()) + dropped := osctrldAuthAuditDropped.WithLabelValues(f.env.Name) + before := metricValue(t, dropped) + for a := 0; a < 7; a++ { + for i := 0; i < 10; i++ { + require.Equal(t, http.StatusForbidden, f.cert("198.51.100."+strconv.Itoa(a), f.env, "wrong")) + } + } + require.EqualValues(t, osctrldAuditPerHour, f.audits(t)) + require.Equal(t, before+70-osctrldAuditPerHour, metricValue(t, dropped)) +} + +// A blocked rejection under an upper-case spelling of the environment is +// audited against the right environment. +func TestOsctrldBlockedAuditResolvesUpperCaseEnv(t *testing.T) { + cfg := config.DefaultRateLimits() + cfg.OsctrldAuth.Burst = 1 + f := newOsctrldFixture(t, cfg) + f.cert("198.51.100.7", f.env, "wrong") + upper := f.env + upper.UUID = strings.ToUpper(upper.UUID) + require.Equal(t, http.StatusTooManyRequests, f.cert("198.51.100.7", upper, f.env.Secret)) + var row auditlog.AuditLog + require.NoError(t, f.db.Order("id desc").First(&row).Error) + require.Contains(t, row.Line, "blocked") + require.Equal(t, f.env.ID, row.EnvironmentID) + require.Contains(t, row.Line, f.env.Name) +} diff --git a/cmd/tls/handlers/post.go b/cmd/tls/handlers/post.go index e120c6376..6c9fe612f 100644 --- a/cmd/tls/handlers/post.go +++ b/cmd/tls/handlers/post.go @@ -952,19 +952,17 @@ func (h *HandlersTLS) FlagsHandler(w http.ResponseWriter, r *http.Request) { utils.HTTPResponse(w, "", http.StatusInternalServerError, []byte("")) return } - // Check if provided secret is valid and if so, prepare flags - if h.checkValidSecret(t.Secret, env) { - flagsStr, err := h.Envs.NodeFlags(env, t.SecrefFile, t.CertFile, *h.OsqueryValues) - if err != nil { - log.Err(err).Msg("error generating flags") - utils.HTTPResponse(w, "", http.StatusInternalServerError, []byte("")) - return - } - response = []byte(h.osctrldFlags(r.Context(), env, flagsStr, t)) - } else { - utils.HTTPResponse(w, "", http.StatusForbidden, []byte("")) + // Check the secret; a wrong one is charged to the failure budget + if !h.osctrldSecret(w, r, t.Secret, env) { return } + flagsStr, err := h.Envs.NodeFlags(env, t.SecrefFile, t.CertFile, *h.OsqueryValues) + if err != nil { + log.Err(err).Msg("error generating flags") + utils.HTTPResponse(w, "", http.StatusInternalServerError, []byte("")) + return + } + response = []byte(h.osctrldFlags(r.Context(), env, flagsStr, t)) // Debug HTTP if env.DebugHTTP { log.Debug().Msgf("Flags: %s", string(response)) @@ -1061,13 +1059,11 @@ func (h *HandlersTLS) CertHandler(w http.ResponseWriter, r *http.Request) { utils.HTTPResponse(w, "", http.StatusInternalServerError, []byte("")) return } - // Check if provided secret is valid and if so, prepare flags - if h.checkValidSecret(t.Secret, env) { - response = []byte(env.Certificate) - } else { - utils.HTTPResponse(w, "", http.StatusForbidden, []byte("uh oh...")) + // Check the secret; a wrong one is charged to the failure budget + if !h.osctrldSecret(w, r, t.Secret, env) { return } + response = []byte(env.Certificate) // Debug HTTP if env.DebugHTTP { log.Debug().Msgf("Certificate: %s", string(response)) @@ -1116,24 +1112,22 @@ func (h *HandlersTLS) VerifyHandler(w http.ResponseWriter, r *http.Request) { utils.HTTPResponse(w, "", http.StatusInternalServerError, []byte("")) return } - // Check if provided secret is valid and if so, prepare flags - if h.checkValidSecret(t.Secret, env) { - flagsStr, err := h.Envs.NodeFlags(env, t.SecrefFile, t.CertFile, *h.OsqueryValues) - if err != nil { - log.Err(err).Msg("error generating flags") - utils.HTTPResponse(w, "", http.StatusInternalServerError, []byte("")) - return - } - response = types.VerifyResponse{ - Certificate: env.Certificate, - Flags: h.osctrldFlags(r.Context(), env, flagsStr, types.FlagsRequest(t)), - OsqueryVersion: h.osqueryVersion(), - OsquerySHA256: h.osquerySHA256(), - } - } else { - utils.HTTPResponse(w, "", http.StatusForbidden, []byte("")) + // Check the secret; a wrong one is charged to the failure budget + if !h.osctrldSecret(w, r, t.Secret, env) { + return + } + flagsStr, err := h.Envs.NodeFlags(env, t.SecrefFile, t.CertFile, *h.OsqueryValues) + if err != nil { + log.Err(err).Msg("error generating flags") + utils.HTTPResponse(w, "", http.StatusInternalServerError, []byte("")) return } + response = types.VerifyResponse{ + Certificate: env.Certificate, + Flags: h.osctrldFlags(r.Context(), env, flagsStr, types.FlagsRequest(t)), + OsqueryVersion: h.osqueryVersion(), + OsquerySHA256: h.osquerySHA256(), + } // Debug HTTP if env.DebugHTTP { log.Debug().Msgf("Certificate: %v", response) @@ -1209,19 +1203,17 @@ func (h *HandlersTLS) ScriptHandler(w http.ResponseWriter, r *http.Request) { utils.HTTPResponse(w, "", http.StatusInternalServerError, []byte("")) return } - // Check if provided secret is valid and if so, prepare flags - if h.checkValidSecret(t.Secret, env) { - script, err := environments.QuickAddScript("osctrl-"+env.Name, actionVar, env) - if err != nil { - log.Err(err).Msg("error preparing script") - utils.HTTPResponse(w, "", http.StatusInternalServerError, []byte("")) - return - } - response = []byte(script) - } else { - utils.HTTPResponse(w, "", http.StatusForbidden, []byte("")) + // Check the secret; a wrong one is charged to the failure budget + if !h.osctrldSecret(w, r, t.Secret, env) { + return + } + script, err := environments.QuickAddScript("osctrl-"+env.Name, actionVar, env) + if err != nil { + log.Err(err).Msg("error preparing script") + utils.HTTPResponse(w, "", http.StatusInternalServerError, []byte("")) return } + response = []byte(script) // Debug HTTP if env.DebugHTTP { log.Debug().Msgf("Script: %s", string(response)) diff --git a/cmd/tls/main.go b/cmd/tls/main.go index b0ac3a1de..15b4c4150 100644 --- a/cmd/tls/main.go +++ b/cmd/tls/main.go @@ -100,7 +100,9 @@ var ( // Function to load the configuration from a single YAML file func loadYAMLConfiguration(file string) (config.TLSConfiguration, error) { - var cfg config.TLSConfiguration + // Rate limits start from the defaults so entries missing from the file + // (e.g. limits added after it was written) keep their default values. + cfg := config.TLSConfiguration{RateLimits: config.DefaultRateLimitsPtr()} v := viper.New() // Load file and read config v.SetConfigFile(file) @@ -368,7 +370,7 @@ func osctrlService() { if flagParams.RateLimits == nil { flagParams.RateLimits = config.DefaultRateLimitsPtr() } - if err := config.ValidateRateLimits(*flagParams.RateLimits, "enroll"); err != nil { + if err := config.ValidateRateLimits(*flagParams.RateLimits, config.TLSRateLimits...); err != nil { log.Fatal().Msgf("Invalid rate limit configuration - %v", err) } settingsCacheTTL := time.Duration(settingsmgr.RefreshSettings(config.ServiceTLS)) * time.Second @@ -560,6 +562,7 @@ func osctrlService() { handlers.WithConfigEndpoints(flagParams.ConfigEndpoints), handlers.WithDebugHTTP(flagParams.Debug), handlers.WithAuditLog(auditLog), + handlers.WithOsctrldLimits(handlers.NewOsctrldLimits(*flagParams.RateLimits)), handlers.WithDBHealth(dbHealth), // nil when DB health monitor disabled ) // ///////////////////////// ALL CONTENT IS UNAUTHENTICATED FOR TLS @@ -1015,17 +1018,21 @@ func registerRoutes(mux *http.ServeMux, h *handlers.HandlersTLS, p *config.Servi if p.Osctrld.Enabled { log.Info().Msg("Enabling osctrld endpoints") // TLS: osctrld retrieve flags - mux.HandleFunc("POST /{env}/"+environments.DefaultFlagsPath, h.FlagsHandler) + // Every osctrld route is rate-limited per client address, and an + // address with too many wrong secrets is blocked per environment. + osctrld := func(f http.HandlerFunc) http.Handler { return h.OsctrldMiddleware(false, f) } + mux.Handle("POST /{env}/"+environments.DefaultFlagsPath, osctrld(h.FlagsHandler)) // TLS: osctrld retrieve certificate - mux.HandleFunc("POST /{env}/"+environments.DefaultCertPath, h.CertHandler) + mux.Handle("POST /{env}/"+environments.DefaultCertPath, osctrld(h.CertHandler)) // TLS: osctrld verification - mux.HandleFunc("POST /{env}/"+environments.DefaultVerifyPath, h.VerifyHandler) + mux.Handle("POST /{env}/"+environments.DefaultVerifyPath, osctrld(h.VerifyHandler)) // TLS: osctrld retrieve script to install/remove osquery - mux.HandleFunc("POST /{env}/{action}/{platform}/"+environments.DefaultScriptPath, h.ScriptHandler) + mux.Handle("POST /{env}/{action}/{platform}/"+environments.DefaultScriptPath, osctrld(h.ScriptHandler)) if h.Extensions != nil { - // TLS: osctrld list and download osquery extensions - mux.HandleFunc("POST /{env}/"+environments.DefaultExtensionsPath, h.ExtensionsHandler) - mux.HandleFunc("POST /{env}/"+environments.DefaultExtensionPath, h.ExtensionDownloadHandler) + // TLS: osctrld list and download osquery extensions; downloads + // also have their own per-address limit + mux.Handle("POST /{env}/"+environments.DefaultExtensionsPath, osctrld(h.ExtensionsHandler)) + mux.Handle("POST /{env}/"+environments.DefaultExtensionPath, h.OsctrldMiddleware(true, http.HandlerFunc(h.ExtensionDownloadHandler))) } } diff --git a/cmd/tls/routes_test.go b/cmd/tls/routes_test.go index 65882390a..54bbf2891 100644 --- a/cmd/tls/routes_test.go +++ b/cmd/tls/routes_test.go @@ -3,6 +3,7 @@ package main import ( "net/http" "net/http/httptest" + "strings" "testing" "github.com/jmpsec/osctrl/cmd/tls/handlers" @@ -10,6 +11,8 @@ import ( "github.com/jmpsec/osctrl/pkg/environments" "github.com/jmpsec/osctrl/pkg/extensions" "github.com/stretchr/testify/require" + "gorm.io/driver/sqlite" + "gorm.io/gorm" ) // TestRegisterRoutesAllEnabled registers every route at once: Go's ServeMux @@ -46,3 +49,52 @@ func TestRegisterRoutesWithoutExtensions(t *testing.T) { _, pattern := mux.Handler(httptest.NewRequest(http.MethodPost, "/11111111-1111-4111-8111-111111111111/"+environments.DefaultExtensionsPath, nil)) require.NotEqual(t, "POST /{env}/"+environments.DefaultExtensionsPath, pattern) } + +// Every osctrld route goes through the osctrld limits; enroll does not. +func TestRegisterRoutesRateLimitsOsctrld(t *testing.T) { + db, err := gorm.Open(sqlite.Open("file:routes_osctrld?mode=memory&cache=shared"), &gorm.Config{}) + require.NoError(t, err) + sqlDB, err := db.DB() + require.NoError(t, err) + t.Cleanup(func() { _ = sqlDB.Close() }) + envs := environments.CreateEnvironment(db) + env := environments.TLSEnvironment{UUID: "11111111-1111-4111-8111-111111111111", Name: "env", Secret: "osctrld-secret"} + require.NoError(t, db.Create(&env).Error) + limits := config.DefaultRateLimits() + limits.OsctrldAuth.Burst = 1 + h := handlers.CreateHandlersTLS( + handlers.WithEnvs(envs), + handlers.WithExtensions(&extensions.Manager{}), + handlers.WithOsqueryValues(&config.YAMLConfigurationOsquery{}), + handlers.WithOsctrldLimits(handlers.NewOsctrldLimits(limits)), + ) + params := &config.ServiceParameters{ + Osquery: &config.YAMLConfigurationOsquery{}, + Osctrld: &config.YAMLConfigurationOsctrld{Enabled: true}, + } + enrolls := 0 + mux := http.NewServeMux() + registerRoutes(mux, h, params, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { enrolls++ })) + post := func(path, secret string) int { + r := httptest.NewRequest(http.MethodPost, "/"+env.UUID+"/"+path, strings.NewReader(`{"secret":"`+secret+`"}`)) + r.RemoteAddr = "198.51.100.7:40000" + w := httptest.NewRecorder() + mux.ServeHTTP(w, r) + return w.Code + } + require.Equal(t, http.StatusForbidden, post(environments.DefaultCertPath, "wrong")) + for _, path := range []string{ + environments.DefaultFlagsPath, + environments.DefaultCertPath, + environments.DefaultVerifyPath, + "enroll/linux/" + environments.DefaultScriptPath, + environments.DefaultExtensionsPath, + environments.DefaultExtensionPath, + } { + require.Equal(t, http.StatusTooManyRequests, post(path, env.Secret), path) + } + for i := 0; i < 5; i++ { + post(environments.DefaultEnrollPath, "wrong") + } + require.Equal(t, 5, enrolls, "enroll keeps its own limiter") +} diff --git a/deploy/config/api.yml b/deploy/config/api.yml index 3d8c31139..f0b01b022 100644 --- a/deploy/config/api.yml +++ b/deploy/config/api.yml @@ -16,7 +16,7 @@ # startup when this differs from the version the binary supports: a lower # number means fields were added since this file was written, a higher # number means the binary is older than this file. See pkg/config/version.go. -version: 4 +version: 5 # Main HTTP service behavior and shared service-level features. service: @@ -221,6 +221,25 @@ rateLimits: evictAfter: 10m retryAfter: 60 maxBuckets: 0 + # Unused by osctrl-api; osctrl-tls osctrld limits. + osctrldAuth: + burst: 10 + period: 1h + evictAfter: 2h + retryAfter: 360 + maxBuckets: 0 + osctrld: + burst: 300 + period: 1m + evictAfter: 10m + retryAfter: 60 + maxBuckets: 0 + osctrldDownload: + burst: 30 + period: 1m + evictAfter: 10m + retryAfter: 60 + maxBuckets: 0 # osquery feature switches used by API handlers and the frontend feature API. osquery: diff --git a/deploy/config/tls.yml b/deploy/config/tls.yml index 3faa4bccc..dd2bda989 100644 --- a/deploy/config/tls.yml +++ b/deploy/config/tls.yml @@ -16,7 +16,7 @@ # startup when this differs from the version the binary supports: a lower # number means fields were added since this file was written, a higher # number means the binary is older than this file. See pkg/config/version.go. -version: 4 +version: 5 # Main HTTP service behavior and shared service-level features. service: @@ -194,6 +194,32 @@ rateLimits: evictAfter: 10m retryAfter: 60 maxBuckets: 0 + # The osctrld limits key on the client IP. Behind a reverse proxy or load + # balancer, set service.trustedProxies to its address, otherwise every + # client shares the proxy's budgets and one bad client can block them all. + # Wrong osctrld secrets per client IP and environment. Only failures are + # charged; once spent, that IP gets 429 on that environment's osctrld + # routes (even with the right secret) until the budget refills. + osctrldAuth: + burst: 10 + period: 1h + evictAfter: 2h + retryAfter: 360 + maxBuckets: 0 + # All osctrld requests (flags, cert, verify, script, extensions) per client IP. + osctrld: + burst: 300 + period: 1m + evictAfter: 10m + retryAfter: 60 + maxBuckets: 0 + # osctrld extension downloads per client IP, on top of the osctrld limit. + osctrldDownload: + burst: 30 + period: 1m + evictAfter: 10m + retryAfter: 60 + maxBuckets: 0 # osquery remote API feature switches. osquery: diff --git a/frontend/src/features/service-config/ServiceConfigPage.tsx b/frontend/src/features/service-config/ServiceConfigPage.tsx index ac1fea860..4f7295791 100644 --- a/frontend/src/features/service-config/ServiceConfigPage.tsx +++ b/frontend/src/features/service-config/ServiceConfigPage.tsx @@ -135,6 +135,9 @@ const FIELD_HELP: Record = { 'api:rateLimits.preAuth': 'Read-only pre-auth routes, such as login environment/method discovery.', 'api:rateLimits.serviceConfigApply': 'POST /api/v1/service-config/apply restart requests.', 'tls:rateLimits.enroll': 'osquery enroll attempts per client IP.', + 'tls:rateLimits.osctrldAuth': 'Wrong osctrld secrets per client IP and environment; once spent, that IP gets 429 on the environment\'s osctrld routes until it refills.', + 'tls:rateLimits.osctrld': 'All osctrld requests per client IP.', + 'tls:rateLimits.osctrldDownload': 'osctrld extension downloads per client IP, on top of the osctrld limit.', // --- YAML-annotated: SAML 'saml.Enabled': 'Enables SAML routes when true.', 'saml.EntityID': 'SP entity ID — what the IdP knows us by, conventionally the metadata URL.', diff --git a/pkg/auditlog/audit.go b/pkg/auditlog/audit.go index 0961eb904..b407f92ec 100644 --- a/pkg/auditlog/audit.go +++ b/pkg/auditlog/audit.go @@ -255,6 +255,19 @@ func (m *AuditLogManager) FailedEnroll(ip, envName, reason string, envID uint) { } } +// FailedOsctrld records a rejected osctrld request for the env in the path: +// a wrong environment secret, or an address blocked after too many of them. +// Severity warning, like FailedEnroll. +func (m *AuditLogManager) FailedOsctrld(ip, envName, reason string, envID uint) { + if !m.Enabled { + return + } + line := fmt.Sprintf("failed osctrld secret for env %s: %s", envName, reason) + if err := m.CreateNew("osctrl-tls", line, ip, LogTypeNode, SeverityWarning, envID); err != nil { + log.Err(err).Msg("error creating failed-osctrld audit log") + } +} + // NewLogout - create new logout audit log entry func (m *AuditLogManager) NewLogout(username, ip string) { if !m.Enabled { diff --git a/pkg/auditlog/retention_test.go b/pkg/auditlog/retention_test.go index b6348dbad..4300e4735 100644 --- a/pkg/auditlog/retention_test.go +++ b/pkg/auditlog/retention_test.go @@ -65,3 +65,19 @@ func TestAuditActionTypes(t *testing.T) { _, supported := LogTypes[LogTypeEnroll] require.True(t, supported) } + +func TestFailedOsctrld(t *testing.T) { + db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) + require.NoError(t, err) + m, err := CreateAuditLogManager(db, "tls", true) + require.NoError(t, err) + m.FailedOsctrld("203.0.113.5", "prod", "invalid secret", 7) + var rows []AuditLog + require.NoError(t, db.Find(&rows).Error) + require.Len(t, rows, 1) + require.EqualValues(t, LogTypeNode, rows[0].LogType) + require.EqualValues(t, SeverityWarning, rows[0].Severity) + require.EqualValues(t, 7, rows[0].EnvironmentID) + require.Equal(t, "203.0.113.5", rows[0].SourceIP) + require.Equal(t, "failed osctrld secret for env prod: invalid secret", rows[0].Line) +} diff --git a/pkg/config/flags.go b/pkg/config/flags.go index 93c0c8500..32141d602 100644 --- a/pkg/config/flags.go +++ b/pkg/config/flags.go @@ -2,6 +2,7 @@ package config import ( "context" + "slices" "strings" "time" @@ -779,7 +780,12 @@ func initRateLimitFlags(params *ServiceParameters, service string) []cli.Flag { params.RateLimits = DefaultRateLimitsPtr() } if service == ServiceTLS { - return rateLimitFlags("enroll", "RATE_LIMIT_ENROLL", ¶ms.RateLimits.Enroll) + return slices.Concat( + rateLimitFlags("enroll", "RATE_LIMIT_ENROLL", ¶ms.RateLimits.Enroll), + rateLimitFlags("osctrld-auth", "RATE_LIMIT_OSCTRLD_AUTH", ¶ms.RateLimits.OsctrldAuth), + rateLimitFlags("osctrld", "RATE_LIMIT_OSCTRLD", ¶ms.RateLimits.Osctrld), + rateLimitFlags("osctrld-download", "RATE_LIMIT_OSCTRLD_DOWNLOAD", ¶ms.RateLimits.OsctrldDownload), + ) } return append( append( diff --git a/pkg/config/flags_test.go b/pkg/config/flags_test.go index 7fac4734c..cbfad8b08 100644 --- a/pkg/config/flags_test.go +++ b/pkg/config/flags_test.go @@ -269,3 +269,27 @@ func TestVulnNVDFlagDefaultsOff(t *testing.T) { } t.Fatal("missing vuln-nvd-enabled flag") } + +func TestTLSRateLimitFlagsIncludeOsctrld(t *testing.T) { + params := &ServiceParameters{} + flags := initRateLimitFlags(params, ServiceTLS) + want := map[string]*int{ + "rate-limit-enroll-burst": ¶ms.RateLimits.Enroll.Burst, + "rate-limit-osctrld-auth-burst": ¶ms.RateLimits.OsctrldAuth.Burst, + "rate-limit-osctrld-burst": ¶ms.RateLimits.Osctrld.Burst, + "rate-limit-osctrld-download-burst": ¶ms.RateLimits.OsctrldDownload.Burst, + } + for _, flag := range flags { + if f, ok := flag.(*cli.IntFlag); ok { + if dest, ok := want[f.Name]; ok { + if f.Destination != dest { + t.Errorf("%s has wrong destination", f.Name) + } + delete(want, f.Name) + } + } + } + for name := range want { + t.Errorf("missing flag %s", name) + } +} diff --git a/pkg/config/types.go b/pkg/config/types.go index 4b1873ffa..0e42939f1 100644 --- a/pkg/config/types.go +++ b/pkg/config/types.go @@ -367,6 +367,14 @@ type YAMLConfigurationRateLimits struct { PreAuth YAMLConfigurationRateLimit `yaml:"preAuth" mapstructure:"preAuth"` ServiceConfigApply YAMLConfigurationRateLimit `yaml:"serviceConfigApply" mapstructure:"serviceConfigApply"` Enroll YAMLConfigurationRateLimit `yaml:"enroll" mapstructure:"enroll"` + // OsctrldAuth is charged only by failed osctrld secrets, per client + // address and environment; once empty, that address gets 429 on that + // environment's osctrld routes until it refills. + OsctrldAuth YAMLConfigurationRateLimit `yaml:"osctrldAuth" mapstructure:"osctrldAuth"` + // Osctrld is charged by every osctrld request, per client address. + Osctrld YAMLConfigurationRateLimit `yaml:"osctrld" mapstructure:"osctrld"` + // OsctrldDownload is charged by every extension download, per client address. + OsctrldDownload YAMLConfigurationRateLimit `yaml:"osctrldDownload" mapstructure:"osctrldDownload"` } // YAMLConfigurationWriter to hold the DB batch writer configuration values diff --git a/pkg/config/validation.go b/pkg/config/validation.go index 2a830afba..caa4ca472 100644 --- a/pkg/config/validation.go +++ b/pkg/config/validation.go @@ -68,9 +68,31 @@ func DefaultRateLimits() YAMLConfigurationRateLimits { EvictAfter: 10 * time.Minute, RetryAfter: 60, }, + // One failure refills every 6 minutes, hence retryAfter 360. + OsctrldAuth: YAMLConfigurationRateLimit{ + Burst: 10, + Period: time.Hour, + EvictAfter: 2 * time.Hour, + RetryAfter: 360, + }, + Osctrld: YAMLConfigurationRateLimit{ + Burst: 300, + Period: time.Minute, + EvictAfter: 10 * time.Minute, + RetryAfter: 60, + }, + OsctrldDownload: YAMLConfigurationRateLimit{ + Burst: 30, + Period: time.Minute, + EvictAfter: 10 * time.Minute, + RetryAfter: 60, + }, } } +// TLSRateLimits names the rate limits osctrl-tls enforces. +var TLSRateLimits = []string{"enroll", "osctrldAuth", "osctrld", "osctrldDownload"} + // DefaultRateLimitsPtr returns DefaultRateLimits as a pointer for optional // config sections. func DefaultRateLimitsPtr() *YAMLConfigurationRateLimits { @@ -85,6 +107,9 @@ func ValidateRateLimits(cfg YAMLConfigurationRateLimits, names ...string) error "preAuth": cfg.PreAuth, "serviceConfigApply": cfg.ServiceConfigApply, "enroll": cfg.Enroll, + "osctrldAuth": cfg.OsctrldAuth, + "osctrld": cfg.Osctrld, + "osctrldDownload": cfg.OsctrldDownload, } for _, name := range names { limit, ok := limits[name] @@ -143,7 +168,7 @@ func ValidateTLSConfigValues(cfg TLSConfiguration) error { } } if cfg.RateLimits != nil { - if err := ValidateRateLimits(*cfg.RateLimits, "enroll"); err != nil { + if err := ValidateRateLimits(*cfg.RateLimits, TLSRateLimits...); err != nil { return err } } diff --git a/pkg/config/validation_test.go b/pkg/config/validation_test.go index 449e50f3d..6d4114c94 100644 --- a/pkg/config/validation_test.go +++ b/pkg/config/validation_test.go @@ -73,3 +73,45 @@ func TestValidateTLSConfigValuesRejectsLocalCarver(t *testing.T) { t.Fatalf("expected local carver to be rejected with a clear message, got %v", err) } } + +func TestDefaultOsctrldRateLimits(t *testing.T) { + d := DefaultRateLimits() + for name, tc := range map[string]struct { + got YAMLConfigurationRateLimit + burst int + period time.Duration + }{ + "osctrldAuth": {d.OsctrldAuth, 10, time.Hour}, + "osctrld": {d.Osctrld, 300, time.Minute}, + "osctrldDownload": {d.OsctrldDownload, 30, time.Minute}, + } { + if tc.got.Burst != tc.burst || tc.got.Period != tc.period { + t.Errorf("%s default = %d per %s, want %d per %s", name, tc.got.Burst, tc.got.Period, tc.burst, tc.period) + } + } + if err := ValidateRateLimits(d, TLSRateLimits...); err != nil { + t.Fatalf("default TLS rate limits should validate: %v", err) + } +} + +func TestValidateOsctrldRateLimits(t *testing.T) { + for _, name := range []string{"osctrldAuth", "osctrld", "osctrldDownload"} { + d := DefaultRateLimits() + bad := YAMLConfigurationRateLimit{Burst: 5, Period: time.Hour, EvictAfter: time.Minute} + switch name { + case "osctrldAuth": + d.OsctrldAuth = bad + case "osctrld": + d.Osctrld = bad + case "osctrldDownload": + d.OsctrldDownload = bad + } + err := ValidateRateLimits(d, TLSRateLimits...) + if err == nil || !strings.Contains(err.Error(), name+" evictAfter") { + t.Errorf("%s: expected evictAfter error, got %v", name, err) + } + if err := ValidateTLSConfigValues(TLSConfiguration{Service: YAMLConfigurationService{Auth: AuthNone}, RateLimits: &d}); err == nil { + t.Errorf("%s: ValidateTLSConfigValues accepted an invalid limit", name) + } + } +} diff --git a/pkg/config/version.go b/pkg/config/version.go index 2814bd7f2..7a5761d92 100644 --- a/pkg/config/version.go +++ b/pkg/config/version.go @@ -8,7 +8,7 @@ import "fmt" // operators whose files predate the change. The sample files in // deploy/config and the files written by the config-generate // subcommands carry the same number in their top-level "version" field. -const ConfigVersion = 4 +const ConfigVersion = 5 // ConfigVersionWarning returns a warning when the "version" field of a // YAML configuration file does not match ConfigVersion, or an empty diff --git a/pkg/config/version_test.go b/pkg/config/version_test.go index a2a6b02a6..c3e7cdf0c 100644 --- a/pkg/config/version_test.go +++ b/pkg/config/version_test.go @@ -58,9 +58,9 @@ func TestConfigVersionWarning(t *testing.T) { } } -func TestConfigVersionIncludesLiveUpdateFields(t *testing.T) { - const liveUpdateConfigVersion = 4 - if ConfigVersion != liveUpdateConfigVersion { - t.Fatalf("ConfigVersion = %d, want %d for service.eventsEnabled/eventsNamespace", ConfigVersion, liveUpdateConfigVersion) +func TestConfigVersionIncludesOsctrldRateLimits(t *testing.T) { + const osctrldRateLimitsConfigVersion = 5 + if ConfigVersion != osctrldRateLimitsConfigVersion { + t.Fatalf("ConfigVersion = %d, want %d for rateLimits.osctrldAuth/osctrld/osctrldDownload", ConfigVersion, osctrldRateLimitsConfigVersion) } } diff --git a/pkg/ratelimit/ratelimit.go b/pkg/ratelimit/ratelimit.go index 2aab123c5..a6fe4672a 100644 --- a/pkg/ratelimit/ratelimit.go +++ b/pkg/ratelimit/ratelimit.go @@ -110,10 +110,17 @@ func (l *Limiter) Allow(key string) bool { now := time.Now() l.mu.Lock() defer l.mu.Unlock() - // Amortized eviction: walk the map only when the throttle says it's - // time. Each Allow is O(1) on the steady-state path. (Cluster-3 - // review item — keeps the lock-held duration bounded under load.) - if now.Sub(l.lastEviction) >= l.evictInterval { + return l.bucket(key, now, true).AllowN(now, 1) +} + +// bucket returns key's limiter: its own bucket, the shared overflow bucket +// when the map is full and key has none, or — when create is false and the +// map has room — nil. create also refreshes lastSeen, creates the bucket, +// and runs the amortized eviction. Callers hold l.mu. +func (l *Limiter) bucket(key string, now time.Time, create bool) *rate.Limiter { + if create && now.Sub(l.lastEviction) >= l.evictInterval { + // Amortized eviction: walk the map only when the throttle says + // it's time, keeping the lock-held duration bounded under load. for k, e := range l.buckets { if now.Sub(e.lastSeen) > l.evictAfter { delete(l.buckets, k) @@ -122,19 +129,59 @@ func (l *Limiter) Allow(key string) bool { l.lastEviction = now } if e, ok := l.buckets[key]; ok { - e.lastSeen = now - return e.limiter.Allow() + if create { + e.lastSeen = now + } + return e.limiter } // New key. If the map is at the cap, route through the shared // overflow bucket — spray attackers can saturate it, but legitimate // keys that already have a bucket still get their own quota. - // if len(l.buckets) >= l.maxBuckets { - return l.overflow.Allow() + return l.overflow + } + if !create { + return nil } e := &entry{limiter: rate.NewLimiter(l.rate, l.burst), lastSeen: now} l.buckets[key] = e - return e.limiter.Allow() + return e.limiter +} + +// Exhausted reports whether key has no token left, without consuming one or +// creating a bucket. A key without its own bucket is exhausted only when the +// map is full and the shared overflow bucket is empty. +func (l *Limiter) Exhausted(key string) bool { + now := time.Now() + l.mu.Lock() + defer l.mu.Unlock() + b := l.bucket(key, now, false) + return b != nil && b.TokensAt(now) < 1 +} + +// Attempt charges key only for failed attempts, atomically: under the lock +// it refuses (allowed false, check not run) when key has no token left, +// otherwise runs check and consumes a token when it fails. Concurrent +// failures therefore never exceed the budget. check runs under the +// limiter's lock, so it must be cheap (e.g. a secret comparison). +func (l *Limiter) Attempt(key string, check func() bool) (allowed, valid bool) { + now := time.Now() + l.mu.Lock() + defer l.mu.Unlock() + if b := l.bucket(key, now, false); b != nil && b.TokensAt(now) < 1 { + return false, false + } + if check() { + return true, true + } + l.bucket(key, now, true).AllowN(now, 1) + return true, false +} + +// Reject writes the limiter's 429 response with its Retry-After header. +func (l *Limiter) Reject(w http.ResponseWriter) { + w.Header().Set("Retry-After", strconv.Itoa(l.retryAfter)) + http.Error(w, "too many requests", http.StatusTooManyRequests) } // HTTPMiddleware returns a middleware that rejects requests with 429 when @@ -151,8 +198,7 @@ func (l *Limiter) HTTPMiddleware(keyFn func(*http.Request) string, onReject func if onReject != nil { onReject(r, key) } - w.Header().Set("Retry-After", strconv.Itoa(l.retryAfter)) - http.Error(w, "too many requests", http.StatusTooManyRequests) + l.Reject(w) return } next.ServeHTTP(w, r) diff --git a/pkg/ratelimit/ratelimit_test.go b/pkg/ratelimit/ratelimit_test.go index 7b860166f..0145d5c0d 100644 --- a/pkg/ratelimit/ratelimit_test.go +++ b/pkg/ratelimit/ratelimit_test.go @@ -3,6 +3,7 @@ package ratelimit import ( "net/http" "net/http/httptest" + "sync" "testing" "time" @@ -202,3 +203,114 @@ func TestMiddlewareXFFRotationDoesNotBypass(t *testing.T) { t.Fatalf("inner handler called %d times, want 2 (only the burst)", called) } } + +// TestExhaustedDoesNotConsume covers the "consume only on failure" use: +// Exhausted reports an empty bucket without taking a token or creating one. +func TestExhaustedDoesNotConsume(t *testing.T) { + l := New(2, time.Hour, 2*time.Hour) + for i := 0; i < 5; i++ { + if l.Exhausted("k") { + t.Fatal("unseen key must not be exhausted") + } + } + l.mu.Lock() + size := len(l.buckets) + l.mu.Unlock() + if size != 0 { + t.Fatalf("Exhausted created %d buckets, want 0", size) + } + l.Allow("k") + if l.Exhausted("k") { + t.Fatal("one token left: key must not be exhausted") + } + l.Allow("k") + if !l.Exhausted("k") { + t.Fatal("budget spent: key must be exhausted") + } + if l.Exhausted("other") { + t.Fatal("keys must not share budgets") + } +} + +func TestRejectWritesRetryAfter(t *testing.T) { + l := NewFromConfig(config.YAMLConfigurationRateLimit{Burst: 1, Period: time.Second, EvictAfter: time.Minute, RetryAfter: 42}) + w := httptest.NewRecorder() + l.Reject(w) + if w.Code != http.StatusTooManyRequests { + t.Fatalf("code = %d, want 429", w.Code) + } + if got := w.Header().Get("Retry-After"); got != "42" { + t.Fatalf("Retry-After = %q, want 42", got) + } +} + +// TestAttemptChargesOnlyFailures: Attempt runs the check only while the key +// has budget, and charges only when the check fails. +func TestAttemptChargesOnlyFailures(t *testing.T) { + l := New(2, time.Hour, 2*time.Hour) + ok := func() bool { return true } + bad := func() bool { return false } + for i := 0; i < 5; i++ { + if allowed, valid := l.Attempt("k", ok); !allowed || !valid { + t.Fatalf("passing attempt #%d: allowed=%v valid=%v", i+1, allowed, valid) + } + } + for i := 0; i < 2; i++ { + if allowed, valid := l.Attempt("k", bad); !allowed || valid { + t.Fatalf("failing attempt #%d: allowed=%v valid=%v", i+1, allowed, valid) + } + } + ran := false + if allowed, _ := l.Attempt("k", func() bool { ran = true; return true }); allowed || ran { + t.Fatalf("exhausted key: allowed=%v, check ran=%v; want neither", allowed, ran) + } + if allowed, valid := l.Attempt("other", ok); !allowed || !valid { + t.Fatal("keys must not share budgets") + } +} + +// TestAttemptConcurrentFailuresBounded: concurrent failing attempts on one +// key never exceed the burst. +func TestAttemptConcurrentFailuresBounded(t *testing.T) { + l := New(3, time.Hour, 2*time.Hour) + var mu sync.Mutex + checks := 0 + var wg sync.WaitGroup + for i := 0; i < 50; i++ { + wg.Add(1) + go func() { + defer wg.Done() + l.Attempt("k", func() bool { mu.Lock(); checks++; mu.Unlock(); return false }) + }() + } + wg.Wait() + if checks != 3 { + t.Fatalf("failed checks = %d, want 3 (the burst)", checks) + } +} + +// TestOverflowKeysBlockedCollectively: past maxBuckets, keys without their +// own bucket share the overflow budget for Exhausted and Attempt too. +func TestOverflowKeysBlockedCollectively(t *testing.T) { + l := NewWithCap(1, time.Hour, 2*time.Hour, 1) + bad := func() bool { return false } + l.Attempt("a", bad) // own bucket, map now full + if l.Exhausted("b") { + t.Fatal("overflow still has budget") + } + if allowed, _ := l.Attempt("b", bad); !allowed { + t.Fatal("first overflow failure must be allowed") + } + if !l.Exhausted("c") { + t.Fatal("overflow spent: unseen keys must be exhausted while the map is full") + } + if allowed, _ := l.Attempt("c", func() bool { return true }); allowed { + t.Fatal("overflow spent: Attempt must refuse unseen keys") + } + l.mu.Lock() + size := len(l.buckets) + l.mu.Unlock() + if size != 1 { + t.Fatalf("bucket map size = %d, want 1", size) + } +} diff --git a/pkg/serviceconfig/serviceconfig.go b/pkg/serviceconfig/serviceconfig.go index a4b619ba9..13633fff4 100644 --- a/pkg/serviceconfig/serviceconfig.go +++ b/pkg/serviceconfig/serviceconfig.go @@ -494,11 +494,13 @@ func validateSectionValue(service, name, value string) error { } } if name == "rateLimits" { - var limits config.YAMLConfigurationRateLimits + // Applying merges into the running limits, so a value without an + // entry (saved before it existed) keeps that entry's default. + limits := config.DefaultRateLimits() if err := json.Unmarshal([]byte(value), &limits); err != nil { return fmt.Errorf("unmarshal rateLimits: %w", err) } - names := []string{"enroll"} + names := config.TLSRateLimits if service == config.ServiceAPI { names = []string{"login", "preAuth", "serviceConfigApply"} } diff --git a/pkg/serviceconfig/serviceconfig_test.go b/pkg/serviceconfig/serviceconfig_test.go index 5cd022ac5..717a59a13 100644 --- a/pkg/serviceconfig/serviceconfig_test.go +++ b/pkg/serviceconfig/serviceconfig_test.go @@ -614,6 +614,22 @@ func TestResolve_RateLimitsSection_OverridesYAML(t *testing.T) { assert.Equal(t, 23, cfg.RateLimits.Login.RetryAfter) } +// A rateLimits value saved before the osctrld limits existed (enroll only) +// still validates for osctrl-tls; the osctrld limits keep their defaults. +// An invalid osctrld limit is rejected. +func TestUpdateSection_TLSRateLimitsOsctrld(t *testing.T) { + db := setupTestDB(t) + m := &ServiceConfigManager{DB: db} + cfg := testTLSParams() + require.NoError(t, m.Seed(config.ServiceTLS, cfg, 0)) + + _, err := m.UpdateSection(config.ServiceTLS, "rateLimits", `{"enroll":{"burst":20,"period":60000000000,"evictAfter":600000000000,"retryAfter":60}}`, 0) + require.NoError(t, err) + + _, err = m.UpdateSection(config.ServiceTLS, "rateLimits", `{"enroll":{"burst":20,"period":60000000000,"evictAfter":600000000000,"retryAfter":60},"osctrldAuth":{"burst":0,"period":3600000000000,"evictAfter":7200000000000}}`, 0) + require.ErrorContains(t, err, "osctrldAuth burst") +} + // ────────────────────────────────────────────────────────────────────────────── // Phase 4 — optional YAML (nil section pointers) // ──────────────────────────────────────────────────────────────────────────────