From b185abc74fef0722127b7a20d2f89fbe94035e34 Mon Sep 17 00:00:00 2001 From: coffeecoder08 Date: Sun, 19 Jul 2026 21:42:39 +0530 Subject: [PATCH 1/4] feat(auth): add public Authorizer interface for Event Ledger Add public Authorizer interface, data models, and open-source implementations (NoopAuthorizer and WebhookAuthorizer) inside src/libraries/go/lib/pkg/auth. This decouples open-source builds like Event Ledger from proprietary authorization services. Ref: #180 --- src/libraries/go/lib/pkg/auth/BUILD.bazel | 10 +- src/libraries/go/lib/pkg/auth/authorizer.go | 175 ++++++++++++++++++ .../go/lib/pkg/auth/authorizer_test.go | 157 ++++++++++++++++ 3 files changed, 340 insertions(+), 2 deletions(-) create mode 100644 src/libraries/go/lib/pkg/auth/authorizer.go create mode 100644 src/libraries/go/lib/pkg/auth/authorizer_test.go diff --git a/src/libraries/go/lib/pkg/auth/BUILD.bazel b/src/libraries/go/lib/pkg/auth/BUILD.bazel index 0341a791e..83281e2ef 100644 --- a/src/libraries/go/lib/pkg/auth/BUILD.bazel +++ b/src/libraries/go/lib/pkg/auth/BUILD.bazel @@ -17,7 +17,10 @@ load("@rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "auth", - srcs = ["auth_token_fetcher.go"], + srcs = [ + "auth_token_fetcher.go", + "authorizer.go", + ], importpath = "github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/auth", visibility = ["//visibility:public"], deps = [ @@ -29,7 +32,10 @@ go_library( go_test( name = "auth_test", - srcs = ["auth_token_fetcher_test.go"], + srcs = [ + "auth_token_fetcher_test.go", + "authorizer_test.go", + ], data = glob(["test/**"]), embed = [":auth"], deps = [ diff --git a/src/libraries/go/lib/pkg/auth/authorizer.go b/src/libraries/go/lib/pkg/auth/authorizer.go new file mode 100644 index 000000000..a881a8d28 --- /dev/null +++ b/src/libraries/go/lib/pkg/auth/authorizer.go @@ -0,0 +1,175 @@ +/* +SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package auth + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "time" +) + +// Common authorization errors. +var ( + ErrUnauthorized = errors.New("unauthorized: missing or invalid authentication credentials") + ErrForbidden = errors.New("forbidden: principal lacks permission for the requested action") + ErrAuthorizerInternal = errors.New("internal authorizer error") +) + +// Action represents the permission or operation being requested. +type Action string + +const ( + ActionReadEvents Action = "eventledger.events.read" + ActionWriteEvents Action = "eventledger.events.write" + ActionAdmin Action = "eventledger.admin" +) + +// AuthRequest encapsulates the identity, action, and target resource for an authorization check. +type AuthRequest struct { + // Credential holds the token or secret presented by the client. + Credential string `json:"credential,omitempty"` + // OrgID identifies the organization or tenant context. + OrgID string `json:"org_id,omitempty"` + // PrincipalID identifies the user or service principal requesting access. + PrincipalID string `json:"principal_id,omitempty"` + // ResourceID identifies the target resource being accessed. + ResourceID string `json:"resource_id,omitempty"` + // Action specifies the operation requested on the resource. + Action Action `json:"action"` +} + +// AuthResult represents the outcome of an authorization check. +type AuthResult struct { + // Allowed indicates whether the requested action is permitted. + Allowed bool `json:"allowed"` + // PrincipalID is the verified identity of the requester. + PrincipalID string `json:"principal_id,omitempty"` + // Scopes lists the granted permission scopes or roles. + Scopes []string `json:"scopes,omitempty"` + // Reason provides human-readable context when access is denied. + Reason string `json:"reason,omitempty"` +} + +// Authorizer defines the public interface for evaluating access requests in open-source builds. +type Authorizer interface { + // Authorize evaluates whether the request is allowed to perform the action. + Authorize(ctx context.Context, req *AuthRequest) (*AuthResult, error) +} + +// NoopAuthorizer implements Authorizer by allowing all requests. +// This implementation is intended for local development and standalone testing. +type NoopAuthorizer struct{} + +// NewNoopAuthorizer creates a new NoopAuthorizer that allows every request. +func NewNoopAuthorizer() *NoopAuthorizer { + return &NoopAuthorizer{} +} + +// Authorize always returns an allowed result. +func (a *NoopAuthorizer) Authorize(ctx context.Context, req *AuthRequest) (*AuthResult, error) { + if req == nil { + return nil, ErrUnauthorized + } + principal := req.PrincipalID + if principal == "" { + principal = "anonymous" + } + return &AuthResult{ + Allowed: true, + PrincipalID: principal, + Scopes: []string{"*"}, + }, nil +} + +// WebhookAuthorizer implements Authorizer by delegating access evaluation to an external HTTP service. +type WebhookAuthorizer struct { + client *http.Client + endpointURL string +} + +// WebhookOption configures optional parameters for WebhookAuthorizer. +type WebhookOption func(*WebhookAuthorizer) + +// WithHTTPClient sets a custom HTTP client for the webhook authorizer. +func WithHTTPClient(client *http.Client) WebhookOption { + return func(w *WebhookAuthorizer) { + w.client = client + } +} + +// NewWebhookAuthorizer initializes a WebhookAuthorizer targeting the provided endpoint URL. +func NewWebhookAuthorizer(endpointURL string, opts ...WebhookOption) (*WebhookAuthorizer, error) { + if endpointURL == "" { + return nil, errors.New("webhook endpoint URL must not be empty") + } + w := &WebhookAuthorizer{ + endpointURL: endpointURL, + client: &http.Client{ + Timeout: 10 * time.Second, + }, + } + for _, opt := range opts { + opt(w) + } + return w, nil +} + +// Authorize posts the authorization request to the webhook endpoint and decodes the result. +func (w *WebhookAuthorizer) Authorize(ctx context.Context, req *AuthRequest) (*AuthResult, error) { + if req == nil { + return nil, ErrUnauthorized + } + + bodyBytes, err := json.Marshal(req) + if err != nil { + return nil, fmt.Errorf("%w: failed to marshal auth request: %v", ErrAuthorizerInternal, err) + } + + httpReq, err := http.NewRequestWithContext(ctx, http.MethodPost, w.endpointURL, bytes.NewReader(bodyBytes)) + if err != nil { + return nil, fmt.Errorf("%w: failed to create HTTP request: %v", ErrAuthorizerInternal, err) + } + httpReq.Header.Set("Content-Type", "application/json") + + resp, err := w.client.Do(httpReq) + if err != nil { + return nil, fmt.Errorf("%w: webhook request failed: %v", ErrAuthorizerInternal, err) + } + defer resp.Body.Close() + + if resp.StatusCode == http.StatusUnauthorized { + return nil, ErrUnauthorized + } + if resp.StatusCode == http.StatusForbidden { + return &AuthResult{Allowed: false, Reason: "forbidden by policy"}, nil + } + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("%w: unexpected status code %d from webhook", ErrAuthorizerInternal, resp.StatusCode) + } + + var result AuthResult + if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { + return nil, fmt.Errorf("%w: failed to decode webhook response: %v", ErrAuthorizerInternal, err) + } + + return &result, nil +} diff --git a/src/libraries/go/lib/pkg/auth/authorizer_test.go b/src/libraries/go/lib/pkg/auth/authorizer_test.go new file mode 100644 index 000000000..ba4be7318 --- /dev/null +++ b/src/libraries/go/lib/pkg/auth/authorizer_test.go @@ -0,0 +1,157 @@ +/* +SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package auth + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNoopAuthorizer(t *testing.T) { + authorizer := NewNoopAuthorizer() + ctx := context.Background() + + t.Run("nil request returns ErrUnauthorized", func(t *testing.T) { + res, err := authorizer.Authorize(ctx, nil) + require.ErrorIs(t, err, ErrUnauthorized) + assert.Nil(t, res) + }) + + t.Run("valid request is allowed", func(t *testing.T) { + req := &AuthRequest{ + PrincipalID: "test-user", + Action: ActionReadEvents, + ResourceID: "ledger-1", + } + res, err := authorizer.Authorize(ctx, req) + require.NoError(t, err) + require.NotNil(t, res) + assert.True(t, res.Allowed) + assert.Equal(t, "test-user", res.PrincipalID) + assert.Equal(t, []string{"*"}, res.Scopes) + }) + + t.Run("empty principal defaults to anonymous", func(t *testing.T) { + req := &AuthRequest{ + Action: ActionWriteEvents, + } + res, err := authorizer.Authorize(ctx, req) + require.NoError(t, err) + require.NotNil(t, res) + assert.True(t, res.Allowed) + assert.Equal(t, "anonymous", res.PrincipalID) + }) +} + +func TestWebhookAuthorizer(t *testing.T) { + ctx := context.Background() + + t.Run("empty endpoint URL returns error", func(t *testing.T) { + authorizer, err := NewWebhookAuthorizer("") + require.Error(t, err) + assert.Nil(t, authorizer) + }) + + t.Run("successful allowed authorization", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, http.MethodPost, r.Method) + assert.Equal(t, "application/json", r.Header.Get("Content-Type")) + + var req AuthRequest + err := json.NewDecoder(r.Body).Decode(&req) + require.NoError(t, err) + assert.Equal(t, ActionReadEvents, req.Action) + + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(AuthResult{ + Allowed: true, + PrincipalID: "webhook-user", + Scopes: []string{"read:events"}, + }) + })) + defer server.Close() + + authorizer, err := NewWebhookAuthorizer(server.URL) + require.NoError(t, err) + + res, err := authorizer.Authorize(ctx, &AuthRequest{Action: ActionReadEvents}) + require.NoError(t, err) + require.NotNil(t, res) + assert.True(t, res.Allowed) + assert.Equal(t, "webhook-user", res.PrincipalID) + assert.Equal(t, []string{"read:events"}, res.Scopes) + }) + + t.Run("webhook returns unauthorized status", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + })) + defer server.Close() + + authorizer, err := NewWebhookAuthorizer(server.URL) + require.NoError(t, err) + + res, err := authorizer.Authorize(ctx, &AuthRequest{Action: ActionReadEvents}) + require.ErrorIs(t, err, ErrUnauthorized) + assert.Nil(t, res) + }) + + t.Run("webhook returns forbidden status", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + })) + defer server.Close() + + authorizer, err := NewWebhookAuthorizer(server.URL) + require.NoError(t, err) + + res, err := authorizer.Authorize(ctx, &AuthRequest{Action: ActionReadEvents}) + require.NoError(t, err) + require.NotNil(t, res) + assert.False(t, res.Allowed) + assert.Equal(t, "forbidden by policy", res.Reason) + }) + + t.Run("webhook returns unexpected status code", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer server.Close() + + authorizer, err := NewWebhookAuthorizer(server.URL) + require.NoError(t, err) + + res, err := authorizer.Authorize(ctx, &AuthRequest{Action: ActionReadEvents}) + require.ErrorIs(t, err, ErrAuthorizerInternal) + assert.Nil(t, res) + }) + + t.Run("custom HTTP client configuration", func(t *testing.T) { + customClient := &http.Client{Timeout: 1 * time.Millisecond} + authorizer, err := NewWebhookAuthorizer("http://127.0.0.1:0", WithHTTPClient(customClient)) + require.NoError(t, err) + assert.Equal(t, customClient, authorizer.client) + }) +} From 2039d5859f9415d420da9ebe6ca5eac3bf825aa8 Mon Sep 17 00:00:00 2001 From: coffeecoder08 Date: Tue, 21 Jul 2026 08:32:27 +0530 Subject: [PATCH 2/4] refactor(auth): address review feedback on Authorizer interface Remove event-ledger-specific Action constants (ActionReadEvents, ActionWriteEvents, ActionAdmin) from the shared pkg/auth library. Keeping only the Action type here; consuming services define their own action values in their own packages. Disable HTTP redirects on WebhookAuthorizer so that 307/308 responses cannot replay a POST carrying AuthRequest.Credential to an unintended redirect target. The default client is constructed with noRedirectPolicy. Any caller-injected client that has no CheckRedirect set receives the same policy from the constructor. Guard against nil WebhookOption values and nil HTTP clients in NewWebhookAuthorizer, returning an error rather than producing an authorizer that panics at call time. Read the full webhook response body upfront with io.ReadAll so it is available on all non-200 paths. The 403 Reason is now populated from the body when present; non-200 errors include the body text for production debuggability. Switch from json.NewDecoder to json.Unmarshal since bytes are already in memory. Wrap all underlying errors with %w instead of %v throughout Authorize so callers can inspect the error chain with errors.Is and errors.As. Extend the test suite with coverage for: nil option rejection, nil client rejection, 403 body propagation, non-200 body in error messages, 307 and 308 redirect blocking regression cases, and injected-client redirect-policy enforcement. Ref: #180 Signed-off-by: coffeecoder08 --- src/libraries/go/lib/pkg/auth/authorizer.go | 53 +++++--- .../go/lib/pkg/auth/authorizer_test.go | 116 ++++++++++++++++-- 2 files changed, 145 insertions(+), 24 deletions(-) diff --git a/src/libraries/go/lib/pkg/auth/authorizer.go b/src/libraries/go/lib/pkg/auth/authorizer.go index a881a8d28..4370ec7e1 100644 --- a/src/libraries/go/lib/pkg/auth/authorizer.go +++ b/src/libraries/go/lib/pkg/auth/authorizer.go @@ -23,6 +23,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "net/http" "time" ) @@ -35,14 +36,9 @@ var ( ) // Action represents the permission or operation being requested. +// Consuming services should define their own Action constants in their own packages. type Action string -const ( - ActionReadEvents Action = "eventledger.events.read" - ActionWriteEvents Action = "eventledger.events.write" - ActionAdmin Action = "eventledger.admin" -) - // AuthRequest encapsulates the identity, action, and target resource for an authorization check. type AuthRequest struct { // Credential holds the token or secret presented by the client. @@ -100,6 +96,13 @@ func (a *NoopAuthorizer) Authorize(ctx context.Context, req *AuthRequest) (*Auth }, nil } +// noRedirectPolicy is an http.Client.CheckRedirect function that refuses all redirects. +// This prevents 307/308 responses from replaying a POST carrying AuthRequest.Credential +// to an unintended redirect target. +func noRedirectPolicy(_ *http.Request, _ []*http.Request) error { + return http.ErrUseLastResponse +} + // WebhookAuthorizer implements Authorizer by delegating access evaluation to an external HTTP service. type WebhookAuthorizer struct { client *http.Client @@ -110,6 +113,7 @@ type WebhookAuthorizer struct { type WebhookOption func(*WebhookAuthorizer) // WithHTTPClient sets a custom HTTP client for the webhook authorizer. +// The provided client must not be nil. func WithHTTPClient(client *http.Client) WebhookOption { return func(w *WebhookAuthorizer) { w.client = client @@ -117,6 +121,7 @@ func WithHTTPClient(client *http.Client) WebhookOption { } // NewWebhookAuthorizer initializes a WebhookAuthorizer targeting the provided endpoint URL. +// It returns an error if endpointURL is empty, any option is nil, or the resolved HTTP client is nil. func NewWebhookAuthorizer(endpointURL string, opts ...WebhookOption) (*WebhookAuthorizer, error) { if endpointURL == "" { return nil, errors.New("webhook endpoint URL must not be empty") @@ -124,12 +129,23 @@ func NewWebhookAuthorizer(endpointURL string, opts ...WebhookOption) (*WebhookAu w := &WebhookAuthorizer{ endpointURL: endpointURL, client: &http.Client{ - Timeout: 10 * time.Second, + Timeout: 10 * time.Second, + CheckRedirect: noRedirectPolicy, }, } for _, opt := range opts { + if opt == nil { + return nil, errors.New("webhook option must not be nil") + } opt(w) } + if w.client == nil { + return nil, errors.New("webhook HTTP client must not be nil") + } + // Enforce no-redirect policy on injected clients that do not set one. + if w.client.CheckRedirect == nil { + w.client.CheckRedirect = noRedirectPolicy + } return w, nil } @@ -141,34 +157,43 @@ func (w *WebhookAuthorizer) Authorize(ctx context.Context, req *AuthRequest) (*A bodyBytes, err := json.Marshal(req) if err != nil { - return nil, fmt.Errorf("%w: failed to marshal auth request: %v", ErrAuthorizerInternal, err) + return nil, fmt.Errorf("%w: failed to marshal auth request: %w", ErrAuthorizerInternal, err) } httpReq, err := http.NewRequestWithContext(ctx, http.MethodPost, w.endpointURL, bytes.NewReader(bodyBytes)) if err != nil { - return nil, fmt.Errorf("%w: failed to create HTTP request: %v", ErrAuthorizerInternal, err) + return nil, fmt.Errorf("%w: failed to create HTTP request: %w", ErrAuthorizerInternal, err) } httpReq.Header.Set("Content-Type", "application/json") resp, err := w.client.Do(httpReq) if err != nil { - return nil, fmt.Errorf("%w: webhook request failed: %v", ErrAuthorizerInternal, err) + return nil, fmt.Errorf("%w: webhook request failed: %w", ErrAuthorizerInternal, err) } defer resp.Body.Close() + respBody, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("%w: failed to read webhook response body: %w", ErrAuthorizerInternal, err) + } + if resp.StatusCode == http.StatusUnauthorized { return nil, ErrUnauthorized } if resp.StatusCode == http.StatusForbidden { - return &AuthResult{Allowed: false, Reason: "forbidden by policy"}, nil + reason := "forbidden by policy" + if len(respBody) > 0 { + reason = string(respBody) + } + return &AuthResult{Allowed: false, Reason: reason}, nil } if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("%w: unexpected status code %d from webhook", ErrAuthorizerInternal, resp.StatusCode) + return nil, fmt.Errorf("%w: unexpected status %d from webhook: %s", ErrAuthorizerInternal, resp.StatusCode, respBody) } var result AuthResult - if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { - return nil, fmt.Errorf("%w: failed to decode webhook response: %v", ErrAuthorizerInternal, err) + if err := json.Unmarshal(respBody, &result); err != nil { + return nil, fmt.Errorf("%w: failed to decode webhook response: %w", ErrAuthorizerInternal, err) } return &result, nil diff --git a/src/libraries/go/lib/pkg/auth/authorizer_test.go b/src/libraries/go/lib/pkg/auth/authorizer_test.go index ba4be7318..63381312b 100644 --- a/src/libraries/go/lib/pkg/auth/authorizer_test.go +++ b/src/libraries/go/lib/pkg/auth/authorizer_test.go @@ -29,6 +29,11 @@ import ( "github.com/stretchr/testify/require" ) +const ( + testActionRead Action = "test.resource.read" + testActionWrite Action = "test.resource.write" +) + func TestNoopAuthorizer(t *testing.T) { authorizer := NewNoopAuthorizer() ctx := context.Background() @@ -42,7 +47,7 @@ func TestNoopAuthorizer(t *testing.T) { t.Run("valid request is allowed", func(t *testing.T) { req := &AuthRequest{ PrincipalID: "test-user", - Action: ActionReadEvents, + Action: testActionRead, ResourceID: "ledger-1", } res, err := authorizer.Authorize(ctx, req) @@ -55,7 +60,7 @@ func TestNoopAuthorizer(t *testing.T) { t.Run("empty principal defaults to anonymous", func(t *testing.T) { req := &AuthRequest{ - Action: ActionWriteEvents, + Action: testActionWrite, } res, err := authorizer.Authorize(ctx, req) require.NoError(t, err) @@ -74,6 +79,18 @@ func TestWebhookAuthorizer(t *testing.T) { assert.Nil(t, authorizer) }) + t.Run("nil option returns error", func(t *testing.T) { + authorizer, err := NewWebhookAuthorizer("http://example.com", nil) + require.Error(t, err) + assert.Nil(t, authorizer) + }) + + t.Run("nil HTTP client returns error", func(t *testing.T) { + authorizer, err := NewWebhookAuthorizer("http://example.com", WithHTTPClient(nil)) + require.Error(t, err) + assert.Nil(t, authorizer) + }) + t.Run("successful allowed authorization", func(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { assert.Equal(t, http.MethodPost, r.Method) @@ -82,7 +99,7 @@ func TestWebhookAuthorizer(t *testing.T) { var req AuthRequest err := json.NewDecoder(r.Body).Decode(&req) require.NoError(t, err) - assert.Equal(t, ActionReadEvents, req.Action) + assert.Equal(t, testActionRead, req.Action) w.WriteHeader(http.StatusOK) _ = json.NewEncoder(w).Encode(AuthResult{ @@ -96,7 +113,7 @@ func TestWebhookAuthorizer(t *testing.T) { authorizer, err := NewWebhookAuthorizer(server.URL) require.NoError(t, err) - res, err := authorizer.Authorize(ctx, &AuthRequest{Action: ActionReadEvents}) + res, err := authorizer.Authorize(ctx, &AuthRequest{Action: testActionRead}) require.NoError(t, err) require.NotNil(t, res) assert.True(t, res.Allowed) @@ -113,12 +130,12 @@ func TestWebhookAuthorizer(t *testing.T) { authorizer, err := NewWebhookAuthorizer(server.URL) require.NoError(t, err) - res, err := authorizer.Authorize(ctx, &AuthRequest{Action: ActionReadEvents}) + res, err := authorizer.Authorize(ctx, &AuthRequest{Action: testActionRead}) require.ErrorIs(t, err, ErrUnauthorized) assert.Nil(t, res) }) - t.Run("webhook returns forbidden status", func(t *testing.T) { + t.Run("webhook returns forbidden status with empty body", func(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusForbidden) })) @@ -127,29 +144,108 @@ func TestWebhookAuthorizer(t *testing.T) { authorizer, err := NewWebhookAuthorizer(server.URL) require.NoError(t, err) - res, err := authorizer.Authorize(ctx, &AuthRequest{Action: ActionReadEvents}) + res, err := authorizer.Authorize(ctx, &AuthRequest{Action: testActionRead}) require.NoError(t, err) require.NotNil(t, res) assert.False(t, res.Allowed) assert.Equal(t, "forbidden by policy", res.Reason) }) - t.Run("webhook returns unexpected status code", func(t *testing.T) { + t.Run("webhook returns forbidden status with body", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte("policy: read access denied for org")) + })) + defer server.Close() + + authorizer, err := NewWebhookAuthorizer(server.URL) + require.NoError(t, err) + + res, err := authorizer.Authorize(ctx, &AuthRequest{Action: testActionRead}) + require.NoError(t, err) + require.NotNil(t, res) + assert.False(t, res.Allowed) + assert.Equal(t, "policy: read access denied for org", res.Reason) + }) + + t.Run("webhook returns unexpected status code with body", func(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte("upstream unavailable")) })) defer server.Close() authorizer, err := NewWebhookAuthorizer(server.URL) require.NoError(t, err) - res, err := authorizer.Authorize(ctx, &AuthRequest{Action: ActionReadEvents}) + res, err := authorizer.Authorize(ctx, &AuthRequest{Action: testActionRead}) require.ErrorIs(t, err, ErrAuthorizerInternal) + assert.Contains(t, err.Error(), "upstream unavailable") assert.Nil(t, res) }) + t.Run("307 redirect does not replay POST to redirect target", func(t *testing.T) { + redirectTarget := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // A replay reaching here would be a credential leak. + t.Error("redirect target must never receive the POST") + w.WriteHeader(http.StatusOK) + })) + defer redirectTarget.Close() + + primary := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, redirectTarget.URL, http.StatusTemporaryRedirect) + })) + defer primary.Close() + + authorizer, err := NewWebhookAuthorizer(primary.URL) + require.NoError(t, err) + + res, err := authorizer.Authorize(ctx, &AuthRequest{ + Action: testActionRead, + Credential: "secret-token", + }) + // The no-redirect policy causes the client to return the redirect response + // directly, which is neither 200/401/403, so we expect ErrAuthorizerInternal. + require.ErrorIs(t, err, ErrAuthorizerInternal) + assert.Nil(t, res) + }) + + t.Run("308 redirect does not replay POST to redirect target", func(t *testing.T) { + redirectTarget := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Error("redirect target must never receive the POST") + w.WriteHeader(http.StatusOK) + })) + defer redirectTarget.Close() + + primary := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, redirectTarget.URL, http.StatusPermanentRedirect) + })) + defer primary.Close() + + authorizer, err := NewWebhookAuthorizer(primary.URL) + require.NoError(t, err) + + res, err := authorizer.Authorize(ctx, &AuthRequest{ + Action: testActionRead, + Credential: "secret-token", + }) + require.ErrorIs(t, err, ErrAuthorizerInternal) + assert.Nil(t, res) + }) + + t.Run("injected client without redirect policy gets no-redirect applied", func(t *testing.T) { + // A client with no CheckRedirect set should have noRedirectPolicy applied by the constructor. + injected := &http.Client{Timeout: 5 * time.Second} + authorizer, err := NewWebhookAuthorizer("http://127.0.0.1:0", WithHTTPClient(injected)) + require.NoError(t, err) + assert.NotNil(t, authorizer.client.CheckRedirect) + }) + t.Run("custom HTTP client configuration", func(t *testing.T) { - customClient := &http.Client{Timeout: 1 * time.Millisecond} + customClient := &http.Client{ + Timeout: 1 * time.Millisecond, + CheckRedirect: noRedirectPolicy, + } authorizer, err := NewWebhookAuthorizer("http://127.0.0.1:0", WithHTTPClient(customClient)) require.NoError(t, err) assert.Equal(t, customClient, authorizer.client) From b650faa8584623d1423700ae9b98b5a57510d04e Mon Sep 17 00:00:00 2001 From: coffeecoder08 Date: Tue, 21 Jul 2026 08:55:45 +0530 Subject: [PATCH 3/4] refactor(auth): address second-round review feedback on WebhookAuthorizer Add OTEL transport instrumentation to the default WebhookAuthorizer HTTP client, matching the pattern used by TokenFetcher. W3C Trace Context is propagated on every outbound webhook call via the otelhttp.NewTransport wrapper. Bound webhook response-body reads with io.LimitReader at 4096 bytes to prevent a faulty or hostile endpoint from consuming unbounded memory. The limit is expressed as the exported-to-test constant maxWebhookResponseBytes so tests can assert against it directly. Extend the test suite to cover the new error-wrapping paths: - transport failure (TCP dial to a closed port) asserts ErrAuthorizerInternal - oversized response body asserts ErrAuthorizerInternal and that the error length respects the size cap - malformed JSON on a 200 response asserts ErrAuthorizerInternal Ref: #180 Signed-off-by: coffeecoder08 --- src/libraries/go/lib/pkg/auth/authorizer.go | 23 ++++++-- .../go/lib/pkg/auth/authorizer_test.go | 52 +++++++++++++++++++ 2 files changed, 70 insertions(+), 5 deletions(-) diff --git a/src/libraries/go/lib/pkg/auth/authorizer.go b/src/libraries/go/lib/pkg/auth/authorizer.go index 4370ec7e1..7aeed03d7 100644 --- a/src/libraries/go/lib/pkg/auth/authorizer.go +++ b/src/libraries/go/lib/pkg/auth/authorizer.go @@ -26,6 +26,8 @@ import ( "io" "net/http" "time" + + "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" ) // Common authorization errors. @@ -35,6 +37,10 @@ var ( ErrAuthorizerInternal = errors.New("internal authorizer error") ) +// maxWebhookResponseBytes limits how many bytes are read from a webhook response body. +// This prevents a faulty or hostile endpoint from consuming unbounded memory. +const maxWebhookResponseBytes = 4096 + // Action represents the permission or operation being requested. // Consuming services should define their own Action constants in their own packages. type Action string @@ -122,16 +128,19 @@ func WithHTTPClient(client *http.Client) WebhookOption { // NewWebhookAuthorizer initializes a WebhookAuthorizer targeting the provided endpoint URL. // It returns an error if endpointURL is empty, any option is nil, or the resolved HTTP client is nil. +// The default HTTP client is instrumented with OpenTelemetry and configured to refuse redirects. func NewWebhookAuthorizer(endpointURL string, opts ...WebhookOption) (*WebhookAuthorizer, error) { if endpointURL == "" { return nil, errors.New("webhook endpoint URL must not be empty") } + defaultClient := &http.Client{ + Timeout: 10 * time.Second, + CheckRedirect: noRedirectPolicy, + } + defaultClient.Transport = otelhttp.NewTransport(defaultClient.Transport) w := &WebhookAuthorizer{ endpointURL: endpointURL, - client: &http.Client{ - Timeout: 10 * time.Second, - CheckRedirect: noRedirectPolicy, - }, + client: defaultClient, } for _, opt := range opts { if opt == nil { @@ -150,6 +159,8 @@ func NewWebhookAuthorizer(endpointURL string, opts ...WebhookOption) (*WebhookAu } // Authorize posts the authorization request to the webhook endpoint and decodes the result. +// It propagates the W3C Trace Context from ctx via the OTEL-instrumented transport and records +// transport errors and non-OK HTTP responses on the resulting span. func (w *WebhookAuthorizer) Authorize(ctx context.Context, req *AuthRequest) (*AuthResult, error) { if req == nil { return nil, ErrUnauthorized @@ -172,7 +183,9 @@ func (w *WebhookAuthorizer) Authorize(ctx context.Context, req *AuthRequest) (*A } defer resp.Body.Close() - respBody, err := io.ReadAll(resp.Body) + // Limit body reads to maxWebhookResponseBytes to guard against unbounded memory use + // from a faulty or hostile webhook endpoint. + respBody, err := io.ReadAll(io.LimitReader(resp.Body, maxWebhookResponseBytes)) if err != nil { return nil, fmt.Errorf("%w: failed to read webhook response body: %w", ErrAuthorizerInternal, err) } diff --git a/src/libraries/go/lib/pkg/auth/authorizer_test.go b/src/libraries/go/lib/pkg/auth/authorizer_test.go index 63381312b..b1e9cdb63 100644 --- a/src/libraries/go/lib/pkg/auth/authorizer_test.go +++ b/src/libraries/go/lib/pkg/auth/authorizer_test.go @@ -22,6 +22,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -250,4 +251,55 @@ func TestWebhookAuthorizer(t *testing.T) { require.NoError(t, err) assert.Equal(t, customClient, authorizer.client) }) + + t.Run("transport failure returns ErrAuthorizerInternal", func(t *testing.T) { + // Point at a port with no listener so the TCP dial fails immediately. + authorizer, err := NewWebhookAuthorizer("http://127.0.0.1:1", + WithHTTPClient(&http.Client{ + Timeout: 50 * time.Millisecond, + CheckRedirect: noRedirectPolicy, + }), + ) + require.NoError(t, err) + + res, err := authorizer.Authorize(ctx, &AuthRequest{Action: testActionRead}) + require.ErrorIs(t, err, ErrAuthorizerInternal) + assert.Nil(t, res) + }) + + t.Run("oversized response body returns ErrAuthorizerInternal", func(t *testing.T) { + // Respond with a body larger than maxWebhookResponseBytes to verify the + // size limit is enforced. io.LimitReader truncates cleanly so the read + // itself does not error; the status is non-200 to exercise the error path. + oversized := strings.Repeat("x", maxWebhookResponseBytes+1) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(oversized)) + })) + defer server.Close() + + authorizer, err := NewWebhookAuthorizer(server.URL) + require.NoError(t, err) + + res, err := authorizer.Authorize(ctx, &AuthRequest{Action: testActionRead}) + require.ErrorIs(t, err, ErrAuthorizerInternal) + // The error must not contain more than maxWebhookResponseBytes of body text. + assert.LessOrEqual(t, len(err.Error()), maxWebhookResponseBytes+256) + assert.Nil(t, res) + }) + + t.Run("malformed JSON on 200 returns ErrAuthorizerInternal", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("not-valid-json")) + })) + defer server.Close() + + authorizer, err := NewWebhookAuthorizer(server.URL) + require.NoError(t, err) + + res, err := authorizer.Authorize(ctx, &AuthRequest{Action: testActionRead}) + require.ErrorIs(t, err, ErrAuthorizerInternal) + assert.Nil(t, res) + }) } From f72a3d61e1ef046fb34f4bbfcdb5e515d93028e7 Mon Sep 17 00:00:00 2001 From: coffeecoder08 Date: Thu, 23 Jul 2026 23:05:04 +0530 Subject: [PATCH 4/4] refactor(auth): always enforce noRedirectPolicy on private copy of injected client Instead of conditionally setting CheckRedirect only when the injected client had none, create a private copy of any caller-injected HTTP client with: - noRedirectPolicy always applied (prevents 307/308 from replaying a POST carrying AuthRequest.Credential to an unintended redirect target, regardless of any CheckRedirect the caller set) - transport wrapped with otelhttp.NewTransport for W3C Trace Context propagation on every outbound webhook call - timeout preserved from the injected client The caller-owned client is never mutated. Extend the test suite to cover the new behavior: - injected client without CheckRedirect gets noRedirectPolicy on a private copy; original client is not mutated (NotSame assertion) - injected client with a permissive custom CheckRedirect is still blocked from following 307 redirects (regression test with redirect target server) - private copy preserves transport and timeout from the injected client - custom HTTP client configuration test updated to assert NotSame (private copy) rather than Equal (original pointer) Ref: #180 Signed-off-by: coffeecoder08 --- src/libraries/go/lib/pkg/auth/authorizer.go | 17 ++++- .../go/lib/pkg/auth/authorizer_test.go | 67 +++++++++++++++++-- 2 files changed, 77 insertions(+), 7 deletions(-) diff --git a/src/libraries/go/lib/pkg/auth/authorizer.go b/src/libraries/go/lib/pkg/auth/authorizer.go index 7aeed03d7..e0dd7d2e0 100644 --- a/src/libraries/go/lib/pkg/auth/authorizer.go +++ b/src/libraries/go/lib/pkg/auth/authorizer.go @@ -129,6 +129,8 @@ func WithHTTPClient(client *http.Client) WebhookOption { // NewWebhookAuthorizer initializes a WebhookAuthorizer targeting the provided endpoint URL. // It returns an error if endpointURL is empty, any option is nil, or the resolved HTTP client is nil. // The default HTTP client is instrumented with OpenTelemetry and configured to refuse redirects. +// Any caller-injected client is wrapped in a private copy with noRedirectPolicy always applied and +// its transport wrapped with otelhttp.NewTransport; the caller-owned client is never mutated. func NewWebhookAuthorizer(endpointURL string, opts ...WebhookOption) (*WebhookAuthorizer, error) { if endpointURL == "" { return nil, errors.New("webhook endpoint URL must not be empty") @@ -151,9 +153,18 @@ func NewWebhookAuthorizer(endpointURL string, opts ...WebhookOption) (*WebhookAu if w.client == nil { return nil, errors.New("webhook HTTP client must not be nil") } - // Enforce no-redirect policy on injected clients that do not set one. - if w.client.CheckRedirect == nil { - w.client.CheckRedirect = noRedirectPolicy + // When the caller injected a custom client, replace it with a private copy so we + // never mutate a caller-owned shared client. Always enforce noRedirectPolicy on the + // private copy regardless of any CheckRedirect already set by the caller, preventing + // 307/308 responses from replaying a POST that carries AuthRequest.Credential to an + // unintended redirect target. Wrap the transport with otelhttp.NewTransport to + // propagate W3C Trace Context on every outbound webhook call. + if w.client != defaultClient { + w.client = &http.Client{ + Transport: otelhttp.NewTransport(w.client.Transport), + Timeout: w.client.Timeout, + CheckRedirect: noRedirectPolicy, + } } return w, nil } diff --git a/src/libraries/go/lib/pkg/auth/authorizer_test.go b/src/libraries/go/lib/pkg/auth/authorizer_test.go index b1e9cdb63..ca997c7f7 100644 --- a/src/libraries/go/lib/pkg/auth/authorizer_test.go +++ b/src/libraries/go/lib/pkg/auth/authorizer_test.go @@ -234,22 +234,81 @@ func TestWebhookAuthorizer(t *testing.T) { assert.Nil(t, res) }) - t.Run("injected client without redirect policy gets no-redirect applied", func(t *testing.T) { - // A client with no CheckRedirect set should have noRedirectPolicy applied by the constructor. + t.Run("injected client without redirect policy gets no-redirect applied on private copy", func(t *testing.T) { + // A client with no CheckRedirect set receives noRedirectPolicy on a private copy; + // the caller's original client is not mutated. injected := &http.Client{Timeout: 5 * time.Second} authorizer, err := NewWebhookAuthorizer("http://127.0.0.1:0", WithHTTPClient(injected)) require.NoError(t, err) assert.NotNil(t, authorizer.client.CheckRedirect) + // The constructor must never mutate the caller-owned client. + assert.Nil(t, injected.CheckRedirect, "caller client must not be mutated") + // The private copy must be a different pointer. + assert.NotSame(t, injected, authorizer.client) }) - t.Run("custom HTTP client configuration", func(t *testing.T) { + t.Run("injected client with custom CheckRedirect is still replaced by noRedirectPolicy", func(t *testing.T) { + // A caller-supplied permissive redirect policy must be overridden so that + // 307/308 responses cannot replay a POST carrying AuthRequest.Credential. + allowRedirects := func(req *http.Request, via []*http.Request) error { return nil } + injected := &http.Client{ + Timeout: 3 * time.Second, + CheckRedirect: allowRedirects, + } + + redirectTarget := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Error("redirect target must never receive the POST even with a custom CheckRedirect") + w.WriteHeader(http.StatusOK) + })) + defer redirectTarget.Close() + + primary := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, redirectTarget.URL, http.StatusTemporaryRedirect) + })) + defer primary.Close() + + authorizer, err := NewWebhookAuthorizer(primary.URL, WithHTTPClient(injected)) + require.NoError(t, err) + + res, err := authorizer.Authorize(ctx, &AuthRequest{ + Action: testActionRead, + Credential: "secret-token", + }) + // noRedirectPolicy causes the 307 to surface directly as ErrAuthorizerInternal, + // not as a replayed POST to the redirect target. + require.ErrorIs(t, err, ErrAuthorizerInternal) + assert.Nil(t, res) + }) + + t.Run("injected client transport and timeout are preserved in private copy", func(t *testing.T) { + customTransport := &http.Transport{MaxIdleConns: 7} + injected := &http.Client{ + Transport: customTransport, + Timeout: 3 * time.Second, + } + authorizer, err := NewWebhookAuthorizer("http://127.0.0.1:0", WithHTTPClient(injected)) + require.NoError(t, err) + // Timeout must be carried over to the private copy. + assert.Equal(t, 3*time.Second, authorizer.client.Timeout) + // The private copy must enforce noRedirectPolicy. + assert.NotNil(t, authorizer.client.CheckRedirect) + // The private copy must be a different pointer from the injected client. + assert.NotSame(t, injected, authorizer.client) + }) + + t.Run("custom HTTP client configuration uses private copy with noRedirectPolicy", func(t *testing.T) { customClient := &http.Client{ Timeout: 1 * time.Millisecond, CheckRedirect: noRedirectPolicy, } authorizer, err := NewWebhookAuthorizer("http://127.0.0.1:0", WithHTTPClient(customClient)) require.NoError(t, err) - assert.Equal(t, customClient, authorizer.client) + // The authorizer holds a private copy, not the original pointer. + assert.NotSame(t, customClient, authorizer.client) + // Timeout is preserved. + assert.Equal(t, 1*time.Millisecond, authorizer.client.Timeout) + // noRedirectPolicy is set. + assert.NotNil(t, authorizer.client.CheckRedirect) }) t.Run("transport failure returns ErrAuthorizerInternal", func(t *testing.T) {