Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discover Key Vault auth scope from challenges #6271

Open
chlowell opened this issue Nov 27, 2024 · 3 comments
Open

Discover Key Vault auth scope from challenges #6271

chlowell opened this issue Nov 27, 2024 · 3 comments
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. feature-request This issue requires a new behavior in the product in order be resolved. KeyVault

Comments

@chlowell
Copy link
Member

Today, Key Vault clients use a heuristic to guess the scope for access tokens given a vault URL:

static std::string GetScopeFromUrl(Azure::Core::Url const& url)

Clients should instead follow this protocol to discover the scope when sending their first request:

  1. remove the request body, if any; save it for later
  2. send the request without setting its Authorization header
  3. parse the Bearer challenge from the response's WWW-Authenticate header
    • the scope for GetToken is the challenge's scope parameter, or its resource parameter + /.default
    • the tenant ID for GetToken is the first path segment of the authorization parameter, which is a URL
    • throw if the response doesn't contain a Bearer challenge or the challenge doesn't contain scope or resource
  4. acquire an access token
  5. authorize the request with the access token
  6. reattach the original request body, if any
  7. send the request again

(see other SDKs' Key Vault auth policies for example implementations)

@chlowell chlowell added Client This issue points to a problem in the data-plane of the library. feature-request This issue requires a new behavior in the product in order be resolved. KeyVault labels Nov 27, 2024
@ahsonkhan
Copy link
Member

@chlowell KeyVault has its own challenge-based authentication policy (deriving from BearerTokenAuthenticationPolicy).
At first, the "guessed" scope that you mentioned, inferred from the vault url is passed into that policy's ctor.
However, the policy does then get the scope from the challenge (what you mentioned in step 3):

bool AuthorizeRequestOnChallenge(
std::string const& challenge,
Core::Http::Request& request,
Core::Context const& context) const override
{
auto const scope = GetScope(challenge);
if (scope.empty())
{
return false;
}
ValidateChallengeResponse(scope, request.GetUrl().GetHost());
auto const tenantId = GetTenantId(GetAuthorization(challenge));
{
std::unique_lock<std::shared_timed_mutex> writeLock(m_tokenRequestContextMutex);
m_tokenRequestContext.TenantId = tenantId;
m_tokenRequestContext.Scopes = {scope};
}
AuthenticateAndAuthorizeRequest(request, m_tokenRequestContext, context);
return true;
}

From what I can tell at a quick glance, the initial scope being set here, after calling GetScopeFromUrl, isn't really used in any meaningful way, and gets overwritten in AuthorizeRequestOnChallenge:

Azure::Core::Credentials::TokenRequestContext tokenContext;
tokenContext.Scopes = {_internal::UrlScope::GetScopeFromUrl(m_vaultUrl)};
perRetryPolicies.emplace_back(
std::make_unique<_internal::KeyVaultChallengeBasedAuthenticationPolicy>(
std::move(credential), std::move(tokenContext)));

It is basically only used in this TokenNeedsRefresh check:
bool TokenNeedsRefresh(
Azure::Core::Credentials::AccessToken const& cachedToken,
Azure::Core::Credentials::TokenRequestContext const& cachedTokenRequestContext,
Azure::DateTime const& currentTime,
Azure::Core::Credentials::TokenRequestContext const& newTokenRequestContext,
bool forceRefresh)
{
return forceRefresh || newTokenRequestContext.TenantId != cachedTokenRequestContext.TenantId
|| newTokenRequestContext.Scopes != cachedTokenRequestContext.Scopes
|| currentTime > (cachedToken.ExpiresOn - newTokenRequestContext.MinimumExpiration);
}

Could you please take a look at the AuthorizeRequestOnChallenge() methods within the KeyVaultChallengeBasedAuthenticationPolicy and let us know if you still see a particular issue here around auth scopes?

@ahsonkhan
Copy link
Member

ahsonkhan commented Dec 4, 2024

From what I can tell at a quick glance, the initial scope being set here, after calling GetScopeFromUrl, isn't really used in any meaningful way, and gets overwritten in AuthorizeRequestOnChallenge

Never mind, it looks like that scope is actually being used in the first GetToken() request (from step 2). If we don't set it (#6274), credential's GetToken() fail. For example az cli cred will throw this error:

2024-12-03T01:18:22.9146535Z ERROR: AADSTS90014: The required field 'scope' is missing from the credential. Ensure that you have all the necessary parameters for the login request.

And mi cred will throw:

Failed to get token from ManagedIdentityCredential: GetToken(): error response: 400 Bad Request
{"error":"invalid_request","error_description":"Required query variable 'resource' is missing"}

send the request without setting its Authorization header

For this step 2 (sending the first request), what should the scope be set to? In GoLang, is it set to nothing/empty string?
https://github.com/Azure/azure-sdk-for-go/blob/edcddf10f57c07506d8deed7ea5e8a5b25be2c95/sdk/security/keyvault/internal/challenge_policy.go#L58-L68

@chlowell
Copy link
Member Author

chlowell commented Dec 4, 2024

The client doesn't need to call GetToken before before sending its first request because it doesn't authorize that request i.e., it doesn't attach an access token. The purpose of the first request is to learn authentication parameters such as the scope from the server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. feature-request This issue requires a new behavior in the product in order be resolved. KeyVault
Projects
Status: Untriaged
Development

No branches or pull requests

3 participants