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

[Bug] Reduce allocations in Aad validation by not using string.Replace when it isn't necessary #2595

Closed
1 of 14 tasks
eerhardt opened this issue May 13, 2024 · 0 comments · Fixed by #2597
Closed
1 of 14 tasks
Assignees
Milestone

Comments

@eerhardt
Copy link
Contributor

eerhardt commented May 13, 2024

Which version of Microsoft.IdentityModel are you using?
latest

Where is the issue?

  • M.IM.JsonWebTokens
  • M.IM.KeyVaultExtensions
  • M.IM.Logging
  • M.IM.ManagedKeyVaultSecurityKey
  • M.IM.Protocols
  • M.IM.Protocols.OpenIdConnect
  • M.IM.Protocols.SignedHttpRequest
  • M.IM.Protocols.WsFederation
  • M.IM.TestExtensions
  • M.IM.Tokens
  • M.IM.Tokens.Saml
  • M.IM.Validators
  • M.IM.Xml
  • S.IM.Tokens.Jwt
  • Other (please describe)

Repro

In doing a allocation profile of validating a token 1,000 times, one area that stuck out was using string.Replace in a few places:

image

One case that can be fixed is:

private static bool IsValidIssuer(string validIssuerTemplate, string tenantId, string actualIssuer)
{
if (string.IsNullOrEmpty(validIssuerTemplate))
return false;
if (validIssuerTemplate.Contains(TenantIdTemplate))
{
return validIssuerTemplate.Replace(TenantIdTemplate, tenantId) == actualIssuer;

We should be able to do that comparison allocation free.

Similarly this check:

#if NET6_0_OR_GREATER
if (!string.IsNullOrEmpty(tokenIssuer) && !tokenIssuer.Contains(tenantIdFromToken, StringComparison.Ordinal))
throw LogHelper.LogExceptionMessage(new SecurityTokenInvalidIssuerException(LogHelper.FormatInvariant(LogMessages.IDX40004, LogHelper.MarkAsNonPII(tokenIssuer), LogHelper.MarkAsNonPII(tenantIdFromToken))));
// creating an effectiveSigningKeyIssuer is required as signingKeyIssuer might contain {tenantid}
var effectiveSigningKeyIssuer = signingKeyIssuer.Replace(AadIssuerValidator.TenantIdTemplate, tenantIdFromToken, StringComparison.Ordinal);
var v2TokenIssuer = openIdConnectConfiguration.Issuer?.Replace(AadIssuerValidator.TenantIdTemplate, tenantIdFromToken, StringComparison.Ordinal);
#else
if (!string.IsNullOrEmpty(tokenIssuer) && !tokenIssuer.Contains(tenantIdFromToken))
throw LogHelper.LogExceptionMessage(new SecurityTokenInvalidIssuerException(LogHelper.FormatInvariant(LogMessages.IDX40004, LogHelper.MarkAsNonPII(tokenIssuer), LogHelper.MarkAsNonPII(tenantIdFromToken))));
// creating an effectiveSigningKeyIssuer is required as signingKeyIssuer might contain {tenantid}
var effectiveSigningKeyIssuer = signingKeyIssuer.Replace(AadIssuerValidator.TenantIdTemplate, tenantIdFromToken);
var v2TokenIssuer = openIdConnectConfiguration.Issuer?.Replace(AadIssuerValidator.TenantIdTemplate, tenantIdFromToken);
#endif
// comparing effectiveSigningKeyIssuer with v2TokenIssuer is required as well because of the following scenario:
// 1. service trusts /common/v2.0 endpoint
// 2. service receieves a v1 token that has issuer like sts.windows.net
// 3. signing key issuers will never match sts.windows.net as v1 endpoint doesn't have issuers attached to keys
// v2TokenIssuer is the representation of Token.Issuer (if it was a v2 issuer)
if (effectiveSigningKeyIssuer != tokenIssuer && effectiveSigningKeyIssuer != v2TokenIssuer)

Should be able to be done without the 2 string allocations that is happening.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants