You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/01/12 10:22:39 UTC
[GitHub] [cassandra] bereng opened a new pull request #1398: PasswordObfuscator should not assume PASSWORD is the last item in the…
bereng opened a new pull request #1398:
URL: https://github.com/apache/cassandra/pull/1398
… WITH clause
--
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
[GitHub] [cassandra] bereng commented on a change in pull request #1398: PasswordObfuscator should not assume PASSWORD is the last item in the…
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #1398:
URL: https://github.com/apache/cassandra/pull/1398#discussion_r783649458
##########
File path: src/java/org/apache/cassandra/audit/AuditLogManager.java
##########
@@ -154,7 +163,21 @@ else if (e instanceof AuthenticationException)
builder.setType(AuditLogEntryType.REQUEST_FAILURE);
}
- builder.appendToOperation(QueryEvents.instance.getObfuscator().obfuscate(e.getMessage()));
+ // A syntax error may reveal the password in the form of 'line 1:33 mismatched input 'secret_password''
+ String safeMsg = e.getMessage();
+ if (e instanceof SyntaxException && queries != null && !queries.isEmpty())
+ {
+ for (String query : queries)
+ {
+ if (query.toLowerCase().contains(PasswordObfuscator.PASSWORD_TOKEN))
+ {
+ safeMsg = "Syntax Exception. Obscured for security reasons.";
+ break;
+ }
+ }
+ }
+
+ builder.appendToOperation(PasswordObfuscator.obfuscate(safeMsg));
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
[GitHub] [cassandra] bereng commented on pull request #1398: PasswordObfuscator should not assume PASSWORD is the last item in the…
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #1398:
URL: https://github.com/apache/cassandra/pull/1398#issuecomment-1010922252
CI [here](https://app.circleci.com/pipelines/github/bereng/cassandra?branch=CASSANDRA-16801-trunk&filter=all)
--
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
[GitHub] [cassandra] blerer commented on a change in pull request #1398: PasswordObfuscator should not assume PASSWORD is the last item in the…
Posted by GitBox <gi...@apache.org>.
blerer commented on a change in pull request #1398:
URL: https://github.com/apache/cassandra/pull/1398#discussion_r783836720
##########
File path: src/java/org/apache/cassandra/cql3/QueryEvents.java
##########
@@ -250,10 +242,19 @@ public void notifyPrepareFailure(@Nullable CQLStatement statement, String query,
}
}
- private String possiblyObfuscateQuery(CQLStatement statement, String query)
+ private String maybeObfuscatePassword(CQLStatement statement, String query)
{
// Statement might be null as side-effect of failed parsing, originates from QueryMessage#execute
- return null == statement || statement instanceof AuthenticationStatement ? passwordObfuscator.obfuscate(query) : query;
+ String res;
+
+ if (statement == null)
+ res = PasswordObfuscator.obfuscate(query);
+ else if (statement instanceof AuthenticationStatement)
+ res = ((AuthenticationStatement) statement).obfuscatePassword(query);
+ else
+ res = query;
+
+ return res;
Review comment:
I do not believe that the impact of branch misprediction will be high in this case. This part of code is also not on the critical path so I would clearly prefer clarity. :-)
--
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
[GitHub] [cassandra] bereng commented on pull request #1398: PasswordObfuscator should not assume PASSWORD is the last item in the…
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #1398:
URL: https://github.com/apache/cassandra/pull/1398#issuecomment-1012815011
CI [j11](https://app.circleci.com/pipelines/github/bereng/cassandra/547/workflows/e068410a-4a97-4353-ba6d-40766d8b0e95)
CI [j8](https://app.circleci.com/pipelines/github/bereng/cassandra/547/workflows/8660793e-2471-4454-899b-db6fe49827c9)
--
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
[GitHub] [cassandra] bereng commented on a change in pull request #1398: PasswordObfuscator should not assume PASSWORD is the last item in the…
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #1398:
URL: https://github.com/apache/cassandra/pull/1398#discussion_r783642456
##########
File path: src/java/org/apache/cassandra/cql3/QueryEvents.java
##########
@@ -250,10 +242,19 @@ public void notifyPrepareFailure(@Nullable CQLStatement statement, String query,
}
}
- private String possiblyObfuscateQuery(CQLStatement statement, String query)
+ private String maybeObfuscatePassword(CQLStatement statement, String query)
{
// Statement might be null as side-effect of failed parsing, originates from QueryMessage#execute
- return null == statement || statement instanceof AuthenticationStatement ? passwordObfuscator.obfuscate(query) : query;
+ String res;
+
+ if (statement == null)
+ res = PasswordObfuscator.obfuscate(query);
+ else if (statement instanceof AuthenticationStatement)
+ res = ((AuthenticationStatement) statement).obfuscatePassword(query);
+ else
+ res = query;
+
+ return res;
Review comment:
Funny. I'd swore I replied already to this in the 4.0 PR but I can't find my answer anywhere now.
This is intentional for performance. On branching you want to avoid returning results as this breaks CPU branching optimizations/prediction. You want the construct I used (as per the perf training we did)
--
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
[GitHub] [cassandra] bereng commented on a change in pull request #1398: PasswordObfuscator should not assume PASSWORD is the last item in the…
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #1398:
URL: https://github.com/apache/cassandra/pull/1398#discussion_r783850937
##########
File path: src/java/org/apache/cassandra/cql3/QueryEvents.java
##########
@@ -250,10 +242,19 @@ public void notifyPrepareFailure(@Nullable CQLStatement statement, String query,
}
}
- private String possiblyObfuscateQuery(CQLStatement statement, String query)
+ private String maybeObfuscatePassword(CQLStatement statement, String query)
{
// Statement might be null as side-effect of failed parsing, originates from QueryMessage#execute
- return null == statement || statement instanceof AuthenticationStatement ? passwordObfuscator.obfuscate(query) : query;
+ String res;
+
+ if (statement == null)
+ res = PasswordObfuscator.obfuscate(query);
+ else if (statement instanceof AuthenticationStatement)
+ res = ((AuthenticationStatement) statement).obfuscatePassword(query);
+ else
+ res = query;
+
+ return res;
Review comment:
I read my option clearer. I think it comes down to personal taste. Changed and pushed. Waiting for CI now.
--
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
[GitHub] [cassandra] bereng commented on a change in pull request #1398: PasswordObfuscator should not assume PASSWORD is the last item in the…
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #1398:
URL: https://github.com/apache/cassandra/pull/1398#discussion_r783642456
##########
File path: src/java/org/apache/cassandra/cql3/QueryEvents.java
##########
@@ -250,10 +242,19 @@ public void notifyPrepareFailure(@Nullable CQLStatement statement, String query,
}
}
- private String possiblyObfuscateQuery(CQLStatement statement, String query)
+ private String maybeObfuscatePassword(CQLStatement statement, String query)
{
// Statement might be null as side-effect of failed parsing, originates from QueryMessage#execute
- return null == statement || statement instanceof AuthenticationStatement ? passwordObfuscator.obfuscate(query) : query;
+ String res;
+
+ if (statement == null)
+ res = PasswordObfuscator.obfuscate(query);
+ else if (statement instanceof AuthenticationStatement)
+ res = ((AuthenticationStatement) statement).obfuscatePassword(query);
+ else
+ res = query;
+
+ return res;
Review comment:
Funny. I'd swore I replied already to this in the 4.0 PR but I can't find my answer anywhere now.
This is intentional for performance. On branching you want to avoid returning results as this breaks CPU branching optimizations/prediction. You want the construct I used (as per the perf training we did).
Also on a personal note, I don't find it any harder to read than the alternative :shrug:
--
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
[GitHub] [cassandra] blerer commented on a change in pull request #1398: PasswordObfuscator should not assume PASSWORD is the last item in the…
Posted by GitBox <gi...@apache.org>.
blerer commented on a change in pull request #1398:
URL: https://github.com/apache/cassandra/pull/1398#discussion_r783055507
##########
File path: src/java/org/apache/cassandra/cql3/QueryEvents.java
##########
@@ -250,10 +242,19 @@ public void notifyPrepareFailure(@Nullable CQLStatement statement, String query,
}
}
- private String possiblyObfuscateQuery(CQLStatement statement, String query)
+ private String maybeObfuscatePassword(CQLStatement statement, String query)
{
// Statement might be null as side-effect of failed parsing, originates from QueryMessage#execute
- return null == statement || statement instanceof AuthenticationStatement ? passwordObfuscator.obfuscate(query) : query;
+ String res;
+
+ if (statement == null)
+ res = PasswordObfuscator.obfuscate(query);
+ else if (statement instanceof AuthenticationStatement)
+ res = ((AuthenticationStatement) statement).obfuscatePassword(query);
+ else
+ res = query;
+
+ return res;
Review comment:
This code can be simplified to:
```
if (statement == null)
return PasswordObfuscator.obfuscate(query);
if (statement instanceof AuthenticationStatement)
return ((AuthenticationStatement) statement).obfuscatePassword(query);
return query;
```
It is much easier to read.
##########
File path: src/java/org/apache/cassandra/audit/AuditLogManager.java
##########
@@ -154,7 +163,21 @@ else if (e instanceof AuthenticationException)
builder.setType(AuditLogEntryType.REQUEST_FAILURE);
}
- builder.appendToOperation(QueryEvents.instance.getObfuscator().obfuscate(e.getMessage()));
+ // A syntax error may reveal the password in the form of 'line 1:33 mismatched input 'secret_password''
+ String safeMsg = e.getMessage();
+ if (e instanceof SyntaxException && queries != null && !queries.isEmpty())
+ {
+ for (String query : queries)
+ {
+ if (query.toLowerCase().contains(PasswordObfuscator.PASSWORD_TOKEN))
+ {
+ safeMsg = "Syntax Exception. Obscured for security reasons.";
+ break;
+ }
+ }
+ }
+
+ builder.appendToOperation(PasswordObfuscator.obfuscate(safeMsg));
Review comment:
My proposal to extract the logic in a separate method was mainly to allow the refactoring of that part of the code. The current logic is confusing because we initialize the safeMsg value to something and then check if it should be changed, if not we keep the initial value. That part is made even more confusing by the fact that we try to obfuscate the message even if we know that it is unecessary. A simpler logic is to check if we should obscured the message otherwise use the obfuscated error message. In this case my previous proposal of extracting the method made the logic easier to read. I got confused multiple time by this piece of code which is why I believe that a rewrite is necessary. :-)
--
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
[GitHub] [cassandra] bereng commented on pull request #1398: PasswordObfuscator should not assume PASSWORD is the last item in the…
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #1398:
URL: https://github.com/apache/cassandra/pull/1398#issuecomment-1011844777
CI [j11](https://app.circleci.com/pipelines/github/bereng/cassandra/544/workflows/e1a0c065-20ef-4251-b437-f3ea77f64c1a)
CI [j8](https://app.circleci.com/pipelines/github/bereng/cassandra/544/workflows/ba76c50c-7b01-4e33-9c6c-1333a059286b)
Failures are unrelated or we can see they are GH timeouts which worked ok in the 'sister' jobs and in previous runs.
--
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
[GitHub] [cassandra] bereng commented on pull request #1398: PasswordObfuscator should not assume PASSWORD is the last item in the…
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #1398:
URL: https://github.com/apache/cassandra/pull/1398#issuecomment-1020016616
Rebased, squahsed and latest nits added.
CI [j8](https://app.circleci.com/pipelines/github/bereng/cassandra/572/workflows/1579dd49-2b94-472c-8087-3b85afd14435)
CI [j11](https://app.circleci.com/pipelines/github/bereng/cassandra/572/workflows/c492b5a6-05b2-40fa-8ecc-9e9c856582f9)
Aligned to current failures LGTM
--
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
[GitHub] [cassandra] smiklosovic closed pull request #1398: PasswordObfuscator should not assume PASSWORD is the last item in the…
Posted by GitBox <gi...@apache.org>.
smiklosovic closed pull request #1398:
URL: https://github.com/apache/cassandra/pull/1398
--
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