-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
WebAuthn support #6842
WebAuthn support #6842
Conversation
4b55da4
to
45aa9bc
Compare
Hello @rwinch, just a gentle ping to see if this PR needs any more polish or anything. |
Hello @rwinch, is there anything I can do for you to move things forward? |
e877b67
to
271180e
Compare
@rwinch One more ping... |
Thanks for the ping @ynojima! I'm sorry this somehow got lost in my notifications. My first reaction is that a pull request with over 300 files is going to take a very long time to merge. Additionally, I love that you have a sample included, but we should try to simplify it a bit. We do not want our users to need to understand Angular to follow the sample (what if they don't know JavaScript, what if they use another JS framework, etc?). We want the focus of the sample to be security not JavaScript. So I suggest we simplify the sample a lot. This will also help with the number of files in the pull request. Once you have that taken care of, I can start reviewing the code more easily. |
Thank you for response! I'll rewrite the sample application without JS framework. To implement the authenticator management feature, I thought writing with Angular would make code simple, but it is not good to require users to have knowledge of Angular. |
ece8b0c
to
c3c765a
Compare
That's imsossible. I have not idea how to implement MFA without modifying spring-security core codebase.
It's also impossible. WebAuthn is not so simple. I added five commits (be92253, b430a71, ca07572, c9411eb, ba25a7e) temporarily to reduce number of classes by removing test classes and exception class variation for review. These commits need to be reverted later, but I hope this helps your review. Now the number of files is 62. lines add is 4800+. I know this is still large, but there is no room to remove without damaging core logic. |
If you are not aware of a way to do it without modifying the Spring Security code, then let's start even smaller then. Do the same steps (continue to use Spring Boot), but do not use Spring Security. Here is how we need to proceed:
|
Without a framework(spring-security), it cannot be simple. much larger number of classes are needed compareing to current pull-request. |
This seems surprising to me that it cannot be simple. Isn't that what the library should do...make it simple to do? |
Here is something similar to what I had in mind https://github.com/Yubico/java-webauthn-server/tree/master/webauthn-server-demo There are 21 java files in it. The difference would be that we should leverage Spring Boot |
WebAuthn4J is independent from Web framework like Spring MVC, WebFlux, ServletAPI, etc. Bridging WebAuthn4J and Web framework is left to Authentication framework like Spring Security. |
That makes sense, but it still seems like this can be done in very little code. Does the sample I provided you help? |
I'm doubtful whether creating a sample application like Yubico's one promote discussions on how to implement MFA into Spring Security. This is because the Yubico demo does not implement two step authentication flow. Two-step authentication flow with WebAuthn (FIDO-U2F) is to be performed in this way: 1.Authenticate a user by first authentication factor (ex. password). (But the authentication must not be regarded as completed at this point.) (Authentication step 1) In the Yubico demo, step 1 is omitted. Moreover, Yubico's webauthn-server-demo contains 8,597 lines in |
@ynojima Thanks for the feedback. We have used this mechanism for providing features in countless features. I am in disbelief that we cannot come up with a minimal sample similar to as I have described. If we cannot provide a minimal sample, I'm worried we will end up with more code than the Spring Security team can maintain. I will spend some time in the next week to investigate further and see. |
@ynojima I put together a simple application that registers a new key and then uses it to authenticate https://github.com/rwinch/spring-security-webauthn This is certainly a work in progress, but it demos how to use webauth4j within a Spring Boot application. Next week I will work on making it so that it actually updates the security authentication. |
Thank you for showing me an example. |
This is still a work in progress, but I have updated the sample to provide a very simple integration with Spring Security. I also added a very simple README to describe the functionality. The next steps will be simplifying the setup for a user.
|
I have cleaned this up a bit more.
Next up is to introduce domain objects and APIs specific to Spring Security. |
Any update on this issue? |
@ynojima Please go ahead and try and work on it based on the sample I have |
As a first step, I've sent a pull request to your repository for updating WebAuthn4J, which has a breaking changes in 0.10.0.RELEASE. You wrote "Next up is to introduce domain objects and APIs specific to Spring Security". |
I merged the PR.
It's been a while since I've looked at this. Perhaps start with finding things to fix in the comments. For example: I'd suggest on keeping changes small (i.e. a single change at a time) so they are easy to review. |
Closing as progress has stalled. The issue remains to track the desire for the feature gh-5238 |
Issue: #5238
Previous PR: #5665
This pull request adds W3C Web Authentication specification support to Spring Security.
It is consisted by 3 parts.
Add MultifactorAuthenticationToken
Make a foundation for multi-factor(step) authentication including WebAuthn.
Changes
MultifactorAuthenticationToken
to represent a user in the middle of multi factor(step) authentication processMFATokenEvaluator
/MFATokenEvaluatorImpl
forAuthentication
type checkExceptionTranslationFilter
,AuthenticationTrustResolverImpl
, andHttpSessionSecurityContextRepository
useMFATokenEvaluator
to support multi-factor authenticationMultiFactorAuthenticationProvider
, which authenticates a user by delegating to anotherAuthenticationProvider
and generatesMultifactorAuthenticationToken
Implement W3C WebAuthentication specification
Adds Web Authentication specification support as
spring-security-webauthn
module.Nothing is changed in existing spring security modules.
Add WebAuthn sample application
It is a sample application demonstrates
spring-security-webauthn
module.Please run with this command.
Reference doc
Reference document is not included in this pull request, but the draft exists here: https://sharplab.github.io/spring-security-webauthn/en/
When the design is finailized after the pull request review, I'll rewrite it to fit Spring Security reference doc.
for reviewers
Sorry for the huge pull-request. LoC is increased by sample application to demonstrate concrete usecase.
As the previous commit is not corrected in the later commit, please read commit by commit.