Skip to content

Fix concurrent refresh of tokens (#55114)#55733

Merged
jkakavas merged 1 commit intoelastic:7.xfrom
jkakavas:backport/7.x/55114
Apr 27, 2020
Merged

Fix concurrent refresh of tokens (#55114)#55733
jkakavas merged 1 commit intoelastic:7.xfrom
jkakavas:backport/7.x/55114

Conversation

@jkakavas
Copy link
Copy Markdown
Contributor

Our handling for concurrent refresh of access tokens suffered from
a race condition where:

  1. Thread A has just finished with updating the existing token
    document, but hasn't stored the new tokens in a new document
    yet
  2. Thread B attempts to refresh the same token and since the
    original token document is marked as refreshed, it decrypts and
    gets the new access token and refresh token and returns that to
    the caller of the API.
  3. The caller attempts to use the newly refreshed access token
    immediately and gets an authentication error since thread A still
    hasn't finished writing the document.

This commit changes the behavior so that Thread B, would first try
to do a Get request for the token document where it expects that
the access token it decrypted is stored(with exponential backoff )
and will not respond until it can verify that it reads it in the
tokens index. That ensures that we only ever return tokens in a
response if they are already valid and can be used immediately

It also adjusts TokenAuthIntegTests
to test authenticating with the tokens each thread receives,
which would fail without the fix.

Resolves: #54289
Backport of #55114

Our handling for concurrent refresh of access tokens suffered from
a race condition where:

1. Thread A has just finished with updating the existing token
document, but hasn't stored the new tokens in a new document
yet
2. Thread B attempts to refresh the same token and since the
original token document is marked as refreshed, it decrypts and
gets the new access token and refresh token and returns that to
the caller of the API.
3. The caller attempts to use the newly refreshed access token
immediately and gets an authentication error since thread A still
hasn't finished writing the document.

This commit changes the behavior so that Thread B, would first try
to do a Get request for the token document where it expects that
the access token it decrypted is stored(with exponential backoff )
and will not respond until it can verify that it reads it in the
tokens index. That ensures that we only ever return tokens in a
response if they are already valid and can be used immediately

It also adjusts TokenAuthIntegTests
to test authenticating with the tokens each thread receives,
which would fail without the fix.

Resolves: elastic#54289
@jkakavas jkakavas merged commit 38b55f0 into elastic:7.x Apr 27, 2020
jkakavas added a commit to jkakavas/elasticsearch that referenced this pull request Apr 27, 2020
Our handling for concurrent refresh of access tokens suffered from
a race condition where:

1. Thread A has just finished with updating the existing token
document, but hasn't stored the new tokens in a new document
yet
2. Thread B attempts to refresh the same token and since the
original token document is marked as refreshed, it decrypts and
gets the new access token and refresh token and returns that to
the caller of the API.
3. The caller attempts to use the newly refreshed access token
immediately and gets an authentication error since thread A still
hasn't finished writing the document.

This commit changes the behavior so that Thread B, would first try
to do a Get request for the token document where it expects that
the access token it decrypted is stored(with exponential backoff )
and will not respond until it can verify that it reads it in the
tokens index. That ensures that we only ever return tokens in a
response if they are already valid and can be used immediately

It also adjusts TokenAuthIntegTests
to test authenticating with the tokens each thread receives,
which would fail without the fix.

Resolves: elastic#54289
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant