Record token last used IP under its own lease

Hardik Galarequested to merge
hgala/29728-pat-last-used-ip-lease into master

Status, 9 Sep: likely superseded. Moving Gitlab::Middleware::IpAddress into the outer Rails stack fixes the bug with no service change and no first-use audit regression. Held open as a fallback pending the auth group's decision.

What does this MR do and why?

Personal access token last-used IP recording writes nothing at all for authenticated API requests. The last_used_ips collection stays empty.

The mechanism, confirmed at runtime by instrumenting the service and capturing both call sites: PersonalAccessTokens::LastUsedService#execute runs twice per request.

Call 1 comes from the rate limiting middleware: lib/gitlab/middleware/labkit_rack_rate_limit.rb resolves the authenticated user to apply the higher authenticated rate limit, reaching find_user_from_web_access_token at auth_finders.rb:144. At that point Gitlab::IpAddressState.current is nil, because Gitlab::Middleware::IpAddress is installed with use inside the Grape app at lib/api/api.rb:8 and so runs downstream. last_used_ip_needs_update? returns false at its first guard, so the IP write is skipped, but last_used_at_needs_update? is true on a fresh token, so update_timestamp runs.

Call 2 is the API's own authentication: api_guard.rb:57 find_current_user! through to find_user_from_access_token at auth_finders.rb:155. The IP is available here, but last_used_at was just written by call 1, so last_used_ip_needs_update? is false against the one minute LAST_USED_IP_TIMEOUT window, last_used_at_needs_update? is false against its ten minute window, and execute returns early.

Net effect: the IP is never written.

The fix, and why it needs two parts

Two changes in PersonalAccessTokens::LastUsedService:

  1. The IP write takes its own lease, pat:last_used_ip_lock:<id>, so a run with no IP never consumes the IP write throttle. The existing pat:last_used_update_lock:<id> lease continues to guard the last_used_at write, and is now only taken when there is actually a timestamp to write.
  2. The IP write is gated on when an IP was last recorded, read from last_used_ips, rather than on last_used_at.

Neither change alone is sufficient. Measured on the flag removal branch in !253596 (merged), where the two examples at spec/requests/api/personal_access_tokens_spec.rb:420 and :632 actually fail:

Change Result
Skip the lease when there is no IP, still write the timestamp Both examples still fail
Gate the IP write on IP recency only One still fails, the one that does not stub the lease
Both together Both pass

The reason: today the freshness gate stops the second call before it reaches the lease, so the lease is not the blocker. Once the gate is corrected, the lease becomes the blocker.

Deliberate behaviour changes, for reviewer judgement

  1. A token whose last_used_at is recent but which has no recorded IP now records one. Previously a recent last_used_at suppressed it. The spec that pinned the old rule in spec/services/personal_access_tokens/last_used_service_spec.rb has been split: the throttle case now seeds a recently recorded IP, and a new example covers recording the first IP.
  2. A taken timestamp lease no longer suppresses the IP write or the unseen-IP audit event, because they are now gated by the IP lease. The spec that pinned the old coupling now stubs the IP lease key.
  3. last_used_ips.maximum(:created_at) adds one read in the path where an IP is present and is not already recorded.

Also worth noting so it is not mistaken for an oversight: the IP lease is deliberately not released, mirroring lease_release? returning false on the timestamp lease. That is what makes it a throttle rather than a mutex, and matches the intent recorded in the commit that made the original lease effective, "Don't release shared lock eagerly when writing IPs".

Database review

The change adds one query, in PersonalAccessTokens::LastUsedService:

SELECT MAX("personal_access_token_last_used_ips"."created_at")
FROM "personal_access_token_last_used_ips"
WHERE "personal_access_token_last_used_ips"."personal_access_token_id" = ?

When it runs: only when an IP is present, last_used_at is already set, and that IP is not already recorded for the token. That is the path where a token is seen from a new address, not every request. The earlier guards in last_used_ip_needs_update? return before it in all other cases.

Cost: the filtered column is index backed by idx_pat_last_used_ips_on_pat_id, a btree on personal_access_token_id, and the table is capped at NUM_IPS_TO_STORE (5) rows per token by the trim in update_pat_ip. So the read is an index lookup over at most five rows.

Execution plan: not included deliberately. The local test table is empty, so a plan generated there would show a sequential scan and would not represent production. Requesting a database reviewer to confirm with a Database Lab plan.

Scope

This fixes the broken write. It does not remove the underlying duplicate resolution: rate limiting still resolves the token before IpAddress runs, and still causes a last_used_at write on every API request. That write is now harmless rather than harmful. The root cause is tracked separately in the issue linked below, where the proposal is a RequestStore backed cache on Gitlab::Auth::RequestAuthenticator.

An alternative was considered and is left for the auth group to weigh: installing Gitlab::Middleware::IpAddress in the outer Rails stack so the IP is available on the first resolution. That would fix this at the source and would additionally give non-API paths such as RSS and ICS feeds IP recording they do not have today, but it widens where Gitlab::IpAddressState is set, which reaches other readers including audit events at lib/api/internal/base.rb:133 and Sidekiq job context propagation at app/workers/concerns/application_worker.rb:233.

Both this merge request and the sibling load balancer middleware order fix need to land before the feature flag removal can proceed.

Test results, all run locally against a real Postgres and Redis

  • spec/services/personal_access_tokens/last_used_service_spec.rb: 33 examples, 0 failures.
  • spec/requests/api/personal_access_tokens_spec.rb and spec/lib/gitlab/auth/auth_finders_spec.rb: 392 examples, 0 failures, 8 pending. The two examples at lines 420 and 632 pass on this branch with or without the change, for the flag reason described under "How to set up and validate locally", so on master they are not a regression guard. They become one once the flag removal in !253596 (merged) lands.
  • spec/lib/gitlab/auth_spec.rb and spec/lib/api/helpers/authentication_spec.rb: 313 examples, 0 failures.
  • spec/models/personal_access_token_spec.rb, spec/requests/api/personal_access_tokens/self_information_spec.rb, spec/requests/api/resource_access_tokens_spec.rb, spec/lib/api/entities/personal_access_token_with_last_used_ips_spec.rb: 382 examples, 0 failures, 11 pending.
  • spec/requests/git_http_spec.rb and ee/spec/requests/api/manage/groups_spec.rb: 496 examples, 0 failures.
  • RuboCop on both changed files: no offences.

Requesting the pipeline:run-all-rspec label given the breadth of the authentication surface this touches.

References

Screenshots or screen recordings

Not applicable, no user interface change.

How to set up and validate locally

An earlier version of this section said the bug reproduces on master. It does not, and correcting that matters for how you read this MR.

spec/support/rate_limiter_labkit_rack_shadow.rb stubs the labkit rack cohort flags off for every example, and on master the middleware starts with return unless any_active_cohort? || plan_rules.active?. So in specs the middleware never runs, the token is resolved once instead of twice, and the bug cannot appear. This branch is based on master, so the same is true here: the affected examples pass on this branch with or without the change, and this MR's own pipeline cannot demonstrate the fix.

The bug reproduces where the middleware runs unconditionally, which is the flag removal branch in !253596 (merged).

  1. Check out hgala/29663-remove-labkit-rack-cohort-flags and run:

    bundle exec rspec spec/requests/api/personal_access_tokens_spec.rb:420 spec/requests/api/personal_access_tokens_spec.rb:632

    Both fail with actual collection contained: [].

  2. Bring in this branch's only production change and run the same command again:

    git checkout hgala/29728-pat-last-used-ip-lease -- app/services/personal_access_tokens/last_used_service.rb

    Both pass.

  3. The service specs do not depend on flag state and run on this branch directly:

    bundle exec rspec spec/services/personal_access_tokens/last_used_service_spec.rb

All four combinations were run locally: flag removal branch with master's service, 2 failures; flag removal branch with this service, 0 failures; this branch with master's service, 0 failures; this branch as pushed, 0 failures.

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.

Edited by Hardik Gala

Merge request reports