[GitHub] drill pull request #1139: DRILL-6189: Security: passwords logging and file p...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/drill/pull/1139


---

[GitHub] drill pull request #1139: DRILL-6189: Security: passwords logging and file p...

[vladimirtkach]

Github user vladimirtkach commented on a diff in the pull request:

https://github.com/apache/drill/pull/1139#discussion_r171588799
  
--- Diff: 
logical/src/main/java/org/apache/drill/common/config/LogicalPlanPersistence.java
 ---
@@ -52,6 +53,7 @@ public LogicalPlanPersistence(DrillConfig conf, 
ScanResult scanResult) {
 mapper.configure(Feature.ALLOW_UNQUOTED_FIELD_NAMES, true);
 mapper.configure(JsonGenerator.Feature.QUOTE_FIELD_NAMES, true);
 mapper.configure(Feature.ALLOW_COMMENTS, true);
+mapper.setFilterProvider(new 
SimpleFilterProvider().setFailOnUnknownId(false));
--- End diff --

submitted physical plan directly to node, it was successfully deserialized


---

[GitHub] drill pull request #1139: DRILL-6189: Security: passwords logging and file p...

[vladimirtkach]

Github user vladimirtkach commented on a diff in the pull request:

https://github.com/apache/drill/pull/1139#discussion_r171579096
  
--- Diff: 
contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/JdbcStorageConfig.java
 ---
@@ -17,13 +17,15 @@
  */
 package org.apache.drill.exec.store.jdbc;
 
+import com.fasterxml.jackson.annotation.JsonFilter;
 import org.apache.drill.common.logical.StoragePluginConfig;
 
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.fasterxml.jackson.annotation.JsonTypeName;
 
 @JsonTypeName(JdbcStorageConfig.NAME)
+@JsonFilter("passwordFilter")
--- End diff --

To apply filter:
1) Mark the entity with you want to filter out fields from.
2) Create filter provider and register property filter with reference to 
your entity
3) When creating ObjectWriter pass your filter provider


---

[GitHub] drill pull request #1139: DRILL-6189: Security: passwords logging and file p...

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1139#discussion_r171511391
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java ---
@@ -91,6 +91,34 @@
   static {
 userConnectionMap = new ConcurrentHashMap<>();
   }
+  public static  String safeLogString(UserToBitHandshake inbound) {
+StringBuilder sb = new StringBuilder();
+sb.append("rpc_version: ");
+sb.append(inbound.getRpcVersion());
+sb.append("\ncredentials:\n\t");
+sb.append(inbound.getCredentials());
+sb.append("properties:");
+java.util.List props = 
inbound.getProperties().getPropertiesList();
+for (Property p: props){
+  if(!p.getKey().equalsIgnoreCase("password")) {
--- End diff --

Please add spaces missing spaces...


---

[GitHub] drill pull request #1139: DRILL-6189: Security: passwords logging and file p...

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1139#discussion_r171512422
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DefaultSqlHandler.java
 ---
@@ -158,7 +162,9 @@ protected void logAndSetTextPlan(final String 
description, final Prel prel, fina
 
   protected void log(final String name, final PhysicalPlan plan, final 
Logger logger) throws JsonProcessingException {
 if (logger.isDebugEnabled()) {
-  String planText = 
plan.unparse(context.getLpPersistence().getMapper().writer());
+  PropertyFilter theFilter = new 
SimpleBeanPropertyFilter.SerializeExceptFilter(Sets.newHashSet("password"));
--- End diff --

Please rename to `filter`.


---

[GitHub] drill pull request #1139: DRILL-6189: Security: passwords logging and file p...

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1139#discussion_r171510779
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java ---
@@ -91,6 +91,34 @@
   static {
 userConnectionMap = new ConcurrentHashMap<>();
   }
+  public static  String safeLogString(UserToBitHandshake inbound) {
--- End diff --

1. Please remove one space -> `static  String`/
2. Can this method be just private? Not public static? If yes, please move 
it to the end of the class.
3. Please add javadoc to the method.
4. Please consider method renaming to depict actual work it does.


---

[GitHub] drill pull request #1139: DRILL-6189: Security: passwords logging and file p...

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1139#discussion_r171512007
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java ---
@@ -320,7 +348,7 @@ protected void consumeHandshake(ChannelHandlerContext 
ctx, UserToBitHandshake in
 
   @Override
   public BitToUserHandshake getHandshakeResponse(UserToBitHandshake 
inbound) throws Exception {
-logger.trace("Handling handshake from user to bit. {}", inbound);
+logger.trace("Handling handshake from user to bit. {}", 
safeLogString(inbound));
--- End diff --

Should we add `if (logger.isTraceEnabled()) {`? so `safeLogString` will be 
called only when we do need it for trace?


---

[GitHub] drill pull request #1139: DRILL-6189: Security: passwords logging and file p...

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1139#discussion_r171511274
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java ---
@@ -91,6 +91,34 @@
   static {
 userConnectionMap = new ConcurrentHashMap<>();
   }
+  public static  String safeLogString(UserToBitHandshake inbound) {
+StringBuilder sb = new StringBuilder();
+sb.append("rpc_version: ");
+sb.append(inbound.getRpcVersion());
+sb.append("\ncredentials:\n\t");
+sb.append(inbound.getCredentials());
+sb.append("properties:");
+java.util.List props = 
inbound.getProperties().getPropertiesList();
--- End diff --

Why do you need full import?


---

[GitHub] drill pull request #1139: DRILL-6189: Security: passwords logging and file p...

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1139#discussion_r171308023
  
--- Diff: 
protocol/src/main/java/org/apache/drill/exec/proto/UserProtos.java ---
@@ -5798,6 +5798,34 @@ public static UserToBitHandshake 
getDefaultInstance() {
 public UserToBitHandshake getDefaultInstanceForType() {
   return defaultInstance;
 }
+public String safeLogString() {
--- End diff --

You cannot add custom methods to proto buffers. Also consider using tabs 
instead of multiple spaces.
Please add to Jira example how log files looked before your changes and 
after.


---

[GitHub] drill pull request #1139: DRILL-6189: Security: passwords logging and file p...

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1139#discussion_r171307607
  
--- Diff: 
logical/src/main/java/org/apache/drill/common/config/LogicalPlanPersistence.java
 ---
@@ -52,6 +53,7 @@ public LogicalPlanPersistence(DrillConfig conf, 
ScanResult scanResult) {
 mapper.configure(Feature.ALLOW_UNQUOTED_FIELD_NAMES, true);
 mapper.configure(JsonGenerator.Feature.QUOTE_FIELD_NAMES, true);
 mapper.configure(Feature.ALLOW_COMMENTS, true);
+mapper.setFilterProvider(new 
SimpleFilterProvider().setFailOnUnknownId(false));
--- End diff --

Will filtering passwords work when profiles are sent between nodes (i.e. 
when we have several major fragments)?


---

[GitHub] drill pull request #1139: DRILL-6189: Security: passwords logging and file p...

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1139#discussion_r171307292
  
--- Diff: 
contrib/storage-jdbc/src/main/java/org/apache/drill/exec/store/jdbc/JdbcStorageConfig.java
 ---
@@ -17,13 +17,15 @@
  */
 package org.apache.drill.exec.store.jdbc;
 
+import com.fasterxml.jackson.annotation.JsonFilter;
 import org.apache.drill.common.logical.StoragePluginConfig;
 
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.fasterxml.jackson.annotation.JsonTypeName;
 
 @JsonTypeName(JdbcStorageConfig.NAME)
+@JsonFilter("passwordFilter")
--- End diff --

Please explain how this works?


---

[GitHub] drill pull request #1139: DRILL-6189: Security: passwords logging and file p...

[vladimirtkach]

GitHub user vladimirtkach opened a pull request:

https://github.com/apache/drill/pull/1139

DRILL-6189: Security: passwords logging and file permisions

1. Overrided serialization methods for instances with passwords
2. Changed file permissions for configuration files

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/vladimirtkach/drill DRILL-6189

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/1139.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1139


commit 9bf7f464fe921cef92ad9802f56c75b72064b0aa
Author: Vladimir Tkach 
Date:   2018-02-28T11:10:50Z

DRILL-6189: Security: passwords logging and file permisions

1. Overrided serialization methods for instances with passwords
2. Changed file permissions for configuration files




---