Skip to content

Make a foundation for multi-factor(step) authentication including WebAuthn #5665

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

Closed

Conversation

ynojima
Copy link
Contributor

@ynojima ynojima commented Aug 14, 2018

This patch is to make a foundation for multi-factor(step) authentication including WebAuthn (#5238).

Changes

* no interface is changed

  • Add MultifactorAuthenticationToken to represent a user in the middle of multi factor(step) authentication process
  • Add MFATokenEvaluator/MFATokenEvaluatorImpl for Authentication type check
  • Make ExceptionTranslationFilter, AuthenticationTrustResolverImpl, and HttpSessionSecurityContextRepository use MFATokenEvaluator to support multi-factor authentication
  • Add MultiFactorAuthenticationProvider, which authenticates a user by delegating to another AuthenticationProvider and generates MultifactorAuthenticationToken

spring-security-webauthn is a concrete user facing code using this patch.
If possible, I'd like to send whole spring-security-webauthn as a pull-request to spring-security now, but
I understand the spring's backward compatibility policy, WebAuthn specification is still updated after the declaration of Candidate Recomendation phase, and browser implementation is still on going. It is not the time.

Meanwhile, spring-security-webauthn requires user feedbacks, and patching to spring-security core is a big roadblocks for users to try spring-security-webauthn.
I extracted minimal changes to spring-security core into this patch. and I suppose future changes to WebAuthn specification will not affect this.

to indicate the state of principal which passed first step of multi step
(factor) authentication.
@ynojima ynojima force-pushed the backward-compatible-mfa-support branch 2 times, most recently from 06fa2b4 to 703dab7 Compare August 15, 2018 09:32
@ynojima ynojima force-pushed the backward-compatible-mfa-support branch from 703dab7 to d1bfb46 Compare August 15, 2018 09:54
@ynojima
Copy link
Contributor Author

ynojima commented Aug 22, 2018

Hi @rwinch, just a gentle ping to see if this PR needs any more polish or anything.
Are you busy on releasing Spring Security 5.1?

@rwinch
Copy link
Member

rwinch commented Aug 22, 2018

@ynojima Thanks for the nudge. Among other things, Spring Security 5.1 is taking a lot of my time. I have added reviewing this to my queue and will get back to you.

NOTE:

I have not looked over the code, but one thing that comes to mind is that I really look for to ensure that we provide a real feature to end users vs just infrastructure changes. This is especially true with large changes where it is hard to anticipate what the infrastructure change needs to be.

So if we have something that is providing a foundation for multifactor authentication, I want code that also can use that foundation in a real scenario ready to merge too. In these cases there will be more than one commit, but I will very rarely merge something that is large without seeing all the moving parts together. The reason is that we really want to ensure that all code is going to provide value to our end users.

@ynojima
Copy link
Contributor Author

ynojima commented Aug 22, 2018

Thank you for your reply!

I want code that also can use that foundation in a real scenario ready to merge too.

spring-security-webauthn is a consumer of the foundation classes in a real scenario. It also has a sample application, but sad to say, it is not ready to be merged into spring-security as browser (ex. Chrome) WebAuthn implementation is still on going.
There is a room I need to change the behavior of spring-security-webauthn to fit for browser implementation, but I assume the foundation classes in this pull-request will not be affected.
These foundation classes are only used for FIDO-U2F token support, and FIDO-U2F token is already supported by WebAuthn implementation of Chrome, Firefox, and Edge, I've confirmed the foundation classes works with these browsers.

I don't mind if you don't merge this patch at this moment (I'll be more happy if you merge it :) ),
I really appriciate if you review the the direction of this patch and spring-security-webauthn again.

Thanks!

@ynojima ynojima mentioned this pull request May 5, 2019
@ynojima
Copy link
Contributor Author

ynojima commented May 5, 2019

As I opened a new pull-request #6842 with wider scopes (concrete use-case), I close this pull-request.

@ynojima ynojima closed this May 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants