Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,4 @@ vendor
e2e_*

.buildxcache/
.claude/*
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ Usage of ./observatorium-api:
The endpoint against which to send read requests for metrics.
-metrics.rules.endpoint string
The endpoint against which to make get requests for listing recording/alerting rules and put requests for creating/updating recording/alerting rules.
-metrics.status.endpoint string
The endpoint against which to make requests for status information about metrics (e.g. '/api/v1/status/tsdb').
-metrics.tenant-header string
The name of the HTTP header containing the tenant ID to forward to the metrics upstreams. (default "THANOS-TENANT")
-metrics.tenant-label string
Expand Down
46 changes: 36 additions & 10 deletions authentication/authentication.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ type tenantHandlers map[string]http.Handler
type ProviderManager struct {
mtx sync.RWMutex
patternHandlers map[string]tenantHandlers
middlewares map[string]Middleware
gRPCInterceptors map[string]grpc.StreamServerInterceptor
middlewares map[string][]Middleware
gRPCInterceptors map[string][]grpc.StreamServerInterceptor
logger log.Logger
registrationRetryCount *prometheus.CounterVec
}
Expand All @@ -59,8 +59,8 @@ func NewProviderManager(l log.Logger, registrationRetryCount *prometheus.Counter
return &ProviderManager{
registrationRetryCount: registrationRetryCount,
patternHandlers: make(map[string]tenantHandlers),
middlewares: make(map[string]Middleware),
gRPCInterceptors: make(map[string]grpc.StreamServerInterceptor),
middlewares: make(map[string][]Middleware),
gRPCInterceptors: make(map[string][]grpc.StreamServerInterceptor),
logger: l,
}
}
Expand Down Expand Up @@ -88,8 +88,8 @@ func (pm *ProviderManager) InitializeProvider(config map[string]interface{},
}

pm.mtx.Lock()
pm.middlewares[tenant] = provider.Middleware()
pm.gRPCInterceptors[tenant] = provider.GRPCMiddleware()
pm.middlewares[tenant] = append(pm.middlewares[tenant], provider.Middleware())
pm.gRPCInterceptors[tenant] = append(pm.gRPCInterceptors[tenant], provider.GRPCMiddleware())
pattern, handler := provider.Handler()
if pattern != "" && handler != nil {
if pm.patternHandlers[pattern] == nil {
Expand All @@ -109,19 +109,45 @@ func (pm *ProviderManager) InitializeProvider(config map[string]interface{},
// Middleware returns an authentication middleware for a tenant.
func (pm *ProviderManager) Middlewares(tenant string) (Middleware, bool) {
pm.mtx.RLock()
mw, ok := pm.middlewares[tenant]
mws, ok := pm.middlewares[tenant]
pm.mtx.RUnlock()

return mw, ok
if !ok || len(mws) == 0 {
return nil, false
}

// If only one middleware, return it directly
if len(mws) == 1 {
return mws[0], true
}

// Chain all middlewares together - each will check its own PathPatterns
// and either authenticate or pass through to the next middleware
return func(next http.Handler) http.Handler {
handler := next
for i := len(mws) - 1; i >= 0; i-- {
handler = mws[i](handler)
}
return handler
}, true
}

// GRPCMiddlewares returns an authentication interceptor for a tenant.
func (pm *ProviderManager) GRPCMiddlewares(tenant string) (grpc.StreamServerInterceptor, bool) {
pm.mtx.RLock()
mw, ok := pm.gRPCInterceptors[tenant]
interceptors, ok := pm.gRPCInterceptors[tenant]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😗👌🏽

pm.mtx.RUnlock()

return mw, ok
if !ok || len(interceptors) == 0 {
return nil, false
}

// If only one interceptor, return it directly
if len(interceptors) == 1 {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this conditional needed? We return interceptors[0], true regardless at the end of this method.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was just for consistency since this doesnt really work here right now and to keep the logic the same, but happy to adapt

return interceptors[0], true
}

return interceptors[0], true
}

// PatternHandler return an http.HandlerFunc for a corresponding pattern.
Expand Down
16 changes: 8 additions & 8 deletions authentication/authentication_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,46 +120,46 @@ func TestTokenExpiredErrorHandling(t *testing.T) {
expiredErr := &oidc.TokenExpiredError{
Expiry: time.Now().Add(-time.Hour), // Expired an hour ago
}

// Test direct error
var tokenExpiredErr *oidc.TokenExpiredError
if !errors.As(expiredErr, &tokenExpiredErr) {
t.Error("errors.As should identify TokenExpiredError")
}

// Test wrapped error
wrappedErr := &wrappedError{
msg: "verification failed",
err: expiredErr,
}

if !errors.As(wrappedErr, &tokenExpiredErr) {
t.Error("errors.As should identify wrapped TokenExpiredError")
}
})

t.Run("Other errors are not identified as TokenExpiredError", func(t *testing.T) {
// Test with a generic error
genericErr := errors.New("generic verification error")

var tokenExpiredErr *oidc.TokenExpiredError
if errors.As(genericErr, &tokenExpiredErr) {
t.Error("errors.As should not identify generic error as TokenExpiredError")
}

// Test with wrapped generic error
wrappedGenericErr := &wrappedError{
msg: "verification failed",
err: genericErr,
}

if errors.As(wrappedGenericErr, &tokenExpiredErr) {
t.Error("errors.As should not identify wrapped generic error as TokenExpiredError")
}
})
}

// Helper type to wrap errors for testing
// Helper type to wrap errors for testing.
type wrappedError struct {
msg string
err error
Expand Down
62 changes: 39 additions & 23 deletions authentication/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"net/http"
"regexp"
"strings"

"github.com/go-chi/chi/v5"
Expand Down Expand Up @@ -31,6 +32,8 @@ const (
tenantKey contextKey = "tenant"
// tenantIDKey is the key that holds the tenant ID in a request context.
tenantIDKey contextKey = "tenantID"
// authenticatedKey is the key that indicates a request has been successfully authenticated.
authenticatedKey contextKey = "authenticated"
)

// WithTenant finds the tenant from the URL parameters and adds it to the request context.
Expand Down Expand Up @@ -131,6 +134,18 @@ func GetAccessToken(ctx context.Context) (string, bool) {
return token, ok
}

// SetAuthenticated marks the request as successfully authenticated.
func SetAuthenticated(ctx context.Context) context.Context {
return context.WithValue(ctx, authenticatedKey, true)
}

// IsAuthenticated checks if the request has been successfully authenticated.
func IsAuthenticated(ctx context.Context) bool {
value := ctx.Value(authenticatedKey)
authenticated, ok := value.(bool)
return ok && authenticated
}

// Middleware is a convenience type for functions that wrap http.Handlers.
type Middleware func(http.Handler) http.Handler

Expand Down Expand Up @@ -161,34 +176,35 @@ func WithTenantMiddlewares(mwFns ...MiddlewareFunc) Middleware {
}
}

// EnforceAccessTokenPresentOnSignalWrite enforces that the Authorization header is present in the incoming request
// for the given list of tenants. Otherwise, it returns an error.
// It protects the Prometheus remote write and Loki push endpoints. The tracing endpoint is not protected because
// it goes through the gRPC middleware stack, which behaves differently from the HTTP one.
func EnforceAccessTokenPresentOnSignalWrite(oidcTenants map[string]struct{}) func(http.Handler) http.Handler {
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i really dont understand why this was here. it was enforcing oidc on all write requests.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it ignores it further down, but good refactor. Your implementation is clearer.

// EnforceAuthentication is a final middleware that ensures at least one authenticator
// has successfully validated the request. This prevents security gaps where requests
// might bypass all authentication checks.
func EnforceAuthentication() Middleware {
return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
tenant := chi.URLParam(r, "tenant")

// If there's no tenant, we're not interested in blocking this request.
if tenant == "" {
next.ServeHTTP(w, r)
if !IsAuthenticated(r.Context()) {
httperr.PrometheusAPIError(w, "request not authenticated by any provider", http.StatusUnauthorized)
return
}

// And we aren't interested in blocking requests from tenants not using OIDC.
if _, found := oidcTenants[tenant]; !found {
next.ServeHTTP(w, r)
return
}

rawToken := r.Header.Get("Authorization")
if rawToken == "" {
httperr.PrometheusAPIError(w, "couldn't find the authorization header", http.StatusBadRequest)
return
}

next.ServeHTTP(w, r)
})
}
}

// Path pattern matching operators.
const (
OperatorMatches = "=~" // Pattern must match
OperatorNotMatches = "!~" // Pattern must NOT match
)

// PathPattern represents a path pattern with an operator for matching.
type PathPattern struct {
Operator string `json:"operator,omitempty"`
Pattern string `json:"pattern"`
}

// PathMatcher represents a compiled path pattern with operator.
type PathMatcher struct {
Operator string
Regex *regexp.Regexp
}
60 changes: 43 additions & 17 deletions authentication/mtls.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"regexp"

"github.com/go-kit/log"
"github.com/go-kit/log/level"
grpc_middleware_auth "github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/auth"
"github.com/mitchellh/mapstructure"
"github.com/prometheus/client_golang/prometheus"
Expand All @@ -29,11 +30,11 @@ func init() {
}

type mTLSConfig struct {
RawCA []byte `json:"ca"`
CAPath string `json:"caPath"`
PathPatterns []string `json:"pathPatterns"`
RawCA []byte `json:"ca"`
CAPath string `json:"caPath"`
Paths []PathPattern `json:"paths,omitempty"`
CAs []*x509.Certificate
pathMatchers []*regexp.Regexp
pathMatchers []PathMatcher
}

type MTLSAuthenticator struct {
Expand Down Expand Up @@ -86,13 +87,25 @@ func newMTLSAuthenticator(c map[string]interface{}, tenant string, registrationR
config.CAs = cas
}

// Compile path patterns
for _, pattern := range config.PathPatterns {
matcher, err := regexp.Compile(pattern)
for _, pathPattern := range config.Paths {
operator := pathPattern.Operator
if operator == "" {
operator = OperatorMatches
}
Comment on lines +92 to +94
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't got to that part of the PR, but we should document that the default behaviour flag docs is matching if we include this handling.


if operator != OperatorMatches && operator != OperatorNotMatches {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if having both isn't redundant. I.e. if a user specifies [{!~, /query}, {=~, /receive} the former is enforced and the latter is ignored. imho we should bias to explicit matching only.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my first iteration, but because go does not support negative lookaheads and only RE2 it is going to make configuration extremely complicate to avoid overlpas if one so wishes.

For example, take a look at

  oidc:
    clientID: observatorium-api
    clientSecret: ZXhhbXBsZS1hcHAtc2VjcmV0
    issuerURL: http://dex.proxy.svc.cluster.local:5556/dex
    redirectURL: http://localhost:8080/oidc/auth-tenant/callback
    usernameClaim: email
    paths:
    - operator: "!~"
      pattern: ".*(loki/api/v1/push|api/v1/receive).*"
  mTLS:
    caPath: /etc/certs/ca.crt
    paths:
    - operator: "=~"
      pattern: ".*(api/v1/receive).*"
    - operator: "=~"
      pattern: ".*(loki/api/v1/push).*"

and imagine the alternative where the operator isnt available.

I would have preferred the original too for simplicity but can confidently say having iterated through the test environment to try to achieve the outcome above, this is much better and leads to much simpler runtime config for end user.

return nil, fmt.Errorf("invalid mTLS path operator %q, must be %q or %q", operator, OperatorMatches, OperatorNotMatches)
}

matcher, err := regexp.Compile(pathPattern.Pattern)
if err != nil {
return nil, fmt.Errorf("failed to compile mTLS path pattern %q: %v", pattern, err)
return nil, fmt.Errorf("failed to compile mTLS path pattern %q: %v", pathPattern.Pattern, err)
}
config.pathMatchers = append(config.pathMatchers, matcher)

config.pathMatchers = append(config.pathMatchers, PathMatcher{
Operator: operator,
Regex: matcher,
})
}

return MTLSAuthenticator{
Expand All @@ -105,24 +118,37 @@ func newMTLSAuthenticator(c map[string]interface{}, tenant string, registrationR
func (a MTLSAuthenticator) Middleware() Middleware {
return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
level.Debug(a.logger).Log("msg", "mTLS middleware processing", "path", r.URL.Path, "tenant", a.tenant, "numPatterns", len(a.config.pathMatchers))

// Check if mTLS is required for this path
if len(a.config.pathMatchers) > 0 {
pathMatches := false
shouldEnforceMTLS := false

for _, matcher := range a.config.pathMatchers {
if matcher.MatchString(r.URL.Path) {
pathMatches = true
regexMatches := matcher.Regex.MatchString(r.URL.Path)
level.Debug(a.logger).Log("msg", "mTLS path pattern check", "path", r.URL.Path, "operator", matcher.Operator, "pattern", matcher.Regex.String(), "matches", regexMatches)

if matcher.Operator == OperatorMatches && regexMatches {
level.Debug(a.logger).Log("msg", "mTLS positive match - enforcing", "path", r.URL.Path)
shouldEnforceMTLS = true
break
} else if matcher.Operator == OperatorNotMatches && !regexMatches {
// Negative match - enforce mTLS (path does NOT match pattern)
level.Debug(a.logger).Log("msg", "mTLS negative match - enforcing", "path", r.URL.Path)
shouldEnforceMTLS = true
break
}
}

// If path doesn't match, skip mTLS enforcement
if !pathMatches {
level.Debug(a.logger).Log("msg", "mTLS enforcement decision", "path", r.URL.Path, "shouldEnforceMTLS", shouldEnforceMTLS)
if !shouldEnforceMTLS {
level.Debug(a.logger).Log("msg", "mTLS skipping enforcement", "path", r.URL.Path)
next.ServeHTTP(w, r)
return
}
}

// Path matches or no paths configured, enforce mTLS
level.Debug(a.logger).Log("msg", "mTLS enforcing authentication", "path", r.URL.Path, "tenant", a.tenant)
if r.TLS == nil {
httperr.PrometheusAPIError(w, "mTLS required but no TLS connection", http.StatusBadRequest)
return
Expand Down Expand Up @@ -174,10 +200,11 @@ func (a MTLSAuthenticator) Middleware() Middleware {
return
}
ctx := context.WithValue(r.Context(), subjectKey, sub)

// Add organizational units as groups.
ctx = context.WithValue(ctx, groupsKey, r.TLS.PeerCertificates[0].Subject.OrganizationalUnit)

// Mark request as successfully authenticated
ctx = SetAuthenticated(ctx)
next.ServeHTTP(w, r.WithContext(ctx))
})
}
Expand All @@ -192,4 +219,3 @@ func (a MTLSAuthenticator) GRPCMiddleware() grpc.StreamServerInterceptor {
func (a MTLSAuthenticator) Handler() (string, http.Handler) {
return "", nil
}

Loading