Stop accepting a deploy token from the instant it expires

José M. Requena Plensrequested to merge
gitlab-community/gitlab-org/gitlab:jmrp-deploy-token-active-app-clock into master

What does this MR do and why?

Follow-up to !261675 (merged), agreed in review there (note).

The DeployToken.active scope backs every lookup that authenticates a deploy token by its value: Gitlab::Auth::AuthFinders#deploy_token_from_request, deploy_token_check in lib/gitlab/auth.rb, Gitlab::APIAuthentication::TokenResolver, DependencyProxy::AuthTokenService, and the packages manager clients and Conan helpers. It compared expires_at with the database's clock and kept a token whose expires_at is that instant:

scope :active, -> { where("revoked = false AND expires_at >= NOW()") }

Expirable, which DeployToken includes, answers the same question with the application's clock and treats that instant as expired: Expirable#expired? is expires_at <= Time.current, and its expired and not_expired scopes draw the line at the same place. So the lookup can find a token that Expirable#expired? reports as expired, in two ways:

  1. At the instant in expires_at: >= against expired?'s <=.
  2. On two clocks: when the application host's clock is ahead of the database host's, a token whose expires_at falls in that difference is found and then reported as expired; when it is behind, the lookup stops finding a token that expired? does not report as expired yet.

This MR builds the scope from Expirable.not_expired, keeping the revoked = false condition as it was:

scope :active, -> { where(revoked: false).not_expired }

The lookup then reads the clock expired? reads and stops at the same instant, which closes both. not_expired also matches expires_at IS NULL, which selects nothing here: deploy_tokens.expires_at is NOT NULL, and a token that never expires stores Forever.date. PersonalAccessToken.active is built the same way (not_revoked.not_expired).

The spec moves the application's clock a day ahead of the database's and a day behind it with travel_to, which does not reach the database's NOW(), and checks a token that expired a second ago, one that expires at that instant and one that expires a second later, next to a revoked token and one that never expires. Both examples fail on master, and each fails as well against the issue's other option, expires_at > NOW(), and against the application's clock with >=.

How this fits with !261675 (merged)

Each stands alone, and either can be merged first. On master, DeployToken still overrides expired? with a private method that compares expires_at with Date.today, and !261675 (merged) removes that override so Expirable#expired? applies. This MR changes only the scope and its spec, touches no line !261675 (merged) changes, and the two branches merge without a conflict. Once both are merged, the scope and DeployToken#expired? read the same clock and draw the line at the same instant, so a token the lookup finds is one active? accepts. Neither closes the interval between the lookup and the checks after it inside one request.

Risks

The scope backs every lookup listed above, so this moves the moment at which a deploy token stops authenticating, for every token and every path: by one instant, and by the difference between the application's and the database's clocks. The deploy token lists of the project and group repository settings, the active filter of the deploy tokens API (through DeployTokens::TokensFinder), DeployToken.gitlab_deploy_token (which sets CI_DEPLOY_USER and CI_DEPLOY_PASSWORD) and DeployTokens::ExpiringWorker, which picks the project deploy tokens it warns about before they expire, use the scope too, and move the same way. The expired field of the deploy tokens API reads expired?, not the scope, so it does not change.

The queries keep their shape and their indexes. The authentication lookups find the row by token_encrypted, which has a unique index, and filter it on revoked and expires_at as before. DeployTokens::ExpiringWorker still reads its interval's partial index on (expires_at, id) over the interval's range; because of the IS NULL arm, the expiry condition filters that range instead of narrowing it, so the seven-day scan also reads the tokens that expired earlier the same day (the longer intervals start days ahead, where the condition never narrowed anything). Only the expiry predicate changes, so I don't think this needs a database review; the SQL is below.

SQL before and after

DeployToken.active.find_by_token(token), as the authentication lookups call it:

-- master
SELECT "deploy_tokens".* FROM "deploy_tokens"
WHERE (revoked = false AND expires_at >= NOW())
  AND "deploy_tokens"."token_encrypted" IN ('...', '...')
LIMIT 1

-- this MR
SELECT "deploy_tokens".* FROM "deploy_tokens"
WHERE "deploy_tokens"."revoked" = FALSE
  AND ("deploy_tokens"."expires_at" > '2026-10-17 15:10:20.982414' OR "deploy_tokens"."expires_at" IS NULL)
  AND "deploy_tokens"."token_encrypted" IN ('...', '...')
LIMIT 1

project.deploy_tokens.active, as the repository settings page lists them:

-- master
SELECT "deploy_tokens".* FROM "deploy_tokens"
INNER JOIN "project_deploy_tokens" ON "deploy_tokens"."id" = "project_deploy_tokens"."deploy_token_id"
WHERE "project_deploy_tokens"."project_id" = 1
  AND (revoked = false AND expires_at >= NOW())

-- this MR
SELECT "deploy_tokens".* FROM "deploy_tokens"
INNER JOIN "project_deploy_tokens" ON "deploy_tokens"."id" = "project_deploy_tokens"."deploy_token_id"
WHERE "project_deploy_tokens"."project_id" = 1
  AND "deploy_tokens"."revoked" = FALSE
  AND ("deploy_tokens"."expires_at" > '2026-10-17 15:10:21.097750' OR "deploy_tokens"."expires_at" IS NULL)

DeployToken.gitlab_deploy_token gains the same predicate in place of (revoked = false AND expires_at >= NOW()), next to "deploy_tokens"."name" = 'gitlab-deploy-token'.

DeployTokens::ExpiringWorker's query, and each keyset batch built from it, gains it in the same place. EXPLAIN of its seven-day query on an empty table, with sequential scans off, shows the index it can use and how:

-- master
Index Scan using index_dts_on_expiring_at_seven_days_notification_sent_at on deploy_tokens
  Index Cond: ((expires_at >= '2026-10-17 00:00:00+00'::timestamp with time zone) AND (expires_at <= '2026-10-24 00:00:00+00'::timestamp with time zone) AND (expires_at >= now()))
  Filter: (deploy_token_type = 2)

-- this MR
Index Scan using index_dts_on_expiring_at_seven_days_notification_sent_at on deploy_tokens
  Index Cond: ((expires_at >= '2026-10-17 00:00:00+00'::timestamp with time zone) AND (expires_at <= '2026-10-24 00:00:00+00'::timestamp with time zone))
  Filter: (((expires_at > '2026-10-17 18:13:05.678988+00'::timestamp with time zone) OR (expires_at IS NULL)) AND (deploy_token_type = 2))

References

Closes #633608 (closed)

Related to !261675 (merged)

Screenshots or screen recordings

Not a UI change. For a token at the instant in its expires_at by the application's clock, while the database's clock is behind it:

master This MR
DeployToken.expired includes it includes it
DeployToken.active includes it does not include it
Specs I ran

All on top of master at 01c4de0ecc6, in a local test environment, with bundle exec rspec.

  • The two new examples:
    • with master's app/models/deploy_token.rb: 2 failures. A day ahead, the scope finds the token that expired a second ago and the one that expires at that instant; a day behind, it misses the one that expires a second later.
    • with this MR: 0 failures.
    • with where("revoked = false AND expires_at > NOW()"), the issue's other option, and with where(revoked: false).where(arel_table[:expires_at].gteq(Time.current)): 2 failures each.
  • spec/models/deploy_token_spec.rb and spec/models/concerns/expirable_spec.rb: 108 examples, 0 failures.
  • A throwaway spec, not part of this MR, that creates tokens a second before, at and a second after the instant, one that never expires and a revoked one, with the application's clock a day ahead, a day behind and unmoved, and checks that DeployToken.active finds exactly the tokens active? accepts:
    • this MR alone: fails in all three, because master's private expired? compares with Date.today.
    • !261675 (merged) alone: fails a day ahead and a day behind, because the scope still reads NOW().
    • both together: passes, along with the model spec, the specs !261675 (merged) adds and changes, and the Expirable spec: 132 examples, 0 failures.
  • The specs of the scope's callers, and the specs that both create deploy tokens and move the clock (one feature spec aside): deploy_token_spec, expirable_spec, deploy_tokens/tokens_finder_spec, api/deploy_tokens_spec, auth/auth_finders_spec, auth_spec, api_authentication/token_resolver_spec, auth/request_authenticator_spec, dependency_proxy/auth_token_service_spec, packages_manager_clients_helpers_spec, conan_token_spec, lfs_token_spec, project_auto_devops_spec, deploy_tokens/expiring_worker_spec, both audit_events/build_service_spec, usage/event_selection_rule_spec, api/helm_packages_spec, api/internal/base_spec, api/generic_packages_spec, the three Conan request specs, jwt_controller_spec, dependency_proxy_auth_controller_spec, the project and group repository settings controller specs and the project one under spec/requests, the project and group deploy tokens controller request specs, group_saml/sso_enforcer_spec, authn/token_exchange_spec and the six virtual registry service specs that use deploy tokens: 4597 examples, 66 failures, 89 pending. Every failure is LoadError: cannot load such file -- sass, raised in this environment by ApplicationController's add_gon_variables before action, before the action runs, and the same 66 examples fail the same way with master's model. The deploy token examples of jwt_controller_spec and dependency_proxy_auth_controller_spec are among them, so here those paths are covered only by the Gitlab::Auth and DependencyProxy::AuthTokenService specs, which pass.
  • ee/spec/lib/gitlab/git_access_spec.rb and ee/spec/requests/api/virtual_registries/packages/maven/endpoints_spec.rb: 246 examples, 40 failures, every one NoMethodError: undefined method 'fetch' for nil from ActionCableWithLoadBalancing when a merge request diff triggers its subscription in this environment; the same 40 examples fail the same way with master's model.
  • The #gitlab_deploy_token groups of spec/models/project_spec.rb and spec/models/group_spec.rb and the deploy token variables of spec/models/ci/build_spec.rb: 17 examples, 0 failures.
  • The console steps below, run as a throwaway spec with a factory project in place of Project.first: [true, true] with master's model, [false, true] with this MR.
  • RuboCop on both files: no offenses. scripts/lint/commit_linter.rb and the Danger commit linter: no problems. git diff --check: clean.

How to set up and validate locally

  1. Open a Rails console in the GDK:

    bin/rails console

  2. Create a deploy token that expires in an hour, then move the application's clock to its expires_at and ask both scopes whether they find it:

    include ActiveSupport::Testing::TimeHelpers

project = Project.first
token = project.deploy_tokens.create!(name: 'clock-check', project_id: project.id, read_repository: true, expires_at: 1.hour.from_now.iso8601)

travel_to(token.expires_at) { [DeployToken.active.exists?(token.id), DeployToken.expired.exists?(token.id)] }

    On master that answers [true, true], because the lookup still compares with the database's clock, an hour earlier. With this MR it answers [false, true].

  3. Clean up:

    token.destroy!

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Merge request reports