-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
handling restricted session token for 2FA #5305
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
Conversation
@Etto91 great getting this started. I am curious why you chose to implement something completely custom, and not use a note/totp library like this https://github.com/guyht/notp I see multiple advantages in using a standard OTP protocol, From the ability to use an app like google Authenticator or Authy to the fact that it is a real standard. |
Also, I recommended a different approach for the session handling, in the issue, that I wish to see in this implementation. If this was unclear, let me know. |
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.
After looking at the code, I am very confused by the implementation. I really enjoy that you are taking on this feature, but as you are touching to security, we need to be extra careful.
A few advices:
- don’t reinvent a 2FA protocol, please use an existing one
- add tests, for everything
- do not over engineer
); | ||
} | ||
const sessionToken = req.info.sessionToken; | ||
if (!req.auth.isMaster) { |
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.
Why only the master user can do that? This does not make any sense.
if ( | ||
req.url === '/login/two-factor-validation' && | ||
info.sessionToken && | ||
info.masterKey !== req.config.masterKey |
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.
Wow! Big security flaw creating a master session when keys mismatch.
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.
obv big mistake
I’ll be closing this Pr as the implementation is very far from what was originally discussed and there is still a lot of work to get a functional 2FA implementation. Please refer to conventional implementations. |
@Etto91 I made a document here on the wiki in order to help with the implementation: https://github.com/parse-community/parse-server/wiki/Draft:-Parse---OTP |
what we thought was the possibility of using the preferred two-factor system. the procedure is: the first step normally log in the user to create the session, after which the developer can implement the two-factor system (google auth, sms, etc.). after verifying the user with any two-factor verification, on the server side, it is necessary to call the session validation. the master key must be used because otherwise it can be called from the user (we have passed a session token on login). in response we have a second session key that must be linked to requests to validate them. all this was done to create two authentication steps and to give the developer the opportunity to develop their second authentication step. I have a question for you. I wanted to ask you why do you prefer that authentication occurs in a single call with username password and otp? as soon as I have some time I try to start the steps you have written in the documentation |
This is completely not necessary with using TOTP.
google auth, sms etc... as just delivery methods for the OTP. This would be configured when the user configures the OTP for his account. I agree this is a 'nice to have' to be able to provide a simple OTP through sms, and yes you could send the OTP to the user's email account, those are all valid delivery strategies, but they should be baked in at registration AND at login, and for now, I do not believe they provide any value thinking about it because you NEED a valid TOTP authentication method to begin with.
In all cases, the underlying implementation is a TOTP. That the email or sms is sent is a detail that can be solved later on.
This is not necessary to create a session just for that. For example Apple implement this flow for iCloud login with OTP:
In all thoses steps an intermediate session is not necessary at all.
Because there is no technical or security reason to add an additional step through creating a temporary session? What guarantee does this provide? |
you're right. i will try to implement totp user setup as soon as possible. |
I am trying a working prototype on my side too. Let's see how your implementation is going :) |
Hey @Etto91 did you manage to complete this? |
@brianmwadime |
@flovilmart Ive been trawling the issues etc and see you are looking into this the most. Im looking to implement this functionality using speakeasy OTP library. Have you managed to get anywhere with an implementation ? or can you give me some pointers on how to get this integrated. |
It was 2 years ago, I don't have a copy of this project anymore |
Its more relevant now than ever, almost every site uses some kind of OTP, MFA etc. is this on the roadmap at all ? Can you give advice if I was to tackle it. My use case is simply to have google authenticator as an extra step after successful login |
@flovilmart is there any way we can have a chat about how you would like to see this implemented, Im willing to submit a PR for this feature as I need it myself but your knowledge of the parse internals and its session management would be really great to have. |
@REPTILEHAUS i don't maintain this project anymore, you can submit a Pr, I guess one of the maintainers will take care of it |
Our goal was to integrate Two Factor Authentication by generating two-factor session token and validating them.
The steps that I developed are:
Available options are the following:
During the login, if 2fa is active, we set expiration time based on firstSessionExpireTime option on the session object.
Login’s response has not changed.
/login/two-factor-validation
The API requires masterKey.
The API creates a token, encrypted with the “token” set on configuration options, updates the session expiration time, save the session, and returns the encrypted token.
X-Parse-Session-Two-Factor-Token
The token needs to be attached to every request.
It should be passed as X-Parse-Session-Two-Factor-Token in the header or as part of the body request as _SessionTwoFactorToken. We tested with REST API, and we made an integration for the JS SDK user (We can make a PR for this)
Auth Middleware update
On the auth middleware we have added a condition that checks if 2FA is required for the case. When is required, we verify the encrypted session token otherwise we return UnAuthorized.
This personally my first PR. I’m not sure the code is matching your ideas but its working. I need some help with the test creation and in updating faling tests.