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

[Identity] Let GetTokenMixin.get_token pass kwargs to MSAL #16397

Closed
wants to merge 2 commits into from

Conversation

jiasli
Copy link
Member

@jiasli jiasli commented Jan 28, 2021

GetTokenMixin is inherited by

  • ClientSecretCredential
  • CertificateCredential
  • ManagedIdentityCredential

However, GetTokenMixin.get_token discards **kwargs when calling MSAL, making it impossible to get an SSH certificate with a Service Principal credential.

After applying this PR, the network trace shows req_cnf is sent correctly:

azure.core.pipeline.policies._universal: Request URL: 'https://login.microsoftonline.com/54826b22-38d6-4fb2-bad9-b7b93a3e9c5a/oauth2/v2.0/token'
azure.core.pipeline.policies._universal: Request method: 'POST'
...
azure.core.pipeline.policies._universal: Request body:
azure.core.pipeline.policies._universal: 
{
    'client_id': 'ee01ddbe-2865-4d12-95ef-cd75ac9285a8', 
    'grant_type': 'client_credentials', 
    'client_info': 1, 
    'client_secret': 'UKx11...', 
    'token_type': 'ssh-cert', 
    'req_cnf': '{
        "kty": "RSA", 
        "n": "...", 
        "e": "AQAB", 
        "kid": "a83576e..."
    }', 
    'key_id': 'a83576e2adc1a2654003f3744486fa1f2595851e5539c6d213011854be1377fb', 
    'scope': 'https://pas.windows.net/CheckMyAccess/Linux/.default'
}

@@ -84,8 +84,8 @@ def _process_response(self, response, request_time):

return token

def get_cached_token(self, *scopes):
# type: (*str) -> Optional[AccessToken]
def get_cached_token(self, *scopes, **kwargs): # pylint:disable=unused-argument
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is more complicated than the change itself:

  • Service Principal credentials support data, but
  • Managed Identity credentials don't. (They may support data in the future.)

Making them share the same GetTokenMixin adds unused kwargs to ManagedIdentityClientBase.get_cached_token.

@chlowell
Copy link
Member

chlowell commented Feb 5, 2021

Whether a credential uses MSAL, and how, are implementation details. Routine maintenance makes it impossible to guarantee two versions of azure-identity will call the same MSAL methods, or require the same version of MSAL (see e.g. #16449). An application trying to pass arguments to an MSAL method through get_token therefore risks breaking every time it upgrades azure-identity. I don't think we should help applications take that risk.

Whether we support acquiring SSH certificates is a separate question. We'll consider doing that in a future version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants