-
Notifications
You must be signed in to change notification settings - Fork 340
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
[draft] sni + mtls #4965
base: main
Are you sure you want to change the base?
[draft] sni + mtls #4965
Conversation
src/client/Microsoft.Identity.Client/ApiConfig/AcquireTokenForClientParameterBuilder.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ApiConfig/AcquireTokenForClientParameterBuilder.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ApiConfig/AcquireTokenForClientParameterBuilder.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ApiConfig/AcquireTokenForClientParameterBuilder.cs
Outdated
Show resolved
Hide resolved
commonParameters.MtlsCertificate = _confidentialClientApplication.Certificate; | ||
commonParameters.AuthenticationOperation = new MtlsPopAuthenticationOperation(_confidentialClientApplication.Certificate); | ||
ServiceBundle.Config.ClientCredential = null; | ||
ServiceBundle.Config.UseMtlsPop = true; |
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.
Avoid having this as the request level and at the app level.
...oft.Identity.Client/ApiConfig/Parameters/AbstractAcquireTokenConfidentialClientParameters.cs
Outdated
Show resolved
Hide resolved
@@ -123,8 +123,9 @@ public string ClientVersion | |||
public bool IsPublicClient => !IsConfidentialClient && !IsManagedIdentity; | |||
|
|||
public Func<AppTokenProviderParameters, Task<AppTokenProviderResult>> AppTokenProvider; | |||
public bool UseMtlsPop { get; internal set; } = false; |
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.
Should not be needed.
KeyId = mtlsCert.Thumbprint; | ||
} | ||
|
||
public int TelemetryTokenType => (int)TokenType.Pop; |
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.
Pls use a different value here. This one is reserved fro SHR POP
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.
@gladjohn @bgavrilMS is the mapping maintained in the enum?
|
||
public void FormatResult(AuthenticationResult authenticationResult) | ||
{ | ||
var header = new JObject(); |
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.
What is this for?
src/client/Microsoft.Identity.Client/AuthScheme/PoP/MtlsPopAuthenticationOperation.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/AuthScheme/PoP/MtlsPopAuthenticationOperation.cs
Outdated
Show resolved
Hide resolved
@@ -14,6 +14,7 @@ internal class RegionDiscoveryProvider : IRegionDiscoveryProvider | |||
{ | |||
private readonly IRegionManager _regionManager; | |||
public const string PublicEnvForRegional = "login.microsoft.com"; | |||
public const string PublicEnvForMtls = "mtlsauth.microsoft.com"; |
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 recommend you rename this to RegionAndMtlsProvider
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.
@bgavrilMS but the region is not included in the const
?
if (requestContext.ServiceBundle.Config.UseMtlsPop) | ||
{ | ||
requestContext.Logger.Info(() => $"[Region discovery] Using MTLS public endpoint: {PublicEnvForMtls}"); | ||
return $"{region}.{PublicEnvForMtls}"; |
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.
This is wrong for non public cloud.
string host = authority.Host; | ||
|
||
if (requestContext.ServiceBundle.Config.UseMtlsPop) |
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.
What about the case of non-regional + mtls POP config? Do we want to support this or just error out?
{ | ||
var dict = new Dictionary<string, string> | ||
{ | ||
[OAuth2Parameter.GrantType] = OAuth2GrantType.ClientCredentials, | ||
[OAuth2Parameter.Scope] = AuthenticationRequestParameters.Scope.AsSingleString() | ||
}; | ||
|
||
// Only add TokenType if useMtlsPop is true | ||
if (useMtlsPop) |
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.
It's better to do this in MtlsPopAuthenticationOperation.cs
internal static class RequestTokenType | ||
{ | ||
public const string Bearer = "bearer"; | ||
public const string MTLSPop = "mtls_pop"; |
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.
Pls consolidate with SHR POP.
...s/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/ClientCredentialsTests.NetFwk.cs
Outdated
Show resolved
Hide resolved
...s/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/ClientCredentialsTests.NetFwk.cs
Outdated
Show resolved
Hide resolved
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
initial draft to see if e2e integ can make it through, it does and fail with an expected error.
to-do ; unit tests, finalize public api names, verify token caching, add integ tests