Add local login, agent extra log paths, IPv4/IPv6 metrics; remediate security audit findings
This is a large squashed commit covering two batches of prior uncommitted work plus a full security-audit remediation pass, kept together because go.mod/go.sum and several shared files (main.go, handler.go) were touched by both and splitting risked non-building intermediate commits. Features (built earlier, previously uncommitted): - Local username/password login for single-tenant deployments with no SSO configured (api/localauth, alerting/internal/sessioncheck, sentryctl users, web/src/routes/login, metadata migrations 0040/0041). - Remotely-editable additional log file paths for agents, on top of their existing primary source (api/agents, agent/sentry-agent extra-file-path diffing, web agent config UI). - IPv4/IPv6 addresses reported alongside other host system metrics. Security audit remediation (this pass, all live-verified in production): - Critical: block ClickHouse SSRF table functions (url/remote/file/s3/...) in the raw-SQL query escape hatch. - High: deny sensitive paths and require Admin to add agent extra_file_paths (Editor could previously point an agent at /etc/shadow or an SSH key); alerting webhook targets now validate against internal/metadata/loopback addresses, both at creation and send time; alerting's session middleware now enforces an Editor+ floor on mutating requests instead of "any authenticated session"; bumped goxmldsig to close a SAML signature-verification bypass (GO-2026-4753). - Medium: per-IP login rate limiting; security response headers (HSTS/CSP/nosniff/X-Frame-Options/Referrer-Policy/Permissions-Policy) on web/nginx.conf; a DevCredentialWarnings check in every Go service's config loader, logging loudly at startup if a deployment is still on docker-compose.yml's literal dev-only credentials; dependency bumps (golang.org/x/text, grpc, x/net, quick-xml, h2) across every affected Go module and both Rust crates, including a previously-uncovered x/net vulnerability in deploy/operator; a new security-scan.yml CI workflow running cargo-deny/govulncheck/npm-audit, mirroring the existing license-compliance.yml matrix shape. - Low: removed sentryctl's plaintext --password flag (shell history/`ps` exposure) in favor of stdin and a --password-stdin flag for reset-password's optional specific-password path; a dummy bcrypt comparison closes a login response-time username-enumeration side-channel.
This commit is contained in:
@@ -4,8 +4,11 @@ import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
"log/slog"
|
||||
"net/http"
|
||||
"path"
|
||||
"strings"
|
||||
|
||||
"github.com/sentry/sentry/api/authz"
|
||||
)
|
||||
@@ -128,6 +131,30 @@ func (h *Handler) handleSetConfig(w http.ResponseWriter, r *http.Request) {
|
||||
return
|
||||
}
|
||||
|
||||
// extra_file_paths is a materially different capability than the
|
||||
// rest of this override: every other field tunes an already-running
|
||||
// source, but this one tells the agent (which runs as root, with no
|
||||
// filesystem sandboxing today -- see the security audit) to read and
|
||||
// ship an arbitrary local file. RoleEditor is the right bar for
|
||||
// "adjust batch size," not for "grant read access to any file on the
|
||||
// host" -- so a request that actually *changes* the set of extra
|
||||
// paths (adds or edits one -- shrinking or clearing never needs
|
||||
// this, since that only removes capability) requires RoleAdmin,
|
||||
// checked here rather than by splitting /agents/{host}/config into
|
||||
// two routes with two RegisterRoutes role floors, which would break
|
||||
// the "PUT replaces the whole override" contract every field here
|
||||
// otherwise shares.
|
||||
// A nil authorizer means no RBAC is configured at all (Phase 0-3
|
||||
// default-open behavior) -- consistent with RequireRole's own
|
||||
// no-op-when-nil posture, this extra gate only applies once an
|
||||
// authorizer resolves a real Identity to check.
|
||||
if identity, ok := authz.IdentityFromContext(r.Context()); ok && !identity.Role.Satisfies(authz.RoleAdmin) {
|
||||
if changesExtraFilePaths(h.currentExtraFilePaths(r.Context(), h.tenantID(r), r.PathValue("host")), override.ExtraFilePaths) {
|
||||
writeError(w, http.StatusForbidden, "extra_file_paths requires the admin role")
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
a, err := h.store.SetOverride(r.Context(), h.tenantID(r), r.PathValue("host"), override, h.updatedBy(r))
|
||||
if err != nil {
|
||||
h.writeStoreErr(w, err, "setting agent config")
|
||||
@@ -202,9 +229,97 @@ func validateOverride(o ConfigOverride) error {
|
||||
if o.HeartbeatIntervalMS != nil && *o.HeartbeatIntervalMS < 5000 {
|
||||
return errors.New("heartbeat_interval_ms must be at least 5000 (5s)")
|
||||
}
|
||||
if len(o.ExtraFilePaths) > 20 {
|
||||
return errors.New("extra_file_paths: at most 20 paths")
|
||||
}
|
||||
for _, p := range o.ExtraFilePaths {
|
||||
if err := validateExtraFilePath(p); err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// extraFilePathDenylistPrefixes blocks whole directory trees that are
|
||||
// never legitimate log-file locations but very commonly hold sensitive
|
||||
// material an agent (which runs as root, unsandboxed, on every host
|
||||
// this deployment has been checked against -- see the security audit)
|
||||
// can otherwise read: OS credential/config storage, home directories,
|
||||
// and kernel/process pseudo-filesystems.
|
||||
var extraFilePathDenylistPrefixes = []string{"/etc/", "/root/", "/home/", "/proc/", "/sys/", "/boot/"}
|
||||
|
||||
// extraFilePathDenylistSubstrings catches credential material that can
|
||||
// live outside the directories above too (e.g. a service account's
|
||||
// SSH/cloud-credential directory under an app's own working directory,
|
||||
// not necessarily /home or /root).
|
||||
var extraFilePathDenylistSubstrings = []string{"/.ssh/", "/.gnupg/", "/.aws/", "/.kube/"}
|
||||
|
||||
// extraFilePathDenylistSuffixes catches specific high-value filenames by
|
||||
// name, regardless of directory -- named here because the audit that
|
||||
// motivated this check demonstrated /etc/shadow and an SSH private key
|
||||
// specifically, and this covers both even outside the prefix-denylisted
|
||||
// directories above (e.g. a private key accidentally copied to /opt).
|
||||
var extraFilePathDenylistSuffixes = []string{"-key.pem", "id_rsa", "id_ecdsa", "id_ed25519", "id_dsa", "/shadow", "/gshadow"}
|
||||
|
||||
func validateExtraFilePath(p string) error {
|
||||
if p == "" || !strings.HasPrefix(p, "/") {
|
||||
return errors.New("extra_file_paths: each path must be a non-empty absolute path")
|
||||
}
|
||||
if strings.Contains(p, "..") {
|
||||
return errors.New(`extra_file_paths: path must not contain ".."`)
|
||||
}
|
||||
if cleaned := path.Clean(p); cleaned != p {
|
||||
return fmt.Errorf("extra_file_paths: %q must be in canonical form (e.g. %q)", p, cleaned)
|
||||
}
|
||||
for _, prefix := range extraFilePathDenylistPrefixes {
|
||||
if p == strings.TrimSuffix(prefix, "/") || strings.HasPrefix(p, prefix) {
|
||||
return fmt.Errorf("extra_file_paths: %q is not an allowed path (under denylisted %s)", p, prefix)
|
||||
}
|
||||
}
|
||||
for _, substr := range extraFilePathDenylistSubstrings {
|
||||
if strings.Contains(p, substr) {
|
||||
return fmt.Errorf("extra_file_paths: %q is not an allowed path", p)
|
||||
}
|
||||
}
|
||||
for _, suffix := range extraFilePathDenylistSuffixes {
|
||||
if strings.HasSuffix(p, suffix) {
|
||||
return fmt.Errorf("extra_file_paths: %q is not an allowed path", p)
|
||||
}
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// currentExtraFilePaths reads back the agent's already-stored override
|
||||
// (empty/nil if the agent or override doesn't exist yet) so
|
||||
// handleSetConfig can tell an addition/change apart from a pure
|
||||
// shrink-or-clear -- see changesExtraFilePaths.
|
||||
func (h *Handler) currentExtraFilePaths(ctx context.Context, tenantID, host string) []string {
|
||||
a, err := h.store.Get(ctx, tenantID, host)
|
||||
if err != nil || a.DesiredOverride == nil {
|
||||
return nil
|
||||
}
|
||||
return a.DesiredOverride.ExtraFilePaths
|
||||
}
|
||||
|
||||
// changesExtraFilePaths reports whether desired introduces any path not
|
||||
// already present in current -- an addition or an edit, either of which
|
||||
// grants the agent read access to something it couldn't read before.
|
||||
// Removing paths (desired is a subset of current) is never a capability
|
||||
// grant, so that alone never requires the stricter role handleSetConfig
|
||||
// applies around this.
|
||||
func changesExtraFilePaths(current, desired []string) bool {
|
||||
existing := make(map[string]struct{}, len(current))
|
||||
for _, p := range current {
|
||||
existing[p] = struct{}{}
|
||||
}
|
||||
for _, p := range desired {
|
||||
if _, ok := existing[p]; !ok {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
func (h *Handler) writeStoreErr(w http.ResponseWriter, err error, action string) {
|
||||
if errors.Is(err, ErrNotFound) {
|
||||
writeError(w, http.StatusNotFound, "agent not found")
|
||||
|
||||
@@ -199,6 +199,116 @@ func TestHandleSetConfigRejectsTooSmallHeartbeatInterval(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestHandleSetConfigExtraFilePathsRoundTrips(t *testing.T) {
|
||||
s := newFakeStore()
|
||||
s.put(Agent{TenantID: "default", Host: "web-01"})
|
||||
h := newTestHandler(s)
|
||||
|
||||
rec := doRequest(t, h, "PUT", "/agents/web-01/config", ConfigOverride{
|
||||
ExtraFilePaths: []string{"/var/log/nginx/access.log", "/var/log/nginx/error.log"},
|
||||
})
|
||||
if rec.Code != http.StatusOK {
|
||||
t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String())
|
||||
}
|
||||
var got Agent
|
||||
if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil {
|
||||
t.Fatalf("decoding response: %v", err)
|
||||
}
|
||||
if got.DesiredOverride == nil || len(got.DesiredOverride.ExtraFilePaths) != 2 {
|
||||
t.Fatalf("unexpected override: %+v", got.DesiredOverride)
|
||||
}
|
||||
}
|
||||
|
||||
func TestHandleSetConfigRejectsRelativeExtraFilePath(t *testing.T) {
|
||||
s := newFakeStore()
|
||||
s.put(Agent{TenantID: "default", Host: "web-01"})
|
||||
h := newTestHandler(s)
|
||||
|
||||
rec := doRequest(t, h, "PUT", "/agents/web-01/config", ConfigOverride{
|
||||
ExtraFilePaths: []string{"relative/path.log"},
|
||||
})
|
||||
if rec.Code != http.StatusBadRequest {
|
||||
t.Fatalf("status = %d, want 400", rec.Code)
|
||||
}
|
||||
}
|
||||
|
||||
func TestHandleSetConfigRejectsTooManyExtraFilePaths(t *testing.T) {
|
||||
s := newFakeStore()
|
||||
s.put(Agent{TenantID: "default", Host: "web-01"})
|
||||
h := newTestHandler(s)
|
||||
|
||||
paths := make([]string, 21)
|
||||
for i := range paths {
|
||||
paths[i] = "/var/log/x.log"
|
||||
}
|
||||
rec := doRequest(t, h, "PUT", "/agents/web-01/config", ConfigOverride{ExtraFilePaths: paths})
|
||||
if rec.Code != http.StatusBadRequest {
|
||||
t.Fatalf("status = %d, want 400", rec.Code)
|
||||
}
|
||||
}
|
||||
|
||||
// TestHandleSetConfigDenylistsSensitivePaths is the regression test for
|
||||
// the security-audit finding that a root, unsandboxed agent plus an
|
||||
// unrestricted extra_file_paths let any Editor read arbitrary files
|
||||
// (e.g. /etc/shadow, SSH keys) and have them shipped into ClickHouse.
|
||||
func TestHandleSetConfigDenylistsSensitivePaths(t *testing.T) {
|
||||
denied := []string{
|
||||
"/etc/shadow",
|
||||
"/etc/passwd",
|
||||
"/root/.bash_history",
|
||||
"/home/alice/.ssh/id_rsa",
|
||||
"/home/alice/.ssh/authorized_keys",
|
||||
"/proc/1/environ",
|
||||
"/etc/sentry-agent/client-key.pem",
|
||||
"/opt/app/../../etc/shadow",
|
||||
"/opt/app/id_ed25519",
|
||||
}
|
||||
for _, p := range denied {
|
||||
s := newFakeStore()
|
||||
s.put(Agent{TenantID: "default", Host: "web-01"})
|
||||
h := newTestHandler(s)
|
||||
rec := doRequest(t, h, "PUT", "/agents/web-01/config", ConfigOverride{ExtraFilePaths: []string{p}})
|
||||
if rec.Code != http.StatusBadRequest {
|
||||
t.Errorf("path %q: status = %d, want 400 (should be denylisted), body=%s", p, rec.Code, rec.Body.String())
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestHandleSetConfigExtraFilePathsRequiresAdminToAdd is the regression
|
||||
// test for the audit's role-floor fix: adding/changing extra_file_paths
|
||||
// needs Admin, not just Editor, since it grants the agent read access to
|
||||
// a new file. Purely shrinking or clearing an existing set stays at the
|
||||
// Editor floor everything else in this override uses.
|
||||
func TestHandleSetConfigExtraFilePathsRequiresAdminToAdd(t *testing.T) {
|
||||
s := newFakeStore()
|
||||
s.put(Agent{TenantID: "default", Host: "web-01"})
|
||||
editor := NewHandler(discardLogger(), s, fakeAuthorizer{role: authz.RoleEditor}, nil)
|
||||
admin := NewHandler(discardLogger(), s, fakeAuthorizer{role: authz.RoleAdmin}, nil)
|
||||
|
||||
rec := doRequest(t, editor, "PUT", "/agents/web-01/config", ConfigOverride{
|
||||
ExtraFilePaths: []string{"/var/log/nginx/access.log"},
|
||||
})
|
||||
if rec.Code != http.StatusForbidden {
|
||||
t.Fatalf("editor adding a path: status = %d, want 403", rec.Code)
|
||||
}
|
||||
|
||||
rec = doRequest(t, admin, "PUT", "/agents/web-01/config", ConfigOverride{
|
||||
ExtraFilePaths: []string{"/var/log/nginx/access.log", "/var/log/nginx/error.log"},
|
||||
})
|
||||
if rec.Code != http.StatusOK {
|
||||
t.Fatalf("admin adding paths: status = %d, want 200, body=%s", rec.Code, rec.Body.String())
|
||||
}
|
||||
|
||||
// Shrinking back down to one path is a pure removal -- Editor should
|
||||
// be allowed to do this even though they couldn't have added it.
|
||||
rec = doRequest(t, editor, "PUT", "/agents/web-01/config", ConfigOverride{
|
||||
ExtraFilePaths: []string{"/var/log/nginx/access.log"},
|
||||
})
|
||||
if rec.Code != http.StatusOK {
|
||||
t.Fatalf("editor removing a path: status = %d, want 200, body=%s", rec.Code, rec.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
func TestHandleSetConfigUnknownHostIsNotFound(t *testing.T) {
|
||||
h := newTestHandler(newFakeStore())
|
||||
interval := int64(30000)
|
||||
|
||||
+6
-5
@@ -35,11 +35,12 @@ func validCommand(c string) bool {
|
||||
// grpcserver.TenantIDHeaderKey, enterprise/internal/apiconfig.AIConfig).
|
||||
// Keep the three in sync by hand.
|
||||
type ConfigOverride struct {
|
||||
BatchMaxSize *int64 `json:"batch_max_size,omitempty"`
|
||||
BatchFlushIntervalMS *int64 `json:"batch_flush_interval_ms,omitempty"`
|
||||
HeartbeatEnabled *bool `json:"heartbeat_enabled,omitempty"`
|
||||
HeartbeatIntervalMS *int64 `json:"heartbeat_interval_ms,omitempty"`
|
||||
JournaldUnit *string `json:"journald_unit,omitempty"`
|
||||
BatchMaxSize *int64 `json:"batch_max_size,omitempty"`
|
||||
BatchFlushIntervalMS *int64 `json:"batch_flush_interval_ms,omitempty"`
|
||||
HeartbeatEnabled *bool `json:"heartbeat_enabled,omitempty"`
|
||||
HeartbeatIntervalMS *int64 `json:"heartbeat_interval_ms,omitempty"`
|
||||
JournaldUnit *string `json:"journald_unit,omitempty"`
|
||||
ExtraFilePaths []string `json:"extra_file_paths,omitempty"`
|
||||
}
|
||||
|
||||
type Agent struct {
|
||||
|
||||
Reference in New Issue
Block a user