Project

General

Profile

Actions

Bug #18808

closed

OIDC token check can fail due to concurrent requests

Added by Tom Clegg 11 months ago. Updated 11 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
02/28/2022
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Description

If two or more concurrent requests present the same OIDC token, and that token has not already been cached as an Arvados token in the database, multiple threads may try to insert a new token, in which case all but one will fail due to the unique constraint on the api_token column.

user supplied log excerpt:

"respBody":"{\"errors\":[\"error adding OIDC access token to database: pq: duplicate key value violates unique constraint \\\"index_api_client_authorizations_on_api_token\\\"\"]}\n" 

In this situation the losing thread should detect the race and use the token that was added by the winning thread, instead of erroring out.


Subtasks 1 (0 open1 closed)

Task #18809: Review 18808-new-token-raceResolvedTom Clegg02/28/2022

Actions
Actions #1

Updated by Tom Clegg 11 months ago

18808-new-token-race @ fb348442584fc62bb670c378f80ab4c943e875cc --

Actions #2

Updated by Lucas Di Pentima 11 months ago

I have just one question: Why do we need to remove the row after doing the rollback? Wouldn't that remove the valid api_client_auth row that the winning thread set up?

Actions #3

Updated by Tom Clegg 11 months ago

In the race situation, winning & losing threads each add a new row, with random UUID and api_token. The winner changes api_token to hmac(oidctoken) on the row it created, which ensures oidctoken is accepted as an Arvados token. The loser can't update api_token on its row, so that row is just an extra UUID and random api_token that haven't been given to anyone. So it behaves as if it had called "select ... from a_c_a where api_token=$1" a little bit later, in which case it would have returned ("token is already in the database") without creating a new UUID.

Actions #4

Updated by Lucas Di Pentima 11 months ago

Thanks, that clarifies my confusion! LGTM.

Actions #5

Updated by Tom Clegg 11 months ago

  • Status changed from In Progress to Resolved
Actions #6

Updated by Tom Clegg 11 months ago

  • Release set to 49
Actions

Also available in: Atom PDF