-
Notifications
You must be signed in to change notification settings - Fork 16
Auth #1
base: master
Are you sure you want to change the base?
Conversation
@@ -20,3 +20,6 @@ config :demo, DemoWeb.Endpoint, | |||
|
|||
# Print only warnings and errors during test | |||
config :logger, level: :warn | |||
|
|||
# Only in tests, remove the complexity from the password encryption algorithm | |||
config :bcrypt_elixir, :log_rounds, 1 |
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.
We should support all three comeonin adapters. We should default to bcrypt except on Windows which should use pkbdf2. Each adapter will require a different line here.
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.
Argon2 is recommended here. Any reason not to use it as the default?
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.
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.
Yes, I am aware folks will have different opinions and takes on the password hashing, that's why I prefer to follow recommendations from OWASP and similar and, AFAIK, it is still bcrypt. If someone has any updated insights or data on that, I would love to hear. :)
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.
It seems that the recommendation is still bcrypt
, except in some cases, as stated in OWASP: Password Storage Cheat Sheet.
Bcrypt is the most widely supported of the algorithms and should be the default choice unless there are specific requirements for PBKDF2, or appropriate knowledge to tune Argon2.
The default work factor for Bcrypt is 10, and this should generally be raised to 12 unless operating on older or lower-powered systems.
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.
any thoughts on passwordless auth? it solves a lot of hassle in pass world like reset, change, forgot, keeps email as source of truth and solve password hashing issues
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.
any thoughts on passwordless auth? it solves a lot of hassle in pass world like reset, change, forgot, keeps email as source of truth and solve password hashing issues
In my opinion, passwordless auth should be avoided most of the time. I guess it depends on your user base, but in my experience, many people use their work email to sign up for services, even on non work related sites. Unless you can verify their identity some other way during sign up, that means they will lose access to their accounts when they change jobs, since you can't trust email update requests from any other email address. This is especially important if your project falls under GDPR.
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.
In my opinion, passwordless auth should be avoided most of the time. I guess it depends on your user base, but in my experience, many people use their work email to sign up for services, even on non work related sites. Unless you can verify their identity some other way during sign up, that means they will lose access to their accounts when they change jobs, since you can't trust email update requests from any other email address. This is especially important if your project falls under GDPR.
even in that cenario, its safer passwordless since the emails belongs to the company and that could be abused by bad actors, I manage and managed the email myself a couple of times, its usual to help someone to recover stuff left in company emails they dont own anymore, passwords dont solve this issue, specially for car rentals, and some other kind of stuff they get important purchase info by mail, the mistake is to use a email you dont own, and when you know is passwordless you think twice to use one you do not own.
I understand that a lot of work was put in this pr but if you think about future solutions of auth, passwordless is one of the most promising for a lot of reasons.
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.
Someone is welcome to implement mix phx.gen.passwordless
. :) This can be a great starting point.
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.
any opinions on https://github.com/zanderxyz/veil?
get "/users/confirm", UserConfirmationController, :new | ||
post "/users/confirm", UserConfirmationController, :create | ||
get "/users/confirm/:token", UserConfirmationController, :confirm | ||
end |
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.
Those routes we can inject at the bottom.
<ul> | ||
<li><a href="https://hexdocs.pm/phoenix/overview.html">Get Started</a></li> | ||
</ul> | ||
<%= render "_user_menu.html", assigns %> |
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.
Note how we generate the menu as a separate file. We can probably add this render automatically to the app layout (or root layout if using LV) after the <body>
tag.
end | ||
|
||
defp maybe_write_remember_me_cookie(conn, token, %{"remember_me" => "true"}) do | ||
put_resp_cookie(conn, @remember_me_cookie, token, sign: true, max_age: @max_age) |
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.
Would be nice if this could use the session cookie opts for attrtibutes like secure
or SameSite
.
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.
Ah, good point. Unfortunately there is no way to retrieve the session options downstream. We would need to add that first. 🤔 At least secure
should be automatically set whenever using https
but or SameSite
you are right. I will at least move these options to the top of the module to make it more visible.
Co-Authored-By: Aaron Renner <[email protected]>
Co-Authored-By: Aaron Renner <[email protected]>
Co-Authored-By: Aaron Renner <[email protected]>
Were there any thoughts about some simple hashing of passwords client-side
before sending them to server?
Is that a common security practice?
The primary motivation in here wouldn't be additional security, but
preventing cleartext passwords from leaking in transit - logged in some
file or appearing in some error report or crashdump.
You'd be using TLS / https though and if the hash leaked and you're
comparing the hash, then that's as good as a plain text password if it
leaks, no?
… |
To be honest, I'm not sure. But I know of some big authentication systems that employ it. There are some big, public examples of logging passwords accidentally:
I would say it's one of those things where the question is not "if", but "when".
To expand, there are two possible flows:
|
I can say that client side hashing (in addition to server side hashing, not as an alternative to) is a killer feature for me, and one of the reasons I was looking to a generator vs one of the existing authentication plugs was to simplify adding it myself. From a personal level there's just a lot of value to removing that level of trust from myself and any theoretical contributors to my projects. And there's a certain value to me to having any client user being able to verify that at least for this moment I'm not collecting a plain text version of their password in any form. I'll note that the obvious biggest downside of such a system is that it either needs an alternate code path or becomes a JavaScript requirement. This is pretty meaningless to me as I'm already using technologies that act as a JS requirement, but it very certainly won't be meaningless to everyone. |
I can say that client side hashing (in addition to server side hashing,
not as an alternative to) is a killer feature for me, and one of the
reasons I was looking to a generator vs one of the existing authentication
plugs was to simplify adding it myself.
So the flow is:
1. Client side hashes the plain text password
2. Client side sends the hash
3. Server side takes that hash and compares it to the hash in the database?
From a personal level there's just a lot of value to removing that level
of trust from myself and any theoretical contributors to my projects. And
there's a certain value to me to having any client user being able to
verify that at least for this moment I'm not collecting a plain text
version of their password in any form.
But something somewhere has to take that plain text password and hash it,
no? Isn't this just down to your logging?
Re. 3 above. A hash in transit which is then compared to the hash in the db
is just the same as a plaintext password. What am I missing? If a db is
compromised and all your doing is comparing the hash sent with the hash in
the db, then that's plaintext, no?
Thanks.
|
In addition to and not as an alternative to being very important. So flow is:
So if someone compromises the hash the client generates they can log into the server with it, because it is the plain text password. The difference is the server never knows the password that could be shared with other sites at all. It doesn't change the security profile or what should be expected from the site at all, it just means that if someone logs passwords they get only a password that works for the site, not a password that might work for their bank account. |
Ah, that makes sense now. Was missing step 3 AND the fact that even though
this *could* be my password for all my sites, because it's hashed client
side, the hash can NOT be used elsewhere.
Thanks.
… In addition to and not as an alternative to being very important. So flow
is:
1. Client side hashes the plain text password
2. Client side sends the hash (this hash basically IS the plain text
password to the site)
3. Server receives the hash and further hashes it, just like they
would hash a password normally.
4. Server side takes this password that was hashed both client side
and server side and compares it to the hash in the database?
So if someone compromises the hash the client generates they can log into
the server with it, because it is the plain text password. The difference
is the server never knows the password that could be shared with other
sites at all. It doesn't change the security profile or what should be
expected from the site at all, it just means that if someone logs
passwords, or sniffs passwords they get only a password that works for the
site, not a password that might work for their bank account.
|
Maybe a really silly comment, but you could just filter out the password parameter from your log files through Phoenix right? It already filters out any password by default (https://hexdocs.pm/phoenix/Phoenix.Logger.html#module-parameter-filtering). |
As previously mentioned, client side password hashing and in browser end to end encryption would break auth for browsers that have JavaScript disabled. That being said, if this is something that is a must have, I suggest taking a look at existing implementations. The Standard File section on Key Generation gives detailed instructions on taking a user inputted password |
Yes. The problem is that this process is manual and error-prone, not that it's not possible. Almost all frameworks have feature like this, and yet, leaks happen regularly. |
The steps I mentioned were left as simplified on purpose to explain the goals, but yes you're going to deal with giving the client a account unique salt somehow, and the client will need to know how many iterations to run as well (though the downsides of baking that into the system are less of an issue). How you get the salt to the client can be varied, especially since salt's aren't secret (though if you're nervous you can use a different one than the server uses). I would recommend avoiding doing something standards file did and putting something like email in salt creation, doing so means that to change the email you must replace the password in the DB, which whether or not is a feature will depend on the project. (should your admins or support be able to change an accounts email address, something that if abused could lead to giving a 3rd party access to the account) In the end you need a salt, there's no inherent need for the process of deriving that salt to be more contrived than the one you would use for normal passwords. You do need to either transmit it, transmit enough information to generate it, or face some very serious trade offs in allowing the client to know enough to generate it with no transmission (tying the salt to something like username or email). Whether or not this is something that adds too much complexity for too little gain for the generator is not a question for me. (though I will say that were it to be added, it should be optional, and likely shouldn't even be the default) I can only say that I think it's a good practice if you have JS already, and that I will do so for all my projects. I just straight don't implicitly trust my servers with other peoples passwords (or other forms of data if I can avoid it, CC numbers for instance), nor do I implicitly trust every person that is granted admin level privileges to my server, including the ones that work for the cloud services that host them. There's no need for me to place that level of responsibility on myself when I don't have to, because at the end of the day if my server leaks something, possibly through no fault of my own, I'm the one who'll have to deal with the legal ramifications. |
conn | ||
|> put_flash(:info, "Password updated successfully.") | ||
|> put_session(:user_return_to, Routes.user_settings_path(conn, :edit)) | ||
|> UserAuth.login_user(user) |
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.
I converted User Settings forms into a LiveView. How do I login the user from LiveView?
conn = fetch_cookies(conn, signed: [@remember_me_cookie]) | ||
|
||
if user_token = conn.cookies[@remember_me_cookie] do | ||
{user_token, put_session(conn, :user_token, user_token)} |
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.
In case the user token is retrieved via the remember me cookie, shouldn't a new remember me token/cookie be issued, and the old token invalidated?
The rationale is:
-
The remember me cookie should be single-use
-
The new remember me cookie would "reset" the expiration, so the login would expire after
@max_age
since the last session (@max_age
of inactivity), as opposed as after@max_age
since the last login, which is a more useful semantics.
Or am I missing something?
) | ||
end | ||
|
||
# Regardless of the outcome, show an impartial success/error message. |
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 show a vague message and not an explicit error message if the email isn't in the system? I've seen this pattern used but I've never understood why. If an attacker is trying to build a list of email addresses they can get that information by probing the sign up form.
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.
@gshaw yes, if you want to prevent user (email address) enumeration, you need to make sure that the sign up form (and the reset password request form) does not leak that information.
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.
@riverrun agreed. My question is: Since sign up does leak emails why take the UX hit on password reset?
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.
@gshaw Because you could make the sign up work in this way that it would not report an error if you are using an already existing email address and just prompt you to confirm your registration using the e-mail that was sent to you.
I abstract here from judging the UX of such a behavior.
UPDATE: See #1 (comment) and "### Enumeration attacks"
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.
@szymon-jez thanks for the requirements link. 👏 I'm going to assume that he is taking the stance that the end product is getting customized by all apps and he is providing a safe default with the understanding that if you want to prevent enumeration attacks you still have some work to do but whenever possible this code will help you out. At the same time it allows people who know they can't prevent enumeration attacks since they are using email as a username to make some simple changes to improve UX on things like the password reset form.
Ultimately it gets back to the point that auth is same but different for all apps. This approach gets you 80% but you are expected to tune it to competition. I think it's a great strategy and learning tool and I could see others variations of this to show alternative ideas.
end | ||
end | ||
|
||
defp maybe_store_return_to(%{method: "GET", request_path: request_path} = conn) do |
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.
So I have a LiveView like so.
@impl true
def mount(params, session, socket) do
if user = session["user_token"] && Accounts.get_user_by_session_token(session["user_token"]) do
{:ok, assign(socket, current_user: user)}
else
socket = socket
|> put_flash(:error, "Login first")
|> push_redirect(to: Routes.user_session_path(socket, :new))
{:ok, assign(socket, current_user: nil)}
end
end
what do you recommend to derive the request_path while calling the push_redirect to signin?
Co-authored-by: Aaron Renner <[email protected]>
Co-authored-by: Aaron Renner <[email protected]>
<%= checkbox f, :remember_me %> | ||
|
||
<div> | ||
<%= submit "Login" %> |
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.
I realise this is totally nit-picking, so feel free to ignore me, but... "login" is a noun, "log in" is the verb. So to me action-oriented UI messages and function names should be "log in"/log_in
. It's the sort of thing I notice but I know I'm a total pedant so 🤷
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.
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.
Good catch @jonleighton! We actually fixed this in our app later but I forgot to back-port it here. :)
This is awesome and represents a whole bunch of work I've done multiple times for new Phoenix applications. I might have missed this somewhere in the hundreds of comments, but have you considered namespacing the generated controllers, views, templates into something like I think it would be easier to understand what was generated, especially in an existing application, and lives in the spirit of the nice separation of concerns like Devise did. |
Earlier discussion https://github.com/dashbitco/mix_phx_gen_auth_demo/pull/1/files#r401883755 perhaps answers your question? |
Are there any considerations / best practise when using this with Liveview? As a POW user, we've got active discussions on how to integrate authenticated sessions with Liveview. So far the issues focus on the Liveview requests not interacting with a standard Plug flow, which checks and renews sessions. Therefore it's often good for the initial auth check but quickly becomes stale. You can see the POW issue here |
@morgz for us, we kept all of the authentication pages outside of LiveView, since they need to set cookies and do other things that we generally cannot do with LiveView. The only Then, for the LiveView integration, we have a module called MountHelpers that we include on all LiveViews. This module has a function called "assign_defaults" that we always call on mount. This "assign_defaults" function looks like this: def assign_defaults(socket, _params, session) do
socket
|> assign_new(:current_user, fn ->
MyApp.Accounts.get_user_by_session_token!(session["user_token"])
end)
end And that's it! |
Thanks @josevalim - So I believe the complexity in POW, comes from the logic which governs the TTLs of the session tokens. Therefore you can find yourself with a stale token after 15 mins because your request hasn't been through a Plug flow to renew the token. I believe @danschultzer includes such logic to invalidate stale/inactive sessions which, If I leave my session open on a public computer I'll no doubt appreciate. lib/demo/accounts/user_token.ex I'm assuming then that the session tokens are longer lived here? So if I had a 59 day session in LiveView which only passed through the plug once during login, I'll still be able to retrieve the user with the token? |
If the TTL is a concern, you can reduce how long a session is valid. Since we keep our session tokens in the database, you can extend their validity without emitting new cookies or without flowing through Plug. Just manipulate it accordingly in your |
I believe this PR has fulfilled its purpose, so I will go ahead and lock this discussion. Please give the |
This is a complete auth system on top of a brand new Phoenix app. It includes:
Sessions and tokens are kept in a separate table, which give users full server side control of when to expires sessions, tokens, etc. For more details, see the README in this PR.
EDIT I believe this PR has fulfilled its purpose, so I will go ahead and lock this discussion. Please give the
mix phx.gen.auth
generator by @aaronrenner a try, as we plan to eventually make it part of Phoenix. Thanks everyone for the feedback! 💯