Ojdbc provider hashicorp #142
Conversation
…ecrets to extract specific fields from JSON secrets.
…cret instead of application name
…Readme and javadocs
…xample-test.properties file
oracle-contributor-agreement
bot
added
the
OCA Verified
… cache, and enhanced README with documentation. Included example properties and unit tests for validation.
…cp vault dedicated a long with unit tests
…ix a typo in auto_detect auth method
… dedicated and hcp vault secrets providers
…riables in github actions pipeline
| * @param authToken The optional Bearer token for authorization. Can be null or empty. | ||
| * @param namespace The optional Vault namespace. Can be null or empty. | ||
| * @return A configured {@link HttpURLConnection}. Never null. | ||
| * @throws Exception if the connection cannot be established. |
There was a problem hiding this comment.
There are other reasons than "the connection cannot be established" for a connection to be thrown.
| * the response as a string. | ||
| * | ||
| * @param conn The configured HTTP connection. Must not be null. | ||
| * @param payload The payload to send. Must not be null. |
There was a problem hiding this comment.
Since the code assumes that it is an UTF8 String, maybe it should be documented.
| public static String sendGetRequestAndGetResponse(HttpURLConnection conn) throws Exception { | ||
| int responseCode = conn.getResponseCode(); | ||
| if (responseCode != HttpURLConnection.HTTP_OK) { | ||
| String errorResponse = ""; |
There was a problem hiding this comment.
Error handling code is duplicated, could be extracted to a method that would be used in both calls. Since the HttpURLConnection can only be used for a single request, is there a point in separating open and send ?
| * @param generator the token generator function. | ||
| * @return a {@code DedicatedVaultCredentials} instance. | ||
| */ | ||
| private static DedicatedVaultToken createScopedToken( |
There was a problem hiding this comment.
Does the token really have a scope? Or is it just a token that is cached and expires?
|
|
||
| switch (method) { | ||
| case VAULT_TOKEN: | ||
| return createTokenCredentials(parameterSet); |
There was a problem hiding this comment.
Can't this token be cached too, buy with a null expiration time ? That would avoid having a special case.
| case VAULT_TOKEN: | ||
| return createTokenCredentials(parameterSet); | ||
| case USERPASS: | ||
| return createScopedToken(parameterSet, method, DedicatedVaultTokenFactory::createUserpassToken); |
There was a problem hiding this comment.
Could the token generator function be moved to the Enum and maybe the function that generates the cache key as well?
| return cachedToken; | ||
| }); | ||
|
|
||
| if (validCachedToken.getToken() instanceof OpaqueAccessToken) { |
There was a problem hiding this comment.
If the token is declared as OpaqueAccessToken, this won't be needed.
| URL url = new URL(urlStr); | ||
| HttpURLConnection conn = (HttpURLConnection) url.openConnection(); | ||
| conn.setRequestMethod(method); | ||
| conn.setRequestProperty("Content-Type", contentType); |
There was a problem hiding this comment.
Seems strange to me to set the contentType on createConnection and not on send. How can you know the content before you send ?
| OracleJsonObject secretJsonObj = | ||
| new OracleJsonFactory() | ||
| .createJsonTextValue(inputStream) | ||
| .asJsonObject(); |
There was a problem hiding this comment.
Lines 97 - 103: isn't that what JsonUtil.parseJsonResponse does? Shouldn't it be used instead?
| .request(parameters) | ||
| .getContent(); | ||
|
|
||
| return new ByteArrayInputStream(secretString.getBytes()); |
There was a problem hiding this comment.
In other places in the code UTF8 encoding has been used but not here, why?
| } | ||
|
|
||
| @Override | ||
| public String getParserType(String arg0) { |
There was a problem hiding this comment.
Can you give the correct name to the argument (even though it is not used)?
| */ | ||
| private static String parseSecretJson(String secretJson, String secretPath) { | ||
| try (InputStream is = new ByteArrayInputStream(secretJson.getBytes(UTF_8))) { | ||
| OracleJsonObject rootObject = JSON_FACTORY.createJsonTextValue(is).asJsonObject(); |
There was a problem hiding this comment.
Here again, is there a reason for creating JsonUtil.parseJsonResponse if it is not used?
| * @throws IllegalArgumentException if the value is unrecognized. | ||
| */ | ||
| private static DedicatedVaultAuthenticationMethod parseAuthentication(String value) { | ||
| switch (value.toUpperCase()) { |
There was a problem hiding this comment.
Can you use DedicatedAuthenticationMethod.valueOf(String) instead?
| /** | ||
| * The username for Userpass authentication. Required for Userpass method. | ||
| */ | ||
| public static final Parameter<String> USERNAME = Parameter.create(REQUIRED); |
There was a problem hiding this comment.
Why are all authentication parameters declared here, if they are never used in this class?
| * @return The extracted client ID | ||
| * @throws IllegalArgumentException if the token is invalid or client_id extraction fails. | ||
| */ | ||
| public static String extractClientIdFromToken(String token) { |
| * @return A valid access token. | ||
| * @throws IOException if authentication fails. | ||
| */ | ||
| public synchronized String getValidAccessToken() throws Exception { |
There was a problem hiding this comment.
Use either a Lock or synchronized(private lock field).
| } | ||
|
|
||
| String content = new String(Files.readAllBytes(credsFilePath), StandardCharsets.UTF_8); | ||
| OracleJsonObject jsonObject = JsonUtil.parseJsonResponse(content).getObject("login"); |
There was a problem hiding this comment.
Catch null exception. If the Json content does not contain the key, getObject will throw a NullPointerException
| throw new IllegalStateException("Missing required parameters for token refresh."); | ||
| } | ||
|
|
||
| String payload = String.format("grant_type=%s&refresh_token=%s&client_id=%s", GRANT_TYPE, refreshToken, clientId); |
| private void updateTokensFromResponse(OracleJsonObject response) { | ||
| accessToken = JsonUtil.extractField(response, "access_token"); | ||
|
|
||
| OracleJsonValue expiresInValue = response.get("expires_in"); |
| */ | ||
| private void updateCredsFile() throws IOException { | ||
| String updatedContent = String.format( | ||
| "{ \"login\": { \"access_token\": \"%s\", \"refresh_token\": \"%s\", \"access_token_expiry\": \"%s\" } }", |
| private HcpVaultSecretToken getCredential(ParameterSet parameterSet) { | ||
| HcpVaultAuthenticationMethod method = parameterSet.getRequired(AUTHENTICATION_METHOD); | ||
|
|
||
| switch (method) { |
There was a problem hiding this comment.
Same comment as for dedicated.
| */ | ||
| private HcpVaultSecretToken getCachedToken(Supplier<HcpVaultSecretToken> tokenSupplier) { | ||
| if (cachedTokenSupplier == null) { | ||
| synchronized (HcpVaultTokenFactory.class) { |
There was a problem hiding this comment.
Use a Lock or syncronize using private lock object.
| * to client credentials. | ||
| * </p> | ||
| */ | ||
| public final class HcpVaultTokenFactory implements ResourceFactory<HcpVaultSecretToken> { |
There was a problem hiding this comment.
Check what happens in case of vault migration and parameters change during application execution.
| * @throws IllegalStateException If the HTTP request fails or the response | ||
| * does not contain the required fields. | ||
| */ | ||
| public static String fetchSecrets(String urlStr, String token) { |
There was a problem hiding this comment.
Singular (one secret). Are you sure this is necessary?
| .addParameter("GITHUB_AUTH_PATH", | ||
| DedicatedVaultParameters.GITHUB_AUTH_PATH) | ||
| .addParameter("type", Parameter.create()) | ||
| .build(); |
There was a problem hiding this comment.
I believe that we can handle system properties and environment variables here if we do something like this:
public static final ParameterSetParser PARAMETER_SET_PARSER =
DedicatedVaultConfigurationParameters.configureBuilder(
ParameterSetParser.builder()
.addParameter("value",
SECRET_PATH)
.addParameter("key",
KEY)
.addParameter("VAULT_ADDR", (value, parameterSetBuilder) -> {
parameterSetBuilder.add("VAULT_ADDR", VAULT_ADDR,
useValueOrFallback(value, ENV_VAULT_ADDR, null));
})
.addParameter("VAULT_TOKEN", (value, parameterSetBuilder) -> {
parameterSetBuilder.add("VAULT_TOKEN", VAULT_TOKEN,
useValueOrFallback(value, ENV_VAULT_TOKEN, null));
})
.addParameter("VAULT_USERNAME", (value, parameterSetBuilder) -> {
parameterSetBuilder.add("VAULT_USERNAME", USERNAME,
useValueOrFallback(value, ENV_VAULT_USERNAME, null));
})
.addParameter("VAULT_PASSWORD", (value, parameterSetBuilder) -> {
parameterSetBuilder.add("VAULT_PASSWORD", PASSWORD,
useValueOrFallback(value, ENV_VAULT_PASSWORD, null));
})
.addParameter("VAULT_NAMESPACE", (value, parameterSetBuilder) -> {
parameterSetBuilder.add("VAULT_NAMESPACE", VAULT_TOKEN,
useValueOrFallback(value, ENV_VAULT_NAMESPACE, DEFAULT_NAMESPACE));
})
.addParameter("GITHUB_TOKEN", (value, parameterSetBuilder) -> {
parameterSetBuilder.add("GITHUB_TOKEN", GITHUB_TOKEN,
useValueOrFallback(value, ENV_GITHUB_TOKEN, null));
})
.addParameter("GITHUB_AUTH_PATH", (value, parameterSetBuilder) -> {
parameterSetBuilder.add("GITHUB_AUTH_PATH", GITHUB_AUTH_PATH,
useValueOrFallback(value, ENV_GITHUB_AUTH_PATH, DEFAULT_USERPASS_PATH));
})
.addParameter("FIELD_NAME",
FIELD_NAME))
.addParameter("ROLE_ID", (value, parameterSetBuilder) -> {
parameterSetBuilder.add("USERPASS_AUTH_PATH", USERPASS_AUTH_PATH,
useValueOrFallback(value, ENV_USERPASS_AUTH_PATH, DEFAULT_USERPASS_PATH));
})
.addParameter("ROLE_ID", (value, parameterSetBuilder) -> {
parameterSetBuilder.add("ROLE_ID", ROLE_ID,
useValueOrFallback(value, ENV_VAULT_ROLE_ID, null));
})
.addParameter("SECRET_ID", (value, parameterSetBuilder) -> {
parameterSetBuilder.add("SECRET_ID", SECRET_ID,
useValueOrFallback(value, ENV_SECRET_ID, null));
})
.addParameter("APPROLE_AUTH_PATH", (value, parameterSetBuilder) -> {
parameterSetBuilder.add("APPROLE_AUTH_PATH", APPROLE_AUTH_PATH,
useValueOrFallback(value, ENV_APPROLE_AUTH_PATH, DEFAULT_APPROLE_PATH));
})
.addParameter("type", Parameter.create()
).build();
private static String useValueOrFallback(String value, String envName, String defaultValue) {
if (value != null) {
return value;
}
String envValue = System.getProperty(envName, System.getenv(envName));
if (envValue == null || envValue.isEmpty()) {
return defaultValue;
}
return envValue;
}I would also move this declaration to DedicatedVaultParameters so that the parameter names and environment variable names are only used there.
| * @return A map containing only the filtered parameters. | ||
| */ | ||
| default Map<String, Object> filterParameters(String[] relevantKeys) { | ||
| Map<String, Object> allParameters = ((ParameterSetImpl) this).getParameterKeyValuePairs(); |
There was a problem hiding this comment.
Can you move the implementation of this method out of the Interface?
| Map<String, Object> allParameters = ((ParameterSetImpl) this).getParameterKeyValuePairs(); | ||
|
|
||
| return allParameters.entrySet().stream() | ||
| .filter(entry -> Arrays.asList(relevantKeys).contains(entry.getKey())) |
There was a problem hiding this comment.
If call Arrays.asList(...) inside the filter method, won't it be done once for each entry?
| "No value defined for parameter \"%s\"", | ||
| name != null ? name : parameter.toString())); | ||
| "No value defined for parameter \"%s\"", | ||
| name != null ? name : parameter.toString())); | ||
| } |
There was a problem hiding this comment.
Please avoid changes that only add/remove empty spaces.
| /** | ||
| * Retrieves the relevant parameter key-value pairs. | ||
| */ | ||
| public Map<String, Object> getParameterKeyValuePairs() { |
| Map<String, String> opts = new HashMap<>(inputOpts); | ||
|
|
||
| opts.putIfAbsent("authentication", "auto_detect"); | ||
| String authStr = opts.get("authentication"); |
| return builder.addParameter( | ||
| // The parameter name is "AUTHENTICATION" | ||
| "AUTHENTICATION", | ||
| // Tied to HashicorpCredentialsFactory.AUTHENTICATION_METHOD |
There was a problem hiding this comment.
Why is this parameter added separately?
No description provided.