[Security] Login link: Encrypt username #39275
Conversation
|
Do we need to provide a layer that accepts the old links? |
|
hi @Nyholm I was reviewing your PR and found a couple of potencial bugs and I took the liberty to propose you a PR on your repo, you can find it at Nyholm#10 for me anyway, +1 to use Sodium to encrypt the full query string, in order to have links like |
|
Thank you. I agree with your changes. I'll do some more work on this PR to make sure we can do this change and still keep BC. |
|
@Nyholm glad I helped in some way :) I'm looking into the tests, because it should have been caught I guess. Feel free to ping me if you want me to do something |
|
Give me 30 minutes to push some new changes. I would be happy to hear your opinions about them. |
Nyholm
force-pushed
the
encrypt-username
branch
from
52b8e57 to
66c6a44
Compare
src/Symfony/Component/Security/Http/LoginLink/LoginLinkHandler.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/LoginLink/LoginLinkHandler.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Encryption/SymmetricEncryption.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Encryption/SymmetricEncryption.php
Outdated
Show resolved
Hide resolved
|
@Nyholm it's taking shape, let me know if you want to delegate implementing tests, cheers |
src/Symfony/Component/Security/Core/Encryption/SymmetricEncryption.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Exception/DecryptionException.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Exception/MalformedCipherException.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/LoginLink/LoginLinkHandler.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Encryption/SymmetricEncryption.php
Outdated
Show resolved
Hide resolved
| // TODO make sure we dont use a static salt. | ||
| // TODO store salt some how between requests | ||
| $salt = "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f"; | ||
|
|
||
| return KeyFactory::deriveEncryptionKey(new HiddenString($this->secret), $salt, KeyFactory::MODERATE); |
There was a problem hiding this comment.
I need help to figure this out. Using a static salt is not a good idea.
The salt also needs to be kept secret. We must store the salt somehow.
There was a problem hiding this comment.
As a salt is merely used to prevent that a same value would always produce the same hash/key. Would it make sense to embed with the message? Encryption uses an Initialization Vector for a similar purpose, and that is part of the message as well.
Maybe you can ping the creator behind Halite for some advice?
|
I removed all encryption stuff to rely on a third party package instead. I moved the encryption stuff to #39344 |
I don't think that's a good idea, better to request a new link to than to support an old "insecure" link. |
|
Hi @Nyholm! What is the status here? It seems like it has sort of stalled, although the changes seem to be complete. Is this ready to vote/merge? |
11 participants
The current implementation of a "magic link" uses the following query parameters: "hash", "user" and "expires". This is fine but it leaves the username exposed to the world. It could arguable be fine to send an url like this to foo@example.com:
However, if the
UserInterface::getUsername()does not return an email, but an other id, like a auth0 id. Im not sure it is a good idea to show publicly.This PR uses sodium to do a symmetric encryption of the username.
Potential improvements: