Double check emulator path when skipping endorsement check

boydc2014 commented 8 months ago

Fixes a potential security risk. #minor

Description

This PR ensures we skip endorsement check only when we are certain about this is emulator path, instead of assuming it is emulator based on the endorsement list get from open id endpoint.

Specific Changes

Add a double check after skip the endorsement check
Add the check afterwards instead of doing a pre-check is to minimize the cost and change size

Testing

Tested locally with positive and negative tests

tracyboehrer commented 8 months ago

This is failing on:

Error Message: at System.IdentityModel.Tokens.Jwt.JwtSecurityTokenHandler.ValidateSignature(String token, TokenValidationParameters validationParameters) at System.IdentityModel.Tokens.Jwt.JwtSecurityTokenHandler.ValidateToken(String token, TokenValidationParameters validationParameters, SecurityToken& validatedToken) at Microsoft.Bot.Connector.Authentication.JwtTokenExtractor.ValidateTokenAsync(String jwtToken, String channelId, String[] requiredEndorsements) in D:\a\1\s\libraries\Microsoft.Bot.Connector\Authentication\JwtTokenExtractor.cs:line 259 at Microsoft.Bot.Connector.Authentication.JwtTokenExtractor.GetIdentityAsync(String scheme, String parameter, String channelId, String[] requiredEndorsements) in D:\a\1\s\libraries\Microsoft.Bot.Connector\Authentication\JwtTokenExtractor.cs:line 197 at Microsoft.Bot.Connector.Authentication.JwtTokenExtractor.GetIdentityAsync(String authorizationHeader, String channelId, String[] requiredEndorsements) in D:\a\1\s\libraries\Microsoft.Bot.Connector\Authentication\JwtTokenExtractor.cs:line 150 at Microsoft.Bot.Connector.Authentication.EmulatorValidation.AuthenticateEmulatorToken(String authHeader, ICredentialProvider credentials, IChannelProvider channelProvider, HttpClient httpClient, String channelId, AuthenticationConfiguration authConfig) in D:\a\1\s\libraries\Microsoft.Bot.Connector\Authentication\EmulatorValidation.cs:line 143 at Microsoft.Bot.Connector.Authentication.JwtTokenValidation.AuthenticateTokenAsync(String authHeader, ICredentialProvider credentials, IChannelProvider channelProvider, String channelId, AuthenticationConfiguration authConfig, String serviceUrl, HttpClient httpClient) in D:\a\1\s\libraries\Microsoft.Bot.Connector\Authentication\JwtTokenValidation.cs:line 252 at Microsoft.Bot.Connector.Authentication.JwtTokenValidation.ValidateAuthHeader(String authHeader, ICredentialProvider credentials, IChannelProvider channelProvider, String channelId, AuthenticationConfiguration authConfig, String serviceUrl, HttpClient httpClient) in D:\a\1\s\libraries\Microsoft.Bot.Connector\Authentication\JwtTokenValidation.cs:line 138 at Microsoft.Bot.Connector.Authentication.JwtTokenValidation.AuthenticateRequest(IActivity activity, String authHeader, ICredentialProvider credentials, IChannelProvider provider, AuthenticationConfiguration authConfig, HttpClient httpClient) in D:\a\1\s\libraries\Microsoft.Bot.Connector\Authentication\JwtTokenValidation.cs:line 86 at Microsoft.Bot.Connector.Authentication.BuiltinBotFrameworkAuthentication.AuthenticateRequestAsync(Activity activity, String authHeader, CancellationToken cancellationToken) in D:\a\1\s\libraries\Microsoft.Bot.Connector\Authentication\BuiltinBotFrameworkAuthentication.cs:line 95 at Microsoft.Bot.Builder.CloudAdapterBase.ProcessActivityAsync(String authHeader, Activity activity, BotCallbackHandler callback, CancellationToken cancellationToken) in D:\a\1\s\libraries\Microsoft.Bot.Builder\CloudAdapterBase.cs:line 298 at Microsoft.Bot.Builder.Integration.AspNet.Core.CloudAdapter.ProcessAsync(HttpRequest httpRequest, HttpResponse httpResponse, IBot bot, CancellationToken cancellationToken) in D:\a\1\s\libraries\integration\Microsoft.Bot.Builder.Integration.AspNet.Core\CloudAdapter.cs:line 105 at Microsoft.Bot.Builder.Integration.AspNet.Core.Tests.CloudAdapterTests.ExpiredTokenShouldThrowUnauthorizedAccessException() in D:\a\1\s\tests\integration\Microsoft.Bot.Builder.Integration.AspNet.Core.Tests\CloudAdapterTests.cs:line 846

tracyboehrer commented 8 months ago

Scratch that. This one:

[xUnit.net 00:00:18.58] Microsoft.Bot.Connector.Tests.Authentication.JwtTokenExtractorTests.JwtTokenExtractor_WithValidCert_ShouldNotAllowCertSigningKey [FAIL] [xUnit.net 00:00:18.58] System.UnauthorizedAccessException : Could not validate endorsement key. [xUnit.net 00:00:18.58] Stack Trace: [xUnit.net 00:00:18.58] D:\a\1\s\libraries\Microsoft.Bot.Connector\Authentication\JwtTokenExtractor.cs(295,0): at Microsoft.Bot.Connector.Authentication.JwtTokenExtractor.ValidateTokenAsync(String jwtToken, String channelId, String[] requiredEndorsements) [xUnit.net 00:00:18.58] D:\a\1\s\libraries\Microsoft.Bot.Connector\Authentication\JwtTokenExtractor.cs(197,0): at Microsoft.Bot.Connector.Authentication.JwtTokenExtractor.GetIdentityAsync(String scheme, String parameter, String channelId, String[] requiredEndorsements) [xUnit.net 00:00:18.58] D:\a\1\s\libraries\Microsoft.Bot.Connector\Authentication\JwtTokenExtractor.cs(150,0): at Microsoft.Bot.Connector.Authentication.JwtTokenExtractor.GetIdentityAsync(String authorizationHeader, String channelId, String[] requiredEndorsements) [xUnit.net 00:00:18.58] D:\a\1\s\libraries\Microsoft.Bot.Connector\Authentication\JwtTokenExtractor.cs(130,0): at Microsoft.Bot.Connector.Authentication.JwtTokenExtractor.GetIdentityAsync(String authorizationHeader, String channelId) [xUnit.net 00:00:18.58] D:\a\1\s\tests\Microsoft.Bot.Connector.Tests\Authentication\JwtTokenExtractorTests.cs(88,0): at Microsoft.Bot.Connector.Tests.Authentication.JwtTokenExtractorTests.JwtTokenExtractor_WithValidCert_ShouldNotAllowCertSigningKey() [xUnit.net 00:00:18.58] --- End of stack trace from previous location --- Failed Microsoft.Bot.Connector.Tests.Authentication.JwtTokenExtractorTests.JwtTokenExtractor_WithValidCert_ShouldNotAllowCertSigningKey [503 ms] Error Message: System.UnauthorizedAccessException : Could not validate endorsement key. Stack Trace: at Microsoft.Bot.Connector.Authentication.JwtTokenExtractor.ValidateTokenAsync(String jwtToken, String channelId, String[] requiredEndorsements) in D:\a\1\s\libraries\Microsoft.Bot.Connector\Authentication\JwtTokenExtractor.cs:line 295 at Microsoft.Bot.Connector.Authentication.JwtTokenExtractor.GetIdentityAsync(String scheme, String parameter, String channelId, String[] requiredEndorsements) in D:\a\1\s\libraries\Microsoft.Bot.Connector\Authentication\JwtTokenExtractor.cs:line 197 at Microsoft.Bot.Connector.Authentication.JwtTokenExtractor.GetIdentityAsync(String authorizationHeader, String channelId, String[] requiredEndorsements) in D:\a\1\s\libraries\Microsoft.Bot.Connector\Authentication\JwtTokenExtractor.cs:line 150 at Microsoft.Bot.Connector.Authentication.JwtTokenExtractor.GetIdentityAsync(String authorizationHeader, String channelId) in D:\a\1\s\libraries\Microsoft.Bot.Connector\Authentication\JwtTokenExtractor.cs:line 130 at Microsoft.Bot.Connector.Tests.Authentication.JwtTokenExtractorTests.JwtTokenExtractor_WithValidCert_ShouldNotAllowCertSigningKey() in D:\a\1\s\tests\Microsoft.Bot.Connector.Tests\Authentication\JwtTokenExtractorTests.cs:line 88

tracyboehrer commented 8 months ago

I believe the test should be:

public async Task JwtTokenExtractor_WithValidCert_ShouldNotAllowCertSigningKey() { if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) { var now = DateTimeOffset.UtcNow; var cn = "test.cert.botframework.com";

    // Create valid self-signed certificate
    var cert = CreateSelfSignedCertificate(cn, from: now.AddDays(-10), to: now.AddDays(9));

    // Build token extractor and use it to validate a token created from the cert
    await Assert.ThrowsAnyAsync<UnauthorizedAccessException>(() => BuildExtractorAndValidateToken(cert));

    DeleteKeyContainer(cn);
}

}

tracyboehrer commented 5 months ago

@boydc2014 Is this still a thing?

coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 388172

Details

0 of 0 changed or added relevant lines in 0 files are covered.
20 unchanged lines in 6 files lost coverage.
Overall coverage decreased (-0.01%) to 78.159%

Files with Coverage Reduction	New Missed Lines	%
/libraries/integration/Microsoft.Bot.Builder.Integration.AspNet.Core/ServiceCollectionExtensions.cs	1	93.55%
/libraries/AdaptiveExpressions/BuiltinFunctions/GetPreviousViableTime.cs	1	90.91%
/libraries/Microsoft.Bot.Streaming/Payloads/StreamManager.cs	1	90.0%
/libraries/AdaptiveExpressions/BuiltinFunctions/GetNextViableTime.cs	1	90.91%
/libraries/Microsoft.Bot.Connector.Streaming/Session/StreamingSession.cs	3	90.09%
/libraries/Microsoft.Bot.Connector/Authentication/JwtTokenExtractor.cs	13	47.66%
<!--	Total:	20	-->

Totals
Change from base Build 387708:	-0.01%
Covered Lines:	26184
Relevant Lines:	33501

💛 - Coveralls

BruceHaley commented 2 months ago

:heavy_check_mark: No Binary Compatibility issues for Microsoft.Bot.Connector.dll

microsoft / botbuilder-dotnet