quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.82k stars 2.69k forks source link

Quarkus fails to start with bcrypt password mapper #38634

Closed wowselim closed 9 months ago

wowselim commented 9 months ago

Describe the bug

When using elytron-security-jdbc and configuring the bcrypt password mapper, the quarkus application fails to start with the following error.

Expected behavior

Application starts successfully.

Actual behavior

We get the following stack trace:

java.lang.RuntimeException: Failed to start quarkus
        at io.quarkus.runner.ApplicationImpl.doStart(Unknown Source)
        at io.quarkus.runtime.Application.start(Application.java:101)
        at io.quarkus.runtime.ApplicationLifecycleManager.run(ApplicationLifecycleManager.java:111)
        at io.quarkus.runtime.Quarkus.run(Quarkus.java:71)
        at io.quarkus.runtime.Quarkus.run(Quarkus.java:44)
        at io.quarkus.runtime.Quarkus.run(Quarkus.java:124)
        at io.quarkus.runner.GeneratedMain.main(Unknown Source)
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
        at java.base/java.lang.reflect.Method.invoke(Method.java:580)
        at io.quarkus.runner.bootstrap.StartupActionImpl$1.run(StartupActionImpl.java:113)
        at java.base/java.lang.Thread.run(Thread.java:1583)
Caused by: java.lang.IllegalArgumentException: COM00001: Parameter 'saltColumn' must not be less than 1
        at org.wildfly.common.Assert.checkMinimumParameter(Assert.java:294)
        at org.wildfly.security.auth.realm.jdbc.mapper.PasswordKeyMapper.<init>(PasswordKeyMapper.java:75)
        at org.wildfly.security.auth.realm.jdbc.mapper.PasswordKeyMapper$Builder.build(PasswordKeyMapper.java:417)
        at io.quarkus.elytron.security.jdbc.BcryptPasswordKeyMapperConfig.toPasswordKeyMapper(BcryptPasswordKeyMapperConfig.java:63)
        at io.quarkus.elytron.security.jdbc.JdbcRecorder.registerPrincipalQuery(JdbcRecorder.java:61)
        at io.quarkus.elytron.security.jdbc.JdbcRecorder.createRealm(JdbcRecorder.java:39)
        at io.quarkus.deployment.steps.ElytronSecurityJdbcProcessor$configureJdbcRealmAuthConfig173765586.deploy_0(Unknown Source)
        at io.quarkus.deployment.steps.ElytronSecurityJdbcProcessor$configureJdbcRealmAuthConfig173765586.deploy(Unknown Source)
        ... 11 more

How to Reproduce?

Add these to your application.properties and run the application:

quarkus.security.jdbc.enabled=true
quarkus.security.jdbc.principal-query.sql=SELECT password, role FROM account WHERE email=?
quarkus.security.jdbc.principal-query.bcrypt-password-mapper.enabled=true
quarkus.security.jdbc.principal-query.bcrypt-password-mapper.password-index=1

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

3.7.1

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

Here's a project that simply adds the elytron jdbc dependency and uses the aforementioned config

jdbc-auth-reproducer.zip

wowselim commented 9 months ago

I can see the config properties that might fix it here:

https://quarkus.io/guides/security-jdbc

But there's virtually no documentation on what value these properties should have.

wowselim commented 9 months ago

Also note that I used io.quarkus.elytron.security.common.BcryptUtil to generate passwords and I'd like to store them in one column together with the salt. I don't know how that would play into the config. Any guidance would be appreciated!

geoand commented 9 months ago

cc @sberyozkin

sberyozkin commented 9 months ago

@wowselim The documentation for the Bcrypt related properties says that the indexes of the columns use 1 based numbering, but you only have specified the password index, but no salt and iteration count indexes, so this is a configuration issue. I haven't used this extension but I believe they should point to the table columns containing the bcrypt salt, iteration count etc.

Also note that I used io.quarkus.elytron.security.common.BcryptUtil to generate passwords and I'd like to store them in one column together with the salt.

I believe you should use quarkus.security.jdbc.principal-query.bcrypt-password-mapper.password-index, quarkus.security.jdbc.principal-query.bcrypt-password-mapper.salt-index and quarkus.security.jdbc.principal-query.bcrypt-password-mapper.iteration-count-index to point to the table columns

wowselim commented 9 months ago

@wowselim The documentation for the Bcrypt related properties says that the indexes of the columns use 1 based numbering, but you only have specified the password index, but no salt and iteration count indexes, so this is a configuration issue. I haven't used this extension but I believe they should point to the table columns containing the bcrypt salt, iteration count etc.

Also note that I used io.quarkus.elytron.security.common.BcryptUtil to generate passwords and I'd like to store them in one column together with the salt.

I believe you should use quarkus.security.jdbc.principal-query.bcrypt-password-mapper.password-index, quarkus.security.jdbc.principal-query.bcrypt-password-mapper.salt-index and quarkus.security.jdbc.principal-query.bcrypt-password-mapper.iteration-count-index to point to the table columns

@sberyozkin what can I do if I want to use bcrypt like it's described here: https://quarkus.io/guides/security-jpa#password-storage-and-hashing

With the default option, passwords are stored and hashed with bcrypt under the Modular Crypt Format (MCF). While using MCF, the hashing algorithm, iteration count, and salt are stored as a part of the hashed value. As such, we do not need dedicated columns to keep them.

Setting all of those other configs to 1 does not work with passwords that were generated using BcryptUtil.

I think having them as separate columns is pretty outdated and most libraries (vert.x, spring boot) do it in this format (as does quarkus as seen above).

sberyozkin commented 9 months ago

@wowselim

https://quarkus.io/guides/security-jpa#password-storage-and-hashing is a different extension, quarkus-security-jpa.

Setting all of those other configs to 1 does not work with passwords that were generated using BcryptUtil.

This issue is about Quarkus failing to start using quarkus-security-jdbc, and I believe it is about configuring it correctly to point to the already populated table.

I think we can close this issue since your requirement is possibly about using quarkus-security-jpa. My proposal is to close this issue and then you start a Discussion how to use Bcrypt with the security-jpa, do you agree ?

wowselim commented 9 months ago

@sberyozkin actually I would like to use quarkus-security-jdbc specifically because I use jooq for the db layer and I don’t want a dependency on hibernate. I just wanted to highlight the inconsistency regarding jpa and jdbc when it comes to auth. The jpa approach seems more modern in this aspect.

I think it would be more appropriate to create a ticket to improve the jdbc docs to show how to handle bcrypt since it’s not clear when the docs only use clear text.

wowselim commented 9 months ago

My proposal is to close this issue and then you start a Discussion how to use Bcrypt with the security-jpa, do you agree ?

i think jpa is much better documented (see the link i posted before). The jdbc docs leave out some important details since they only focus on clear text auth.

sberyozkin commented 9 months ago

@wowselim Sure, security-jdbc was the very first identity provider extension, while indeed we focused on security-jpa and recommend it due its ease of use.

I suppose we can resolve this issue by updating the JDBC docs, like I said, I haven't worked with this extension, but what I understand is that you can use BCryptUtil method to create a hash with a known salt and count, and then, in the tutorial, you can update that Create Table instruction to include the hash, salt and count and refer to their column indexes in the configuration. I agree it is a fairly low level approach, but it is done by design in security-jdbc.

Would you like to test it and it works I can update the docs ?

wowselim commented 9 months ago

@sberyozkin Sure, I would like to improve the docs and provide a bcrypt example. I've found this on the web:

https://issues.redhat.com/browse/ELY-1497

So it seems like support for this format should already be in wildfly jdbc security realm (org.wildfly.security.auth.realm.jdbc.JdbcSecurityRealm). I would like to do some more research and maybe even add support for this in quarkus.

wowselim commented 9 months ago

Hey, I've asked around on the elytron zulip server and it looks like there's no interest in this feature or a contribution. This issue can be closed since it's essentially a duplicate of #5667.

sberyozkin commented 9 months ago

@wowselim Hi, can you please try what is suggested in https://github.com/quarkusio/quarkus/issues/38634#issuecomment-1932095959 here ?

wowselim commented 9 months ago

@wowselim Hi, can you please try what is suggested in #38634 (comment) here ?

@sberyozkin it will not work because BCryptUtil uses the modular crypt format which is not supported by the jdbc security extension. See the javadoc for the hashing function:

https://github.com/quarkusio/quarkus/blob/main/extensions/elytron-security-common/runtime/src/main/java/io/quarkus/elytron/security/common/BcryptUtil.java#L64

sberyozkin commented 9 months ago

@wowselim If you'd like you can consider opening a PR where BcryptUtil class will have another method for producing a format understood by the JDBC extension ?

wowselim commented 9 months ago

@wowselim If you'd like you can consider opening a PR where BcryptUtil class will have another method for producing a format understood by the JDBC extension ?

@sberyozkin I don't like the idea of having the three (password, salt, iteration count) split to be honest. It's quite unconventional and not used anywhere else as far as I know. I wrote some custom identity providers to achieve what I wanted. For anyone else interested here's a write up: https://blog.selim.co/2024/02/17/quarkus-form-authentication-using-jdbc.html

sberyozkin commented 9 months ago

Sounds good @wowselim, thanks, do you have an account on X ? If yes, please consider promoting your post there as well. OK, let me close this issue since you have agreed to it

wowselim commented 9 months ago

@sberyozkin I shared my post here: https://twitter.com/libslim/status/1758896395379982393