Skip to content

HYPERFLEET-492 - refactor: replace OCM auth client with JWT-based auth#120

Open
kuudori wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
kuudori:HYPERFLEET-492
Open

HYPERFLEET-492 - refactor: replace OCM auth client with JWT-based auth#120
kuudori wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
kuudori:HYPERFLEET-492

Conversation

@kuudori
Copy link
Copy Markdown
Contributor

@kuudori kuudori commented Apr 30, 2026

Summary

  • HYPERFLEET-492

Test Plan

  • Unit tests added/updated
  • make test-all passes
  • make lint passes
  • Helm chart changes validated with make test-helm (if applicable)
  • Deployed to a development cluster and verified (if Helm/config changes)
  • E2E tests passed (if cross-component or major changes)

Summary by CodeRabbit

  • New Features

    • Standalone JWT authentication (RS256, JWKS rotation) with issuer/audience options and new server JWT CLI flags.
  • Removed

    • OCM SDK/CLI integration, related runtime flags/env vars, clients, and test mocks.
  • Documentation

    • All docs and examples switched to JWT flow; OCM configuration/instructions removed.
  • Tests

    • Comprehensive JWT handler tests added to validate auth, audiences, public paths, and failure modes.
  • Chores

    • Build/secrets make targets adjusted (OCM secret files no longer created).

@openshift-ci openshift-ci Bot requested review from Mischulee and vkareh April 30, 2026 18:43
@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Apr 30, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign tirthct for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 30, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: 787f213d-3a66-44ed-b690-96bc3b5b4caf

📥 Commits

Reviewing files that changed from the base of the PR and between 30e2172 and 79b8af6.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (35)
  • CHANGELOG.md
  • Makefile
  • PREREQUISITES.md
  • cmd/hyperfleet-api/environments/e_development.go
  • cmd/hyperfleet-api/environments/e_integration_testing.go
  • cmd/hyperfleet-api/environments/e_production.go
  • cmd/hyperfleet-api/environments/e_unit_testing.go
  • cmd/hyperfleet-api/environments/framework.go
  • cmd/hyperfleet-api/environments/framework_test.go
  • cmd/hyperfleet-api/environments/types.go
  • cmd/hyperfleet-api/server/api_server.go
  • configs/config.yaml.example
  • docs/authentication.md
  • docs/config.md
  • docs/development.md
  • docs/logging.md
  • go.mod
  • pkg/auth/authz_middleware.go
  • pkg/auth/context.go
  • pkg/auth/jwt_handler.go
  • pkg/auth/jwt_handler_test.go
  • pkg/client/ocm/authorization.go
  • pkg/client/ocm/authorization_mock.go
  • pkg/client/ocm/client.go
  • pkg/config/config.go
  • pkg/config/dump.go
  • pkg/config/flags.go
  • pkg/config/helpers_test.go
  • pkg/config/loader.go
  • pkg/config/loader_test.go
  • pkg/config/ocm.go
  • pkg/config/server.go
  • pkg/logger/ocm_bridge.go
  • test/helper.go
  • test/mocks/ocm.go
💤 Files with no reviewable changes (16)
  • cmd/hyperfleet-api/environments/framework_test.go
  • pkg/logger/ocm_bridge.go
  • PREREQUISITES.md
  • test/mocks/ocm.go
  • pkg/config/loader_test.go
  • pkg/config/config.go
  • cmd/hyperfleet-api/environments/e_integration_testing.go
  • pkg/config/ocm.go
  • configs/config.yaml.example
  • pkg/client/ocm/authorization_mock.go
  • cmd/hyperfleet-api/environments/e_development.go
  • cmd/hyperfleet-api/environments/e_unit_testing.go
  • pkg/config/helpers_test.go
  • pkg/client/ocm/client.go
  • pkg/auth/authz_middleware.go
  • pkg/client/ocm/authorization.go
✅ Files skipped from review due to trivial changes (4)
  • docs/logging.md
  • pkg/config/dump.go
  • docs/config.md
  • pkg/config/flags.go
🚧 Files skipped from review as they are similar to previous changes (6)
  • pkg/config/server.go
  • cmd/hyperfleet-api/server/api_server.go
  • pkg/auth/jwt_handler.go
  • cmd/hyperfleet-api/environments/types.go
  • docs/authentication.md
  • pkg/auth/jwt_handler_test.go

Walkthrough

This PR removes OCM/ocm-sdk-go integration and related clients/mocks/logger bridge/flags, and replaces the OCM-based authentication with a new JWKS-backed JWT middleware (RS256, issuer/audience validation, JWKS rotation) using golang-jwt v5 and keyfunc/jwkset. It updates config (new server.jwt fields, flags, loader validation), test helpers to use a local TestAccount and JWTs, adjusts environment/client initialization to stop creating OCM clients, and updates docs and CHANGELOG accordingly.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant JWTHandler as JWT Handler
    participant Parser as JWT Parser
    participant JWKS as JWKS Provider (file/HTTP)
    participant MainHandler

    Client->>JWTHandler: HTTP Request (Authorization: Bearer <token>)
    alt Public Path (regex match)
        JWTHandler->>MainHandler: Forward Request
        MainHandler->>Client: Response
    else Protected Path
        JWTHandler->>JWTHandler: Check Authorization header format
        alt Missing/Malformed
            JWTHandler->>Client: 401 Unauthorized
        else Well-formed Bearer
            JWTHandler->>Parser: Parse & validate token (RS256, exp, issuer, audience)
            Parser->>JWKS: Resolve key by kid (read file or fetch/refresh from URL)
            alt Key resolution or parse error
                Parser->>JWTHandler: Validation Error
                JWTHandler->>Client: 401 Unauthorized
            else Validation success
                JWTHandler->>JWTHandler: Store *jwt.Token in request context
                JWTHandler->>MainHandler: Forward Request (with token context)
                MainHandler->>Client: Response
            end
        end
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 31.82% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and accurately summarizes the main change: replacing OCM authentication client with JWT-based authentication across the entire codebase.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Review rate limit: 9/10 reviews remaining, refill in 6 minutes.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
docs/logging.md (1)

535-538: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Remove stale OCM environment variable references.

The test examples still reference OCM_ENV=integration_testing, but the OCM integration has been removed from the codebase per this PR's objectives. These environment variable references should be removed to align with the JWT-based authentication changes and prevent user confusion.

📝 Proposed fix to remove OCM_ENV references
 # Run tests with debug logging
-HYPERFLEET_LOGGING_LEVEL=debug OCM_ENV=integration_testing go test ./test/integration/...
+HYPERFLEET_LOGGING_LEVEL=debug go test ./test/integration/...
 
 # Run tests without OTel
-HYPERFLEET_TRACING_ENABLED=false OCM_ENV=integration_testing go test ./...
+HYPERFLEET_TRACING_ENABLED=false go test ./...
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/logging.md` around lines 535 - 538, The doc snippets still export the
removed environment variable OCM_ENV (e.g., "OCM_ENV=integration_testing") in
the test command examples; remove all occurrences of the
OCM_ENV=integration_testing token from the examples in docs/logging.md (and any
other similar command examples) so the lines read only the remaining env vars
(e.g., HYPERFLEET_LOGGING_LEVEL=debug go test ... and
HYPERFLEET_TRACING_ENABLED=false go test ...); search for the symbol OCM_ENV in
the file and delete those assignments to avoid referencing the removed OCM
integration.
🧹 Nitpick comments (1)
test/helper.go (1)

577-589: ⚡ Quick win

Include audience claim in helper JWTs when configured.

Line 579 mirrors configured issuer, but helper tokens ignore configured audience. When server.jwt.audience is set, these tokens won’t match middleware expectations.

Proposed update
 func (helper *Helper) CreateJWTString(account *TestAccount) string {
 	claims := jwt.MapClaims{
 		"iss":        helper.Env().Config.Server.JWT.IssuerURL,
 		"username":   strings.ToLower(account.Username),
 		"first_name": account.FirstName,
 		"last_name":  account.LastName,
 		"typ":        "Bearer",
 		"iat":        time.Now().Unix(),
 		"exp":        time.Now().Add(1 * time.Hour).Unix(),
 	}
+	if aud := helper.Env().Config.Server.JWT.Audience; aud != "" {
+		claims["aud"] = aud
+	}
 	if account.Email != "" {
 		claims["email"] = account.Email
 	}

As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/helper.go` around lines 577 - 589, The CreateJWTString helper currently
sets "iss" but omits the configured audience; update Helper.CreateJWTString to
read the configured audience (helper.Env().Config.Server.JWT.Audience) and, when
non-empty, set claims["aud"] = that value so generated test JWTs include the
expected audience claim and will match middleware validation; locate this change
inside the CreateJWTString method where claims are built and add the conditional
audience assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@CHANGELOG.md`:
- Around line 12-13: Update the placeholder PR links in CHANGELOG.md that point
to "/pull/TBD" (e.g., the JWT auth entry and the Hard deletion entry and the
other occurrences at lines referenced) by replacing "TBD" with the actual PR
numbers for those changes; search for the "/pull/TBD" pattern in CHANGELOG.md
and substitute each with the correct numeric PR identifier so the links resolve
correctly and repeat for the occurrences noted (lines ~32-33 and ~45).

In `@docs/development.md`:
- Around line 121-125: Split the single blocking code block into two separate
terminal examples so readers can run the server and then call the API: create a
"Terminal 1" code block containing only the blocking command `make run` to start
the server, and create a "Terminal 2" code block containing the `curl -H
"Authorization: Bearer ${TOKEN}"
http://localhost:8000/api/hyperfleet/v1/clusters` command; ensure the text
labels clarify that Terminal 1 must be running before executing Terminal 2.

In `@pkg/config/flags.go`:
- Around line 32-33: Add documentation entries for the two new CLI flags added
via cmd.Flags().String for "--server-jwt-issuer-url" and "--server-jwt-audience"
to the configuration reference (docs/config.md): describe each flag, show the
default values (defaults.JWT.IssuerURL and defaults.JWT.Audience), indicate that
issuer URL is used for token validation and audience is optional, and add them
to the CLI flags table so operators can discover and configure server JWT
validation.

---

Outside diff comments:
In `@docs/logging.md`:
- Around line 535-538: The doc snippets still export the removed environment
variable OCM_ENV (e.g., "OCM_ENV=integration_testing") in the test command
examples; remove all occurrences of the OCM_ENV=integration_testing token from
the examples in docs/logging.md (and any other similar command examples) so the
lines read only the remaining env vars (e.g., HYPERFLEET_LOGGING_LEVEL=debug go
test ... and HYPERFLEET_TRACING_ENABLED=false go test ...); search for the
symbol OCM_ENV in the file and delete those assignments to avoid referencing the
removed OCM integration.

---

Nitpick comments:
In `@test/helper.go`:
- Around line 577-589: The CreateJWTString helper currently sets "iss" but omits
the configured audience; update Helper.CreateJWTString to read the configured
audience (helper.Env().Config.Server.JWT.Audience) and, when non-empty, set
claims["aud"] = that value so generated test JWTs include the expected audience
claim and will match middleware validation; locate this change inside the
CreateJWTString method where claims are built and add the conditional audience
assignment.
🪄 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: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: a150046b-e9c4-44a3-97cd-a4d7d090571c

📥 Commits

Reviewing files that changed from the base of the PR and between f0ca89d and bc2e1ba.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (35)
  • CHANGELOG.md
  • Makefile
  • PREREQUISITES.md
  • cmd/hyperfleet-api/environments/e_development.go
  • cmd/hyperfleet-api/environments/e_integration_testing.go
  • cmd/hyperfleet-api/environments/e_production.go
  • cmd/hyperfleet-api/environments/e_unit_testing.go
  • cmd/hyperfleet-api/environments/framework.go
  • cmd/hyperfleet-api/environments/framework_test.go
  • cmd/hyperfleet-api/environments/types.go
  • cmd/hyperfleet-api/server/api_server.go
  • configs/config.yaml.example
  • docs/authentication.md
  • docs/config.md
  • docs/development.md
  • docs/logging.md
  • go.mod
  • pkg/auth/authz_middleware.go
  • pkg/auth/context.go
  • pkg/auth/jwt_handler.go
  • pkg/auth/jwt_handler_test.go
  • pkg/client/ocm/authorization.go
  • pkg/client/ocm/authorization_mock.go
  • pkg/client/ocm/client.go
  • pkg/config/config.go
  • pkg/config/dump.go
  • pkg/config/flags.go
  • pkg/config/helpers_test.go
  • pkg/config/loader.go
  • pkg/config/loader_test.go
  • pkg/config/ocm.go
  • pkg/config/server.go
  • pkg/logger/ocm_bridge.go
  • test/helper.go
  • test/mocks/ocm.go
💤 Files with no reviewable changes (16)
  • pkg/config/helpers_test.go
  • PREREQUISITES.md
  • configs/config.yaml.example
  • pkg/client/ocm/authorization_mock.go
  • pkg/auth/authz_middleware.go
  • pkg/logger/ocm_bridge.go
  • cmd/hyperfleet-api/environments/e_integration_testing.go
  • pkg/config/ocm.go
  • cmd/hyperfleet-api/environments/e_development.go
  • cmd/hyperfleet-api/environments/e_unit_testing.go
  • cmd/hyperfleet-api/environments/framework_test.go
  • pkg/config/config.go
  • pkg/config/loader_test.go
  • pkg/client/ocm/client.go
  • pkg/client/ocm/authorization.go
  • test/mocks/ocm.go

Comment thread CHANGELOG.md Outdated
Comment thread docs/development.md
Comment thread pkg/config/flags.go
Comment thread pkg/config/server.go
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/authentication.md`:
- Around line 35-52: The docs still reference JWT_ISSUER / JWT_AUDIENCE and
AUTH_ENABLED but the app loader expects HYPERFLEET_SERVER_JWT_* env vars; update
the Production Mode section and examples to use HYPERFLEET_SERVER_JWT_ENABLED,
HYPERFLEET_SERVER_JWT_ISSUER_URL, and HYPERFLEET_SERVER_JWT_AUDIENCE (and
replace any AUTH_ENABLED mentions with HYPERFLEET_SERVER_JWT_ENABLED), ensure
the example curl/launch commands and the "JWT Authentication" paragraph reflect
these exact env var names and mention RS256 verification remains in use so
operators can configure auth correctly.
🪄 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: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: 98e77616-8752-421c-a705-3ede03ff6493

📥 Commits

Reviewing files that changed from the base of the PR and between bc2e1ba and 30e2172.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (35)
  • CHANGELOG.md
  • Makefile
  • PREREQUISITES.md
  • cmd/hyperfleet-api/environments/e_development.go
  • cmd/hyperfleet-api/environments/e_integration_testing.go
  • cmd/hyperfleet-api/environments/e_production.go
  • cmd/hyperfleet-api/environments/e_unit_testing.go
  • cmd/hyperfleet-api/environments/framework.go
  • cmd/hyperfleet-api/environments/framework_test.go
  • cmd/hyperfleet-api/environments/types.go
  • cmd/hyperfleet-api/server/api_server.go
  • configs/config.yaml.example
  • docs/authentication.md
  • docs/config.md
  • docs/development.md
  • docs/logging.md
  • go.mod
  • pkg/auth/authz_middleware.go
  • pkg/auth/context.go
  • pkg/auth/jwt_handler.go
  • pkg/auth/jwt_handler_test.go
  • pkg/client/ocm/authorization.go
  • pkg/client/ocm/authorization_mock.go
  • pkg/client/ocm/client.go
  • pkg/config/config.go
  • pkg/config/dump.go
  • pkg/config/flags.go
  • pkg/config/helpers_test.go
  • pkg/config/loader.go
  • pkg/config/loader_test.go
  • pkg/config/ocm.go
  • pkg/config/server.go
  • pkg/logger/ocm_bridge.go
  • test/helper.go
  • test/mocks/ocm.go
💤 Files with no reviewable changes (16)
  • cmd/hyperfleet-api/environments/e_unit_testing.go
  • cmd/hyperfleet-api/environments/e_integration_testing.go
  • pkg/client/ocm/authorization_mock.go
  • cmd/hyperfleet-api/environments/e_development.go
  • PREREQUISITES.md
  • cmd/hyperfleet-api/environments/framework_test.go
  • pkg/config/loader_test.go
  • pkg/logger/ocm_bridge.go
  • pkg/client/ocm/authorization.go
  • pkg/config/helpers_test.go
  • configs/config.yaml.example
  • pkg/config/ocm.go
  • pkg/client/ocm/client.go
  • test/mocks/ocm.go
  • pkg/auth/authz_middleware.go
  • pkg/config/config.go
✅ Files skipped from review due to trivial changes (2)
  • cmd/hyperfleet-api/environments/e_production.go
  • docs/logging.md
🚧 Files skipped from review as they are similar to previous changes (3)
  • Makefile
  • cmd/hyperfleet-api/environments/framework.go
  • CHANGELOG.md

Comment thread docs/authentication.md
Remove the OCM client dependency and replace authentication with a
self-contained JWT handler using JWKS endpoint validation.

- Add pkg/auth/jwt_handler.go with RS256 JWT validation via JWKS
- Add pkg/auth/jwt_handler_test.go with full handler test coverage
- Remove pkg/client/ocm/ (authorization, authorization_mock, client)
- Remove pkg/config/ocm.go and OCM-specific config flags
- Remove pkg/logger/ocm_bridge.go (OCM SDK log adapter)
- Remove test/mocks/ocm.go (no longer needed)
- Update pkg/auth/authz_middleware.go to use new JWT handler
- Update pkg/auth/context.go to remove OCM identity types
- Update environments to drop OCM env var requirements
- Update go.mod/go.sum: add MicahParks/jwkset, keyfunc; remove ocm-sdk-go
- Update docs/authentication.md, docs/config.md to reflect new auth setup
- Update configs/config.yaml.example with JWT/JWKS config fields
authnLogger := logger.NewOCMLoggerBridge()

// Create the handler that verifies that tokens are valid:
var err error
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Warning

Blocking

Category: Bug

NewJWTHandler receives context.Background() here, but keyfunc.NewDefaultCtx / keyfunc.New inside buildKeyfunc starts a background goroutine for JWKS key refresh. Since context.Background() never cancels, there's no way to stop that goroutine on shutdown.

The old OCM client had e.Clients.OCM.Close() in Teardown() — this PR removes it with no replacement for the new JWKS resource.

Suggested approach:

  1. Pass a cancellable context to NewJWTHandler (e.g., from a server-lifecycle context)
  2. Store the cancel func on jwtHandler and add a Close() method
  3. Call Close() during server shutdown (or from Teardown())
type jwtHandler struct {
    keyfunc  keyfunc.Keyfunc
    parser   *jwt.Parser
    publicPatterns []*regexp.Regexp
    next     http.Handler
    cancel   context.CancelFunc // add this
}

func (h *jwtHandler) Close() {
    if h.cancel != nil {
        h.cancel()
    }
}

And in api_server.go:

ctx, cancel := context.WithCancel(serverCtx) // not context.Background()
mainHandler, err := auth.NewJWTHandler(ctx, auth.JWTHandlerConfig{...})
// store cancel or handler for shutdown

Comment thread pkg/auth/jwt_handler.go
return &jwtHandler{
keyfunc: kf,
parser: jwt.NewParser(parserOpts...),
publicPatterns: publicPatterns,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Warning

Blocking

Category: Bug

If buildKeyfunc succeeds (potentially starting a JWKS refresh goroutine) but a subsequent regexp.Compile for public paths fails, NewJWTHandler returns an error without cancelling the keyfunc's background goroutine. This leaks the goroutine.

Fix: use a cancellable context and cancel on error:

ctx, cancel := context.WithCancel(ctx)
kf, err := buildKeyfunc(ctx, cfg)
if err != nil {
    cancel()
    return nil, fmt.Errorf("failed to build JWKS keyfunc: %w", err)
}

// ... compile regexes ...
for _, pattern := range cfg.PublicPaths {
    re, err := regexp.Compile(pattern)
    if err != nil {
        cancel() // stop the keyfunc goroutine
        return nil, fmt.Errorf("failed to compile public path pattern %q: %w", pattern, err)
    }
    publicPatterns = append(publicPatterns, re)
}

// Store cancel on the handler for later shutdown
return &jwtHandler{
    keyfunc: kf,
    parser:  parser,
    publicPatterns: publicPatterns,
    next:    cfg.Next,
    cancel:  cancel,
}, nil

"github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger"
)

type AuthorizationMiddleware interface {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Warning

Blocking

Category: Security

The authzMiddleware struct and NewAuthzMiddleware constructor (which used OCM's AccessReview) have been completely removed. Only the bare AuthorizationMiddleware interface and a passthrough mock remain.

The authzMiddleware parameter is still threaded through RouteRegistrationFunc, LoadDiscoveredRoutes, and plugin route registrations — but it's always the mock now, permitting all requests.

If server.authz.enabled=true was ever set in production, authorization enforcement is now silently gone. Please verify:

  1. Was server.authz.enabled ever true in any deployed environment?
  2. If so, is there a follow-up planned for JWT-claims-based authorization?

If authz was never enabled, consider removing the dead AuthorizationMiddleware interface and all mock/plumbing for it to avoid confusion.

Comment thread pkg/config/server.go

// ACLConfig holds access control list configuration
// Deprecated: ACLConfig is kept for Helm values.yaml backward compatibility.
// ACL checking was provided by the OCM SDK handler and is no longer functional.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Warning

Blocking

Category: Security

The old OCM handler consumed ACLConfig.File via .ACLFile() for access control enforcement. The new JWT handler has no ACL support — the config value is never read.

The --server-acl-file flag and SERVER_ACL_FILE env var are still registered and accepted without error, but they silently do nothing. If any deployment relied on ACL files for access control, that restriction is now gone with no warning.

Suggested actions:

  1. Verify no production deployment uses ACL files
  2. Either remove the ACLConfig struct, flag, and env var entirely, or add a startup warning when server.acl.file is configured: "ACL file is configured but no longer supported after OCM removal"
  3. Update docs/config.md to mark the ACL config as deprecated/non-functional

Comment thread pkg/auth/jwt_handler.go
if err != nil {
return nil, fmt.Errorf("failed to read JWKS file %q: %w", cfg.KeysFile, err)
}
return keyfunc.NewJWKSetJSON(json.RawMessage(data))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Warning

Blocking

Category: Standards

Three return statements in buildKeyfunc pass external library errors through without wrapping. This violates the HyperFleet error model standard (errors from external calls must be wrapped with context using fmt.Errorf("context: %w", err)).

Notably, line 139 in the same function does wrap correctly: fmt.Errorf("failed to parse JWKS file: %w", err) — so this is also an intra-file inconsistency.

Fix for each:

// Line 128 (file-only path):
kf, err := keyfunc.NewJWKSetJSON(json.RawMessage(data))
if err != nil {
    return nil, fmt.Errorf("failed to parse JWKS file: %w", err)
}
return kf, nil

// Line 132 (URL-only path):
kf, err := keyfunc.NewDefaultCtx(ctx, []string{cfg.KeysURL})
if err != nil {
    return nil, fmt.Errorf("failed to create JWKS client from URL: %w", err)
}
return kf, nil

// Line 154 (combined path):
kf, err := keyfunc.New(keyfunc.Options{...})
if err != nil {
    return nil, fmt.Errorf("failed to create combined JWKS keyfunc: %w", err)
}
return kf, nil

w.Header().Set("Content-Type", "application/json")
fmt.Fprintf(w, `{"keys":[%s]}`, string(jwkBytes))
}))
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Warning

Blocking

Category: Pattern

All tests use the URL-only JWKS path via newJWKSServer(). Two code paths in buildKeyfunc have zero test coverage:

  1. File-only path (KeysFile set, KeysURL empty) — reads JWKS from disk via os.ReadFile + keyfunc.NewJWKSetJSON
  2. Combined path (both set) — creates jwkset.NewHTTPClient with merged storage, the most complex branch

Also, JWTConfig.Validate() has no dedicated tests (JWT disabled → nil, JWT enabled + empty IssuerURL → error).

Suggested test additions:

func TestBuildKeyfunc_FileOnly(t *testing.T) {
    // Write a JWKS JSON file to t.TempDir()
    // Call NewJWTHandler with KeysFile set, KeysURL empty
    // Verify tokens signed with the file's key are accepted
}

func TestBuildKeyfunc_Combined(t *testing.T) {
    // Set up both a JWKS file and a JWKS HTTP server
    // Call NewJWTHandler with both KeysFile and KeysURL
    // Verify tokens signed with either key are accepted
}

func TestJWTConfig_Validate(t *testing.T) {
    // Enabled=false → nil
    // Enabled=true, IssuerURL="" → error
    // Enabled=true, IssuerURL="https://..." → nil
}

RegisterTestingT(t)
token := signToken(t, privateKey, jwt.MapClaims{
"iss": "https://test-issuer.example.com",
"exp": time.Now().Add(-time.Hour).Unix(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Warning

Blocking

Category: Pattern

All 401 tests only check rr.Code but never verify the response body. The distinct error codes (CodeAuthNoCredentials, CodeAuthExpiredToken, CodeAuthInvalidCredentials) and RFC 9457 Problem Details format are untested.

A regression that changes the response body format or returns wrong error codes would not be caught.

Suggestion — at minimum, assert on the error code for each case:

t.Run("expired token returns 401 with expired code", func(t *testing.T) {
    // ... existing setup ...
    rr := serve(handler, "/protected", "Bearer "+token)
    Expect(rr.Code).To(Equal(http.StatusUnauthorized))
    Expect(rr.Header().Get("Content-Type")).To(ContainSubstring("application/problem+json"))
    Expect(rr.Body.String()).To(ContainSubstring("HYPERFLEET-AUTH-EXPIRED"))
})

t.Run("missing header returns 401 with no-credentials code", func(t *testing.T) {
    rr := serve(handler, "/protected", "")
    Expect(rr.Code).To(Equal(http.StatusUnauthorized))
    Expect(rr.Body.String()).To(ContainSubstring("HYPERFLEET-AUTH-NO-CREDENTIALS"))
})

type Clients struct {
OCM *ocm.Client
}
type Clients struct{}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tip

nit — non-blocking suggestion

Category: Architecture

After removing the OCM client, several abstractions are now vestigial:

  • Clients struct is empty — OverrideClients across all environment impls does nothing
  • AuthorizationMiddleware interface has no real implementation (only the passthrough mock). It's still threaded through RouteRegistrationFunc and plugin registrations as dead weight
  • ConfigDefaults struct appears unreferenced

Consider a follow-up PR to clean up this dead plumbing, or track it as tech debt.

@rafabene
Copy link
Copy Markdown
Contributor

rafabene commented May 5, 2026

Tip

nit — non-blocking suggestion

Category: Pattern — cmd/hyperfleet-api/environments/types.go:17

EnvironmentStringKey = "OCM_ENV" is the last OCM reference in the code. Renaming would break existing deployments, so keeping it is reasonable — but a comment explaining the legacy name would help future readers:

// EnvironmentStringKey is the env var used to select the runtime environment.
// Named "OCM_ENV" for backward compatibility with existing deployments.
EnvironmentStringKey = "OCM_ENV"

Or consider a follow-up ticket to rename it with a deprecation period.

@rafabene
Copy link
Copy Markdown
Contributor

rafabene commented May 5, 2026

Tip

nit — non-blocking suggestion

Category: Improvement — docs/config.md + pkg/config/flags.go:37

docs/config.md still lists server.acl.file as a valid config in three places (server config table, env var table, CLI flags table) with no deprecation notice. The --server-acl-file flag is still registered in code.

Since ACL enforcement was removed with the OCM handler, these silently do nothing. Consider:

  • Marking the ACL entries as deprecated in config.md
  • Adding a startup log warning when server.acl.file is set: "ACL file configuration is deprecated and has no effect"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants