[GitHub] [cxf] ashakirin opened a new pull request #681: CXF-8099: mask sensitive logging elements

classic Classic list List threaded Threaded
27 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] ashakirin opened a new pull request #681: CXF-8099: mask sensitive logging elements

GitBox

ashakirin opened a new pull request #681:
URL: https://github.com/apache/cxf/pull/681


   Added an option to mask sensible XML and JSON elements in logging


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

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] rmannibucau commented on a change in pull request #681: CXF-8099: mask sensitive logging elements

GitBox

rmannibucau commented on a change in pull request #681:
URL: https://github.com/apache/cxf/pull/681#discussion_r446559733



##########
File path: rt/features/logging/pom.xml
##########
@@ -38,6 +38,10 @@
             <groupId>org.slf4j</groupId>
             <artifactId>slf4j-api</artifactId>
         </dependency>
+        <dependency>

Review comment:
       maybe a bit overkill for the actual usage? IMHO it would be great to drop or reuse cxf utils.




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

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] asdf913 commented on pull request #681: CXF-8099: mask sensitive logging elements

GitBox
In reply to this post by GitBox

asdf913 commented on pull request #681:
URL: https://github.com/apache/cxf/pull/681#issuecomment-650703405


   transform() method not found in **org.apache.cxf.ext.logging.AbstractLoggingInterceptor**
   ![image](https://user-images.githubusercontent.com/2301325/85939615-a6c68280-b949-11ea-88d3-c33fc40d371e.png)
   


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

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] asdf913 edited a comment on pull request #681: CXF-8099: mask sensitive logging elements

GitBox
In reply to this post by GitBox

asdf913 edited a comment on pull request #681:
URL: https://github.com/apache/cxf/pull/681#issuecomment-650703405


   transform() method not found in **org.apache.cxf.ext.logging.AbstractLoggingInterceptor** in **org.apache.cxf:cxf-rt-features-logging:3.3.7** dependency
   
   ![image](https://user-images.githubusercontent.com/2301325/85939615-a6c68280-b949-11ea-88d3-c33fc40d371e.png)
   


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

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] ashakirin commented on a change in pull request #681: CXF-8099: mask sensitive logging elements

GitBox
In reply to this post by GitBox

ashakirin commented on a change in pull request #681:
URL: https://github.com/apache/cxf/pull/681#discussion_r447274728



##########
File path: rt/features/logging/pom.xml
##########
@@ -38,6 +38,10 @@
             <groupId>org.slf4j</groupId>
             <artifactId>slf4j-api</artifactId>
         </dependency>
+        <dependency>

Review comment:
       Perhaps, I will check




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

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] ashakirin commented on pull request #681: CXF-8099: mask sensitive logging elements

GitBox
In reply to this post by GitBox

ashakirin commented on pull request #681:
URL: https://github.com/apache/cxf/pull/681#issuecomment-651389614


   > transform() method not found in **org.apache.cxf.ext.logging.AbstractLoggingInterceptor** in **org.apache.cxf:cxf-rt-features-logging:3.3.7** dependency
   
   transform() method was recently introduced and exists only in 3.4.0-SNAPSHOT version
   


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

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] asdf913 commented on pull request #681: CXF-8099: mask sensitive logging elements

GitBox
In reply to this post by GitBox

asdf913 commented on pull request #681:
URL: https://github.com/apache/cxf/pull/681#issuecomment-651683537


   Thanks you for your work!!


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

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] coheigea commented on a change in pull request #681: CXF-8099: mask sensitive logging elements

GitBox
In reply to this post by GitBox

coheigea commented on a change in pull request #681:
URL: https://github.com/apache/cxf/pull/681#discussion_r447568313



##########
File path: rt/features/logging/pom.xml
##########
@@ -38,6 +38,10 @@
             <groupId>org.slf4j</groupId>
             <artifactId>slf4j-api</artifactId>
         </dependency>
+        <dependency>

Review comment:
       Agreed, there is no need to import it just for StringUtils.containsIgnoreCase




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

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] coheigea commented on a change in pull request #681: CXF-8099: mask sensitive logging elements

GitBox
In reply to this post by GitBox

coheigea commented on a change in pull request #681:
URL: https://github.com/apache/cxf/pull/681#discussion_r447572519



##########
File path: rt/features/logging/src/main/java/org/apache/cxf/ext/logging/MaskSensitiveHelper.java
##########
@@ -0,0 +1,74 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cxf.ext.logging;
+
+import java.util.List;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.cxf.message.Message;
+
+import static org.apache.commons.lang3.ObjectUtils.isEmpty;
+
+public class MaskSensitiveHelper {
+    private static final String ELEMENT_NAME_TEMPLATE = "-ELEMENT_NAME-";
+    private static final String MATCH_PATTERN_XML = "<-ELEMENT_NAME->(.*?)</-ELEMENT_NAME->";
+    private static final String MATCH_PATTERN_JSON = "\"-ELEMENT_NAME-\"[ \\t]*:[ \\t]*\"(.*?)\"";
+    private static final String REPLACEMENT_PATTERN_XML = "<-ELEMENT_NAME->XXX</-ELEMENT_NAME->";
+    private static final String REPLACEMENT_PATTERN_JSON = "\"-ELEMENT_NAME-\": \"XXX\"";
+
+    private static final String XML_CONTENT = "xml";
+    private static final String HTML_CONTENT = "html";
+    private static final String JSON_CONTENT = "json";
+
+    final List<String> sensitiveElementNames;
+
+    public MaskSensitiveHelper(final List<String> sensitiveElementNames) {
+        this.sensitiveElementNames = sensitiveElementNames;
+    }
+
+    public String maskSensitiveElements(
+            final Message message,
+            final String originalLogString) {
+        if (isEmpty(sensitiveElementNames)) {
+            return originalLogString;
+        }
+        String contentType = (String) message.get(Message.CONTENT_TYPE);
+        if (StringUtils.containsIgnoreCase(contentType, XML_CONTENT)
+                || StringUtils.containsIgnoreCase(contentType, HTML_CONTENT)) {
+            return applyExpression(originalLogString, MATCH_PATTERN_XML, REPLACEMENT_PATTERN_XML);
+        } else if (StringUtils.containsIgnoreCase(contentType, JSON_CONTENT)) {
+            return applyExpression(originalLogString, MATCH_PATTERN_JSON, REPLACEMENT_PATTERN_JSON);
+        } else {
+            return originalLogString;
+        }
+    }
+
+    private String applyExpression(
+            final String originalLogString,
+            final String matchPatternTemplate,
+            final String replacementTemplate) {
+        String resultString = originalLogString;
+        for (final String elementName : sensitiveElementNames) {
+            final String matchPattern = matchPatternTemplate.replaceAll(ELEMENT_NAME_TEMPLATE, elementName);
+            final String replacement = replacementTemplate.replaceAll(ELEMENT_NAME_TEMPLATE, elementName);

Review comment:
       Could we find a way to do this matching in the constructor so it doesn't have to be done every time the expression is evaluated?

##########
File path: rt/features/logging/src/main/java/org/apache/cxf/ext/logging/AbstractLoggingInterceptor.java
##########
@@ -102,8 +110,12 @@ public void createExchangeId(Message message) {
         }
     }
 
-    protected String transform(final String originalLogString) {
+    protected String transform(final Message message, final String originalLogString) {
         return originalLogString;
     }
 
+    protected String maskSensitiveElements(final Message message, String originalLogString) {
+        return (new MaskSensitiveHelper(sensitiveElementNames))

Review comment:
       MaskSensitiveHelper is thread-safe, so it would be more performant only to create it once when sensitiveElementNames are set?




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

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] ashakirin commented on a change in pull request #681: CXF-8099: mask sensitive logging elements

GitBox
In reply to this post by GitBox

ashakirin commented on a change in pull request #681:
URL: https://github.com/apache/cxf/pull/681#discussion_r449715765



##########
File path: rt/features/logging/src/main/java/org/apache/cxf/ext/logging/MaskSensitiveHelper.java
##########
@@ -0,0 +1,74 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cxf.ext.logging;
+
+import java.util.List;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.cxf.message.Message;
+
+import static org.apache.commons.lang3.ObjectUtils.isEmpty;
+
+public class MaskSensitiveHelper {
+    private static final String ELEMENT_NAME_TEMPLATE = "-ELEMENT_NAME-";
+    private static final String MATCH_PATTERN_XML = "<-ELEMENT_NAME->(.*?)</-ELEMENT_NAME->";
+    private static final String MATCH_PATTERN_JSON = "\"-ELEMENT_NAME-\"[ \\t]*:[ \\t]*\"(.*?)\"";
+    private static final String REPLACEMENT_PATTERN_XML = "<-ELEMENT_NAME->XXX</-ELEMENT_NAME->";
+    private static final String REPLACEMENT_PATTERN_JSON = "\"-ELEMENT_NAME-\": \"XXX\"";
+
+    private static final String XML_CONTENT = "xml";
+    private static final String HTML_CONTENT = "html";
+    private static final String JSON_CONTENT = "json";
+
+    final List<String> sensitiveElementNames;
+
+    public MaskSensitiveHelper(final List<String> sensitiveElementNames) {
+        this.sensitiveElementNames = sensitiveElementNames;
+    }
+
+    public String maskSensitiveElements(
+            final Message message,
+            final String originalLogString) {
+        if (isEmpty(sensitiveElementNames)) {
+            return originalLogString;
+        }
+        String contentType = (String) message.get(Message.CONTENT_TYPE);
+        if (StringUtils.containsIgnoreCase(contentType, XML_CONTENT)
+                || StringUtils.containsIgnoreCase(contentType, HTML_CONTENT)) {
+            return applyExpression(originalLogString, MATCH_PATTERN_XML, REPLACEMENT_PATTERN_XML);
+        } else if (StringUtils.containsIgnoreCase(contentType, JSON_CONTENT)) {
+            return applyExpression(originalLogString, MATCH_PATTERN_JSON, REPLACEMENT_PATTERN_JSON);
+        } else {
+            return originalLogString;
+        }
+    }
+
+    private String applyExpression(
+            final String originalLogString,
+            final String matchPatternTemplate,
+            final String replacementTemplate) {
+        String resultString = originalLogString;
+        for (final String elementName : sensitiveElementNames) {
+            final String matchPattern = matchPatternTemplate.replaceAll(ELEMENT_NAME_TEMPLATE, elementName);
+            final String replacement = replacementTemplate.replaceAll(ELEMENT_NAME_TEMPLATE, elementName);

Review comment:
       I made some measurements year ago - there are no different in modern java with or without pre-compiled patterns - it is automatically optimized. However I find code with replaceAll more readable as with pattern. Can change it, but win is minimal




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

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] ashakirin commented on a change in pull request #681: CXF-8099: mask sensitive logging elements

GitBox
In reply to this post by GitBox

ashakirin commented on a change in pull request #681:
URL: https://github.com/apache/cxf/pull/681#discussion_r449715924



##########
File path: rt/features/logging/src/main/java/org/apache/cxf/ext/logging/AbstractLoggingInterceptor.java
##########
@@ -102,8 +110,12 @@ public void createExchangeId(Message message) {
         }
     }
 
-    protected String transform(final String originalLogString) {
+    protected String transform(final Message message, final String originalLogString) {
         return originalLogString;
     }
 
+    protected String maskSensitiveElements(final Message message, String originalLogString) {
+        return (new MaskSensitiveHelper(sensitiveElementNames))

Review comment:
       yes, it is make sense




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

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] ashakirin commented on a change in pull request #681: CXF-8099: mask sensitive logging elements

GitBox
In reply to this post by GitBox

ashakirin commented on a change in pull request #681:
URL: https://github.com/apache/cxf/pull/681#discussion_r449716595



##########
File path: rt/features/logging/src/main/java/org/apache/cxf/ext/logging/AbstractLoggingInterceptor.java
##########
@@ -102,8 +110,12 @@ public void createExchangeId(Message message) {
         }
     }
 
-    protected String transform(final String originalLogString) {
+    protected String transform(final Message message, final String originalLogString) {
         return originalLogString;
     }
 
+    protected String maskSensitiveElements(final Message message, String originalLogString) {
+        return (new MaskSensitiveHelper(sensitiveElementNames))

Review comment:
       only the thing that can contradict: I would like to add sensitive http headers names as well, if new separate setter for headers names will be introduced, instantiation of helper will be weird




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

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] ashakirin commented on a change in pull request #681: CXF-8099: mask sensitive logging elements

GitBox
In reply to this post by GitBox

ashakirin commented on a change in pull request #681:
URL: https://github.com/apache/cxf/pull/681#discussion_r449716595



##########
File path: rt/features/logging/src/main/java/org/apache/cxf/ext/logging/AbstractLoggingInterceptor.java
##########
@@ -102,8 +110,12 @@ public void createExchangeId(Message message) {
         }
     }
 
-    protected String transform(final String originalLogString) {
+    protected String transform(final Message message, final String originalLogString) {
         return originalLogString;
     }
 
+    protected String maskSensitiveElements(final Message message, String originalLogString) {
+        return (new MaskSensitiveHelper(sensitiveElementNames))

Review comment:
       only the thing that can contradict: I would like to add sensitive http headers names as well, if new separate setter for headers names will be introduced, instantiation of helper will be weird, 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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] ashakirin commented on a change in pull request #681: CXF-8099: mask sensitive logging elements

GitBox
In reply to this post by GitBox

ashakirin commented on a change in pull request #681:
URL: https://github.com/apache/cxf/pull/681#discussion_r449715765



##########
File path: rt/features/logging/src/main/java/org/apache/cxf/ext/logging/MaskSensitiveHelper.java
##########
@@ -0,0 +1,74 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cxf.ext.logging;
+
+import java.util.List;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.cxf.message.Message;
+
+import static org.apache.commons.lang3.ObjectUtils.isEmpty;
+
+public class MaskSensitiveHelper {
+    private static final String ELEMENT_NAME_TEMPLATE = "-ELEMENT_NAME-";
+    private static final String MATCH_PATTERN_XML = "<-ELEMENT_NAME->(.*?)</-ELEMENT_NAME->";
+    private static final String MATCH_PATTERN_JSON = "\"-ELEMENT_NAME-\"[ \\t]*:[ \\t]*\"(.*?)\"";
+    private static final String REPLACEMENT_PATTERN_XML = "<-ELEMENT_NAME->XXX</-ELEMENT_NAME->";
+    private static final String REPLACEMENT_PATTERN_JSON = "\"-ELEMENT_NAME-\": \"XXX\"";
+
+    private static final String XML_CONTENT = "xml";
+    private static final String HTML_CONTENT = "html";
+    private static final String JSON_CONTENT = "json";
+
+    final List<String> sensitiveElementNames;
+
+    public MaskSensitiveHelper(final List<String> sensitiveElementNames) {
+        this.sensitiveElementNames = sensitiveElementNames;
+    }
+
+    public String maskSensitiveElements(
+            final Message message,
+            final String originalLogString) {
+        if (isEmpty(sensitiveElementNames)) {
+            return originalLogString;
+        }
+        String contentType = (String) message.get(Message.CONTENT_TYPE);
+        if (StringUtils.containsIgnoreCase(contentType, XML_CONTENT)
+                || StringUtils.containsIgnoreCase(contentType, HTML_CONTENT)) {
+            return applyExpression(originalLogString, MATCH_PATTERN_XML, REPLACEMENT_PATTERN_XML);
+        } else if (StringUtils.containsIgnoreCase(contentType, JSON_CONTENT)) {
+            return applyExpression(originalLogString, MATCH_PATTERN_JSON, REPLACEMENT_PATTERN_JSON);
+        } else {
+            return originalLogString;
+        }
+    }
+
+    private String applyExpression(
+            final String originalLogString,
+            final String matchPatternTemplate,
+            final String replacementTemplate) {
+        String resultString = originalLogString;
+        for (final String elementName : sensitiveElementNames) {
+            final String matchPattern = matchPatternTemplate.replaceAll(ELEMENT_NAME_TEMPLATE, elementName);
+            final String replacement = replacementTemplate.replaceAll(ELEMENT_NAME_TEMPLATE, elementName);

Review comment:
       I made some measurements year ago - there are no performance difference in modern java with or without pre-compiled patterns - it is automatically optimized. However I find code with replaceAll more readable as with pattern. Can change it, but win is minimal

##########
File path: rt/features/logging/src/main/java/org/apache/cxf/ext/logging/MaskSensitiveHelper.java
##########
@@ -0,0 +1,74 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cxf.ext.logging;
+
+import java.util.List;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.cxf.message.Message;
+
+import static org.apache.commons.lang3.ObjectUtils.isEmpty;
+
+public class MaskSensitiveHelper {
+    private static final String ELEMENT_NAME_TEMPLATE = "-ELEMENT_NAME-";
+    private static final String MATCH_PATTERN_XML = "<-ELEMENT_NAME->(.*?)</-ELEMENT_NAME->";
+    private static final String MATCH_PATTERN_JSON = "\"-ELEMENT_NAME-\"[ \\t]*:[ \\t]*\"(.*?)\"";
+    private static final String REPLACEMENT_PATTERN_XML = "<-ELEMENT_NAME->XXX</-ELEMENT_NAME->";
+    private static final String REPLACEMENT_PATTERN_JSON = "\"-ELEMENT_NAME-\": \"XXX\"";
+
+    private static final String XML_CONTENT = "xml";
+    private static final String HTML_CONTENT = "html";
+    private static final String JSON_CONTENT = "json";
+
+    final List<String> sensitiveElementNames;
+
+    public MaskSensitiveHelper(final List<String> sensitiveElementNames) {
+        this.sensitiveElementNames = sensitiveElementNames;
+    }
+
+    public String maskSensitiveElements(
+            final Message message,
+            final String originalLogString) {
+        if (isEmpty(sensitiveElementNames)) {
+            return originalLogString;
+        }
+        String contentType = (String) message.get(Message.CONTENT_TYPE);
+        if (StringUtils.containsIgnoreCase(contentType, XML_CONTENT)
+                || StringUtils.containsIgnoreCase(contentType, HTML_CONTENT)) {
+            return applyExpression(originalLogString, MATCH_PATTERN_XML, REPLACEMENT_PATTERN_XML);
+        } else if (StringUtils.containsIgnoreCase(contentType, JSON_CONTENT)) {
+            return applyExpression(originalLogString, MATCH_PATTERN_JSON, REPLACEMENT_PATTERN_JSON);
+        } else {
+            return originalLogString;
+        }
+    }
+
+    private String applyExpression(
+            final String originalLogString,
+            final String matchPatternTemplate,
+            final String replacementTemplate) {
+        String resultString = originalLogString;
+        for (final String elementName : sensitiveElementNames) {
+            final String matchPattern = matchPatternTemplate.replaceAll(ELEMENT_NAME_TEMPLATE, elementName);
+            final String replacement = replacementTemplate.replaceAll(ELEMENT_NAME_TEMPLATE, elementName);

Review comment:
       I made some measurements year ago - there are no real performance difference in modern java with or without pre-compiled patterns - it is automatically optimized. However I find code with replaceAll more readable as with pattern. Can change it, but win is minimal




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

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] ashakirin commented on a change in pull request #681: CXF-8099: mask sensitive logging elements

GitBox
In reply to this post by GitBox

ashakirin commented on a change in pull request #681:
URL: https://github.com/apache/cxf/pull/681#discussion_r449715765



##########
File path: rt/features/logging/src/main/java/org/apache/cxf/ext/logging/MaskSensitiveHelper.java
##########
@@ -0,0 +1,74 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cxf.ext.logging;
+
+import java.util.List;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.cxf.message.Message;
+
+import static org.apache.commons.lang3.ObjectUtils.isEmpty;
+
+public class MaskSensitiveHelper {
+    private static final String ELEMENT_NAME_TEMPLATE = "-ELEMENT_NAME-";
+    private static final String MATCH_PATTERN_XML = "<-ELEMENT_NAME->(.*?)</-ELEMENT_NAME->";
+    private static final String MATCH_PATTERN_JSON = "\"-ELEMENT_NAME-\"[ \\t]*:[ \\t]*\"(.*?)\"";
+    private static final String REPLACEMENT_PATTERN_XML = "<-ELEMENT_NAME->XXX</-ELEMENT_NAME->";
+    private static final String REPLACEMENT_PATTERN_JSON = "\"-ELEMENT_NAME-\": \"XXX\"";
+
+    private static final String XML_CONTENT = "xml";
+    private static final String HTML_CONTENT = "html";
+    private static final String JSON_CONTENT = "json";
+
+    final List<String> sensitiveElementNames;
+
+    public MaskSensitiveHelper(final List<String> sensitiveElementNames) {
+        this.sensitiveElementNames = sensitiveElementNames;
+    }
+
+    public String maskSensitiveElements(
+            final Message message,
+            final String originalLogString) {
+        if (isEmpty(sensitiveElementNames)) {
+            return originalLogString;
+        }
+        String contentType = (String) message.get(Message.CONTENT_TYPE);
+        if (StringUtils.containsIgnoreCase(contentType, XML_CONTENT)
+                || StringUtils.containsIgnoreCase(contentType, HTML_CONTENT)) {
+            return applyExpression(originalLogString, MATCH_PATTERN_XML, REPLACEMENT_PATTERN_XML);
+        } else if (StringUtils.containsIgnoreCase(contentType, JSON_CONTENT)) {
+            return applyExpression(originalLogString, MATCH_PATTERN_JSON, REPLACEMENT_PATTERN_JSON);
+        } else {
+            return originalLogString;
+        }
+    }
+
+    private String applyExpression(
+            final String originalLogString,
+            final String matchPatternTemplate,
+            final String replacementTemplate) {
+        String resultString = originalLogString;
+        for (final String elementName : sensitiveElementNames) {
+            final String matchPattern = matchPatternTemplate.replaceAll(ELEMENT_NAME_TEMPLATE, elementName);
+            final String replacement = replacementTemplate.replaceAll(ELEMENT_NAME_TEMPLATE, elementName);

Review comment:
       I made some measurements year ago - there are no real performance difference in modern java with or without pre-compiled patterns - it is automatically optimized. However I find code with replaceAll more readable as with pattern. Can change it, but win is minimal. 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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] ashakirin commented on a change in pull request #681: CXF-8099: mask sensitive logging elements

GitBox
In reply to this post by GitBox

ashakirin commented on a change in pull request #681:
URL: https://github.com/apache/cxf/pull/681#discussion_r449716746



##########
File path: rt/features/logging/pom.xml
##########
@@ -38,6 +38,10 @@
             <groupId>org.slf4j</groupId>
             <artifactId>slf4j-api</artifactId>
         </dependency>
+        <dependency>

Review comment:
       updated




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

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] ashakirin commented on a change in pull request #681: CXF-8099: mask sensitive logging elements

GitBox
In reply to this post by GitBox

ashakirin commented on a change in pull request #681:
URL: https://github.com/apache/cxf/pull/681#discussion_r449717002



##########
File path: rt/features/logging/src/main/java/org/apache/cxf/ext/logging/AbstractLoggingInterceptor.java
##########
@@ -102,8 +110,12 @@ public void createExchangeId(Message message) {
         }
     }
 
-    protected String transform(final String originalLogString) {
+    protected String transform(final Message message, final String originalLogString) {
         return originalLogString;
     }
 
+    protected String maskSensitiveElements(final Message message, String originalLogString) {
+        return (new MaskSensitiveHelper(sensitiveElementNames))

Review comment:
       But perhaps it doesn't make sense to introduce separate setters for elements and headers, user can put them in the same list




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

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] ashakirin commented on a change in pull request #681: CXF-8099: mask sensitive logging elements

GitBox
In reply to this post by GitBox

ashakirin commented on a change in pull request #681:
URL: https://github.com/apache/cxf/pull/681#discussion_r449717002



##########
File path: rt/features/logging/src/main/java/org/apache/cxf/ext/logging/AbstractLoggingInterceptor.java
##########
@@ -102,8 +110,12 @@ public void createExchangeId(Message message) {
         }
     }
 
-    protected String transform(final String originalLogString) {
+    protected String transform(final Message message, final String originalLogString) {
         return originalLogString;
     }
 
+    protected String maskSensitiveElements(final Message message, String originalLogString) {
+        return (new MaskSensitiveHelper(sensitiveElementNames))

Review comment:
       But perhaps it doesn't make sense to introduce separate setters for elements and headers, user can just put them in the same list. Any opinions?




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

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] coheigea commented on a change in pull request #681: CXF-8099: mask sensitive logging elements

GitBox
In reply to this post by GitBox

coheigea commented on a change in pull request #681:
URL: https://github.com/apache/cxf/pull/681#discussion_r450965523



##########
File path: rt/features/logging/src/main/java/org/apache/cxf/ext/logging/AbstractLoggingInterceptor.java
##########
@@ -102,8 +115,13 @@ public void createExchangeId(Message message) {
         }
     }
 
-    protected String transform(final String originalLogString) {
+    protected String transform(final Message message, final String originalLogString) {
         return originalLogString;
     }
 
+    protected String maskSensitiveElements(final Message message, String originalLogString) {
+        return Optional.of(maskSensitiveHelper)

Review comment:
       Why Optional here when maskSensitiveHelper is declared as final?

##########
File path: rt/features/logging/src/main/java/org/apache/cxf/ext/logging/AbstractLoggingInterceptor.java
##########
@@ -73,6 +78,14 @@ public long getInMemThreshold() {
         return threshold;
     }
 
+    public void addSensitiveElementNames(final List<String> sensitiveElementNames) {
+        maskSensitiveHelper.addSensitiveElementNames(sensitiveElementNames);
+    }
+
+    public void addSensitiveHeaders(final List<String> sensitiveHeaders) {

Review comment:
       Maybe rename to HTTPHeaders to make the meaning clear (as opposed to say SOAP headers)




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

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] coheigea commented on a change in pull request #681: CXF-8099: mask sensitive logging elements

GitBox
In reply to this post by GitBox

coheigea commented on a change in pull request #681:
URL: https://github.com/apache/cxf/pull/681#discussion_r450965362



##########
File path: rt/features/logging/src/main/java/org/apache/cxf/ext/logging/AbstractLoggingInterceptor.java
##########
@@ -73,6 +78,14 @@ public long getInMemThreshold() {
         return threshold;
     }
 
+    public void addSensitiveElementNames(final List<String> sensitiveElementNames) {
+        maskSensitiveHelper.addSensitiveElementNames(sensitiveElementNames);
+    }
+
+    public void addSensitiveHeaders(final List<String> sensitiveHeaders) {

Review comment:
       Maybe rename to HTTPHeaders or ProtocolHeaders to make the meaning clear (as opposed to say SOAP headers)




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

For queries about this service, please contact Infrastructure at:
[hidden email]


12