[GitHub] [cassandra] blerer commented on a change in pull request #1293: PasswordObfuscator should not assume PASSWORD is the last item in the…

[GitBox <gi...@apache.org>]

blerer commented on a change in pull request #1293:
URL: https://github.com/apache/cassandra/pull/1293#discussion_r741933504



##########
File path: src/java/org/apache/cassandra/cql3/PasswordObfuscator.java
##########
@@ -35,6 +46,25 @@ public String obfuscate(String sourceString)
         if (passwordTokenStartIndex < 0)
             return sourceString;
 
-        return sourceString.substring(0, passwordTokenStartIndex + PASSWORD_TOKEN.length()) + OBFUSCATION_TOKEN;
+        return sourceString.substring(0, passwordTokenStartIndex + PASSWORD_TOKEN.length()) + " " + OBFUSCATION_TOKEN;
+    }
+
+    /**
+     * Obfuscates the passwords in a query
+     * 
+     * @param query The query whose passwords to obfuscate
+     * @param opts The options containing the password to obfuscate
+     * @return The query with obfuscated passwords
+     */
+    public static String obfuscate(String query, RoleOptions opts)
+    {
+        if (opts == null || query == null || query.isEmpty())
+            return query;
+
+        Optional<String> pass = opts.getPassword();
+        if (!pass.isPresent() || pass.get().isEmpty())
+            return query;
+        else
+            return query.replaceAll(pass.get(), PasswordObfuscator.OBFUSCATION_TOKEN);

Review comment:
       Using `replaceAll` does not look safe to me. If the password is `role` for example we will end up with a wrongly obfuscated statement that can reveal the actual password.  

##########
File path: src/java/org/apache/cassandra/audit/AuditLogManager.java
##########
@@ -135,7 +136,13 @@ 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 instanceof SyntaxException
+                         && e.getMessage().toLowerCase().contains(PasswordObfuscator.PASSWORD_TOKEN.toLowerCase())
+                             ? "Syntax Exception. Obscured for security reasons."
+                             : e.getMessage();
+
+        builder.appendToOperation(PasswordObfuscator.obfuscate(safeMsg));

Review comment:
       I do not believe that this logic is really bulletproof.
    
   It is not guaranty that the `PASSWORD_TOKEN`  will be part of the message depending on how the original query was formatted. The `ErrorCollector` slicing logic might let that part our depending on where is the syntax issue and how is the query formatted (multiple lines for example).
   
   Another use case that might go undetected by the logic is a typo on the password keyword (eg. pssword)




-- 
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