LightWorker authorization mechanism doesn't work

[bruno-brant]

LightWorker has a interesting authorization mechanism that can be understood from its code:

https://github.com/Avanade/Liquid-Application-Framework/blob/f312143f5daf023eeb7c62b4f6bbdef7d1a24c94/src/Liquid.Activation/Worker/LightWorker.cs#L150-L165

Reading the code we can see that, if the method from the worker is annotated with Authorize, Liquid will try to check if the roles declared in the Authorize attribute are in the message. For that, it checks if the message that was received has a Context.User field that is a ClaimsPrincipal, and asks the principal if it's on the desired role.

From what I could gather, this mechanism never works:

1. The class LightMessage has a JsonIgnore on the Context field, so the context isn't propagated
2. Most ClaimsPrincipal implementations can't be serialized directly, throwing a PlatformNotSupportedException
3. If multiple roles are informed in the AuthorizeAttribute, then the method will fail because the roles property is passed in full to the IsInRole method.
4. Policy is being fed into the FindFirst(string type) method, so Liquid is trying to find a claim with a type that was specified by a policy.

The feature seems interesting but the implementation is flawed; I considering simply removing it and creating an issue to implement it correctly.

[guilhermeluizsp]

From my experience with Liquid, the LightMessage purposefully ignores the Context property because it generates a new claims principal from its TokenJwt property (which is serialized).

[bruno-brant]

@guilhermeluizsp do you have any idea where the code write the TokenJwt property when sending the message? I've scanned the code and there's no mention of TokenJwt anywhere, which may lead me to conclude that the mechanism is actually broken.

[guilhermeluizsp]

I was asking the same question to myself weeks ago, while debugging. Perhaps we should set it manually?

Also, I was revisiting the implementation of the VerifyTokenReceived method and just realized that it does not validate the JWT, just accepts it as it is.

https://github.com/Avanade/Liquid-Application-Framework/blob/d437ed2e596cec3d5547373e8b6a456d94a3e7ff/src/Liquid.Runtime/Auth/JwtSecurityCustom.cs#L43-L47

[bruno-brant]

Yeah, I've also seem that. I think I listed it elsewhere.

[bruno-brant]

Also: after a LOT of debugging, I think I spotted the issue that was surfacing from my test, that even if I create a ClaimsPrincipal that has a role claim of a certain type, Liquid won't be able to obtain that role, because, after serializing and deserializing the Token using LightMessage, the claim type is corrupted.

(do bear in mind that I'm not an expert in Claims Authentication / WIF)

The issue seems to stem from the fact that the code is asymmetrical in how it parses tokens as opposed to how it produces the, i.e., differences between LightMessage.TokenJwt.set vs LightMessage.TokenJwt.get.

Set

Set is actually easier to understand - it calls JwtSecurityCustom.VerifyTokenReceived(token) which feeds the token string value to a JwtSecurityToken(jwtEncodedString: protectedText). This class parses the full token text (all three parts) and decodes it.

(nevermind the mock code here...)

https://github.com/Avanade/Liquid-Application-Framework/blob/79207fa0461411acac8f42c7fbca8d93232496da/src/Liquid.Runtime/Auth/JwtSecurityCustom.cs#L45-L46

We can see that afterwards only the claims are extracted from the token and that is, in turn, fed into a hollow ClaimsIdentity which is wrapped in a ClaimsPrincipal. The principal gets assigned to the Context.User of the LightMessage and everything else is discarded from the token.

Get

Get is where things get weird. Instead of using JwtSecurityToken, it creates a JwtSecurityTokenHandler, then a SecurityTokenDescriptor, etc.:

https://github.com/Avanade/Liquid-Application-Framework/blob/79207fa0461411acac8f42c7fbca8d93232496da/src/Liquid.Activation/Worker/LightMessage.cs#L41-L46

The main issue lies on the magic done inside JwtSecurityTokenHandler.CreateToken. Inspecting its source we can see a call to OutboundClaimTypeTransform:

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/8105136d80790d9ab383632b948fc4529c69e0d7/src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs#L496

Which maps claims and, in this particular case, transform the Role claim type ("http://schemas.microsoft.com/ws/2008/06/identity/claims/role") to simple "role":

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/8105136d80790d9ab383632b948fc4529c69e0d7/src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs#L600

We can see the mapping here:

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/6e7a53e241e4566998d3bf365f03acd0da699a31/src/System.IdentityModel.Tokens.Jwt/ClaimTypeMapping.cs#L121

TL;DR We should be either using AzureAD's classes on both reading from the message (set) and writing to it (get) OR we should be using JwtSecurityToken on both set and get, but using one mechanism on each one is resulting in a bug.

[bruno-brant]

Of course, one final comment here is that the code is full of bad practices -- for instance, the crazy side effects that trying to get/set a property can have (get can and will change data on many instances), the fact that LightContext has no meaning, etc.

We can also note that AzureServiceBus doesn't enforces that messages be a LightMessage... the methods are generic but the parameter has no restrictions and could easily be replaced by an object.

My take here is this: I've spent the best part of a day analyzing yet another feature that is half implemented at best and broken at worse; I suggest we simply remove the Authorization code from LightMessage and LightWorker and leave a issue open to reimplement it. What do you guys think?

[guilhermeluizsp]

I suggest we simply remove the Authorization code from LightMessage and LightWorker and leave a issue open to reimplement it. What do you guys think?

Completely agree. As you just mentioned, LightContext has no meaning. It's just another (useless) layer on top of the existing .NET security model, also used by LightController (which, by the way, inherits from Controller, which already has its own ClaimsPrincipal User property) and LightDomain. Broken semantics, coupling to ASP.NET Core...oh God...Refactor FTW here

[bruno-brant]

Good, tomorrow I'll create a branch that removes this code completely and open an issue to implement authentication on queue/topics.

I also have some concerns over it - tokens expire which might result in invalid messages...