search

spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.48k stars 5.77k forks source link

WebAuthn support #6842

Closed ynojima closed 2 years ago

ynojima commented 5 years ago

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

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.

cd <project root dir>
./gradlew spring-security-samples-javaconfig-webauthn-spa:bootRun

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.

ynojima commented 5 years ago

Hello @rwinch, just a gentle ping to see if this PR needs any more polish or anything.

ynojima commented 5 years ago

Hello @rwinch, is there anything I can do for you to move things forward?

ynojima commented 4 years ago

@rwinch One more ping... Could you take a look at this please?

rwinch commented 4 years ago

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.

ynojima commented 4 years ago

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. If you have other concern, please let me know. I'll fix it.

ynojima commented 4 years ago

@rwinch I rewrote the sample application without JS framework and simplified. Could you review it?

rwinch commented 4 years ago

Thanks for the updates. I'd still like to see the sample simplified significantly. I'd like the sample to demonstrate the minimum amount of work that a user needs to do to use the multifactor authentication. Right now that work is over 50 files which seems a bit much for hello world. Can you please try and find some ways to minimize this.

Keep in mind less is more, so I don't want to see a lot of files in Spring Security's core libraries either.

ynojima commented 4 years ago

@rwinch Now the sample application is reduced to 19 files, 1463 lines. Could you review it again?

ynojima commented 4 years ago

@rwinch One more ping... Could you take a look at this please?

ynojima commented 4 years ago

Thank you for your review. I'm ok to iterate with smaller goals, but it is not possible to create a separate self contained application because it is inevitable to make code change to spring-security-core to realize two step authentication. Is it ok for you to keep working on this pull request? I'll make change to this pull request to reduce feature to highlight the core MFA logic.

ynojima commented 4 years ago

@rwinch I simplified the code base by dropping single factor authentication support. Could you review again?

rwinch commented 4 years ago

While WebAuthn can be done along with passwords, it is meant to work without passwords. For that reason, I think it makes sense to decouple WebAuthn from UserDetails. Instead, of WebAuthnUserDetails extending UserDetails and creating a WebAuthnUserDetailsService that extends UserDetailsService I think we should separate the two APIs. We would create WebAuthnAuthenticatorService which would be able to find and save the WebAuthn by an identifier. I don't think we need to have the other methods or arguments, but please correct me if I am wrong here.

I think we should remove the custom implementations of the Spring Security APIs in the samples and instead rely on the default implementations. We want the code to be as simple as possible for users to follow in a sample rather than showing all the bells and whistles. This means that we should have a simple in memory implementation of the service for storing WebAuthn information provided by spring-security-webauthn.

Spring Security APIs cannot leak implementation details of the underylying library. The APIs need to be exposed via interfaces that do not expose underlying library classes in arguments or return types. One place this needs changed is OptionsProvider which exposes Challenge.

I'm not sure that WebAuthnAuthenticator is even needed as I don't see any references to it except in the tests. If we do need it, then WebAuthnAuthenticator cannot be a public API without implementing a Spring Security specific interface. Right now this API is tied to webauthn4j which is not ideal.

Proposal

More generally, I'd like us to take a step back and try to figure out exactly what the API looks like. I'd propose we have two main interfaces.

WebAuthnChallengeRepository

This API looks a lot like what is already there for ChallengeRepository. The main difference is that we cannot use the Challenge that is provided by webauthn4j.

WebAuthnService

This API just saves the values given to it. It does not perform any validation. The rational is that we want to allow users to be able to update the storage mechanism without needing to know the validation logic. It would not rely on the HttpServletRequest. The methods would be

WebAuthnAuthenticationManager

We would then create a WebAuthnAuthenticationManager interface. The implementation would use WebAuthnService and perform any necessary validation. It would not rely on the HttpServletRequest. The methods would be:

WebAuthnAuthenticationProvider

There would be a WebAuthnAuthenticationProvider implementation of AuthenticationProvider. This would use WebAuthnAuthenticationManager to verify the values passed in.

Sample

I'd really like the sample to be as bare bones as possible. We should only use JavaScript where absolutely necessary. It should not use JavaScript to submit the forms. It can use it to populate the WebAuthn properties, but that should be it.

ynojima commented 4 years ago

Thank you for showing the direction. I'll look into and start rewriting the code!

ynojima commented 4 years ago

While WebAuthn can be done along with passwords, it is meant to work without passwords. For that reason, I think it makes sense to decouple WebAuthn from UserDetails. Instead, of WebAuthnUserDetails extending UserDetails and creating a WebAuthnUserDetailsService that extends UserDetailsService I think we should separate the two APIs. We would create WebAuthnAuthenticatorService which would be able to find and save the WebAuthn by an identifier.

I see. I'll decouple these interfaces.

I think we should remove the custom implementations of the Spring Security APIs in the samples and instead rely on the default implementations.

got it. I'll move the in-memory implementation from the sample to the library code.

Spring Security APIs cannot leak implementation details of the underylying library. The APIs need to be exposed via interfaces that do not expose underlying library classes in arguments or return types. One place this needs changed is OptionsProvider which exposes Challenge.

I understand your concern, but not only challenge but also many webauthn4j types are exposed to curve WebAuthn types. (ex. AttestationOptions and AssertionOptions exposes PublicKeyCredentialRpEntity, PublicKeyCredentialUserEntity, etc.) Without these types, we need to re-implement and maintain all these types.

TYsewyn commented 4 years ago

First and foremost, awesome job on the new functionality! I was wondering if you're planning on adding reactive support to this? It would be a great addition for applications which are built on the reactive stack. Happy to help if needed! 😄 I was also wondering if we should call it "WebAuthn", since you're implementing the server side of the FIDO2 protocol. WebAuthn is the name of the browser JS API within the FIDO2 protocol. Using the newly added API we shouldn't need to use WebAuthn on the client side, we could use anything really as long as we stick to the API. 🙂

ynojima commented 4 years ago

Hello TYsewyn, Thank you for your comment. I'm interested in adding reactive support, but it is out of first scope. I need to fix servlet-based API design first.

I was also wondering if we should call it "WebAuthn", since you're implementing the server side of the FIDO2 protocol.

W3C WebAuthn specification defines not only JS API but also how to verify WebAuthn attestation/assertion on server side. https://www.w3.org/TR/webauthn/#rp-operations

It is not uncommon to use the keyword WebAuthn for server-side implementations. https://github.com/duo-labs/webauthn https://github.com/cedarcode/webauthn-ruby https://github.com/web-auth/webauthn-framework

In addition, I suppose the keyword WebAuthn is more recognized by users than the keyword FIDO.

@rwinch, how do you think?

rwinch commented 4 years ago

I think it makes sense to use WebAuthn as this specific implementation of 2 factor authentication. More generally, we want to ensure that we have thought through how 2 factor authentication works and then provide WebAuthn support to enable it

ynojima commented 4 years ago

@rwinch, I've updated the pull-request to address the points you indicated. This still needs polishment, more test cases, but I'd like to hear your ideas about the class design.

Changes

Decoupling WebAuthnUserDetails from UserDetails

WebAuthnUserDetails is decoupled from UserDetails to support password-less user. To support password + WebAuthn use-case, WebAuthnAndPasswordUserDetails, which inherits WebAuthnUserDetails nad UserDetails is added.

Default implementation

InMemoryWebAuthnAndPasswordUserDetailsManager is add as default implementation for WebAuthnAndPasswordUserDetailsService. WebAuthnAndPasswordUser and WebAuthnAuthenticator is now used in the sample application too. Now no custom implementations of the Spring Security APIs exists in the sample application.

Hide WebAuthn4J types from API surface

To hide WebAuthn4J types from API surface, following new data types are introduced:

OptionsEndPointFilter(s) are removed as it rely on WebAuthn4J types heavily. Instead of it, WebAuthnOptionWebHelper is introduced to provide chanllenge and credentialId(s) to WebMVC controllers.

WebAuthnManager is introduced to abstract WebAuthn verification logics. WebAuthn4JWebAuthnManager implements it. If neccesary, underlying library can be switched by replacing the concrete class. ( You proposed a name WebAuthnAuthenticationManager, but it looks too similar to AuthenticationManager, which Spring Security already use for other purpose.)

Sample

Reduced JavaScript code to fetch WebAuthn options.

ankurpathak commented 4 years ago

@rwinch @ynojima Can we have faster work on this. Spring Security currently no way to login without password. Login with OTP, Finger Print, Retina Scan, Face Recognization, QR are all used in Modern Web Apps and we neeed to move with time. If time is issue please point here, so community can come in and take part in development to move it fast.

ynojima commented 4 years ago

@rwinch I've updated the pull-request. Could you review again?

ynojima commented 4 years ago

@rwinch One more ping... Could you take a look at this please?

ynojima commented 4 years ago

@rwinch friendly ping, see above

ynojima commented 4 years ago

@jgrandja, @jzheaux, @eleftherias

Sorry for bothering you, but I cound't receive any response from @rwinch for a month on this issue. Could you contact him? I suppose this is awaited by many people (see emoji(s) on this issue), and worth investing time.

ynojima commented 4 years ago

It's a shame that I couldn't get response from you for 2 weeks. I have no idea how to move things forward. Any actions from anyone are welcome.

😕

TYsewyn commented 4 years ago

Hi @ynojima. Please bear with us. There is a lot going on at this moment. Spring 5.2 had to be released, and all other projects following after the framework are being released as we speak. (see https://spring.io/blog) SpringOnePlatform is also taking place next week where a lot of the maintainers and contributors are giving talks, which also have/had to be prepared. Whenever there is time, I'm sure they will come back to you! 🙂

jgrandja commented 4 years ago

@ynojima Apologies for the delay. The team has been working very hard on our 5.2.0 GA release that came out yesterday. We have limited resources on the team and there is only so much we can do with all the work we have on hand. We prioritize tasks based on what's going in for next release. We were not planning on bringing WebAuthn support into 5.2.0 and therefore this was lower priority.

Now that 5.2.0 is out we can shift our priorities and start looking at what will go into 5.3.0. We please ask the community to be patient (/cc @ankurpathak) as we are doing our absolute best to get in features that our users need.

SpringOne Platform is next week so we're busy preparing for that as well. You can expect a response from a member on the team the week of Oct 14 (after SpringOne).

Thanks for your patience.

ynojima commented 4 years ago

@jgrandja, @TYsewyn, Thank you for your response and congratulations for 5.2.0 GA release. I just worried about no response. I understand your circumstances and priorities. please review when you have time. 🙂

rwinch commented 4 years ago

@ynojima

Thank you for all your effort on MFA. I apologize for not being clear up front. I'm still quite overwhelmed reviewing a pull request that includes 100+ files. The size of the PR makes it challenging for me to review and slow to give you feedback.

In order to proceed, we need to start over with a more iterative approach. I understand that starting over is frustrating, but it will allow us to move much faster. Additionally, I suspect you will still be able to leverage much of the work you have already done so I'd keep it around.

Here is how we need to proceed:

Thanks again for all your work on MFA. I'm looking forward to tackling MFA with you.

ynojima commented 4 years ago

@rwinch

For now all of the code should be self contained within the sample application. Pretend (for now) that you are not allowed to modify the Spring Security code base

That's imsossible. I have not idea how to implement MFA without modifying spring-security core codebase. spring-security doesn't have any way to represent a user who have provided first authentication factor but not provided second factor (in the middle of authentication steps). That's why I send this pull-request. At least this patch ( https://github.com/spring-projects/spring-security/commit/f5859e9c6d6c0fc6b726d17064bb65dbfe5870a2 ) is needed. If there is a way, would you propose a sample code?

This means the sample should not have any new interface or domain objects. I'd expect that the number of .java classes be less than 10.

It's also impossible. WebAuthn is not so simple. I added five commits (https://github.com/spring-projects/spring-security/pull/6842/commits/be922536a38d00262e2e96266367cdad31976108, https://github.com/spring-projects/spring-security/pull/6842/commits/b430a7166b5358266d4fe66efdffc8fbb2e89947, https://github.com/spring-projects/spring-security/pull/6842/commits/ca075724f4676900746220793d547083a3daa136, https://github.com/spring-projects/spring-security/pull/6842/commits/c9411eb6a88c5335e0803def6a6fc559c7537089, https://github.com/spring-projects/spring-security/pull/6842/commits/ba25a7e79792e20286c1126b0dfd5be761292d14) 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 need to focus on smaller part, could we focus on MFA patch ( https://github.com/spring-projects/spring-security/commit/f5859e9c6d6c0fc6b726d17064bb65dbfe5870a2 ) review first?

rwinch commented 4 years ago

That's imsossible. I have not idea how to implement MFA without modifying spring-security core codebase. spring-security doesn't have any way to represent a user who have provided first authentication factor but not provided second factor (in the middle of authentication steps).

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:

ynojima commented 4 years ago

Without a framework(spring-security), it cannot be simple. much larger number of classes are needed compareing to current pull-request.

rwinch commented 4 years ago

This seems surprising to me that it cannot be simple. Isn't that what the library should do...make it simple to do?

rwinch commented 4 years ago

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

ynojima commented 4 years ago

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. Without Authentication framework, application code need to cover it. That what I mean to say.

rwinch commented 4 years ago

Without Authentication framework, application code need to cover it.

That makes sense, but it still seems like this can be done in very little code. Does the sample I provided you help?

ynojima commented 4 years ago

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) 2．The server return the credentialId (https://www.w3.org/TR/2019/REC-webauthn-1-20190304/#credential-id) of the user identified in #1 to the front end

  1. Call WebAuthn JS API in the frontend to generate Assertion. (You must pass credentialId as one of the parameters of WebAuthn JS API)
  2. Send the assertion to server and authenticate (Authentication step 2)

In the Yubico demo, step 1 is omitted. How to implement Step 1 is the key point of how to implement WebAuthn in Spring Security. It greatly affects the design. Without that, size of code decreases, but it won't be an effective design discussion.

Moreover, Yubico's webauthn-server-demo contains 8,597 lines in /src/main directory. It's not small sample application.

rwinch commented 4 years ago

@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.

rwinch commented 4 years ago

@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.

ynojima commented 4 years ago

Thank you for showing me an example. It is simpler than a thing I thought I need to implement. I'm looking forward to see your next update.

rwinch commented 4 years ago

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.

rwinch commented 4 years ago

I have cleaned this up a bit more.

Next up is to introduce domain objects and APIs specific to Spring Security.

ynojima commented 4 years ago

Any update on this issue? If you cannot take time to implement this feature by yourself for now, let me try it again based on your PoC (https://github.com/rwinch/spring-security-webauthn).

rwinch commented 4 years ago

@ynojima Please go ahead and try and work on it based on the sample I have

ynojima commented 4 years ago

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. https://github.com/rwinch/spring-security-webauthn/pull/2

You wrote "Next up is to introduce domain objects and APIs specific to Spring Security". Could you elaborate it?

rwinch commented 4 years ago

I merged the PR.

You wrote "Next up is to introduce domain objects and APIs specific to Spring Security". Could you elaborate it?

It's been a while since I've looked at this. Perhaps start with finding things to fix in the comments. For example:

https://github.com/rwinch/spring-security-webauthn/blob/8dde03044ae183ba6b347c9ed01fc0bf4292ec13/src/main/java/org/springframework/security/web/webauthn/WebAuthnManager.java#L36-L39

https://github.com/rwinch/spring-security-webauthn/blob/8dde03044ae183ba6b347c9ed01fc0bf4292ec13/src/main/java/org/springframework/security/web/webauthn/WebAuthnManager.java#L57

I'd suggest on keeping changes small (i.e. a single change at a time) so they are easy to review.

rwinch commented 2 years ago

Closing as progress has stalled. The issue remains to track the desire for the feature gh-5238