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

Update AAD token request target resource uri for ACR access #4654

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

ShaunDonn2
Copy link

@ShaunDonn2 ShaunDonn2 commented Sep 27, 2024

Description

In alignment with this documentation, we should use the ACR-specific target resource URI rather than the generic ARM URI in order to get the "xms_az_tm" claim on the token that we need to authenticate as a Trusted Service with ACR. This value is passed in and used as the scopes value for the token request in the AuthenticationProvider.

This PR effectively updates the name of the config field as "armResourceManagerId" is not descriptive of the purpose of this field, which is to determine the target resource URI for getting access tokens for the ACR. The "management.azure.com" value will still work here, so we will keep it as the default so as not to break any OSS or other scenarios. We will override this value in our PaaS code to point to the ACR-specific value which enabled Trusted Services scenarios.

Related issues

Addresses bug 126662.

Testing

Validated this change by passing in the values of a prod Credential Bundle and the updated target resource URI into the token request in the AuthenticationProvider. With the generic ARM URI, the token comes back without the xms_az_tm claim; with the ACR-specific URI, the request comes back with the claim included.

FHIR Team Checklist

  • Update the title of the PR to be succinct and less than 65 characters
  • Add a milestone to the PR for the sprint that it is merged (i.e. add S47)
  • Tag the PR with the type of update: Bug, Build, Dependencies, Enhancement, New-Feature or Documentation
  • Tag the PR with Open source, Azure API for FHIR (CosmosDB or common code) or Azure Healthcare APIs (SQL or common code) to specify where this change is intended to be released.
  • Tag the PR with Schema Version backward compatible or Schema Version backward incompatible or Schema Version unchanged if this adds or updates Sql script which is/is not backward compatible with the code.
  • CI is green before merge Build Status
  • Review squash-merge requirements

Semver Change (docs)

Patch|Skip|Feature|Breaking (reason)

@ShaunDonn2 ShaunDonn2 requested a review from a team as a code owner September 27, 2024 22:52
@ShaunDonn2 ShaunDonn2 added Bug Bug bug bug. Azure API for FHIR Label denotes that the issue or PR is relevant to the Azure API for FHIR labels Sep 27, 2024
Copy link

@rainyjyu rainyjyu left a comment

Choose a reason for hiding this comment

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

Does the ACR audience need to be parameterized in config for GovCloud or other non-PublicCloud environments?

rainyjyu
rainyjyu previously approved these changes Sep 27, 2024
@ShaunDonn2
Copy link
Author

Does the ACR audience need to be parameterized in config for GovCloud or other non-PublicCloud environments?

I was planning on doing this in the Health-PaaS repo, where we had the old ARM value parameterized, but I just saw in the docs that the value I have added is constant across Azure Clouds. I will still need to update the value in Health-PaaS, but it seems that we can have the same value for all environments.

abiisnn
abiisnn previously approved these changes Sep 30, 2024
@ShaunDonn2 ShaunDonn2 dismissed stale reviews from ms-teli and abiisnn via 26f6541 September 30, 2024 21:53
abiisnn
abiisnn previously approved these changes Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure API for FHIR Label denotes that the issue or PR is relevant to the Azure API for FHIR Bug Bug bug bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants