Skip to content

Conversation

@mvanchaa
Copy link
Contributor

@mvanchaa mvanchaa commented Apr 26, 2025

Why this change?

Some users are facing intermittent issues with broker mode while invoking build commands which internally invoke AzureAuth.

If they want to switch to a different mode, they need to change the build scripts and use the changed binaries which is not ideal and is difficult for the end user.

Hence, added this support, so that the user can set a mode that's not failing while we investigate the problem with the default mode.

@mvanchaa mvanchaa requested a review from a team as a code owner April 26, 2025 00:35
keyuxuan
keyuxuan previously approved these changes Apr 28, 2025
@varshini-elayappen
Copy link

@mvanchaa would it be useful to measure how often the default auth mode isn't used? If the number is high, it might help us debug the circumstances in which the default auth mode isn't preferred.

Copy link

@rewrlution rewrlution left a comment

Choose a reason for hiding this comment

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

I left some comments to this PR.

@Manuha
Copy link

Manuha commented Apr 28, 2025

@mvanchaa would it be useful to measure how often the default auth mode isn't used? If the number is high, it might help us debug the circumstances in which the default auth mode isn't preferred.

Done.

@mijpeterson
Copy link
Contributor

mijpeterson commented Apr 28, 2025

More of a general comment for us to think about (and not something to take action on for this PR): I think we're starting to enter the territory where I believe AzureAuth is getting more complex than it should be.

This PR, as well as at least one other we've made in the past, are adding changes to address issues that I believe ultimately stem from a problem with our broker auth flow. I think applying these workarounds to unblock customers is an absolutely valid path forward, but I also think we should start to assess why broker/WAM is consistently failing to auth silently for many of our customers.

This doc even calls out that WAM is meant to be an even more effective way of retrieving tokens silently (over IWA which is not supported for Entra-backed "managed accounts"), and yet we're finding that the local MSAL cache and IWA (for service accounts) continue to work better for many of our customers. And the reality is that these flows are starting to be flagged as "non-compliant".

I think it'd be a good idea for us to start looking at fixing our broker flow, and re-standardize our customers' auth flow towards the "compliant" solution.

rewrlution
rewrlution previously approved these changes Apr 28, 2025
Copy link

@rewrlution rewrlution left a comment

Choose a reason for hiding this comment

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

Ship it!

mijpeterson
mijpeterson previously approved these changes Apr 28, 2025
@mvanchaa
Copy link
Contributor Author

More of a general comment for us to think about (and not something to take action on for this PR): I think we're starting to enter the territory where I believe AzureAuth is getting more complex than it should be.

This PR, as well as at least one other we've made in the past, are adding changes to address issues that I believe ultimately stem from a problem with our broker auth flow. I think applying these workarounds to unblock customers is an absolutely valid path forward, but I also think we should start to assess why broker/WAM is consistently failing to auth silently for many of our customers.

This doc even calls out that WAM is meant to be an even more effective way of retrieving tokens silently (over IWA which is not supported for Entra-backed "managed accounts"), and yet we're finding that the local MSAL cache and IWA (for service accounts) continue to work better for many of our customers. And the reality is that these flows are starting to be flagged as "non-compliant".

I think it'd be a good idea for us to start looking at fixing our broker flow, and re-standardize our customers' auth flow towards the "compliant" solution.

One hundred percent agreed!

@mvanchaa mvanchaa enabled auto-merge (squash) April 28, 2025 21:27
@mvanchaa mvanchaa added the enhancement New feature or request label Apr 28, 2025
@mvanchaa mvanchaa dismissed stale reviews from mijpeterson and rewrlution via 0c92487 April 28, 2025 22:27
@mvanchaa mvanchaa merged commit a151ef0 into main Apr 28, 2025
7 of 9 checks passed
@mvanchaa mvanchaa deleted the read_mode_from_env branch April 28, 2025 23:33
@mvanchaa mvanchaa changed the title Added support to read mode from environment variables for aad subcommands Add support to read mode from environment variables for aad subcommands Apr 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants