Implement stateless AAD authentication filter which uses appRoles (#494)

[wmitzel-airplus]

Summary

This pull request implements a stateless authentication filter that uses id_token and the appRole feature of AAD. Some details and motivation why this makes sense have been discussed in #494.

Issue Type

• New Feature

Starter Names

• active directory spring boot starter

Additional Information

I've created a new authentication filter which is similar to the already existing AADAuthenticationFilter. However, it does not use HttpSession and thus can be used for stateless Spring backends. The basic JWT claims are checked by the UserPrincipalManager. Additionally, the roles scope is read and the roles are converted to GrantedAuthory which are then attached to the principal.

I've also added an example project which demonstrates the usage.

[msftclas]

All CLA requirements met.

[Incarnation-p-lee]

@wmitzel-airplus
Officially, we recommend contributor to send PR that do one thing at a time, and code changes not more than 200~300 lines. It is easy and more effective for both contributor and reviewers. How do you think about it?

[wmitzel-airplus]

Hi @Incarnation-p-lee
I tried to follow HowToContribute.md where it mentions a passing build, updated documents, sample code, readme and code coverage. Maybe it makes sense to add the recommendation regarding scope and size of a PR to this guideline.

I agree that working on smaller PR can lead to more efficiency. This PR currently consists of 3 parts:

1. the actual implementation in azure-spring-boot/ (~159 lines containing the actual change plus ~155 lines of tests). I would argue that splitting the implementation further down makes only little sense as it only consists of one Filter that parses the token to get the roles.
2. update of documentation in azure-spring-boot-starters (30 lines)
3. an example project demonstrating the new feature in azure-spring-boot-samples/azure-active-directory-spring-boot-app-role-sample (~290 lines + 80 lines readme)

I would suggest to split it into two PRs:

• the first PR (this one) containing 1) and 2): implementation + basic documentation
• the second PR holding the example project 3). I would extract the example project from this PR and will create a new one for it.

[Incarnation-p-lee]

@wmitzel-airplus
Great, following the rule do one thing at one time, I suggest you can separate into 3 PRs.

• Implementation code including unit test
• Sample code.
• Documentation for sample code, like README... etc.

Just personal suggestion, you can make you own choose there.

[wmitzel-airplus]

Hi, sorry for the long delay. Today is the first day of work after my Christmas leave. I've removed the sample module and will add it as a separate PR later.
I am wondering whether I should rebase my changes onto master before we go on?

[Incarnation-p-lee]

@wmitzel-airplus
Thanks for your effort and contribution. You can just merge master tree code when your PR is ready. Please let me know if your PR is ready. Thanks again.

[wmitzel-airplus]

@Incarnation-p-lee
I've updated the PR and it's now ready for review.

[Incarnation-p-lee]

As I understand, if we leverage stateless filter we cannot use normal session based auth together. So from AuthProperty you may need to add some option like statelessEnabled to tell this. If then you can add configuration to AuthProperty instead of inherit from it.

[Incarnation-p-lee]

As stateless, you may need to cleanup SecurityContext after all filterChain done.

[wmitzel-airplus]

What type of "cleanup" do you have in mind?
In the default configuration, Spring uses a ThreadLocal to store the security context and also makes sure to clear it once the request is processed.

[Incarnation-p-lee]

I mean set the security context to null after request processed to make sure the stateless. Yes the spring security framewrok will did that for us if we config stateless session policy. But from our filter review, we cannot trust the user config and need to make sure the context is stateless. How do you think of it ?

[wmitzel-airplus]

I was referring to the first paragraph of SecurityContextHolder, SecurityContext and Authentication Objects in the spring security docs where it says:

[...] Using a ThreadLocal in this way is quite safe if care is taken to clear the thread after the present principal’s request is processed. Of course, Spring Security takes care of this for you automatically, so there is no need to worry about it.

I've validated this behaviour and seen that this code cleans up each request: https://github.com/spring-projects/spring-security/blob/f234a5fbdbf43dbceecfdfe3d41f10b09aa5f197/web/src/main/java/org/springframework/security/web/context/SecurityContextPersistenceFilter.java#L111-L116

But as a defense in depth measure, we can clear the security context after the doFilter call. I suggest to do:

        boolean cleanupRequired = false;
        [...]
        SecurityContextHolder.getContext().setAuthentication(authentication);
        cleanupRequired = true;
        [...]
        try {
            filterChain.doFilter(request, response);
        } finally {
            if (cleanupRequired) {
                //Clear context after execution
                SecurityContextHolder.clearContext();
            }
        }

What do you think?

[Incarnation-p-lee]

Yes, Spring security will do that for you as you mentioned in manual but I mean the authentication in spring context. Sorry for misleading here.

It is OK that if user configuration sessionless, here I just want to make sure the authentication of context is clear before the session filter process(only if user forget to enable stateless policy). Like this SecurityContextHolder.getContext().setAuthentication(null);

Even if use forget to enable stateless, the SecurityContextHolder.getContext().setAuthentication(null); will ensure the null authentication will be store to repo in below code.

		finally {
			SecurityContext contextAfterChainExecution = SecurityContextHolder
					.getContext();
			// Crucial removal of SecurityContextHolder contents - do this before anything
			// else.
			SecurityContextHolder.clearContext();
			repo.saveContext(contextAfterChainExecution, holder.getRequest(),
					holder.getResponse()); // clean auth before finally to make sure auth in context is null
			request.removeAttribute(FILTER_APPLIED);

			if (debug) {
				logger.debug("SecurityContextHolder now cleared, as request processing completed");
			}
		}

[Incarnation-p-lee]

better to add some message with cause here. And for BadJOSEException and JOSEException, should we put it together with BadJWTException ? as it all validation related.

[wmitzel-airplus]

I added BadJWTException as a separate catch block and log statement as this exception is thrown by com.microsoft.azure.spring.autoconfigure.aad.UserPrincipalManager in case the time-related claims of the JWT are invalid:

• the JWT is expired
• or before the JWT is valid

I think this is a widespread type of exception that will occur regularly and is not something that should be logged with error level but instead with warn or even info.
In contrast BadJOSEException and JOSEException are used if there are cryptographic errors (wrong signature, bad encryption) which should not occur during regular operation.

Regarding the log message: do you have any suggestion? In the described cases of expired tokens the result is: Invalid JWT: Expired JWT or Invalid JWT: JWT before use time.

[Incarnation-p-lee]

For log message for exception, my suggestion may looks like this throw new Exception("Invalid JWT: Expired xxxxx", ex); which include some message to user, as well as the cause.

[wmitzel-airplus]

I am not sure I understand what you mean. When this exception is catched, ex.getMessage() will contain either Expired JWT or JWT before use time. From my point of view, there is no additional information I can provide in order to make more sense out of this exception. Or do you mean the fact that here throw new ServletException(ex); no message is specified and only available in the causedBy?

[Incarnation-p-lee]

I mean if there are some message here, we can get the hint from log instead of debugging or something like that ex.getMessage() to find the cause. It is not big issue here, just from the users view about the exception throw.

[Incarnation-p-lee]

please follow the naming conversion as AADAuthenticationProperties, use azure.activedirectory instead of azure.aad.app-role.

[wmitzel-airplus]

The property "namespace" azure.activedirectory is already bound to com.microsoft.azure.spring.autoconfigure.aad.AADAuthenticationProperties. I did not manage to use azure.activedirectory.app-role for my new configuration without having side effects of AADAuthenticationProperties being instantiated and beans being created automatically. As I didn't want to break the existing feature I've chosen this different property name.
Is there a way of doing it without the side effects I've described?

[Incarnation-p-lee]

Please use HttpHeaders.AUTHORIZATION.

[Incarnation-p-lee]

final ?

[Incarnation-p-lee]

hasText instead of != null ?

[Incarnation-p-lee]

leverage below PROPERTY_PREFIX ?

[Incarnation-p-lee]

space after '//'

[Incarnation-p-lee]

duplicated space after =

[wmitzel-airplus]

Thank you very much for the code review. I've fixed all of the issues.

[Incarnation-p-lee]

@wmitzel-airplus
Thanks a lot for your great work and contribution, very appreciate. I think the code is ready to merge and only one thing about the filter name. As it is session stateless, I think the filter should emphasize this, like AADRoleAuthenticationStatelessFilter.java,how do you think of it ?

[wmitzel-airplus]

@wmitzel-airplus
Thanks a lot for your great work and contribution, very appreciate. I think the code is ready to merge and only one thing about the filter name. As it is session stateless, I think the filter should emphasize this, like AADRoleAuthenticationStatelessFilter.java,how do you think of it ?

I like the idea of adding "Stateless" to the name of the filter. However, I'd prefer not to abbreviate "AppRole" to "Role" as the official feature in AAD is called "approle". If you search on the internet for the term "app role" you directly get to the documentation on how to add application roles to the manifest of your application registration.
So my suggestion would be AadAppRoleStatelessAuthenticationFilter. What do you think?

[Incarnation-p-lee]

sounds good, please help to update the name.

[Incarnation-p-lee]

Thanks a lot but could u please following the convention use AAD instead of Aad ? as well as update the name of REAMD.ME ?

[Incarnation-p-lee]

Thanks a lot for you contribution, very appreciate. Could you please help to add sample code for this scenario ? like this

[geoaxis]

👍
PS: when can we expect a maven release for this?

[strivedi-mrc]

Any idea on the ETA to make a maven release for this code in master ?