Auth: Support required endorsements #2019
Conversation
There was a problem hiding this comment.
I think the logic is right but method comments are holdover and need to be cleaned up.
A fallacy I want to avoid is devs thinking: if I call EndorsementsValidator.Validate(), my token is valid.
Instead, I want them to think: if I call EndorsementsValidator.Validate(), I have performed one step in proving the token is valid.
| @@ -17,18 +17,18 @@ public static class EndorsementsValidator | |||
| /// <see cref="Schema.Activity.ChannelId"/> property is set to "webchat" and the signing party | |||
| /// of the JWT token must have a corresponding endorsement of “Webchat”. | |||
| /// </summary> | |||
| /// <param name="channelId">The ID of the channel to validate, typically extracted from the activity's | |||
| /// <param name="expectedEndorsement">The expected endorsement. Generally the ID of the channel to validate, typically extracted from the activity's | |||
There was a problem hiding this comment.
The right way to think about this is:
- In nearly all cases, the channel ID is the required endorsement
- In some cases, the developer may elect to require an additional endorsement, but this is always additive to the channel ID endorsement
- In very rare cases, a developer will elect to remove the channel ID endorsement, but they are operating in a less secure / unsupported mode at this point.
| /// <see cref="Schema.Activity.ChannelId"/> property, that to which the Activity is affinitized.</param> | ||
| /// <param name="endorsements">The JWT token’s signing party is permitted to send activities only for | ||
| /// specific channels. That list, the set of channels the service can sign for, is called the the endorsement list. | ||
| /// The activity’s <see cref="Schema.Activity.ChannelId"/> MUST be found in the endorsement list, or the incoming | ||
| /// activity is not considered valid.</param> | ||
| /// <returns>True if the channel ID is found in the endorsements list; otherwise, false.</returns> | ||
| public static bool Validate(string channelId, HashSet<string> endorsements) | ||
| public static bool Validate(string expectedEndorsement, HashSet<string> endorsements) |
There was a problem hiding this comment.
Following the logic above, I expect that new endorsements are added to the channel ID endorsement.
We could have a mode where all endorsements are removed, but this is advanced and I'm not sure we have a use case for it.
There was a problem hiding this comment.
Ahh, I see how the channel ID endorsement verification is enforced at the layer above. Seems OK to me. Maybe just comment that multiple endorsements are enforced by calling this method multiple times.
There was a problem hiding this comment.
Reading more through the comment on this method, it should be rewritten to something more like:
/// <summary>
/// Verify that the specified endorsement exists on the JWT token. Call this method multiple times to validate multiple endorsements.
/// </summary>
/// <remarks>
/// JWT token signing keys contain endorsements matching channel IDs they're approved to sign for. They also can contain keywords representing compliance certifications. This code ensures that a channel ID or compliance certification is present on the signing key used for the request's token.
/// </remarks>
There was a problem hiding this comment.
Yes, EndorsementsValidator.Validate is only a collection existence check which was somewhat underwhelming to find out.
libraries/Microsoft.Bot.Connector/Authentication/AuthenticationConfiguration.cs
Outdated
Show resolved
Hide resolved
libraries/Microsoft.Bot.Connector/Authentication/AuthenticationConfiguration.cs
Show resolved
Hide resolved
libraries/Microsoft.Bot.Connector/Authentication/EnterpriseChannelValidation.cs
Show resolved
Hide resolved
libraries/Microsoft.Bot.Connector/Authentication/EnterpriseChannelValidation.cs
Show resolved
Hide resolved
libraries/Microsoft.Bot.Connector/Authentication/GovernmentChannelValidation.cs
Show resolved
Hide resolved
libraries/Microsoft.Bot.Connector/Authentication/GovernmentChannelValidation.cs
Show resolved
Hide resolved
| @@ -124,7 +132,13 @@ public async Task<ClaimsIdentity> GetIdentityAsync(string scheme, string paramet | |||
|
|
|||
| private bool HasAllowedIssuer(string jwtToken) | |||
| { | |||
| if (!_tokenValidationParameters.ValidateIssuer) | |||
| { | |||
There was a problem hiding this comment.
I don't understand this line. Has this ever worked? The logic has me quite confused...
There was a problem hiding this comment.
During tests I was trying to skip issuer validation and noticed that even if I set it to false it kept validating issuers. And it was because this line was not there.
In production, generally ValidateIssuer is true, so it was always correct to validate the issuer.
libraries/Microsoft.Bot.Connector/Authentication/JwtTokenExtractor.cs
Outdated
Show resolved
Hide resolved
|
I ran the CI build against this branch, and got this error: |
|
(I was trying to get the "Compat" list. I'll try that again, although it means running a build w/ no tests...) |
|
Our Binary Compat tool reports the following: and this: |
|
Thanks for the reviews @cleemullins and @dandriscoll! All the comments are completely reasonable, I'll address them tonight and send an update. |
I believe this ctor should call the newer ctor, as the overlap in what they do is nearly complete. (only the _endorsementsData is different) Refers to: libraries/Microsoft.Bot.Connector/Authentication/JwtTokenExtractor.cs:72 in 0f66b75. [](commit_id = 0f66b75, deletion_comment = False) |
|
A Passing build (Tests + Compat) for this branch is here: |
|
Thanks for the reviews! |
Uh oh!
There was an error while loading. Please reload this page.