-
Notifications
You must be signed in to change notification settings - Fork 237
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
Add support for authenticating using azureauth cli #1464
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much @demoray for jumping on this! This will unlock easy usage across a lot of developer tools!
sdk/identity/src/token_credentials/azureauth_cli_credentials.rs
Outdated
Show resolved
Hide resolved
After discussing with some members of the Azure SDK Identity team, they have no plans to support azureauth at this time but are aware of it and have talked with the team; however, they were fine with the idea to make it an optional feature in rust - something our other languages don't enjoy. If/when we support azureauth we can revisit but our understanding is that this is mostly (only) an internal-only tool at this point. So can you instead put this behind an optional off-by-default feature? |
@heaths That is understandable, we very much appreciate the inclusion as an optional feature :) (ty, Rust). AzureAuth is OSS and might be used outside the company, but we explicitly have no inherent telemetry as we care about data privacy first and foremost. |
Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feature flag, 2 remaining small comments :)
All, | ||
#[cfg(target_os = "windows")] | ||
IntegratedWindowsAuth, | ||
Broker, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broker, | |
#[cfg(target_os = "windows")] | |
Broker, |
Broker would also need it's own cfg
atribute right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@demoray ping.
self | ||
} | ||
|
||
fn get_access_token(&self, resource: &str) -> azure_core::Result<CliTokenResponse> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we take scopes: &Vec<String>
or similar here instead of a single resource
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@demoray After reading the TokenCredential
trait source, I see that resource
here is actually a space separated list of scopes, or potentially the concept of a resource as parsed by the az CLI
. But MSAL only takes a list of scopes, which AzureAuth will turn --resource into, but we recommend passing --scope
repeatedly for each scope needed.
I think we need to test this a little further and likely split this "resource" (maybe rename it scopes), and pass each value as it's own --scope
for azureauth to correctly handle them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to do this as a follow-up PR, as I think the underlying trait should be updated to take a list of scopes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So long as this doesn't ship, a follow up PR seems okay. I do not think this will work as is.
@demoray , One more flag we highly encourage passing when using AzureAuth is the Perhaps to future proof, we can add a |
Oh, @demoray sorry, I realized that method takes a resource, because the Trait requires it to do so... Perhaps, we can adhere to that, but internally pass |
I've added |
@demoray While testing this internally, we quickly found (and confirmed by reading the source for Calling AzureAuth can be expensive (300-1000ms depending on what's going on under the hood). So caching this will be important for performance in developer facing scenarios, which is exactly where AzureAuth is designed to be used :). |
@kyle-rader-msft Regarding token caching, there's an existing wrapper for this - see AutoRefreshingTokenCredential. See my azure_devops_rust_api example code for typical usage. This is clearly not a great situation, as it is too easy for people to use the existing credential providers with no caching. Ideally we would integrate the token caching into the credential providers so that it is done by default (which the other Azure SDKs seem to do). I raised an issue about this earlier this year, but have not yet had a chance to implement it: One further snippet on token caching - we had an issue raised previously which observed that the token cache needs to be per-resource. I fixed this in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we still need 1 cfg
on the broker mode (I could also see just leaving out the concept of modes and instead taking an arbitrary list of extra args to pass, so that as the args evolve over time this wrapper need not chance).
The other question I have is on the "resource" passed and it actually being a space separated list of scopes.
@johnbatty - Thank you for calling that out!! |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 given impending follow up PR.
Functionality was placed behind a feature gate. utf8 encoding was verified.
sdk/identity/Cargo.toml
Outdated
@@ -50,6 +50,7 @@ development = [] | |||
test_e2e = [] | |||
client_certificate = ["openssl"] | |||
vendored_openssl = ["openssl/vendored"] | |||
azureauth-cli = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we use_
in the other features
This PR adds support for calling out to the azureauth CLI.
https://github.com/AzureAD/microsoft-authentication-cli