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

Support CAE in azure-identity #16323

Merged
merged 4 commits into from
Feb 3, 2021
Merged

Support CAE in azure-identity #16323

merged 4 commits into from
Feb 3, 2021

Conversation

chlowell
Copy link
Member

@chlowell chlowell commented Jan 25, 2021

With this, user credentials configure MSAL with client capability "CP1", and accept an optional "claims" argument to get_token.

@chlowell chlowell added Client This issue points to a problem in the data-plane of the library. Azure.Identity labels Jan 25, 2021
Copy link
Member

@mccoyp mccoyp left a comment

Choose a reason for hiding this comment

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

I see recordings for test_device_code and test_username_password from test_cae.py, but are there any other recordings that can be/need to be added?

# decode the recorded token
body = json.loads(six.ensure_str(response["body"]["string"]))
header, encoded_payload, signed = body["id_token"].split(".")
decoded_payload = base64.b64decode(encoded_payload + "=" * (4 - len(encoded_payload) % 4))
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, what are the "=" added onto the payload for?

Copy link
Member Author

Choose a reason for hiding this comment

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

A base64 encoded string should be padded with "=" to make its length divisible by 4. CPython's base64 strictly requires padding. However, because a decoder can infer the padding, encoders commonly omit it.

msal_app = Mock()
msal_app.get_accounts.return_value = [{"home_account_id": record.home_account_id}]
msal_app.acquire_token_silent_with_error.return_value = dict(
build_aad_response(access_token="**", id_token=build_id_token())
Copy link
Member

Choose a reason for hiding this comment

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

Does the SharedTokenCacheCredential not have an id_token_claims portion of the return value (like this one) because it's the only credential of these that doesn't subclass InteractiveCredential?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. That's relevant because MSAL token acquisition methods return a dict containing both the raw ID token and its claims. InteractiveCredential uses these claims to build an AuthenticationRecord. SharedTokenCacheCredential does not, because it already has that information.

real = urlparse(self.cae_settings["arm_url"])
self.scrubber.register_name_pair(real.netloc, "management.azure.com")
self.scrubber.register_name_pair(self.cae_settings["tenant_id"], "tenant")
self.scrubber.register_name_pair(self.cae_settings["username"], "username")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.scrubber.register_name_pair(self.cae_settings["username"], "username")
self.scrubber.register_name_pair(self.cae_settings["username"], "username")
self.scrubber.register_name_pair(self.cae_settings["password"], "password")

Would this be necessary as well, or are passwords already hidden in recordings?

Copy link
Member Author

Choose a reason for hiding this comment

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

Passwords are in request bodies, which aren't recorded.

@chlowell
Copy link
Member Author

I didn't record InteractiveBrowserCredential or SharedTokenCacheCredential because playback is complex for them (note the elaborate mocking in their tests). I think manual testing is acceptable for now, for this feature. I expect to revisit recorded tests in the future, to investigate covering more scenarios with them.

Comment on lines +192 to +194
result = app.acquire_token_silent_with_error(
list(scopes), account=account, claims_challenge=kwargs.get("claims")
)
Copy link
Member

Choose a reason for hiding this comment

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

The removal of **kwargs will make acquiring SSH certificate stop working as data is passed in kwargs.

Azure CLI is aiming to support acquiring SSH certificate for Service Principal as well (#16397).

Copy link
Member Author

Choose a reason for hiding this comment

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

Summarizing offline discussion here, there are two orthogonal concerns: acquiring SSH certificates, and allowing applications to pass keyword arguments through get_token to MSAL. Neither is supported today. Acquiring SSH certificates may be supported in a future version. Passing MSAL arguments through get_token is possible in some cases but really isn't supportable because routine maintenance requires internal changes (e.g. #16449) that can break applications relying on this behavior. My goal with this change is to prevent applications from taking a dependency on such implementation details.

@yonzhan yonzhan self-requested a review January 29, 2021 14:54
@chlowell chlowell merged commit ef46a5c into Azure:master Feb 3, 2021
@chlowell chlowell deleted the cae-identity branch February 3, 2021 18:34
iscai-msft added a commit that referenced this pull request Feb 4, 2021
…into enum-meta

* 'master' of https://github.com/Azure/azure-sdk-for-python:
  bump six dependencies in some libraries (#16496)
  call on_error if timeout in flush (#16485)
  Sync eng/common directory with azure-sdk-tools for PR 1365 (#16505)
  Fix min dependency tests - update azure core (#16504)
  Sync eng/common directory with azure-sdk-tools for PR 1364 (#16503)
  Ma arch feedback (#16502)
  Adding a new limitation to the README file. (#16475)
  [Blob][Datalake] STG76 Preview (#16349)
  append code coverage over each other (#16202)
  Arch preview feedback (#16441)
  Support CAE in azure-identity (#16323)
  [EventHubs] Support for Custom endpoint adddress and custom certificate  (#16295)
  [Communication] - Phone Number Management - Added support for AAD auth (#16075)
  fix live tests (#16495)
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request Feb 4, 2021
…into analyze_redesign

* 'master' of https://github.com/Azure/azure-sdk-for-python: (32 commits)
  Adopt new MSAL auth code flow API (Azure#16449)
  [formrecognizer] use ARM template for tests (Azure#16432)
  T2 kusto 2021 02 04 (Azure#16527)
  T2 applicationinsights 2021 02 04 (Azure#16525)
  Sync eng/common directory with azure-sdk-tools for PR 1366 (Azure#16506)
  [Python] python track2 new pipeline fix (Azure#16494)
  Added package properties SDKType and NewSDK (Azure#16476)
  bump six dependencies in some libraries (Azure#16496)
  call on_error if timeout in flush (Azure#16485)
  Sync eng/common directory with azure-sdk-tools for PR 1365 (Azure#16505)
  Fix min dependency tests - update azure core (Azure#16504)
  Sync eng/common directory with azure-sdk-tools for PR 1364 (Azure#16503)
  Ma arch feedback (Azure#16502)
  Adding a new limitation to the README file. (Azure#16475)
  [Blob][Datalake] STG76 Preview (Azure#16349)
  append code coverage over each other (Azure#16202)
  Arch preview feedback (Azure#16441)
  Support CAE in azure-identity (Azure#16323)
  [EventHubs] Support for Custom endpoint adddress and custom certificate  (Azure#16295)
  [Communication] - Phone Number Management - Added support for AAD auth (Azure#16075)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Identity Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants