edelfour
commented
1 year ago Hello @vsevel, @sberyozkin, @kdubb
By any chance, would you have time to have a look at this issue and the proposed fix ?
Thanks in advance :-)
Closed edelfour closed 1 year ago
edelfour
commented
1 year ago Hello @vsevel, @sberyozkin, @kdubb
By any chance, would you have time to have a look at this issue and the proposed fix ?
Thanks in advance :-)
gsmet
commented
1 year ago @edelfour created a PR here: https://github.com/quarkiverse/quarkus-vault/pull/190
vsevel
commented
1 year ago Map<String, String> readSecret(String path) is a shortcut to read secrets that are maps of strings.
for other situations we have Map<String, Object> readSecretJson(String path)
would that fit your need?
that feels strange converting those non string values to strings.
edelfour
commented
1 year ago Hello @vsevel
We don't call those methods directly in our code.
We just have the quarkus-vault dependency in our pom file and the vault properties in our application.properties file.
If I understand well magic happens in io.quarkus.vault.runtime.config.VaultConfigSource which implements org.eclipse.microprofile.config.spi.ConfigSource and calls readSecret method.
VaultConfigSource implements public Map<String, String> getProperties() and public String getValue(String propertyName) and that's the reason why in VaultConfigSource readSecret is called because ConfigSource wants property value as String.
vsevel
commented
1 year ago I see. so may be it is an issue with the VaultConfigSource, not the VaultKvManager.
the issue I see with your implementation is that you are going to take the toString of any top level entry you are going to find in your secret. so if your value is a list or a map you are going to take the java toString of those objects. I do not know if people are expecting this.
may be we should change VaultConfigSource.fetchSecrets() instead, with something like this:
private Map<String, String> fetchSecrets(String path, String prefix) {
Map<String, Object> secretJson = getVaultKVSecretEngine().readSecretJson(path).await().indefinitely();
// ignore list and map, honor null, get as string scalar types
Map<String, String> secret = secretJson.entrySet().stream()
.filter(entry -> isScalar(entry.getValue()))
.collect(toMap(entry -> entry.getKey(), entry -> toString(entry.getValue())));
return prefixMap(secret, prefix);
}
private boolean isScalar(Object value) {
return !(value instanceof List || value instanceof Map);
}
private String toString(Object value) {
return value == null ? null : value.toString();
}yes for sure :-) Perhaps, instead of ignoring List, List could be converted to a String with coma separated values as ConfigSource can convert this format to a List ?
vsevel
commented
1 year ago yes assuming this is not a list of list or a list of maps
edelfour
commented
1 year ago yes :-)
perhaps the documentation of quarkus vault also should mention that values of type list or map are not supported
if it's ok for you, I will close my PR as there is a better solution.
Any idea in which release the fix will be available ?
vsevel
commented
1 year ago if it's ok for you, I will close my PR as there is a better solution.
yes are you willing to develop the fix?
edelfour
commented
1 year ago If it can help, I can do the copy/paste :-) and the update of the tests
vsevel
commented
1 year ago that would be fantastic you can try implementing your suggestion to support scalar list as well (or we leave this for another time)
vsevel
commented
1 year ago for your tests, instead of using the WiremockProxy, you can add a true secret using a file such as secret.json, and push it into the vault from the VaultTestExtension using:
.withClasspathResourceMapping("config.json", "/tmp/config.json", READ_ONLY)
...
execVault(format("vault kv put %s/config-json @/tmp/config.json", SECRET_PATH_V1));you would have to change the policy as well:
path "secret-v1/config-json" {
capabilities = ["read"]
}then add another quarkus.vault.secret-config-kv-path in application-vault.properties, beside the existing value config:
quarkus.vault.secret-config-kv-path=config,config-jsonand you would probably have to do it for v2 as well:
execVault(format("vault kv put %s/config-json @/tmp/config.json", SECRET_PATH_V2));
...Thanks for the tips :-)
edelfour
commented
1 year ago Hello @vsevel A PR is ready for review following your recommendation. I did not manage the scalar list in this PR to avoid mixing fix and enhancement. I'll do another PR to manage the scalar list support.
Hello, We are currently migrating to Quarkus 3.4.3 which uses quarkus-vault:3.2.0
In our vault, we have entries of boolean type, for example: "dev-mode":true
At the startup we encoutered the following exception:
The problems seems to come from
convertmethod fromVaultKvManagerclasse. The method do a String cast on value which throws the exception when it's a Boolean object.Thanks in advance for your help.