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



##########
File path: src/java/org/apache/cassandra/audit/AuditLogManager.java
##########
@@ -135,7 +143,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 != null && e instanceof SyntaxException && queries != null && !queries.isEmpty())

Review comment:
       The `e != null` check can be removed apprently as the line above would have fired a NPE otherwise.

##########
File path: src/java/org/apache/cassandra/cql3/PasswordObfuscator.java
##########
@@ -35,6 +46,26 @@ 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

Review comment:
       No real need for the `else`

##########
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);

Review comment:
       we can simply return the result it will avoid the need for the else-if

##########
File path: src/java/org/apache/cassandra/audit/AuditLogManager.java
##########
@@ -135,7 +143,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 != null && 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:
       I would have use extracted the logic creating the message into:
   ```
   public String obfuscatePasswordInformation(Exception e, List<String> queries)
   {
       if (e instanceof SyntaxException && queries != null && !queries.isEmpty())
       {
           for (String query : queries)
           {
               if (query.toLowerCase().contains(PasswordObfuscator.PASSWORD_TOKEN))
               {
                   return "Syntax Exception. Obscured for security reasons.";
               }
           }
       }
   
       return PasswordObfuscator.obfuscate(e.getMessage());
   }
   ```




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