CASSSIDECAR-407 Pluggable withCredentials provider for file access#318
CASSSIDECAR-407 Pluggable withCredentials provider for file access#318burmanm wants to merge 3 commits intoapache:trunkfrom
Conversation
server/src/main/java/org/apache/cassandra/sidecar/modules/ConfigurationModule.java
Outdated
Show resolved
Hide resolved
|
Thanks for the patch, I will take a look at it today |
…luggable withCredentials provider. Add two possible implementations, ConfigProvider (which is the current implementation) and a new one, FileProvider which reads username and password from the disk. Also deprecate the old username/password parameters, but do not remove support for them (simply transform their input to the ConfigProvider).
burmanm
force-pushed
the
secrets_change
branch
from
9ac1fb6 to
3ca4d57
Compare
yifan-c
left a comment
yifan-c
left a comment
There was a problem hiding this comment.
Thanks for the patch! It is a good idea to have the auth provider pluggable.
server/src/main/java/org/apache/cassandra/sidecar/cluster/auth/CqlAuthProvider.java
Show resolved
Hide resolved
| public String username() | ||
| { | ||
| return readSecret(usernamePath, USERNAME_PATH_PARAM); | ||
| } | ||
|
|
||
| @Override | ||
| public String password() | ||
| { | ||
| return readSecret(passwordPath, PASSWORD_PATH_PARAM); | ||
| } |
There was a problem hiding this comment.
It reads from the file every single time per call. Do you expect the username/password to change?
And the indentation is wrong.
There was a problem hiding this comment.
I'd expect the password/username to be modifiable, yes. Same as with TLS secrets. But if you want, I can modify this to be read-once and need to restart sidecar for changes.
Why isn't there automated test for the indentations and a formatter?
There was a problem hiding this comment.
There is an "opinionated" guide on setting up the project in IntellJ (https://github.com/apache/cassandra-sidecar/blob/trunk/CONTRIBUTING.md#integration-with-intellij-idea). It should take care of the indentation.
There is no formatter to help, if you are not using IntelliJ, ATM.
There was a problem hiding this comment.
I'd expect the password/username to be modifiable, yes. Same as with TLS secrets.
ok. I guess it is for the scenarios such as password rotation.
server/src/main/java/org/apache/cassandra/sidecar/cluster/auth/ConfigProvider.java
Show resolved
Hide resolved
server/src/main/java/org/apache/cassandra/sidecar/modules/ConfigurationModule.java
Show resolved
Hide resolved
| // Fallback to the old one | ||
| String username = driverConfiguration.username() != null ? driverConfiguration.username() : ""; | ||
| String password = driverConfiguration.password() != null ? driverConfiguration.password() : ""; |
There was a problem hiding this comment.
I do not think defaulting to empty string is the old behavior, which allows null for username and password.
The reason of defaulting to the empty string seems to only to conform the Map.of() constructor. A workaround could be wrap HashMap (which permits null value) with unmodifiable.
|
Tests are failing. please take a look |
3 participants
Add new settings under driver_parameters to support pluggable withCredentials provider. Add two possible implementations, ConfigProvider (which is the current implementation) and a new one, FileProvider which reads username and password from the disk.
Also deprecate the old username/password parameters, but do not remove support for them (simply transform their input to the ConfigProvider).