quarkiverse / quarkus-vault

Quarkus HashiCorp Vault extension
Apache License 2.0
19 stars 26 forks source link

NullPointerException when retrieving Vault contents with null values #327

Open joeyy-watts opened 3 days ago

joeyy-watts commented 3 days ago

This issue is found in version 4.0.1, although I still see the problematic code in 4.1.0.

Issue

In my use case, I have a JSON secret in Vault which can contain null values, e.g:

{
     "password": "foo",
     "someotherfield": null
}

When I try to read this secret with quarkus-vault, the following exception is thrown:

2024-09-26 12:45:19,744 WARN  [io.agr.pool] (agroal-21) Datasource '<default>': Failed to create connection due to NullPointerException
2024-09-26 12:45:19,744 ERROR [io.sma.health] (executor-thread-1) SRHCK01000: Error processing Health Checks: java.lang.NullPointerException
    at java.base/java.util.Objects.requireNonNull(Unknown Source)
    at java.base/java.util.stream.Collectors.lambda$uniqKeysMapAccumulator$1(Unknown Source)
    at java.base/java.util.stream.ReduceOps$3ReducingSink.accept(Unknown Source)
    at java.base/java.util.Iterator.forEachRemaining(Unknown Source)
    at java.base/java.util.Spliterators$IteratorSpliterator.forEachRemaining(Unknown Source)
    at java.base/java.util.stream.AbstractPipeline.copyInto(Unknown Source)
    at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(Unknown Source)
    at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(Unknown Source)
    at java.base/java.util.stream.AbstractPipeline.evaluate(Unknown Source)
    at java.base/java.util.stream.ReferencePipeline.collect(Unknown Source)
    at io.quarkus.vault.runtime.VaultKvManager.convert(VaultKvManager.java:38)

On this line in VaultKvManager.java I've found the cause to be the way the library is casting the JSON value to String.

(String) entry.getValue()

In the case that the entry's value is null this will also return null, causing the exception above.

I suspect it will also fail if the value is of a non-String type, with the following exception (although I haven't tested with the library; just ran some scratch code):

Exception in thread "main" java.lang.ClassCastException: class java.lang.Integer cannot be cast to class java.lang.String (java.lang.Integer and java.lang.String are in module java.base of loader 'bootstrap')

How to Reproduce the Issue

Attempt to read a secret with at least one null value.


Proposed Fix

To fix this issue, I have applied this patch to use String.valueOf() instead. I've used this in my own fork of the library, and confirmed it to be working:

private Map<String, String> convert(Map<String, Object> map) {
        return map.entrySet().stream().collect(Collectors.toMap(Map.Entry::getKey, entry -> String.valueOf(entry.getValue())));
}
kdubb commented 2 days ago

Are you able to provide a PR? If possible with a test covering the use case.

joeyy-watts commented 2 days ago

After going through the code more thoroughly and finding this comment I've decided to change the fix.

(I'd rather keep VaultKvManager as-is to avoid unexpected side effects elsewhere)

I've opted to update VaultCredentialsProvider instead to use .readSecretJson (which doesn't do the problematic conversion), and using String.valueOf() to cast it where it's actually used.

Although this does mean it no longer (implicitly) guarantees the password to never be null. I'm open to your input here @kdubb

~I'll test it with my use case and let you know how it goes.~ Tested on my own fork, and confirmed to be working.