Skip to content

fix: authenticate pool lifecycle requests to Gateway API - #1207

Open
benben wants to merge 1 commit into
mainfrom
ben/fix-pool-gateway-basic-auth
Open

benben wants to merge 1 commit into
mainfrom
ben/fix-pool-gateway-basic-auth

Conversation

@benben

@benben benben commented Sep 20, 2026

Copy link
Copy Markdown
Member

Summary

A form-authenticated Gateway requires an API-role identity before checking the pool administration capability. The pool client previously sent an opaque token as Bearer authentication, which the outer form-session authenticator rejected with HTTP 403.

  • Add optional APIUsername and wire it from existing DUCKGRES_TRINO_MANAGED_GATEWAY_USERNAME.
  • Send Basic authentication with the existing token as password and retain X-Gateway-Transaction-Admin-Token for the separate capability check.
  • Preserve token-only Bearer behavior when no API username is configured. No new secrets or Gateway authorization exceptions.

Validation

  • Red/green HTTP regression reproduces 403 before the fix and passes with both authentication layers.
  • Covers missing/wrong Basic credentials, missing/wrong capability, pool configure, backend list/add/delete, invalid usernames, and credential-safe errors.
  • Verified HTTPS-to-HTTP redirects are refused without disabling TLS verification.
  • Gateway client tests, focused startup/pool wiring tests, and just lint pass.
  • Independent adversarial review completed. Live pool acceptance awaits the published image and configuration deployment.

Rollout

Merge this source PR first and let its image publish. Then merge charts #15954, which supplies the existing API username to the pool operator. This PR does not claim real-cluster acceptance has passed.

@benben
benben requested a review from a team September 20, 2026 20:15
@github-actions

Copy link
Copy Markdown

Test Impact Plan

Deterministic summary of how this PR changes tests, CI runners, and coverage-risk signals.

Summary

Area Added Changed Deleted
Test files 1 2 0
E2E/journey files 0 0 0
Workflow files 0 0 0

Signals

  • Test cases: +4 / -0
  • Assertions: +11 / -0
  • Skips or known failures added: 0
  • Workflow continue-on-error added: 0
  • Workflow path filters added: 0
  • Test commands removed from justfile: 0
  • E2E/journey retry lines added: 0

Coverage risk: neutral or increased

No coverage-reduction warnings detected.

@bill-ph bill-ph left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed 49fe6d8. No actionable findings or P0 blocker identified.

The shared request path now uses Basic authentication when an API username is configured while retaining the separate capability header. An unset username preserves the existing Bearer behavior. Username validation rejects ambiguous Basic-auth separators and control characters; the default transport still requires HTTPS, verifies TLS, and refuses redirects. The new tests exercise both authentication layers, backend operations, invalid usernames, and redirect refusal.

Overengineering/scope-creep check: the optional username reuses existing configuration and credentials without adding an authorization exception or a second client implementation. Tests and documentation are proportional to the change.

Validation: static review of the full diff and surrounding client construction/request handling; no local tests or live Gateway acceptance run. Coordinate deployment with charts #15954 and verify active-pool acceptance after the new image and username configuration are deployed.

— Robo Bill

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