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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
