-
Notifications
You must be signed in to change notification settings - Fork 88
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
Enable using confidential.Client only for token caching #344
Conversation
aff2edc
to
a160a09
Compare
apps/confidential/confidential.go
Outdated
// TokenProviderResult is the authentication result returned by custom token providers | ||
type TokenProviderResult = exported.TokenProviderResult | ||
|
||
// NewCredFromTokenProvider creates a Credential from a function that provides access tokens. This |
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.
For the public comments, please add a note that this extensibility point is only meant for Azure SDK to cache MSI tokens. We want to be explicit about this, so that other users do not try to use this. Long term we want to move all MSI logic into MSALs, so we could even drop this extensiblity point.
base.WithX5C(opts.SendX5C), | ||
} | ||
if cred.tokenProvider != nil { | ||
// The caller will handle all details of authentication, using Client only as a token cache. |
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.
That's a very nice detail
func WithKnownAuthorityHosts(hosts []string) Option { | ||
return func(c *Client) { | ||
cp := make([]string, len(hosts)) | ||
copy(cp, hosts) |
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.
go newbie question - why do you need to copy the hosts slice to cp? Is it because MSAL might append data to it?
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.
That's the gist. A slice value is a pointer. Appending copies a slice without modifying the original but elements are mutable otherwise. This explicit copying will prevent sharing memory with applications once this API is publicly accessible and hosts
is provided by users.
metadata, err := m.getMetadataEntry(ctx, authParameters.AuthorityInfo) | ||
if err != nil { | ||
return TokenResponse{}, err | ||
// fetch metadata iff the authority isn't explicitly trusted |
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.
typo iff
@@ -12,3 +12,23 @@ type AssertionRequestOptions struct { | |||
// TokenEndpoint is the intended token endpoint. Used as the assertion's "aud" claim. | |||
TokenEndpoint string | |||
} | |||
|
|||
// TokenProviderParameters is the authentication parameters passed to token providers | |||
type TokenProviderParameters struct { |
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.
Consider naming this AppTokenProvider...
since it only works for client_credential flow and not for user flows (auth_code, obo etc.); Or I guess in the future, if we decide to extend this functionality to user flows, you can add more parameters to these objects.
a160a09
to
9b04f7b
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Closes #302 by adding
confidential.NewCredFromTokenProvider()
, which enables creating a confidential client that invokes a callback to acquire access tokens. Applications can use this to handle all the details of authentication themselves while relying on the MSAL client for token caching. I mostly followed the design doc but want to call out one change:TokenProviderResult
here doesn't have aRefreshInSeconds
field. I omitted this because MSAL for Go currently uses a hardcoded refresh window and I don't want to imply to users that settingRefreshInSeconds
would have an effect. Adding this field later won't break the API.To prevent unnecessary authority validation and metadata discovery, I added an internal known authority API. This isn't part of the public API but is a start on #301.