Add form based authentication #4821
Conversation
stuartwdouglas
force-pushed
the
form-auth
branch
from
c7ed0c5 to
86ad410
Compare
| clustered HTTP session support. Instead the authentication information is stored in an encrypted cookie, which can | ||
| be read by all members of the cluster (provided they all share the same encryption key). | ||
|
|
||
| The encryption key can be set using the `auth.session.encryption-key` property, and it must be at least 16 characters |
There was a problem hiding this comment.
Very minor but should it be:
| The encryption key can be set using the `auth.session.encryption-key` property, and it must be at least 16 characters | |
| The encryption key can be set using the `quarkus.http.auth.session.encryption-key` property, and it must be at least 16 characters |
?
| if (System.currentTimeMillis() > expire) { | ||
| return null; | ||
| } | ||
| return new RestoreResult(result.substring(sep + 1), (System.currentTimeMillis() - expire) > 1000 * 60); //new cookie every minute |
There was a problem hiding this comment.
Shouldn't the newCookieNeeded timeout be configurable with the default of 1 minute ?
| Cipher cipher = Cipher.getInstance("AES/ECB/PKCS5Padding"); | ||
|
|
||
| StringBuilder contents = new StringBuilder(); | ||
| //TODO: do we need random padding? |
There was a problem hiding this comment.
Can you take a look at this change set pedroigor@fb0b13d ?
It is about using AES GCM so that we have another random value to encrypt blocks. I have also changed to 128 which I think is a enough and help to balance with performance. We can make this configurable too.
I'm not using association data as that would require us to keep some state. But would be nice in order to make sure the user presenting the cookie is in fact the one to which the cookie was set. But it does not compromise the security.
stuartwdouglas
force-pushed
the
form-auth
branch
from
86ad410 to
6ed9c71
Compare
stuartwdouglas
force-pushed
the
form-auth
branch
from
6ed9c71 to
883a485
Compare
stuartwdouglas
force-pushed
the
form-auth
branch
from
883a485 to
a8e89d5
Compare
|
I have addressed all the comments |
| * is not suitable for production environments. This must be more than 16 characters long for security reasons | ||
| */ | ||
| @ConfigItem(name = "auth.session.encryption-key") | ||
| public String encryptionKey; |
There was a problem hiding this comment.
@stuartwdouglas should an auth.session.encryption-key-location be added and recommended by default ? encryptionKey is a good start but I'm not sure it is good for the production either, unless we have #3937, which would be good, as the users would just type
auth.session.encryption-key=${my.encryption.key.from.vault}
There was a problem hiding this comment.
You can just set the QUARKUS_HTTP_AUTH_SESSION_ENCRYPTION_KEY env var and it will override.
There was a problem hiding this comment.
Either way this can be done as a future enhancement if there is demand for it. FORM is supposed to be mostly for learning/small apps, for big apps keycloak is our recommended solution.
There was a problem hiding this comment.
@stuartwdouglas That is fine. I'll create an issue to follow up. Was not really my intention to slow down the PR - but if we let the users to for ex provide the user passwords with the elytron-security-jdbc then there should be a way for them not put the secret key for protecting the cookie data in the clear form. But as I said, with Vault and the config indirection it will already be possible; adding a simple check to load it from a (hopefully secure) file system will just complement it.
Fixes #4348