Security: support Modular Crypt Format or its successor PHC

[FroMage]

ATM our security extensions support several sorts of algorithms:

• security-properties says clear or HEX( MD5( username ":" realm ":" password ) )
• security-jdbc says clear or bCrypt (but not in MCF/PHC format: all fields separated)

We should support MCF and/or its successor PHC for both options, even if we don't support all the algo types.

[FroMage]

https://www.npmjs.com/package/@phc/bcrypt has examples of the PHC format for bcrypt.

[FroMage]

http://wildfly-security.github.io/wildfly-elytron/1.9.x/api-javadoc/org/wildfly/security/password/util/ModularCrypt.html has support for MCF, so it's just a matter of being able to use it.

As for PHC, we probably need to add support for it in Elytron, WDYT @dmlloyd ?

[dmlloyd]

If PHC is, as they claim, a "well defined subset of the Modular Crypt Format" then it's just a question of adding support for whatever additional formats we are missing. The PHC document at https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md mentions a few password types which are not supported presently by Elytron, specifically the "argon" group of algorithms.

/cc @darranl, @fjuma

[sberyozkin]

Hey @FroMage, can you please discuss with @wowselim how to proceed here, he is interested in this enhancement, see #38634, thanks.

[FroMage]

OK.

Though it turns out, looking at the sources of elytron-jdbc, that they appear to support MCF, given this code:

            if (saltColumn == -1 && iterationCountColumn == -1) {
                // try modular crypt
                final String s = getStringColumn(metaData, resultSet, hashColumn);
                if (s != null) {
                    final char[] chars = s.toCharArray();
                    final String identified = ModularCrypt.identifyAlgorithm(chars);
                    if (identified != null) {
                        try {
                            Password modularCryptPassword = ModularCrypt.decode(chars);
                            if (log.isTraceEnabled()) {
                                log.tracef("Key Mapper: Password credential created using Modular Crypt algorithm [%s]", identified);
                            }
                            return new PasswordCredential(modularCryptPassword);
                        } catch (InvalidKeySpecException e) {
                            log.tracef(e, "Key Mapper: Unable to identify Modular Crypt algorithm [%s]", identified);
                        }
                    }
                }
            }

And in the builder, saltColumn and iterationCountColumn both default to -1 (in PasswordKeyMapper.builder) though our config has no default, and no mention of these special values.

It's entirely possible that at least for security-jdbc we already support MCF, if quarkus.security.jdbc.principal-query.bcrypt-password-mapper.salt-index and quarkus.security.jdbc.principal-query.bcrypt-password-mapper.iteration-count-index are set to -1.

Could you test this please @wowselim and report back? If it works, then we might just have to update our config defaults and documentation.

Thanks a lot.

[wowselim]

@FroMage you're right, it's supported indeed. Here's a project with a working test to demonstrate:

https://github.com/wowselim/quarkus-jdbc-auth-reproducer

[FroMage]

Awesome!

So the config required is:

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
quarkus.security.jdbc.principal-query.bcrypt-password-mapper.salt-index=-1
quarkus.security.jdbc.principal-query.bcrypt-password-mapper.iteration-count-index=-1

I think we should document this, and also make salt-index and iteration-count-index non-required and default to -1 like they do upstream.

WDYT @sberyozkin ?

[dmlloyd]

It's probably best to change their type to OptionalInt then, and only accept -1 on a compatibility basis.

[FroMage]

Sure.

[wowselim]

I've created two PRs updating the documentation as well as the quickstart project. See #39122

[FroMage]

The docs are great, but we should also fix the default values of the salt/iteration config values to simplify.