Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Quarkus fails to start with bcrypt password mapper #38634

Closed
wowselim opened this issue Feb 7, 2024 · 17 comments
Closed

Quarkus fails to start with bcrypt password mapper #38634

wowselim opened this issue Feb 7, 2024 · 17 comments
Labels
area/security kind/bug Something isn't working

Comments

@wowselim
Copy link
Contributor

wowselim commented Feb 7, 2024

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 wowselim added the kind/bug Something isn't working label Feb 7, 2024
@wowselim
Copy link
Contributor Author

wowselim commented Feb 7, 2024

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
Copy link
Contributor Author

wowselim commented Feb 7, 2024

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
Copy link
Contributor

geoand commented Feb 7, 2024

cc @sberyozkin

@sberyozkin
Copy link
Member

sberyozkin commented Feb 7, 2024

@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
Copy link
Contributor Author

wowselim commented Feb 7, 2024

@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
Copy link
Member

@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
Copy link
Contributor Author

wowselim commented Feb 7, 2024

@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
Copy link
Contributor Author

wowselim commented Feb 7, 2024

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
Copy link
Member

sberyozkin commented Feb 7, 2024

@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
Copy link
Contributor Author

wowselim commented Feb 7, 2024

@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
Copy link
Contributor Author

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
Copy link
Member

sberyozkin commented Feb 16, 2024

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

@wowselim
Copy link
Contributor Author

@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
Copy link
Member

sberyozkin commented Feb 16, 2024

@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
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security kind/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants