Skip to content

THREESCALE-14969: Allow authentication by zync token#4304

Draft
jlledom wants to merge 4 commits into
masterfrom
THREESCALE-14969-auth-by-zync-token
Draft

THREESCALE-14969: Allow authentication by zync token#4304
jlledom wants to merge 4 commits into
masterfrom
THREESCALE-14969-auth-by-zync-token

Conversation

@jlledom
Copy link
Copy Markdown
Contributor

@jlledom jlledom commented May 21, 2026

What this PR does / why we need it:

The issue description explains the situation, but summarizing, #4236 the integration with zync. Porta calls PUT /tenant endpoint on zync and pushes the access token to Zync, which will use that access token later to pull data from porta. After #4236, now the access token is sent hashed to zync, which is useless as authentication method, so zync can't retrieve anymore data from porta.

In order to fix it, the approach is to take advantage from the fact that Zync already sends the X-Zync-Token header with an auth token in all requests to porta. This PR adds a new ByZyncToken authentication method for API endpoints, which grants access to the exact few resources Zync needs if they are requested from a request that includes the Zync token, even when they don't include a valid access token.

This is the simpler solution and doesn't require making any changes in Zync itself.However, this is arguably not good news from the security perspective, but the threat vector is not much bigger now than before. This is Claude's description about how this PR affects security:

 Attack surface with a leaked X-Zync-Token
  
  Read access on 3 endpoints, any tenant:
  - GET /admin/api/provider.json — provider account details
  - GET /admin/api/services/:id/proxy.json — proxy config, backend URLs, OIDC client secrets
  - GET /admin/api/applications/find.json — application credentials (user keys, app IDs)
  
  Cross-tenant: The token is a shared secret. By setting the Host or X-Forwarded-Host header, an attacker can target any provider, not just one.

  Soft-deleted providers: ProviderDomainConstraint has always used zync? to allow routing to soft-deleted provider domains, because Zync needs to reach them during OIDC cleanup. This was already the case before this PR — a
  leaked Zync token already bypassed the "deleted provider" routing gate. Our PR does not introduce or worsen this behavior.

  Hard limits (unchanged by this PR):
  - Master account is explicitly rejected in authenticate_zync_request
  - Only show/find actions — write actions (update, destroy) are excluded
  - Read-only DB transaction enforced as defense in depth even if the action filter is bypassed
  - Only 3 controllers include ByZyncToken — all other Admin API endpoints are inaccessible

  ---
  How this PR changes the threat model
  
  Before this PR, exploiting a leaked Zync token required two secrets:
  1. X-Zync-Token — for routing (including soft-deleted providers)
  2. A valid oidc_sync access token for the target provider — for Admin API authentication
  
  After this PR, only X-Zync-Token is needed.

  However, those two secrets lived in the same trust boundary: the Zync token travels in every HTTP header, and oidc_sync tokens were stored in plaintext in Zync's database. An attacker with access to Zync's environment could
  extract both trivially. In practice, having one did not meaningfully prevent having the other.

Additionally, it's worth to mention that Zync stores a copy of alls OIDC access tokens in plain text in its database. After #4236 all those tokens are going to be gradually replaced by invalid values, but in the meantime we need to immediately invalidate all those tokens, they are a risk and we don't need them now we auth via zync token.

The PR is easy to review commit by commit. Also AI-generated commit messages can be useful as well. I'll add some comments to the PR for further explanation.

Which issue(s) this PR fixes

https://redhat.atlassian.net/browse/THREESCALE-14969

Verification steps

For the 3 affected endpoints:

  • GET /admin/api/provider.json
  • GET /admin/api/services/:id/proxy.json
  • GET /admin/api/applications/find.json

You should be able to:

  • You can call them with the X-Zync-Token header and they should work
  • You can add the X-Forwarded-Host header to target a different endpoint than the one receiving the request.
  • You can't send PUT, POST, or DELETE requests to them without an access token
  • You can't target the master portal in any way
  • No other endpoint accepts X-Zync-Token header as authentication method

jlledom added 3 commits May 21, 2026 12:32
Introduces a new authentication mechanism for Zync service requests
using the X-Zync-Token shared secret header instead of per-provider
oidc_sync access tokens. This fixes the Zync integration broken by
access token hashing (SHA384) in PR #4236, where hashed token values
became unusable for authentication.

The module authenticates Zync requests on read-only actions (show,
find) and enforces read-only database transactions as defense in depth.
It explicitly rejects requests targeting the master account domain to
prevent unauthorized master API access via the shared Zync token.

Authentication resolves to the provider's impersonation admin or
first admin, determined by the domain in the Host header (or
X-Forwarded-Host in proxy mode). For non-Zync requests, it delegates
to ByAccessToken for regular access token authentication.

Assisted-by: Claude Code
Enables Zync authentication on the three Admin API endpoints that Zync
calls to fetch provider configuration: provider details, service proxy
configuration, and application lookups.

These are the only endpoints where Zync needs read access. Including
ByZyncToken only in these controllers (not in BaseController) limits
the attack surface — even with a valid X-Zync-Token, an attacker cannot
access billing, analytics, user management, or other Admin API
endpoints.

Assisted-by: Claude Code
Removes the oidc_sync access token mechanism now that Zync
authenticates via X-Zync-Token instead. This change:

1. Rejects oidc_sync tokens at the authentication layer, invalidating
   all existing oidc_sync tokens immediately (including plaintext
   copies sitting in Zync databases from before the hashing migration).

2. Removes AccessToken.oidc_sync class method — new oidc_sync tokens
   can no longer be created. The OIDC_SYNC_TOKEN constant remains for
   the rejection logic.

3. Simplifies ZyncWorker.provider_access_token to return a placeholder
   string 'zync' instead of fetching a per-provider token. Zync no
   longer uses this for authentication, but the field is sent to satisfy
   Zync's Tenant model validation (validates :access_token, presence:
   true).

This eliminates the security issue of storing plaintext tokens in
Zync's database and removes the mechanism that broke when access tokens
were hashed in PR #4236.

Assisted-by: Claude Code
@jlledom jlledom self-assigned this May 21, 2026
token = domain_account.access_tokens.find_from_value(given_token)

return if token.blank? || token.expired?
return if token.blank? || token.expired? || token.name == AccessToken::OIDC_SYNC_TOKEN
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This line invalidates all OIDC access tokens from now on, so it doesn't matter if they still sit plain text in the Zync DB.

extend ActiveSupport::Concern

included do
prepend_before_action :authenticate_zync_request, only: %i[show find]
Copy link
Copy Markdown
Contributor Author

@jlledom jlledom May 21, 2026

Choose a reason for hiding this comment

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

The zync authenticated actions, in particular, setting current_user to the impersonation admin or the first admin, are only enforced when @zync_authenticated == true, and that ivar can only be set to true in show and find actions.

Comment on lines -148 to -151
def self.oidc_sync
create_with(scopes: %w[account_management], permission: 'ro').find_or_create_by!(name: OIDC_SYNC_TOKEN)
end

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We stop creating new OIDC tokens

Comment on lines +175 to +176
def self.provider_access_token(_provider)
'zync'
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not super fan of this, but the PUT /tenant endpoint at Zync requires this parameter, we must send anything, so we send this dumb value.

Adds unit and integration tests covering the new ByZyncToken
authentication module and the oidc_sync token deprecation:

Unit tests (by_zync_token_test.rb):
- authenticate_zync_request flag setting and rejection (invalid token,
  master domain)
- current_user resolution (impersonation admin preference, fallback to
  first_admin!, delegation to super for non-Zync requests)
- read-only transaction enforcement for Zync-authenticated requests

Integration tests (by_zync_token_test.rb):
- Regular access token auth still works on Zync-capable endpoints
- oidc_sync tokens rejected on Zync-capable endpoints
- X-Zync-Token authentication on read actions (show, find)
- Rejection scenarios (invalid token, master domain, write actions)
- Read-only database transaction enforcement (write attempts fail)
- Domain routing via Host and X-Forwarded-Host headers

Additional test coverage:
- ByAccessToken: oidc_sync token rejection at authentication layer
- AccessToken model: oidc_sync method removal, constant preservation
- ZyncWorker: provider_access_token returns non-empty placeholder

All tests passing: 17 unit, 11 integration, plus existing coverage in
related test files.

Assisted-by: Claude Code
@jlledom jlledom force-pushed the THREESCALE-14969-auth-by-zync-token branch from 6239c11 to f9a1c77 Compare May 21, 2026 15:14
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.

1 participant