Quarkus OIDC CredenitalsProvider does not follow conventions of allowing a separate "bean name" and "keyring name"

[ryandens]

Describe the bug

Other CredentialsProvdider integrations, such as the ones built by Quarkus Data source, RabbitMQ, and Quarkiverse GitHub app extensions allow for separate configuration of a "bean name" of the CredentialsProvider implementation to be used by the extension and a "keyring name". This is also laid out in CredentialsProvider documentation here.

However, the Quarkus OIDC extension only allows for the configuration of quarkus.oidc.credentials.client-secret.provider.name (which is used here to first lookup the CDI bean and then used here to look up the secret from the keyring

I think the only way this could be successfully used today is by always supplying what's supposed to be an optional bean name for the CredentialsProvider implementation because otherwise the call to CredentialsProvider.getCredentials(String) will always be provided null.

This mandates that the bean name and the value in the keyring be the same. For my use case, which is a CredentialsProvider implementation backed by data in AWS SecretsManager, in means I need a separate CredentialsProvider implementation for just this extension where the name of the secret in SecretsManager matches the bean name of the CredentialsProvider implementation.

Expected behavior

We should adapt the OIDC extension to look more like other usages of CredentialsProviderFinder.find, like this

 private static Supplier<? extends String> fromCredentialsProvider(Provider provider) {
        return new Supplier<String>() {

            @Override
            public String get() {
                if (provider.key.isPresent()) {
                    String providerName = provider.name.orElse(null);
                    CredentialsProvider credentialsProvider = CredentialsProviderFinder.find(providerName);
                    if (provider.keyringName.isPresent()) {
                        credentialsProvider.getCredentials(provider.keyringName.get()).get(provider.key.get());
                    }
                    else {
                        return credentialsProvider.getCredentials(providerName).get(provider.key.get());
                    }
                }
                return null;
            }
        };
    }

This does the following:

1. remove a redundant null check, because CredentialsProviderFinder.find throws a RuntimeException if the CredentialsProvider resolved from the Arc container is null, and therefore this method will never return null
2. Adds a new config property for the keyring that is used if it is present.

In a future release of Quarkus, the breaking change to require the user to provide a keyring name (like all other extensions that support CredentialsProvider should be considered, but this change was structured in this way to be backward compatible.

I'm definitely open to other suggestions for implementing a change like this! I'm also appy to contribute this change myself if the workaround to conform to conventions is accepted.

Note, normally quarkus.oidc.credentials.client-secret.provider.name would be the name of the keyring, not the name of the CDI bean, but since it's already being used here for both in order to be backward compatible we have to introduce a new property for when users want to configure the keyring name (which is usually non-optional when configuring extensions to use CredentialsProvider implementations

Actual behavior

No response

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

No response

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

No response

Additional information

No response

[sberyozkin]

@ryandens There is a test SecretsProvider in the oidc deployment, it does not seem to have the limitation you describe above

[ryandens]

hi @sberyozkin, thanks for your response! I'm assuming you're talking about this file: https://github.com/quarkusio/quarkus/blob/79dbe9ff7a6575633d1aed1e8222720224c6e52c/extensions/oidc/deployment/src/test/java/io/quarkus/oidc/test/SecretProvider.java

This is a CredentialsProvider that always returns the same value (a singleton map with a constant key and value). This works well for a hardcoded secret in a test context, but wouldn't map well to resolving credentials from an actual vault instance. Note this CredentialsProvider implementation does not consider the keyring name credentialsProviderName parameter. In this case, since the bean name is vault-secret-provider this CredentialsProvider is invoked and returns the same value each time.

Imagine for a moment that this test SecretProvider was actually resolving the client secret from a vault-like API - would it make sense to provide this SecretProvider its own bean name via the credentialProviderName parameter to the SecretProvider.getCredentials(String) implementation? I don't think so, and neither does any other extension in the quarkus codebase that integrates with CredenitalsProvider that I can find

[sberyozkin]

@ryandens Hi, OK, I see now. For simple cases it should not be a problem, but I can imagine why it can actually be a problem for more advanced implementations.
Please open a PR with the suggested fix. In fact, I'd just pass provider.keyringName.orElse(null) and not attempt to pass the provider name at all, since it is an obvious bug.
For testing, you can probably update SecretProvider to use a concatenation of the keyring name and the key name as a map key, to prove the correct keyring name is passed
Thanks

[ryandens]

Sounds good, thank you for the help!

[ryandens]

👋 just checking back in here - is this fix targeting the 3.13 milestone? I noticed #41223 is but that this issue/PR is not

If there's something I need to do/can do to help get it in a future milestone, let me know!