You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "maulin-vasavada (via GitHub)" <gi...@apache.org> on 2025/02/18 04:08:49 UTC
[PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
maulin-vasavada opened a new pull request, #3908:
URL: https://github.com/apache/cassandra/pull/3908
…ord_file options
Thanks for sending a pull request! Here are some tips if you're new here:
* Ensure you have added or run the [appropriate tests](https://cassandra.apache.org/_/development/testing.html) for your PR.
* Be sure to keep the PR description updated to reflect all changes.
* Write your PR title to summarize what this PR proposes.
* If possible, provide a concise example to reproduce the issue for a faster review.
* Read our [contributor guidelines](https://cassandra.apache.org/_/development/index.html)
* If you're making a documentation change, see our [guide to documentation contribution](https://cassandra.apache.org/_/development/documentation.html)
Commit messages should follow the following format:
```
<One sentence description, usually Jira title or CHANGES.txt summary>
<Optional lengthier description (context on patch)>
patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####
Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>
```
The [Cassandra Jira](https://issues.apache.org/jira/projects/CASSANDRA/issues/)
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3908:
URL: https://github.com/apache/cassandra/pull/3908#discussion_r1975004720
##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -294,5 +301,38 @@ protected boolean passwordMatchesIfPresent(String keyPassword)
{
return StringUtils.isEmpty(password) || keyPassword.equals(password);
}
+
+ private static String resolvePassword(String keystoreFilePath, String password, String passwordFilePath)
+ {
+ if (password != null)
+ return password;
+
+ if (StringUtils.isEmpty(passwordFilePath))
+ return password;
Review Comment:
I have tested that it is indeed true, `a_config:` will be `null` in the code and `a_config:""` will be `""` (empty string) in the code.
What we do not seem to support is "empty string in a file". If a `password` config is commented out and `password_file` is not, then it will read it and it will get 0 lines. So in that case, I think that we should return `""` - empty password. Because having empty file (its size is 0) represents a file _with an empty password_. Not _null password_.
If there is no password at all, meaning `null`, both password and password_file configuration properties will be commented out.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
Posted by "maulin-vasavada (via GitHub)" <gi...@apache.org>.
maulin-vasavada commented on code in PR #3908:
URL: https://github.com/apache/cassandra/pull/3908#discussion_r1976128180
##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -294,5 +301,38 @@ protected boolean passwordMatchesIfPresent(String keyPassword)
{
return StringUtils.isEmpty(password) || keyPassword.equals(password);
}
+
+ private static String resolvePassword(String keystoreFilePath, String password, String passwordFilePath)
+ {
+ if (password != null)
+ return password;
+
+ if (StringUtils.isEmpty(passwordFilePath))
+ return password;
Review Comment:
@smiklosovic and @Maxwell-Guo you both are right. I think I had a hangover from a previous change :) I corrected it to return null when password file path is empty and all the security package unit tests are passing locally.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3908:
URL: https://github.com/apache/cassandra/pull/3908#discussion_r1974970152
##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -294,5 +301,38 @@ protected boolean passwordMatchesIfPresent(String keyPassword)
{
return StringUtils.isEmpty(password) || keyPassword.equals(password);
}
+
+ private static String resolvePassword(String keystoreFilePath, String password, String passwordFilePath)
+ {
+ if (password != null)
+ return password;
+
+ if (StringUtils.isEmpty(passwordFilePath))
+ return password;
Review Comment:
@maulin-vasavada I dont understand. Returning of empty password when it is empty is already covered by `if (password != null) return password`.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic closed pull request #3908: CASSANDRA-13428 - provide keystore_password_file and truststore_passw…
URL: https://github.com/apache/cassandra/pull/3908
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3908:
URL: https://github.com/apache/cassandra/pull/3908#discussion_r1975005152
##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -294,5 +301,38 @@ protected boolean passwordMatchesIfPresent(String keyPassword)
{
return StringUtils.isEmpty(password) || keyPassword.equals(password);
}
+
+ private static String resolvePassword(String keystoreFilePath, String password, String passwordFilePath)
+ {
+ if (password != null)
+ return password;
+
+ if (StringUtils.isEmpty(passwordFilePath))
+ return password;
+
+ File keystorePasswordFile = new File(passwordFilePath);
+
+ if (!keystorePasswordFile.exists())
+ {
+ final String msg = format("keystore password file %s does not exist", keystorePasswordFile.path());
+ throw new ConfigurationException(msg);
+ }
+
+ try
+ {
+ // we expect a password to be on the first line
+ List<String> lines = FileUtils.readLines(keystorePasswordFile);
+ if (lines.isEmpty())
+ return null;
Review Comment:
@maulin-vasavada here we should return `""`.
and file stuff should be like:
if (StringUtils.isEmpty(passwordFilePath))
return null;
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3908:
URL: https://github.com/apache/cassandra/pull/3908#discussion_r1966744304
##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -308,12 +308,15 @@ private static String resolvePassword(String keystoreFilePath, String password,
return password;
if (StringUtils.isEmpty(passwordFilePath))
- return null;
+ return password;
Review Comment:
Aha I see what you did here. OK. Because `isEmpty(password)` will return false only if it is not null AND String does not have length of 0.
But here you covered the case of returning password of empty string instead of null ...
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
Posted by "maulin-vasavada (via GitHub)" <gi...@apache.org>.
maulin-vasavada commented on code in PR #3908:
URL: https://github.com/apache/cassandra/pull/3908#discussion_r1974208707
##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -294,5 +301,38 @@ protected boolean passwordMatchesIfPresent(String keyPassword)
{
return StringUtils.isEmpty(password) || keyPassword.equals(password);
}
+
+ private static String resolvePassword(String keystoreFilePath, String password, String passwordFilePath)
+ {
+ if (password != null)
+ return password;
+
+ if (StringUtils.isEmpty(passwordFilePath))
+ return password;
Review Comment:
So there are existing test cases- [link](https://github.com/apache/cassandra/pull/3908/files#diff-877c6be441d503a35c8402b7277fa7fb2e860616d1f11161d1f61250f58146a2R96) - which ensures that we don't fail on `""` password. This is essentially to just support those. Here the intent is to return whatever `password` has when the file path is empty. So in case when the password is not null but empty, we will return empty instead of null.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3908:
URL: https://github.com/apache/cassandra/pull/3908#discussion_r1974978920
##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -294,5 +301,38 @@ protected boolean passwordMatchesIfPresent(String keyPassword)
{
return StringUtils.isEmpty(password) || keyPassword.equals(password);
}
+
+ private static String resolvePassword(String keystoreFilePath, String password, String passwordFilePath)
+ {
+ if (password != null)
+ return password;
+
+ if (StringUtils.isEmpty(passwordFilePath))
+ return password;
Review Comment:
Having logic
if (password != null)
return password;
if (StringUtils.isEmpty(passwordFile))
return null;
Then having this in yaml returns:
# password:
# password_file:
In this case, it will return null
password:
# password_file
in this case it will return null
password: ""
# password_file
in this case it will return empty
# password
password file: abc
in this case it will return what is in abc or null if file is empty.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3908:
URL: https://github.com/apache/cassandra/pull/3908#discussion_r1975005152
##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -294,5 +301,38 @@ protected boolean passwordMatchesIfPresent(String keyPassword)
{
return StringUtils.isEmpty(password) || keyPassword.equals(password);
}
+
+ private static String resolvePassword(String keystoreFilePath, String password, String passwordFilePath)
+ {
+ if (password != null)
+ return password;
+
+ if (StringUtils.isEmpty(passwordFilePath))
+ return password;
+
+ File keystorePasswordFile = new File(passwordFilePath);
+
+ if (!keystorePasswordFile.exists())
+ {
+ final String msg = format("keystore password file %s does not exist", keystorePasswordFile.path());
+ throw new ConfigurationException(msg);
+ }
+
+ try
+ {
+ // we expect a password to be on the first line
+ List<String> lines = FileUtils.readLines(keystorePasswordFile);
+ if (lines.isEmpty())
+ return null;
Review Comment:
@maulin-vasavada here we should return `""`.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #3908:
URL: https://github.com/apache/cassandra/pull/3908#discussion_r1973535463
##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -294,5 +301,38 @@ protected boolean passwordMatchesIfPresent(String keyPassword)
{
return StringUtils.isEmpty(password) || keyPassword.equals(password);
}
+
+ private static String resolvePassword(String keystoreFilePath, String password, String passwordFilePath)
+ {
+ if (password != null)
+ return password;
+
+ if (StringUtils.isEmpty(passwordFilePath))
+ return password;
+
+ File keystorePasswordFile = new File(passwordFilePath);
+
+ if (!keystorePasswordFile.exists())
+ {
+ final String msg = format("keystore password file %s does not exist", keystorePasswordFile.path());
+ throw new ConfigurationException(msg);
+ }
+
+ try
+ {
+ // we expect a password to be on the first line
+ List<String> lines = FileUtils.readLines(keystorePasswordFile);
Review Comment:
should we skip the first line if it is empty ?
I tested if we set two lines in a password_file with
```
cassandra
```
then I got two lines here with first line of "".
So does this meet our expectations?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
Posted by "maulin-vasavada (via GitHub)" <gi...@apache.org>.
maulin-vasavada commented on code in PR #3908:
URL: https://github.com/apache/cassandra/pull/3908#discussion_r1974215190
##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -294,5 +301,38 @@ protected boolean passwordMatchesIfPresent(String keyPassword)
{
return StringUtils.isEmpty(password) || keyPassword.equals(password);
}
+
+ private static String resolvePassword(String keystoreFilePath, String password, String passwordFilePath)
+ {
+ if (password != null)
+ return password;
+
+ if (StringUtils.isEmpty(passwordFilePath))
+ return password;
+
+ File keystorePasswordFile = new File(passwordFilePath);
+
+ if (!keystorePasswordFile.exists())
+ {
+ final String msg = format("keystore password file %s does not exist", keystorePasswordFile.path());
+ throw new ConfigurationException(msg);
+ }
+
+ try
+ {
+ // we expect a password to be on the first line
+ List<String> lines = FileUtils.readLines(keystorePasswordFile);
Review Comment:
I feel it should be okay to have the real first line expectation. Any password configuration gets many eyes for review generally so it should be straightforward to remove first empty line if that got there by accident. Also, as you note we do not support comments either with `#` in this file preceding the password. For me, that is intentional. I don't see a strong reason to make it less strict by allowing empty first line.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3908:
URL: https://github.com/apache/cassandra/pull/3908#discussion_r1974978920
##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -294,5 +301,38 @@ protected boolean passwordMatchesIfPresent(String keyPassword)
{
return StringUtils.isEmpty(password) || keyPassword.equals(password);
}
+
+ private static String resolvePassword(String keystoreFilePath, String password, String passwordFilePath)
+ {
+ if (password != null)
+ return password;
+
+ if (StringUtils.isEmpty(passwordFilePath))
+ return password;
Review Comment:
Having logic
if (password != null)
return password;
if (StringUtils.isEmpty(passwordFile))
return null;
Then having this in yaml returns:
# password:
# password_file:
In this case, it will return null
password:
# password_file
in this case it will return null
password: ""
# password_file
in this case it will return empty
# password
password file: abc
in this case it will return what is in abc or null if file is empty.
Do you want to say that we need to cover a case when a file specified and exists but its length is zero? That way it should be returning null.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3908:
URL: https://github.com/apache/cassandra/pull/3908#discussion_r1974992498
##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -294,5 +301,38 @@ protected boolean passwordMatchesIfPresent(String keyPassword)
{
return StringUtils.isEmpty(password) || keyPassword.equals(password);
}
+
+ private static String resolvePassword(String keystoreFilePath, String password, String passwordFilePath)
+ {
+ if (password != null)
+ return password;
+
+ if (StringUtils.isEmpty(passwordFilePath))
+ return password;
+
+ File keystorePasswordFile = new File(passwordFilePath);
+
+ if (!keystorePasswordFile.exists())
+ {
+ final String msg = format("keystore password file %s does not exist", keystorePasswordFile.path());
+ throw new ConfigurationException(msg);
+ }
+
+ try
+ {
+ // we expect a password to be on the first line
+ List<String> lines = FileUtils.readLines(keystorePasswordFile);
Review Comment:
yeah this is fine as it is
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3908:
URL: https://github.com/apache/cassandra/pull/3908#discussion_r1974991299
##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -294,5 +301,38 @@ protected boolean passwordMatchesIfPresent(String keyPassword)
{
return StringUtils.isEmpty(password) || keyPassword.equals(password);
}
+
+ private static String resolvePassword(String keystoreFilePath, String password, String passwordFilePath)
+ {
+ if (password != null)
+ return password;
+
+ if (StringUtils.isEmpty(passwordFilePath))
+ return password;
Review Comment:
If we do
if (password != null)
return password;
Is not this returning empty ("") already when in yaml it is `password: ""`?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #3908:
URL: https://github.com/apache/cassandra/pull/3908#discussion_r1978608485
##########
conf/cassandra_latest.yaml:
##########
@@ -1718,8 +1739,17 @@ client_encryption_options:
# accepted_protocols: [TLSv1.2,TLSv1.3,TLSv1.1]
# keystore: conf/cassandra_ssl.keystore
# keystore_password: cassandra
+ # Optional configuration to specify password for keystore in a separate file
+ # When keystore_password and keystore_password_file both are specified, the keystore_password will take precedence
+ # The password in the file should be on the first line
+ # keystore_password_file: conf/keystore_passwordfile.txt
# truststore: conf/cassandra_ssl.truststore
# truststore_password: cassandra
+ # Optional configuration to specify password for truststore in a separate file
+ # When truststore_password and truststore_password_file both are specified, the truststore_password will take precedence
+ # The password in the file should be on the first line
+ # truststore_password_file: conf/truststore_passwordfile.txt
+
Review Comment:
I think we can remove this line , and we can do this on commit.
##########
conf/cassandra.yaml:
##########
@@ -1844,8 +1865,17 @@ client_encryption_options:
# accepted_protocols: [TLSv1.2,TLSv1.3,TLSv1.1]
# keystore: conf/cassandra_ssl.keystore
# keystore_password: cassandra
+ # Optional configuration to specify password for keystore in a separate file
+ # When keystore_password and keystore_password_file both are specified, the keystore_password will take precedence
+ # The password in the file should be on the first line
+ # keystore_password_file: conf/keystore_passwordfile.txt
# truststore: conf/cassandra_ssl.truststore
# truststore_password: cassandra
+ # Optional configuration to specify password for truststore in a separate file
+ # When truststore_password and truststore_password_file both are specified, the truststore_password will take precedence
+ # The password in the file should be on the first line
+ # truststore_password_file: conf/truststore_passwordfile.txt
+
Review Comment:
I think we can remove this line , and we can do this on commit.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #3908:
URL: https://github.com/apache/cassandra/pull/3908#discussion_r1973437100
##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -294,5 +301,38 @@ protected boolean passwordMatchesIfPresent(String keyPassword)
{
return StringUtils.isEmpty(password) || keyPassword.equals(password);
}
+
+ private static String resolvePassword(String keystoreFilePath, String password, String passwordFilePath)
+ {
+ if (password != null)
+ return password;
+
+ if (StringUtils.isEmpty(passwordFilePath))
+ return password;
Review Comment:
what about just return null here ?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3908:
URL: https://github.com/apache/cassandra/pull/3908#discussion_r1966743811
##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -308,12 +308,15 @@ private static String resolvePassword(String keystoreFilePath, String password,
return password;
if (StringUtils.isEmpty(passwordFilePath))
- return null;
+ return password;
Review Comment:
@maulin-vasavada strange change, `password` here will be `null` anyway because if it was not then we would return in the `if` above.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3908:
URL: https://github.com/apache/cassandra/pull/3908#discussion_r1974978920
##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -294,5 +301,38 @@ protected boolean passwordMatchesIfPresent(String keyPassword)
{
return StringUtils.isEmpty(password) || keyPassword.equals(password);
}
+
+ private static String resolvePassword(String keystoreFilePath, String password, String passwordFilePath)
+ {
+ if (password != null)
+ return password;
+
+ if (StringUtils.isEmpty(passwordFilePath))
+ return password;
Review Comment:
Having logic
if (password != null)
return password;
if (StringUtils.isEmpty(passwordFile))
return null;
Then having this in yaml returns:
# password:
# password_file:
In this case, it will return null
password:
# password_file
in this case it will return null
password: ""
# password_file
in this case it will return empty
# password
password file: abc
in this case it will return what is in abc or null
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
Posted by "maulin-vasavada (via GitHub)" <gi...@apache.org>.
maulin-vasavada commented on code in PR #3908:
URL: https://github.com/apache/cassandra/pull/3908#discussion_r1976129989
##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -294,5 +301,38 @@ protected boolean passwordMatchesIfPresent(String keyPassword)
{
return StringUtils.isEmpty(password) || keyPassword.equals(password);
}
+
+ private static String resolvePassword(String keystoreFilePath, String password, String passwordFilePath)
+ {
+ if (password != null)
+ return password;
+
+ if (StringUtils.isEmpty(passwordFilePath))
+ return password;
+
+ File keystorePasswordFile = new File(passwordFilePath);
+
+ if (!keystorePasswordFile.exists())
+ {
+ final String msg = format("keystore password file %s does not exist", keystorePasswordFile.path());
+ throw new ConfigurationException(msg);
+ }
+
+ try
+ {
+ // we expect a password to be on the first line
+ List<String> lines = FileUtils.readLines(keystorePasswordFile);
+ if (lines.isEmpty())
+ return null;
Review Comment:
done
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3908:
URL: https://github.com/apache/cassandra/pull/3908#discussion_r1975004720
##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -294,5 +301,38 @@ protected boolean passwordMatchesIfPresent(String keyPassword)
{
return StringUtils.isEmpty(password) || keyPassword.equals(password);
}
+
+ private static String resolvePassword(String keystoreFilePath, String password, String passwordFilePath)
+ {
+ if (password != null)
+ return password;
+
+ if (StringUtils.isEmpty(passwordFilePath))
+ return password;
Review Comment:
I have tested that it is indeed true, `a_config:` will be `null` in the code and `a_config:""` will be `""` (empty string) in the code.
What we do not seem to support is "empty string in a file". If a `password` config is commented out and `password_file` is not, then it will read it and it will get 0 lines. So in that case, I think that we should return `""` - empty password. Because having empty file (its size is 0) represents a file _with an empty password_. Not _null password_.
If there is no password at all, both password and password_file configuration properties will be commented out.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #3908:
URL: https://github.com/apache/cassandra/pull/3908#discussion_r1974683637
##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -294,5 +301,38 @@ protected boolean passwordMatchesIfPresent(String keyPassword)
{
return StringUtils.isEmpty(password) || keyPassword.equals(password);
}
+
+ private static String resolvePassword(String keystoreFilePath, String password, String passwordFilePath)
+ {
+ if (password != null)
+ return password;
+
+ if (StringUtils.isEmpty(passwordFilePath))
+ return password;
+
+ File keystorePasswordFile = new File(passwordFilePath);
+
+ if (!keystorePasswordFile.exists())
+ {
+ final String msg = format("keystore password file %s does not exist", keystorePasswordFile.path());
+ throw new ConfigurationException(msg);
+ }
+
+ try
+ {
+ // we expect a password to be on the first line
+ List<String> lines = FileUtils.readLines(keystorePasswordFile);
Review Comment:
@smiklosovic WDYT ?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3908:
URL: https://github.com/apache/cassandra/pull/3908#discussion_r1973526592
##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -294,5 +301,38 @@ protected boolean passwordMatchesIfPresent(String keyPassword)
{
return StringUtils.isEmpty(password) || keyPassword.equals(password);
}
+
+ private static String resolvePassword(String keystoreFilePath, String password, String passwordFilePath)
+ {
+ if (password != null)
+ return password;
+
+ if (StringUtils.isEmpty(passwordFilePath))
+ return password;
Review Comment:
@maulin-vasavada that is what I was mentioning here
https://issues.apache.org/jira/browse/CASSANDRA-13428?focusedCommentId=17929657&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17929657
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-13428 - provide keystore_password_file and truststore_passw… [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3908:
URL: https://github.com/apache/cassandra/pull/3908#discussion_r1967320667
##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -308,12 +308,15 @@ private static String resolvePassword(String keystoreFilePath, String password,
return password;
if (StringUtils.isEmpty(passwordFilePath))
- return null;
+ return password;
Review Comment:
@maulin-vasavada I have changed my mind: https://issues.apache.org/jira/browse/CASSANDRA-13428?focusedCommentId=17929657&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17929657
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org