Skip to content
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

Authentication Scheme #134

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Authentication Scheme #134

wants to merge 5 commits into from

Conversation

erikzhang
Copy link
Member

No description provided.

@igormcoelho
Copy link

Amazing path @erikzhang ! It was less than a month ago that @vncoelho and I got a discussion on the matter. Structure looks nice, it would be interesting to provide some example perhaps, with Alice and Bob, and maybe sone contextualization over existing OAuth 2. Neo can certainly be such pioneer, specially with oracle technology.

@erikzhang
Copy link
Member Author

erikzhang commented Apr 17, 2021

Any comment? @neo-project/everyone

shargon
shargon previously approved these changes Apr 17, 2021
Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good to me :)


A 64-bit unsigned integer used to identify this authentication.

The server should record the expiration time of the nonce and check it when the response is received.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires the authentication server to be stateful. That could be avoided by including a signature for the nonce and timestamp in the request (and requiring that the client provide the signature in the response).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the server is stateless, how to identify the clients?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

State will definitely be needed to provide any useful authorization (e.g. a table mapping from Neo address to roles and/or other application-specific identity data). I'm just showing an opportunity to remove a database table and to avoid a database write on the initial authentication request (which could become a vector for unauthenticated denial-of-service).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the server does not record the nonce, it will become reusable. We need a mechanism to ensure that when a client is successfully authenticated, the nonce is immediately invalidated.

Copy link
Member

@djnicholson djnicholson Apr 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to mention here that the server is required to prevent nonce re-use:

When the server receives the response, it should check whether the nonce has expired. If the nonce has expired or not exists, the authentication fails.

It's currently not clear that this is required and a server implementor might choose to delete a nonce after successful use rather than invalidate it. I'm also curious as to why preventing nonce re-use is required (what security property breaks if the server doesn't do that?)

I'd still lean towards having the nonce invalidation happen post-authentication though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, I log in to a website by scanning a QR code. If the nonce can be reused, the second person who scans the code will kick my login.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like the nonce is being used as a session ID? If that is the case there is a vulnerability: Anyone who has observed the QR code can later hijack the session (it wouldn't be safe to use this flow in a room with security cameras, for example).

To mitigate this, the nonce should be swapped for a session ID and this swap should only be allowed to happen once (note that there is still a race condition where an attacker can steal the session, though--to prevent that you need some sort of communication channel from the phone back to the browser).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nonce is a one-time random number. There should be a nonce to sessionId mapping table on the server side.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires the authentication server to be stateful.

This is an interesting point @djnicholson, I need to study more on authentication. But perhaps, one way of not storing these nonces is to have some good pseudo-random algorithm, such as mersenne twister on server, starting with some secret seed, and then strongly hash mersenne twister outputs (via SHA-256, for example) to generate the nonce. The good thing is that, as long as some timestamp is involved, server can re-execute seeding process to find the nonce, instead of storing it. The bad thing is that, as soon as seed is leaked on server, nonces could be predicted.
Is it Neo servers or other servers that require being stateful?

@chenzhitong
Copy link
Member

Rename nonce to UUID?

@chenzhitong
Copy link
Member

chenzhitong commented Apr 20, 2021

To prevent site A from using site B's QR code to trick you into logging in. We should add the website name to the Challenge payload. When you scan the code, you will be prompted with "You will be logged in to **** website". It's like having the website name in the SMS verification code.

@chenzhitong
Copy link
Member

Do we need to add an expiration timestamp to the Challenge payload.


#After the wallet recognizes the QR code, a pop-up window asks the user to confirm login.

#If the user cancels the login, the authentication process ends. Otherwise, go to the next step.
Copy link
Member

@chenzhitong chenzhitong Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How the server gets the authentication refused/cancel message? Add State in Response data, include scaned, confirmed, refused, to optimize user experience?

Copy link
Member Author

@erikzhang erikzhang Apr 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the authentication is cancelled, the server won't receive the response payload. So nonce has an expiration time, and it is recommended to be 5 minutes.

@djnicholson
Copy link
Member

To prevent site A from using site B's QR code to trick you into logging in. We should add the website name to the Challenge payload. When you scan the code, you will be prompted with "You will be logged in to **** website". It's like having the website name in the SMS verification code.

The challenge payload will also need to be signed by the website (or the attacking site can just modify it)

@erikzhang
Copy link
Member Author

Rename nonce to UUID?

It's not UUID. It is a one-time random number.

Do we need to add an expiration timestamp to the Challenge payload.

I don't think it's necessary.

@erikzhang
Copy link
Member Author

To prevent site A from using site B's QR code to trick you into logging in.

In QR mode, there is a callback field. The client should show the domain of the callback url to the user. And the user can verify whether the website they are logging in is the domain name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants