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

Remove identifier uris for aro SP as it's not used #1816

Merged
merged 1 commit into from
Oct 28, 2021

Conversation

bennerv
Copy link
Collaborator

@bennerv bennerv commented Oct 27, 2021

Which issue this PR addresses:

ADO-12455056

Fixes breaking AAD change in Azure: https://docs.microsoft.com/en-us/azure/active-directory/develop/reference-breaking-changes#appid-uri-in-single-tenant-applications-will-require-use-of-default-scheme-or-verified-domains

What this PR does / why we need it:

We can't create service principals in single tenants (customer tenants) with an unverified custom domain (az.aro.azure.com). I don't believe we're even using the identifier_uris field at all in the app registration as well.

Test plan for issue:

Create a cluster using CLUSTER=test go run ./hack/cluster create

Create a cluster using az aro create

Is there any documentation that needs to be updated for this PR?

Not that I'm aware of.

@bennerv
Copy link
Collaborator Author

bennerv commented Oct 27, 2021

@m1kola @rogbas @hawkowl
Not sure why we ever set identifierURIs on our application registration. Seeing as there is now a breaking change in AAD if you attempt to create an app registration with an identifierURI that isn't a verified domain in your tenant.

I think this is a proper approach as we're not using the field anywhere, but not sure on the history of it if it's used elsewhere.

It's working in our INT subscription because azure.com is a verified custom domain in that subscription.

@bennerv bennerv force-pushed the update-identifier-uri branch from c3ec9b7 to 0a2b040 Compare October 27, 2021 20:30
identifier_uris=[
self.get_managed_app_url()
],
identifier_uris=[],
Copy link
Collaborator Author

@bennerv bennerv Oct 27, 2021

Choose a reason for hiding this comment

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

this is a required parameter to pass to ApplicationCreateParameters hence us still passing it, but with an empty array.

Copy link
Member

Choose a reason for hiding this comment

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

This PR is not related to Azure CLI, as it is calling AD Graph SDK azure.graphrbac directly.

Azure CLI has announced the change for idefntifierUris in Azure/azure-cli#19892.

@bennerv
Copy link
Collaborator Author

bennerv commented Oct 28, 2021

/azp run e2e

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bennerv bennerv merged commit b9e9ac9 into Azure:master Oct 28, 2021
@bennerv bennerv deleted the update-identifier-uri branch October 28, 2021 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants