feat(auth): add public Authorizer interface for Event Ledger#256
feat(auth): add public Authorizer interface for Event Ledger#256raza-khan0108 wants to merge 3 commits into
Conversation
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: NVIDIA#180
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a public Go authorization interface with no-op and webhook implementations, exported request/result types, HTTP status mapping, bounded response handling, unit tests, and Bazel source registration. ChangesAuthorization providers
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant WebhookAuthorizer
participant HTTPClient
participant AuthorizationService
Caller->>WebhookAuthorizer: Authorize(ctx, AuthRequest)
WebhookAuthorizer->>HTTPClient: POST JSON request
HTTPClient->>AuthorizationService: HTTP authorization request
AuthorizationService-->>HTTPClient: status and AuthResult
HTTPClient-->>WebhookAuthorizer: HTTP response
WebhookAuthorizer-->>Caller: AuthResult or authorization error
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/libraries/go/lib/pkg/auth/authorizer.go (1)
147-155: 🩺 Stability & Availability | 🔵 TrivialInstrument the webhook authorization call. Wrap the client transport with
otelhttp.NewTransportand add redacted structured logs on failures. Do not logCredentialor the request body; if you add metrics, keep labels bounded and cover any new label values with tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libraries/go/lib/pkg/auth/authorizer.go` around lines 147 - 155, Instrument the webhook request in the authorizer flow around httpReq and w.client.Do by using an otelhttp.NewTransport-wrapped client transport and adding structured, redacted failure logs. Exclude Credential and request body from all logs; if introducing metrics, use only bounded labels and add tests for every new label value.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/libraries/go/lib/pkg/auth/authorizer.go`:
- Around line 130-133: Update the authorizer constructor around the opts loop to
skip nil WebhookOption values before invoking them, then validate the resolved
HTTP client and return an error when it is nil instead of returning a usable
authorizer. Add coverage for both nil options and WithHTTPClient(nil).
- Around line 142-171: Update the error-wrapping paths in the authorizer request
flow—covering json.Marshal, http.NewRequestWithContext, w.client.Do, and the
response decoder—to preserve each underlying cause for errors.Is/errors.As. Use
wrapping that retains both ErrAuthorizerInternal and the original error, while
keeping the existing contextual messages and sentinel behavior.
- Around line 124-128: Configure the http.Client created in WebhookAuthorizer to
reject redirects, and apply the same no-redirect policy to any injected client
before webhook requests are sent. Update the WebhookAuthorizer-related tests
with a regression case verifying 307/308 responses do not replay the POST or
expose AuthRequest.Credential to the redirect target.
---
Nitpick comments:
In `@src/libraries/go/lib/pkg/auth/authorizer.go`:
- Around line 147-155: Instrument the webhook request in the authorizer flow
around httpReq and w.client.Do by using an otelhttp.NewTransport-wrapped client
transport and adding structured, redacted failure logs. Exclude Credential and
request body from all logs; if introducing metrics, use only bounded labels and
add tests for every new label value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e38b79d2-3341-4565-8433-d25c6da3916c
📒 Files selected for processing (3)
src/libraries/go/lib/pkg/auth/BUILD.bazelsrc/libraries/go/lib/pkg/auth/authorizer.gosrc/libraries/go/lib/pkg/auth/authorizer_test.go
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: NVIDIA#180 Signed-off-by: coffeecoder08 <razawarsi828@gmail.com>
|
Pushed as commit 1. Action constants moved out of
|
| Test case | What it covers |
|---|---|
nil option returns error |
Constructor rejects nil WebhookOption |
nil HTTP client returns error |
WithHTTPClient(nil) rejected at construction |
webhook returns forbidden status with body |
403 body propagated to Reason |
webhook returns unexpected status code with body |
Non-200 body present in error string |
307 redirect does not replay POST to redirect target |
Redirect-blocking regression |
308 redirect does not replay POST to redirect target |
Redirect-blocking regression |
injected client without redirect policy gets no-redirect applied |
Constructor enforces policy on injected clients |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/libraries/go/lib/pkg/auth/authorizer.go (1)
145-148: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEnforce redirect blocking for every injected client.
A non-nil custom
CheckRedirectis preserved here, so it can follow a 307/308 and replayAuthRequest.Credential. Always installnoRedirectPolicyon the authorizer's private client instance; do not mutate a caller-owned shared client. Update the custom-client test to verify its transport/timeout are retained while redirects remain blocked.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libraries/go/lib/pkg/auth/authorizer.go` around lines 145 - 148, Update the authorizer initialization around w.client and noRedirectPolicy to always install noRedirectPolicy, regardless of any injected CheckRedirect value, on a private client instance rather than mutating the caller-owned client. Preserve the injected client’s transport and timeout settings, and update the custom-client test to verify those settings remain intact while redirects are blocked.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/libraries/go/lib/pkg/auth/authorizer_test.go`:
- Around line 171-185: Extend the webhook authorizer tests around the existing
unexpected-status case with injected-client scenarios for transport failure,
response-body read failure, and malformed JSON on a 200 response. For each case,
assert that authorization returns no response and that errors.Is recognizes both
ErrAuthorizerInternal and the corresponding underlying sentinel error.
In `@src/libraries/go/lib/pkg/auth/authorizer.go`:
- Around line 175-177: Update the response-body handling in the authorizer
request flow to read through a size-limited reader instead of calling io.ReadAll
directly, and return ErrAuthorizerInternal when the configured maximum is
exceeded. Preserve existing read-error wrapping and status handling for
responses within the limit, and add a test covering an oversized webhook
response.
- Around line 169-171: Instrument the outbound request in the authorizer flow
around w.client.Do with an OTEL client span and W3C Trace Context propagation,
following the existing outbound HTTP patterns in the same authorizer
implementation. Mark transport errors and non-OK HTTP responses on the span
while preserving the current error returns and response handling.
---
Duplicate comments:
In `@src/libraries/go/lib/pkg/auth/authorizer.go`:
- Around line 145-148: Update the authorizer initialization around w.client and
noRedirectPolicy to always install noRedirectPolicy, regardless of any injected
CheckRedirect value, on a private client instance rather than mutating the
caller-owned client. Preserve the injected client’s transport and timeout
settings, and update the custom-client test to verify those settings remain
intact while redirects are blocked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9eb97021-00c4-4913-a33a-870835a28b89
📒 Files selected for processing (2)
src/libraries/go/lib/pkg/auth/authorizer.gosrc/libraries/go/lib/pkg/auth/authorizer_test.go
| 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: testActionRead}) | ||
| require.ErrorIs(t, err, ErrAuthorizerInternal) | ||
| assert.Contains(t, err.Error(), "upstream unavailable") | ||
| assert.Nil(t, res) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Cover the new error-wrapping paths.
Add injected-client tests for transport failure, response-body read failure, and malformed 200 JSON. Assert both ErrAuthorizerInternal and the underlying sentinel via errors.Is.
As per coding guidelines, “Code changes must include tests”; as per path instructions, “Tests are included for behavior changes—especially request/auth-path logic and error handling.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/libraries/go/lib/pkg/auth/authorizer_test.go` around lines 171 - 185,
Extend the webhook authorizer tests around the existing unexpected-status case
with injected-client scenarios for transport failure, response-body read
failure, and malformed JSON on a 200 response. For each case, assert that
authorization returns no response and that errors.Is recognizes both
ErrAuthorizerInternal and the corresponding underlying sentinel error.
Sources: Coding guidelines, Path instructions
| resp, err := w.client.Do(httpReq) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("%w: webhook request failed: %w", ErrAuthorizerInternal, err) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and inspect relevant regions.
git ls-files src/libraries/go/lib/pkg/auth/authorizer.go
wc -l src/libraries/go/lib/pkg/auth/authorizer.go
cat -n src/libraries/go/lib/pkg/auth/authorizer.go | sed -n '1,260p'
# Search for tracing / otel usage in the auth package and nearby code.
rg -n "otel|trace\.|propagat|Inject|Span|Start\(" src/libraries/go/lib/pkg/auth src/libraries/go/lib/pkg -g'*.go'Repository: NVIDIA/nvcf
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the existing instrumented HTTP client pattern in the auth package.
cat -n src/libraries/go/lib/pkg/auth/auth_token_fetcher.go | sed -n '1,180p'
# Also inspect the generic HTTP client helper used elsewhere for tracing conventions.
cat -n src/libraries/go/lib/pkg/nvkit/clients/http_client.go | sed -n '220,260p'Repository: NVIDIA/nvcf
Length of output: 8345
Instrument the webhook authorization request. Add an OTEL client span around w.client.Do, propagate W3C Trace Context, and mark transport/non-OK failures as errors to match the other outbound HTTP paths in src/libraries/go/lib/pkg/auth/authorizer.go:169-191.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/libraries/go/lib/pkg/auth/authorizer.go` around lines 169 - 171,
Instrument the outbound request in the authorizer flow around w.client.Do with
an OTEL client span and W3C Trace Context propagation, following the existing
outbound HTTP patterns in the same authorizer implementation. Mark transport
errors and non-OK HTTP responses on the span while preserving the current error
returns and response handling.
Sources: Coding guidelines, Path instructions
| respBody, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("%w: failed to read webhook response body: %w", ErrAuthorizerInternal, err) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound webhook response-body reads.
io.ReadAll lets a faulty or hostile webhook consume unbounded memory before status handling. Read through a size-limited reader and return ErrAuthorizerInternal when the limit is exceeded; add an oversized-response test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/libraries/go/lib/pkg/auth/authorizer.go` around lines 175 - 177, Update
the response-body handling in the authorizer request flow to read through a
size-limited reader instead of calling io.ReadAll directly, and return
ErrAuthorizerInternal when the configured maximum is exceeded. Preserve existing
read-error wrapping and status handling for responses within the limit, and add
a test covering an oversized webhook response.
…izer 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: NVIDIA#180 Signed-off-by: coffeecoder08 <razawarsi828@gmail.com>
shelleyshen-0
left a comment
shelleyshen-0
left a comment
There was a problem hiding this comment.
Approved. Thanks for your contribution.
|
@raza-khan0108 can you please rebase and resolve or address code rabbit suggestions, then should be good to merge |
3 participants
TL;DR
Add a public Go
Authorizerinterface, structured data models (AuthRequest,AuthResult,Action), and open-source implementations (NoopAuthorizer,WebhookAuthorizer) insidesrc/libraries/go/lib/pkg/auth. This decouples open-source builds such as Event Ledger from proprietary internal authorization services.Additional Details
Authorizerinterface with anAuthorize(ctx context.Context, req *AuthRequest) (*AuthResult, error)contract, aligned with established pluggable patterns innats-auth-calloutandllm-api-gateway.NoopAuthorizerfor local development, standalone testing, and single-tenant deployments (Allowed: truefor all requests).WebhookAuthorizerto delegate access evaluation via HTTP POST callouts (application/json) to external endpoints, enabling integration with enterprise identity gateways or Open Policy Agent (OPA) sidecars.Authorizerinterface (pkg/auth.Authorizer) instead of directly importing proprietary client libraries.For the Reviewer
Please review the interface definition and error handling in:
src/libraries/go/lib/pkg/auth/authorizer.gosrc/libraries/go/lib/pkg/auth/authorizer_test.gosrc/libraries/go/lib/pkg/auth/BUILD.bazelVerify that the
AuthRequestandAuthResultstructs provide sufficient context for downstream control plane and compute plane services adopting open-source authorization.For QA
NoopAuthorizerallows valid requests and rejectsnilrequests withErrUnauthorized.WebhookAuthorizerusinghttptest.Serveragainst200 OKallowed,200 OKdenied,401 Unauthorized,403 Forbidden, and500 Internal Server Errorscenarios.srcsinsrc/libraries/go/lib/pkg/auth/BUILD.bazel.Is QA Needed? No manual QA needed until integrated into downstream service charts.
Issues
Fixes #180
Checklist
Summary by CodeRabbit