NIFI-15221 - Major upgrade to Spring 7 #10554
Conversation
| Authentication authentication = new AuthenticationRequestToken( | ||
| authenticationRequest, | ||
| identityProvider.getClass(), | ||
| servletRequest.getRemoteAddr()); | ||
| logger.debug("Adding credentials claim to SecurityContext to be authenticated. Credentials extracted by {}: {}", | ||
| identityProvider.getClass().getSimpleName(), | ||
| authenticationRequest); | ||
| SecurityContextHolder.getContext().setAuthentication(authentication); | ||
| // This filter's job, which is merely to search for and extract an identity claim, is done. | ||
| // The actual authentication of the identity claim will be handled by a corresponding IdentityAuthenticationProvider | ||
| if (authenticationManager != null) { | ||
| try { | ||
| Authentication authenticated = authenticationManager.authenticate(authentication); | ||
| SecurityContextHolder.getContext().setAuthentication(authenticated); | ||
| } catch (AuthenticationException ex) { | ||
| logger.debug("Authentication failed in IdentityFilter for provider {}: {}", identityProvider.getClass().getSimpleName(), ex.getMessage()); | ||
| throw ex; | ||
| } | ||
| } else { | ||
| SecurityContextHolder.getContext().setAuthentication(authentication); | ||
| } |
There was a problem hiding this comment.
A larger refactor would be needed for NiFi Registry when it comes to authentication but this is the only approach I could find to make it work with all the upgrades. Thoughts @exceptionfactory ?
There was a problem hiding this comment.
Thanks for flagging this issue @pvillard31. The Authentication Manager is a central component of Spring Security, so it should never be null unless there is some out-of-order initialization happening.
This deprecated implementation is a long-standing problem for NiFi Registry. I will take a closer look, but I think it will probably require a more substantial refactor.
There was a problem hiding this comment.
Thanks @pvillard31 and @exceptionfactory. I'll take a look at the Registry changes required to support this dependency upgrade.
There was a problem hiding this comment.
I dug up this old nifi-registry PR that shows the original design to make authentication a framework extension point (via the IdentityProvider API) and how to bridge the configured NiFi Registry IdentityProvider into Spring Security.
This worked using the AuthenticationProvider interface in older versions of Spring Security, which looks like it has been deprecated, and the implicit call is no longer happening. I'm looking into refactoring the original extension point to work with the newer version of Spring Security
There was a problem hiding this comment.
@pvillard31 @exceptionfactory After some more research, it looks like the Spring Security AuthenticationProvider is not deprecated, it has only been discussed: spring-projects/spring-security#11428
The AuthenticationManager we use in NiFi Registry the delegates to one or more AuthenticationProviders is still valid, but it is no longer being invoked implicitly. @pvillard31's approach to invoke it directly from the NiFi Registry IdentityFilter is correct. I proposed some small changes here:
Note, one important functional change that I made is not to rethrow the authentication exception in the IdentityFilter. This will result in HTTP 500 errors due to unmapped exception handing in the servlet. I tested this by passing an invalid bearer token. Instead, my code will allow the request to continue without an authenticated authentication object in the ServletContext, resulting in our configured http401AuthenticationEntryPoint returning a 401 response.
I think this is good for Spring Security 7 for as long as we need it to function this way. I think at this point, the IdentityProvider extension point could be removed, along with some of the included providers (Kerberos, etc) and we could simplify Registry by allowing for configuring OIDC (or similar) to integrate with an external IdentityProvider without allowing for extensibility for a custom identity provider. But I think that should be out of scope for now as it requires its own discussion.
exceptionfactory
left a comment
exceptionfactory
left a comment
There was a problem hiding this comment.
Thanks for the work on this migration @pvillard31! The initial review looks good with the exception of the questions related to the IdentityFilter already noted. Thanks @kevdoran for offering to evaluate the options.
3 participants
Summary
NIFI-15221 - Major upgrade to Spring 7
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000Pull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation