-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Add Multi-factor Authentication Support #17775
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
d230b08
to
a399b9f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @jzheaux! I've provided feedback inline.
@@ -263,4 +336,128 @@ private RequestCache getRequestCache(H http) { | |||
return new HttpSessionRequestCache(); | |||
} | |||
|
|||
private static final class AuthenticationFactorDelegatingAuthenticationEntryPoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we can delete AuthenticationFactorDelegatingAuthenticationEntryPoint
because we don't need to control the log in page based upon the factors.
@@ -225,4 +235,28 @@ private <C> C getSharedOrBean(H http, Class<C> type) { | |||
return context.getBeanProvider(type).getIfUnique(); | |||
} | |||
|
|||
private static final class AuthorityGrantingAuthenticationProvider implements AuthenticationProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be removed in favor of PreAuthenticatedAuthenticationProvider
adding the additional factor? If the factor needs to vary, perhaps allow injecting it into PreAuthenticatedAuthenticationProvider
.
@@ -165,6 +185,26 @@ public ExceptionHandlingConfigurer<H> defaultAuthenticationEntryPointFor(Authent | |||
return this; | |||
} | |||
|
|||
public ExceptionHandlingConfigurer<H> defaultAuthenticationEntryPointFor(AuthenticationEntryPoint entryPoint, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this method. It is only used once and the value of preferredMatcher is AnyRequestMatcher
. If users need an AuthenticationEntryPoint
that can delegate by RequestMatcher
, then they can inject DelegatingAuthenticationEntryPoint
.
This should also allow changing the entryPoints
member variable to be of type Map<String, AuthenticationEntryPoint>
@@ -75,6 +93,8 @@ public final class ExceptionHandlingConfigurer<H extends HttpSecurityBuilder<H>> | |||
|
|||
private LinkedHashMap<RequestMatcher, AccessDeniedHandler> defaultDeniedHandlerMappings = new LinkedHashMap<>(); | |||
|
|||
private Map<String, LinkedHashMap<RequestMatcher, AuthenticationEntryPoint>> entryPoints = new LinkedHashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try renaming this to something that helps describe what it does. Something like missingAuthorityToEntryPoint
makes it clear what the key is.
NOTE: They type can be simplified to Map<String, AuthenticationEntryPoint>
as described in this comment.
@@ -391,4 +410,52 @@ public ApplicationContext getContext() { | |||
return this.context; | |||
} | |||
|
|||
private static final class PostAuthenticationEntryPoint implements AuthenticationEntryPoint { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This introduces a CSRF attack that could lead to leaking the OTT through social engineering. Consider a user who is authenticated, but who has not performed MFA with OTT. They then go to evil.example.com which performs a request to a resource that requires OTT. This will automatically submit a token to the user which evil.example.com could say we sent you a OTT please paste it in this form on evil.example.com
What's more is there is no controls in place for how many OTT are created and submitted. I think that we want a confirmation page that looks much like the login page, but does not allow the username to be changed.
} | ||
|
||
private Collection<GrantedAuthority> authorizationRequest(Exception ex) { | ||
Throwable[] chain = this.throwableAnalyzer.determineCauseChain(ex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is costly. If we keep the ThrowableAnalyzer
logic in this class, we should do an instanceof check on the ex
argument first. Perhaps we should have a way for this to be built into ThrowableAnalyzer
if it isn't there already.
|
||
} | ||
|
||
private static final class AuthenticationFactorDelegatingAccessDeniedHandler implements AccessDeniedHandler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be extracted into a class in spring-security-web. I believe it makes sense as a sibling of AccessDeniedHandler
, but change for tangles due to authentication APIs in here (access historically has had AuthenticationManager
in it though so I think this works).
UsernamePasswordAuthenticationToken result = UsernamePasswordAuthenticationToken | ||
.authenticated(principal, authentication.getCredentials(), | ||
this.authoritiesMapper.mapAuthorities(user.getAuthorities())) | ||
.toBuilder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid creating twoUsernamePasswordAuthentiationToken
instances?
this.authoritiesMapper.mapAuthorities(user.getAuthorities())); | ||
UsernamePasswordAuthenticationToken result = UsernamePasswordAuthenticationToken | ||
.authenticated(user, password, this.authoritiesMapper.mapAuthorities(user.getAuthorities())) | ||
.toBuilder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid creating two instances of UsernamePasswordAuthenticationToken
?
return this.defaults; | ||
} | ||
|
||
private Collection<GrantedAuthority> authorizationRequest(Exception ex) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to something like missingAuthorities
Related to spring-projects/spring-security-samples#351
Implement N authentication factors and they will be required in the order that they are declared:
This will ask for a username/password first and a one-time token second. Thereafter, the user will be considered sufficiently authenticated.
Note that you can also publish an
AuthorizationManagerFactory<Object>
bean that checks forFACTOR_PASSWORD
andFACTOR_OTT
; however, this has not been added to this PR.You can also specify a custom action to perform when a given factor is missing:
Note that authentication factors already integrate with
defaultAuthenticationEntryPointFor
in this PR. The above is needed for WebAuthn since it doesn't expose a custom entry point page in its DSL.