Update Security docs and properties files #4309
Conversation
stuartwdouglas
force-pushed
the
sec-docs
branch
from
34aed03
to
51d3079
Compare
|
I went through the doc update quickly and it looks good. I would personally merge this quickly assuming the tests pass so people can start looking into what is changing with security. |
|
The gradle plugin test failure seems totally unrelated. I'll restart it when the rest of the tests finish. |
There was a problem hiding this comment.
I added a commit for doc formatting and a couple of comments.
My main concern is about the names of the extensions:
- do we really want to expose
elytron? Until now we refrained to do it. - is the Vert.x component of
vertx-keycloakimportant for the users? My main concern is that in the vast majority of cases, people won't know they are using Vert.x as they will just have included the RESTEasy extension. Thus they might think thatvertx-keycloakis not for them.
| @@ -48,6 +48,7 @@ | |||
| public static final String SCALA = "scala"; | |||
| public static final String SCHEDULER = "scheduler"; | |||
| public static final String SECURITY = "security"; | |||
| public static final String SECURITY_PROPERTIES_FILE = "elytron-security-properties-file"; | |||
There was a problem hiding this comment.
| public static final String SECURITY_PROPERTIES_FILE = "elytron-security-properties-file"; | |
| public static final String SECURITY_PROPERTIES_FILE = "security-properties-file"; |
would be better I think. We totally removed the elytron part from the other extension names.
| @@ -346,6 +346,11 @@ | |||
| <artifactId>quarkus-elytron-security-deployment</artifactId> | |||
| <version>${project.version}</version> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>io.quarkus</groupId> | |||
| <artifactId>quarkus-elytron-security-properties-file-deployment</artifactId> | |||
There was a problem hiding this comment.
I'm wondering if we should introduce elytron now? We named everything security before to not expose it.
There was a problem hiding this comment.
The artifacts were still called Elytron. I am ok with renaming it.
There was a problem hiding this comment.
Yeah, just saw that. I would say let's not block this PR for that. We can get it in and then decide to rename all of them consistently if we want to do so.
| Add the following to your `pom.xml`: | ||
|
|
||
| [source,xml] | ||
| -- |
| <2> User `jdoe` has password defined as `p4ssw0rd` | ||
|
|
||
| This file has the usernames and passwords stored in plain text, which is not recommended. If plain-text is set to false | ||
| (or omitted) in the config then passwords must be stored in the form `MD5 ( username : realm : password )`. This can |
There was a problem hiding this comment.
Is there any other hash available? md5 is really not considered secured these days.
There was a problem hiding this comment.
LegacyProperyRealm in Elytron only supports this, and it's not supposed to be used in production anyway. The other advantage is that lots of systems have md5 available so you can generate the password by with echo -n user:realm:password | md5.
We could change it but it would be a bit of work as it would mean forking the Elytron implementation, and we would also need to come up with some kind of tool to enable the users to generate the passwords ( I want to do this eventually anyway, but not as an ad-hoc thing).
There was a problem hiding this comment.
Yeah, probably not worth it. It's just that having md5 in a security related documentation could look like a joke.
|
|
||
| Quarkus provides support for properties file based authentication that is intended for | ||
| development and testing purposes. It is not recommended that this be used in production as at present only | ||
| plaintext passwords are supported. |
There was a problem hiding this comment.
Oops, I wrote this before I decided I was going to go ahead and implement the hashed passwords
|
|
||
| |=== | ||
| |Property Name|Default|Description | ||
| |quarkus.users.file.enabled|false|Determine whether security via the file realm is enabled |
There was a problem hiding this comment.
I wonder if it would be better to have a security prefix (so quarkus.security.users). For users, it might be self explanatory but it's not a given it will be for everything.
|
|
||
| ### Embedded Realm Configuration | ||
| The embedded realm also supports mapping of users to password and users to roles. It uses the main application.properties Quarkus configuration file to embed this information. To enable and configure it, the following configuration properties are used: | ||
| |link:keycloak-guide.html[quarkus-vertx-keycloak] |
There was a problem hiding this comment.
why did we rename it vertx-keycloak? It doesn't work at all in a servlet environment?
There was a problem hiding this comment.
Because our naming format is productname-feature. In this case vertx is providing Keycloak, so according to our standard naming rules this should be the name. I am happy to change it to just keycloak, but it might even need to be something more generic (as it may just be the open id connect implementation)
There was a problem hiding this comment.
Can we have a discussion about the naming on the mailing list? In respect to the elytron thing and vertx-keycloak? Let's not block this PR for that.
stuartwdouglas
force-pushed
the
sec-docs
branch
from
8043dff
to
adc97e4
Compare
gsmet
added
the
triage/waiting-for-ci
This moves the properties file config out of the core Elytron module, and also adds support for hashed passwords to remove the need for plaintext passwords in the files.
@gsmet this has a high chance to conflict so a prompt review would be appreciated.