Xiao/DisarmSecurityArtifact #2064

ciaozhang · 2023-04-25T22:24:27Z

Remove the signatures from JWS and authentication tag from JWE when PII is on.

For SecurityToken: A new interface is introduced: Microsoft.IdentityModel.Logging. ISafeLogSecurityArtifact with a single method UnsafeToString(). SecurityToken will implementations (JsonWebToken, SAML, etc.) the interface and will provide an implementation of UnsafeToString().
For type of object that is not able modified (such as ‘string’): Struct SecurityArtifact can be used to wrap the object and provide a callback to Log appropriately.

src/Microsoft.IdentityModel.JsonWebTokens/JsonWebToken.cs

TimHannMSFT · 2023-04-26T23:27:26Z

@ciaozhang can you add more details to the description as well as a section which can be copied directly into the release notes? I'm happy to help construct if you need help. @jmprieur would also likely be able to help.

In reply to: 1524185285

src/Microsoft.IdentityModel.JsonWebTokens/JsonWebToken.cs

src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.cs

src/Microsoft.IdentityModel.Logging/SecurityArtifact.cs

src/Microsoft.IdentityModel.Protocols.OpenIdConnect/OpenIdConnectProtocolValidator.cs

src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs

src/Microsoft.IdentityModel.Protocols.OpenIdConnect/OpenIdConnectProtocolValidator.cs

src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs

src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.cs

src/Microsoft.IdentityModel.Protocols.OpenIdConnect/OpenIdConnectProtocolValidator.cs

TimHannMSFT

Thanks for the changes so far Xiao.

You mentioned to me you also had two questions for Brent/Team? Can you please add those as comments in the PR and we can resolve there?

victorm-hernandez · 2023-04-27T21:52:51Z

@ciaozhang minor observation, the title of the PR is very descriptive but the commit message is not. Let's try to keep our commit messages readable and with enough context for anyone to understand a change.

In reply to: 1526562219

src/Microsoft.IdentityModel.JsonWebTokens/JwtTokenUtilities.cs

test/Microsoft.IdentityModel.JsonWebTokens.Tests/JwtTokenUtilitiesTests.cs

src/Microsoft.IdentityModel.JsonWebTokens/JwtTokenUtilities.cs

src/Microsoft.IdentityModel.Logging/SecurityArtifact.cs

victorm-hernandez

🕐

jmprieur

Thanks @ciaozhang
I've asked questions.

Not sure why this is so complicated?
why not just:

if (PII)
{
write token;
}
else
{
write token.SubString(0, token.LastIndexOf('.'));
}

src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.cs

src/Microsoft.IdentityModel.JsonWebTokens/JsonWebToken.cs

src/Microsoft.IdentityModel.JsonWebTokens/JwtTokenUtilities.cs

src/Microsoft.IdentityModel.Logging/IdentityModelEventSource.cs

src/Microsoft.IdentityModel.Logging/NonPII.cs

revoking review

src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.cs

src/Microsoft.IdentityModel.Logging/LogHelper.cs

src/Microsoft.IdentityModel.JsonWebTokens/JsonWebToken.cs

src/Microsoft.IdentityModel.Logging/SecurityArtifact.cs

src/Microsoft.IdentityModel.Protocols.OpenIdConnect/OpenIdConnectProtocolValidator.cs

src/System.IdentityModel.Tokens.Jwt/JwtSecurityToken.cs

victorm-hernandez

🕐

rename the interface temp save temp save santize logic resolve comments address comments address comments

victorm-hernandez · 2023-06-01T23:16:50Z

src/Microsoft.IdentityModel.JsonWebTokens/JwtTokenUtilities.cs

@@ -429,6 +429,24 @@ private static long ParseTimeValue(JToken jToken, string claimName)
            throw LogHelper.LogExceptionMessage(new FormatException(LogHelper.FormatInvariant(LogMessages.IDX14300, LogHelper.MarkAsNonPII(claimName), jToken.ToString(), LogHelper.MarkAsNonPII(typeof(long)))));
        }

+        internal static string SafeLogJwtToken(object obj)


object

I thought this was meant for strings only, for any other object type we should be able to use the provided interface #Closed

It is the callback func we used in SecurityArtifact struct. The reason we take obj as the type of parameter is LogHelper treat all arguments as object.

I see, still regardless of the parameter type this is meant to be consumed by using string version of tokens correct?

how do you justify the code for non strings that you have here?

It will use the default ToString() method return the GetType().ToString()

src/Microsoft.IdentityModel.JsonWebTokens/JwtTokenUtilities.cs

victorm-hernandez · 2023-06-01T23:18:19Z

src/Microsoft.IdentityModel.JsonWebTokens/JwtTokenUtilities.cs

+            int lastDot = token.LastIndexOf(".");
+
+            // no dots, maybe not a JWT
+            if (lastDot == -1)


lastDot

Since this is meant to parse JWT token strings we should throw an argument exception here. #Closed

We will throw in the ReadToken() method if the string token is not an encoded JWT token.

This should be safe regardless of the caller, for non strings, non tokens we should return null.

src/Microsoft.IdentityModel.Logging/IdentityModelEventSource.cs

victorm-hernandez · 2023-06-06T03:15:11Z

src/Microsoft.IdentityModel.Logging/ISafeLogSecurityArtifact.cs

+namespace Microsoft.IdentityModel.Logging
+{
+    /// <summary>
+    /// Interface that provides an unsafe method to log a security artifact.


Interface

You may want to bring the description you have for the SecurityArtifact class to the interface, this would address the questions that Henrik had here. #Resolved

victorm-hernandez · 2023-06-06T03:59:02Z

src/Microsoft.IdentityModel.Logging/LogHelper.cs

+        /// <param name="arg">A log message argument to be marked as SecurityArtifact.</param>
+        /// <param name="callback">A callback function to log the security artifact safely.</param>
+        /// <returns>An argument marked as SecurityArtifact.</returns>
+        public static object MarkAsSecurityArtifact(object arg, Func<object, string> callback)


Why do we receive an object here, this should receive and return a string. #Resolved

victorm-hernandez

TimHannMSFT

Approving based on Victor's approval and seeing that the new direction is more consistent. Thanks for the changes @ciaozhang!

ciaozhang force-pushed the Xiao/DisarmSecurityArtifact branch 3 times, most recently from 0c583ae to de6d050 Compare April 26, 2023 16:29

TimHannMSFT reviewed Apr 26, 2023

View reviewed changes

src/Microsoft.IdentityModel.JsonWebTokens/JsonWebToken.cs Outdated Show resolved Hide resolved

brentschmaltz reviewed Apr 27, 2023

View reviewed changes

src/Microsoft.IdentityModel.JsonWebTokens/JsonWebToken.cs Outdated Show resolved Hide resolved

TimHannMSFT reviewed Apr 27, 2023

View reviewed changes

src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.cs Outdated Show resolved Hide resolved

TimHannMSFT reviewed Apr 27, 2023

View reviewed changes

src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.cs Outdated Show resolved Hide resolved

TimHannMSFT reviewed Apr 27, 2023

View reviewed changes

src/Microsoft.IdentityModel.Logging/SecurityArtifact.cs Outdated Show resolved Hide resolved

TimHannMSFT reviewed Apr 27, 2023

View reviewed changes

src/Microsoft.IdentityModel.Logging/SecurityArtifact.cs Outdated Show resolved Hide resolved

TimHannMSFT reviewed Apr 27, 2023

View reviewed changes

src/Microsoft.IdentityModel.Logging/SecurityArtifact.cs Outdated Show resolved Hide resolved

TimHannMSFT reviewed Apr 27, 2023

View reviewed changes

src/Microsoft.IdentityModel.Protocols.OpenIdConnect/OpenIdConnectProtocolValidator.cs Outdated Show resolved Hide resolved

brentschmaltz reviewed Apr 27, 2023

View reviewed changes

src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs Outdated Show resolved Hide resolved