You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2022/05/23 14:45:41 UTC
[GitHub] [drill] jnturton opened a new pull request, #2558: DRILL-8232: Add support for user credentials to VaultCredentialsProvider
jnturton opened a new pull request, #2558:
URL: https://github.com/apache/drill/pull/2558
# [DRILL-8232](https://issues.apache.org/jira/browse/DRILL-8232): Add support for user credentials to VaultCredentialsProvider
## Description
The VaultCredentialsProvider can join the PlainCredentialsProvider in supporting user credentials, credentials that stored for each each Drill query user and which are needed for the user_translation auth mode, by constructing a Vault secret path dynamically based on the name of the query user. A variable named `$user` in the VaultCredentialsProvider's `secretPath` is replaced with the query user's name at query time.
## Documentation
To be added once the new auth mode implementation has stabilised.
## Testing
CredentialsProviderImplementationsTest::testVaultUserCredentialsPresent
CredentialsProviderImplementationsTest::testVaultUserCredentialsAbsent
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jnturton commented on pull request #2558: DRILL-8232: Add support for user credentials to VaultCredentialsProvider
Posted by GitBox <gi...@apache.org>.
jnturton commented on PR #2558:
URL: https://github.com/apache/drill/pull/2558#issuecomment-1138691328
CI has passed but I was missing an Apache license header in a test resource.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre merged pull request #2558: DRILL-8232: Add support for user credentials to VaultCredentialsProvider
Posted by GitBox <gi...@apache.org>.
cgivre merged PR #2558:
URL: https://github.com/apache/drill/pull/2558
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a diff in pull request #2558: DRILL-8232: Add support for user credentials to VaultCredentialsProvider
Posted by GitBox <gi...@apache.org>.
cgivre commented on code in PR #2558:
URL: https://github.com/apache/drill/pull/2558#discussion_r879554968
##########
exec/java-exec/src/main/java/org/apache/drill/exec/store/security/vault/VaultCredentialsProvider.java:
##########
@@ -37,9 +40,11 @@
*/
public class VaultCredentialsProvider implements CredentialsProvider {
+ private static final Logger LOGGER = LoggerFactory.getLogger(VaultCredentialsProvider.class);
Review Comment:
Nit: There seems to be a Drill convention to call the logger `logger` in all lower case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jnturton commented on a diff in pull request #2558: DRILL-8232: Add support for user credentials to VaultCredentialsProvider
Posted by GitBox <gi...@apache.org>.
jnturton commented on code in PR #2558:
URL: https://github.com/apache/drill/pull/2558#discussion_r882972296
##########
docs/dev/PluginCredentialsProvider.md:
##########
@@ -118,7 +121,7 @@ Once it is set, we can configure storage plugin to use this way of obtaining cre
}
```
-`secretPath` property specifies the Vault key value from which to read
+`secretPath` property specifies the Vault key value from which to read. If the plugin's `authMode` is set to `user_translation` then the `secretPath` may include a variable named `$user` which will be replaced with the Drill query username at query execution time.
Review Comment:
@cgivre it's still optional and you could deliberately omit it as a trick to make user_translation translate to a single shared set of creds. To me me it seemed okay to allow that...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jnturton commented on pull request #2558: DRILL-8232: Add support for user credentials to VaultCredentialsProvider
Posted by GitBox <gi...@apache.org>.
jnturton commented on PR #2558:
URL: https://github.com/apache/drill/pull/2558#issuecomment-1138533245
Please note that this PR includes a breaking change in that the Vault credentials provider switches from using a static Vault token to using a static app role to obtain tokens dynamically.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a diff in pull request #2558: DRILL-8232: Add support for user credentials to VaultCredentialsProvider
Posted by GitBox <gi...@apache.org>.
cgivre commented on code in PR #2558:
URL: https://github.com/apache/drill/pull/2558#discussion_r882962178
##########
docs/dev/PluginCredentialsProvider.md:
##########
@@ -118,7 +121,7 @@ Once it is set, we can configure storage plugin to use this way of obtaining cre
}
```
-`secretPath` property specifies the Vault key value from which to read
+`secretPath` property specifies the Vault key value from which to read. If the plugin's `authMode` is set to `user_translation` then the `secretPath` may include a variable named `$user` which will be replaced with the Drill query username at query execution time.
Review Comment:
NIT/Question: If `user_translation` mode is enabled, isn't it a requirement that the `secretPath` contain `$user`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org