[GitHub] drill pull request #1020: DRILL-5921: Display counter metrics in table

[prasadns14]

GitHub user prasadns14 opened a pull request:

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

DRILL-5921: Display counter metrics in table

Listed the counter metrics in a table
@arina-ielchiieva please review

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

$ git pull https://github.com/prasadns14/drill DRILL-5921

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

https://github.com/apache/drill/pull/1020.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 #1020


commit 3f55847fe93d3296740901f717b4e0cf50736311
Author: Prasad Nagaraj Subramanya 
Date:   2017-11-02T01:59:38Z

DRILL-5921: Display counter metrics in table




---

[jira] [Created] (DRILL-5921) Counters metrics should be listed in table

[Prasad Nagaraj Subramanya (JIRA)]

Prasad Nagaraj Subramanya created DRILL-5921:


 Summary: Counters metrics should be listed in table
 Key: DRILL-5921
 URL: https://issues.apache.org/jira/browse/DRILL-5921
 Project: Apache Drill
  Issue Type: Bug
  Components: Client - HTTP
Affects Versions: 1.11.0
Reporter: Prasad Nagaraj Subramanya
Assignee: Prasad Nagaraj Subramanya
Priority: Minor
 Fix For: 1.12.0


Counter metrics are currently displayed as json string in the Drill UI. They 
should be listed in a table similar to other metrics.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[GitHub] drill issue #984: DRILL-5783 Made a unit test for generated Priority Queue. ...

[ilooner]

Github user ilooner commented on the issue:

https://github.com/apache/drill/pull/984
  
@paul-rogers This is ready for another round of review. I have also 
responded to / addressed your comments.



---

[GitHub] drill issue #970: DRILL-5832: Migrate OperatorFixture to use SystemOptionMan...

[ilooner]

Github user ilooner commented on the issue:

https://github.com/apache/drill/pull/970
  
@paul-rogers Will this be going in anytime soon? I'd like to fix the 
annoying

```
test(String.format("alter session..."))
```

statements in the tests after this goes in.


---

[GitHub] drill pull request #984: DRILL-5783 Made a unit test for generated Priority ...

[ilooner]

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

https://github.com/apache/drill/pull/984#discussion_r148396417
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/test/TableFileBuilder.java ---
@@ -0,0 +1,85 @@
+/*
+ * 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.drill.test;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Lists;
+
+import java.io.BufferedOutputStream;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.util.List;
+
+public class TableFileBuilder
+{
+  private final List columnNames;
+  private final List columnFormatters;
+  private final List rows = Lists.newArrayList();
+  private final String rowString;
+
+  public TableFileBuilder(List columnNames, List 
columnFormatters) {
--- End diff --

I've switched this around a bit. The general flow is now the following:

  1. A **RowSet** is constructed with a **RowSetBuilder**
  1. A **JsonFileBuilder** is created and configured to use the **RowSet** 
that was just created.
  1. The **JsonFileBuilder** reads the **RowSet** and writes it out to a 
file in a json format.



---

[GitHub] drill pull request #984: DRILL-5783 Made a unit test for generated Priority ...

[ilooner]

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

https://github.com/apache/drill/pull/984#discussion_r148395420
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/writer/TestCorruptParquetDateCorrection.java
 ---
@@ -377,21 +386,21 @@ public void 
testReadOldMetadataCacheFileOverrideCorrection() throws Exception {
 
   @Test
   public void testReadNewMetadataCacheFileOverOldAndNewFiles() throws 
Exception {
-String table = format("dfs.`%s`", new 
Path(getDfsTestTmpSchemaLocation(), 
MIXED_CORRUPTED_AND_CORRECT_PARTITIONED_FOLDER));
-copyMetaDataCacheToTempReplacingInternalPaths(
+File meta = dirTestWatcher.copyResourceToRoot(
 
"parquet/4203_corrupt_dates/mixed_version_partitioned_metadata.requires_replace.txt",
-MIXED_CORRUPTED_AND_CORRECT_PARTITIONED_FOLDER, 
Metadata.METADATA_FILENAME);
+Paths.get(MIXED_CORRUPTED_AND_CORRECT_PARTITIONED_FOLDER, 
Metadata.METADATA_FILENAME).toString());
--- End diff --

I've cleaned this up a bit. Of the two patterns for building paths and 
files I prefer pattern **A** because it is cleaner.

**Pattern A:**
```
myPath.resolve("subDir1")
  .resolve("subDir2")
  .toFile();
```

**Pattern B:**
```
new File(new File(myFile, "subDir1"), "subDir2")
```


---

[GitHub] drill pull request #984: DRILL-5783 Made a unit test for generated Priority ...

[ilooner]

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

https://github.com/apache/drill/pull/984#discussion_r148394268
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/xsort/TestExternalSort.java
 ---
@@ -138,34 +141,34 @@ public void testNewColumnsManaged() throws Exception {
 testNewColumns(false);
   }
 
-
   @Test
   public void testNewColumnsLegacy() throws Exception {
 testNewColumns(true);
   }
 
   private void testNewColumns(boolean testLegacy) throws Exception {
 final int record_count = 1;
-String dfs_temp = getDfsTestTmpSchemaLocation();
-System.out.println(dfs_temp);
-File table_dir = new File(dfs_temp, "newColumns");
-table_dir.mkdir();
-try (BufferedOutputStream os = new BufferedOutputStream(new 
FileOutputStream(new File(table_dir, "a.json" {
-  String format = "{ a : %d, b : %d }%n";
-  for (int i = 0; i <= record_count; i += 2) {
-os.write(String.format(format, i, i).getBytes());
-  }
+final String tableDirName = "newColumns";
+
+TableFileBuilder tableA = new TableFileBuilder(Lists.newArrayList("a", 
"b"), Lists.newArrayList("%d", "%d"));
--- End diff --

I've made the JsonFileBuilder, which takes a RowSet and writes it out to a 
file as json.


---

[GitHub] drill pull request #984: DRILL-5783 Made a unit test for generated Priority ...

[ilooner]

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

https://github.com/apache/drill/pull/984#discussion_r148393893
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/test/BatchUtils.java ---
@@ -0,0 +1,280 @@
+/*
+ * 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.drill.test;
+
+import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
+import org.apache.drill.exec.record.VectorContainer;
+import org.apache.drill.exec.record.VectorWrapper;
+import org.apache.drill.exec.record.selection.SelectionVector4;
+import org.apache.drill.exec.vector.ValueVector;
+import org.junit.Assert;
+
+import java.io.UnsupportedEncodingException;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.List;
+import java.util.Map;
+
+public class BatchUtils {
+  static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(BatchUtils.class);
+
+  public static Map 
containerToObjects(VectorContainer vectorContainer) {
--- End diff --

Removed BatchUtils


---

Re: Drill SASL Forward Compatibility

[Sorabh Hamirwasia]

Parth,

Your understanding is correct.


I am also debating for 1(A) specially in context of security mechanisms like 
Kerberos which guarantees prevention from MITM during handshake.


But with 1( B ) we are saying in case of Drill it's possible and fine since no 
data is compromised.


Thanks,
Sorabh


From: Parth Chandra 
Sent: Wednesday, November 1, 2017 1:42:14 PM
To: dev
Subject: Re: Drill SASL Forward Compatibility

I sort of lost track of the arguments in the thread. Is my understanding
below correct ?


1) A handshake from a (1.12) client expecting authentication and encryption
is intercepted by a rogue server. The server then responds with a success
message and bypasses the auth and encryption for the session.

2) The client is now connected, but not to the server it wanted to connect
to.

3) The rogue server can now feed any bogus response to the client.


Question 1 - Is #3 a security issue?


Answer 1 (A) - Yes. The handshake has been compromised. The client is no
longer connected to an authentic server.


Answer 1 (B) - No. There is no data that has been compromised. Just a
client that has been misled.



I believe this is a security issue. A rogue server can now feed invalid
results to the client and that is not safe. Perhaps others with more
experience on industrial grade security can chime in.

Question 2 - If this is a security issue, is it severe enough to break
forward compatibility?

In general, I'm -1 on breaking backward compatibility and -0 on breaking
forward compatibility. I believe it is a very desirable goal to maintain
both backward and forward compatibility. However, forward compatibility
cannot be guaranteed unless we bake it into the RPC protocol and design
clients to be version and feature aware. This itself would be a breaking
change and should be one of the goals for V2.

In this case, I'm inclined to go with what Arina is suggesting.

[GitHub] drill issue #976: DRILL-5797: Choose parquet reader from read columns

[paul-rogers]

Github user paul-rogers commented on the issue:

https://github.com/apache/drill/pull/976
  
Drill follows SQL rules and is case insensitive. If case sensitivity has 
snuck in somewhere (perhaps due to the use of `equals()` rather than 
`equalsIgnorCase()` or the use of a case-sensitive map), then we should fix 
that.

Note also that column aliases should not be visible to the Parquet reader.


---

[GitHub] drill pull request #1019: DRILL-5909: Added new Counter metrics

[prasadns14]

GitHub user prasadns14 opened a pull request:

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

DRILL-5909: Added new Counter metrics

Added new metrics to capture succeeded, failed and canceled queries count.

@paul-rogers please review

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

$ git pull https://github.com/prasadns14/drill DRILL-5909

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

https://github.com/apache/drill/pull/1019.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 #1019


commit a05094a7d190f81cfe7b5222a48dce8707467313
Author: Prasad Nagaraj Subramanya 
Date:   2017-11-01T20:49:43Z

DRILL-5909: Added new Counter metrics




---

Re: Drill SASL Forward Compatibility

[Parth Chandra]

I sort of lost track of the arguments in the thread. Is my understanding
below correct ?


1) A handshake from a (1.12) client expecting authentication and encryption
is intercepted by a rogue server. The server then responds with a success
message and bypasses the auth and encryption for the session.

2) The client is now connected, but not to the server it wanted to connect
to.

3) The rogue server can now feed any bogus response to the client.


Question 1 - Is #3 a security issue?


Answer 1 (A) - Yes. The handshake has been compromised. The client is no
longer connected to an authentic server.


Answer 1 (B) - No. There is no data that has been compromised. Just a
client that has been misled.



I believe this is a security issue. A rogue server can now feed invalid
results to the client and that is not safe. Perhaps others with more
experience on industrial grade security can chime in.

Question 2 - If this is a security issue, is it severe enough to break
forward compatibility?

In general, I'm -1 on breaking backward compatibility and -0 on breaking
forward compatibility. I believe it is a very desirable goal to maintain
both backward and forward compatibility. However, forward compatibility
cannot be guaranteed unless we bake it into the RPC protocol and design
clients to be version and feature aware. This itself would be a breaking
change and should be one of the goals for V2.

In this case, I'm inclined to go with what Arina is suggesting.

[GitHub] drill issue #1015: DRILL-5889: Simple pattern matchers can work with DrillBu...

[sachouche]

Github user sachouche commented on the issue:

https://github.com/apache/drill/pull/1015
  
Padma, DRILL-5889 is the wrong JIRA (sqlline loses RPC..); I think your 
JIRA is 5899


Regards,

Salim


From: Padma Penumarthy 
Sent: Wednesday, November 1, 2017 11:15:20 AM
To: apache/drill
Cc: Salim Achouche; Mention
Subject: Re: [apache/drill] DRILL-5889: Simple pattern matchers can work 
with DrillBuf directly (#1015)


@sachouche 
@paul-rogers can you please review ?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on 
GitHub, or 
mute the 
thread.



---

[GitHub] drill issue #1018: Drill-5834 Add Networking Functions #971

[cgivre]

Github user cgivre commented on the issue:

https://github.com/apache/drill/pull/1018
  
@paul-rogers @arina-ielchiieva 
I'm terrible at git so here is a clean copy of the PR with one commit. 



---

[GitHub] drill pull request #971: Drill-5834 Add Networking Functions

[cgivre]

Github user cgivre closed the pull request at:

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


---

[GitHub] drill pull request #1018: Drill-5834 Add Networking Functions #971

[cgivre]

GitHub user cgivre opened a pull request:

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

Drill-5834 Add Networking Functions #971

This is a collection of Networking Functions to facilitate network 
analysis.  The functions include:

- **inet_aton(``)**:  Converts an IPv4 address into an integer.
- **inet_ntoa( ``)**:  Converts an integer IP into dotted decimal 
notation
- **in_network( ``,`` )**: Returns true if the IP address is in 
the given CIDR block
- **address_count( `` )**:  Returns the number of IPs in a given CIDR 
block
- **broadcast_address( `` )**: Returns the broadcast address for a 
given CIDR block
- **netmask(`` )**: Returns the netmask for a given CIDR block. 
- **low_address(``)**:  Returns the first address in a given CIDR 
block.
- **high_address(``)**:  Returns the last address in a given CIDR 
block.
- **url_encode( `` )**:  Returns a URL encoded string.
- **url_decode( `` )**:   Decodes a URL encoded string.
- **is_valid_IP(``)**: Returns true if the IP is a valid IP address
- **is_private_ip(``)**: Returns true if the IP is a private IPv4 
address
- **is_valid_IPv4(``)**: Returns true if the IP is a valid IPv4 address
- **is_valid_IPv6(``)**: Returns true if the IP is a valid IPv6 address

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

$ git pull https://github.com/cgivre/drill network-functions-final

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

https://github.com/apache/drill/pull/1018.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 #1018


commit e5b4e729e8abe920ca6acb2e6d2c36784421b051
Author: cgivre 
Date:   2017-11-01T18:15:38Z

DRILL-5834 Add Networking Functions




---

[jira] [Resolved] (DRILL-5920) Drill incorrectly projects column aliases to scan operator

[Paul Rogers (JIRA)]

 [ 
https://issues.apache.org/jira/browse/DRILL-5920?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Paul Rogers resolved DRILL-5920.

Resolution: Not A Bug

> Drill incorrectly projects column aliases to scan operator
> --
>
> Key: DRILL-5920
> URL: https://issues.apache.org/jira/browse/DRILL-5920
> Project: Apache Drill
>  Issue Type: Bug
>Affects Versions: 1.10.0
>Reporter: Paul Rogers
>Priority: Major
>
> The {{TestNewTextReader.ensureColumnNameDisplayedinError}} unit test runs 
> this query:
> {code}
> select max(columns[1]) as col1
> from cp.`textinput/input1.csv`
> where col1 is not null
> {code}
> The following appears in the {{SubScan}} for the {{TextFormatPlugin}}:
> {noformat}
> [`col1`, `columns`[1]]
> {noformat}
> This is clearly wrong. The actual table column is {{columns}} (and, 
> specifically, element 1.) {{col1} is an alias that should never have been 
> pushed down to the data source because the data source does not know about 
> aliases.
> Further, the projection list makes no distinction between the "real" and 
> "alias" columns, so, to the data source, both look like real table columns.
> The current workaround is to create a nullable int column for {{col1}} which 
> is, presumably, replaced by a later projection operator.
> Because this behavior is wrong, we must think though all the possible failure 
> cases and how to handle them in this incorrect design. What if the alias 
> matches an (expensive) table column? What if the alias is the same as some 
> base column in the same query?
> {code}
> SELECT a as b, b as c FROM ...
> {code}
> Incorrect name handling may work in many cases, but it does lead to problems 
> because the behavior is not following the accepted SQL standards.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[GitHub] drill issue #1015: DRILL-5889: Simple pattern matchers can work with DrillBu...

[ppadma]

Github user ppadma commented on the issue:

https://github.com/apache/drill/pull/1015
  
@sachouche @paul-rogers can you please review ?


---

[GitHub] drill issue #976: DRILL-5797: Choose parquet reader from read columns

[sachouche]

Github user sachouche commented on the issue:

https://github.com/apache/drill/pull/976
  
Looking at the stack trace:
- The code definitely is initializing a column of type REPEATABLE
- The Fast Reader didn't expect this scenario so it used a default 
container (NullableVarBinary) for VL binary DT

Why this is happening?
- The code in ReadState::buildReader() is processing all selected columns
- This information is obtained from the ParquetSchema
- Looking at the code, this seems a case-sensitivity issue
- The ParquetSchema is case-insensitive whereas the Parquet GroupType is not
- Damien added a catch handler (column not found) to handle use-cases where 
we are projecting non-existing columns
- This basically is leading to an unforeseen use-case
- Assume column XYZ is complex
- User uses an alias (xyz)
- The new code will allow this column to pass and treat is as simple
- The ParquetSchema is being case insensitive will process this column
- and thus the exception in the test suite

Suggested Fix
- Create a map (key to-lower-case) and register all current row-group 
columns
- Use this map to locate a selected column type



---

[jira] [Created] (DRILL-5920) Drill incorrectly projects column aliases to scan operator

[Paul Rogers (JIRA)]

Paul Rogers created DRILL-5920:
--

 Summary: Drill incorrectly projects column aliases to scan operator
 Key: DRILL-5920
 URL: https://issues.apache.org/jira/browse/DRILL-5920
 Project: Apache Drill
  Issue Type: Bug
Affects Versions: 1.10.0
Reporter: Paul Rogers
Priority: Major


The {{TestNewTextReader.ensureColumnNameDisplayedinError}} unit test runs this 
query:
{code}
select max(columns[1]) as col1
from cp.`textinput/input1.csv`
where col1 is not null
{code}
The following appears in the {{SubScan}} for the {{TextFormatPlugin}}:

{noformat}
[`col1`, `columns`[1]]
{noformat}

This is clearly wrong. The actual table column is {{columns}} (and, 
specifically, element 1.) {{col1} is an alias that should never have been 
pushed down to the data source because the data source does not know about 
aliases.

Further, the projection list makes no distinction between the "real" and 
"alias" columns, so, to the data source, both look like real table columns.

The current workaround is to create a nullable int column for {{col1}} which 
is, presumably, replaced by a later projection operator.

Because this behavior is wrong, we must think though all the possible failure 
cases and how to handle them in this incorrect design. What if the alias 
matches an (expensive) table column? What if the alias is the same as some base 
column in the same query?

{code}
SELECT a as b, b as c FROM ...
{code}

Incorrect name handling may work in many cases, but it does lead to problems 
because the behavior is not following the accepted SQL standards.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

Re: Drill SASL Forward Compatibility

[Arina Yelchiyeva]

Hi Sorabh,

regarding your question:

>
> We have to either back port above forward compatibility fix into 1.10 and
> 1.11 or just fix in current release and support forward compatibility post
> 1.12+.
> Arina/Sudheesh - Please suggest if the analysis and fix sounds good and
> what are the releases we should consider the fix for. Considering 1.12
> release is planned soon can we take the fix in 1.12 release ?
>

I think we add the fix (if needed) in 1.12 and support forward
compatibility post 1.12+. As far as I know Drill never claimed it is
backward compatible and explicitly discourages users to use different Drill
versions in a cluster as well as for client and server.

Kind regards
Arina

On Wed, Nov 1, 2017 at 5:48 PM, Laurent Goujon  wrote:

> My comments inline:
>
> On Tue, Oct 31, 2017 at 6:11 PM, Sorabh Hamirwasia 
> wrote:
>
> > - if using Kerberos, things are a bit different: even if a MITM
> intercepts
> >
> > the token, only the server can decode it properly and set up encryption
> so
> > that only client can use it (a proxy server would not be able to decode
> the
> > traffic). So what you need to ensure is that you actually use Kerberos as
> > the only authentication mechanism, and that the channel is encrypted (if
> > channel is not encryted, see above). This is things you should do by
> > configuring the client by not sending the password (no need to), only
> > authorize Kerberos authentication, and verify that encryption is enabled
> > (which is already done I believe).
> >
> >
> > [Sorabh] - You are correct about the Kerberos preventing MITM which is
> > what I mentioned in the last response. But this is guaranteed if client
> and
> > server reach to the point of SASL handshake in their communication, since
> > SASL handshake exchanges all the bits related to Kerberos protocol.
> Before
> > that point is reached there are still few messages which is exchanged
> > between DrillClient and Drillbit to detect whether server side needs
> > authentication or not and what are supported mechanisms. This is where
> the
> > security flaw can cause client to believe Authentication is successfully
> > completed (even when Drillbit/DrillClient are authenticating using
> Kerberos
> > with or without encryption). This is what patch for DRILL-5582 is trying
> to
> > address.
> >
> >
> You still haven't answered what is the issue/security risk for the client
> here: sure the client didn't authenticate with the server, but at the same
> time it didn't get access to the server either...
>
> Also, it doesn't take very long to modify the rogue server to fake a SASL
> authentication. So, now you are "authenticated", but still not to the right
> server...
>
>
>
> >
> > As for the other issue at hand (compatibility between 1.11 client and
> 1.10
> > server), I am not sure to understand the proposed fix: is the logic you
> > proposed to be added to 1.10 server? why not simply add the missing enum
> > value (and that's it! no more values after that!)?
> >
> >
> > [Sorabh] - Just adding the missing enum value in 1.10 will not help since
> > in future if any other enum value is introduced then the same issue will
> be
> > seen. Moreover 1.10 doesn't support Encryption so that enum value should
> > not be added in that release. Instead the fix is to treat the return
> value
> > of UNKNOWN_SASL_SERVER while retrieving messages from future client as
> > valid default value and take decision based on that.
> >
>
> But 1.10.1 could consider that a client supporting encryption also support
> authentication? The code is already here in 1.10 btw:
> https://github.com/apache/drill/blob/1.10.0/exec/java-
> exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java#L297
>
> Alternatively, you could just check that the field sasl_support is set and
> not check the value alltogether. I'm not convinced you need to do some
> extra logic around UNKNOWN_SASL_SERVER which would just keep people
> confused (although it doesn't seem something you need to apply to 1.11 or
> higher)
>
>
>
> >
> >
> > Thanks,
> > Sorabh
> >
> > 
> > From: Laurent Goujon 
> > Sent: Tuesday, October 31, 2017 5:42 PM
> > To: dev
> > Cc: Arina Lelchieva; sudhe...@apache.org
> > Subject: Re: Drill SASL Forward Compatibility
> >
> > Regarding DRILL-5582 patch which broke compatibility with 1.9 version
> > (which is less than a year old):
> > I'm still not clear what we are trying to prevent here: stealing user
> > credentials and/or data? connecting to a rogue server which could return
> > corrupted data to the user? The patch gives a false sense of security
> > because it prevents none of it:
> > - if using password-based authentication, client is sending it in clear
> in
> > the initial handshake so I guess it's already game over!
> > - You could configure a client to not sent password over wire by choosing
> > another authentication algorithm, BUT if there's 

Re: Drill SASL Forward Compatibility

[Laurent Goujon]

My comments inline:

On Tue, Oct 31, 2017 at 6:11 PM, Sorabh Hamirwasia 
wrote:

> - if using Kerberos, things are a bit different: even if a MITM intercepts
>
> the token, only the server can decode it properly and set up encryption so
> that only client can use it (a proxy server would not be able to decode the
> traffic). So what you need to ensure is that you actually use Kerberos as
> the only authentication mechanism, and that the channel is encrypted (if
> channel is not encryted, see above). This is things you should do by
> configuring the client by not sending the password (no need to), only
> authorize Kerberos authentication, and verify that encryption is enabled
> (which is already done I believe).
>
>
> [Sorabh] - You are correct about the Kerberos preventing MITM which is
> what I mentioned in the last response. But this is guaranteed if client and
> server reach to the point of SASL handshake in their communication, since
> SASL handshake exchanges all the bits related to Kerberos protocol. Before
> that point is reached there are still few messages which is exchanged
> between DrillClient and Drillbit to detect whether server side needs
> authentication or not and what are supported mechanisms. This is where the
> security flaw can cause client to believe Authentication is successfully
> completed (even when Drillbit/DrillClient are authenticating using Kerberos
> with or without encryption). This is what patch for DRILL-5582 is trying to
> address.
>
>
You still haven't answered what is the issue/security risk for the client
here: sure the client didn't authenticate with the server, but at the same
time it didn't get access to the server either...

Also, it doesn't take very long to modify the rogue server to fake a SASL
authentication. So, now you are "authenticated", but still not to the right
server...



>
> As for the other issue at hand (compatibility between 1.11 client and 1.10
> server), I am not sure to understand the proposed fix: is the logic you
> proposed to be added to 1.10 server? why not simply add the missing enum
> value (and that's it! no more values after that!)?
>
>
> [Sorabh] - Just adding the missing enum value in 1.10 will not help since
> in future if any other enum value is introduced then the same issue will be
> seen. Moreover 1.10 doesn't support Encryption so that enum value should
> not be added in that release. Instead the fix is to treat the return value
> of UNKNOWN_SASL_SERVER while retrieving messages from future client as
> valid default value and take decision based on that.
>

But 1.10.1 could consider that a client supporting encryption also support
authentication? The code is already here in 1.10 btw:
https://github.com/apache/drill/blob/1.10.0/exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java#L297

Alternatively, you could just check that the field sasl_support is set and
not check the value alltogether. I'm not convinced you need to do some
extra logic around UNKNOWN_SASL_SERVER which would just keep people
confused (although it doesn't seem something you need to apply to 1.11 or
higher)



>
>
> Thanks,
> Sorabh
>
> 
> From: Laurent Goujon 
> Sent: Tuesday, October 31, 2017 5:42 PM
> To: dev
> Cc: Arina Lelchieva; sudhe...@apache.org
> Subject: Re: Drill SASL Forward Compatibility
>
> Regarding DRILL-5582 patch which broke compatibility with 1.9 version
> (which is less than a year old):
> I'm still not clear what we are trying to prevent here: stealing user
> credentials and/or data? connecting to a rogue server which could return
> corrupted data to the user? The patch gives a false sense of security
> because it prevents none of it:
> - if using password-based authentication, client is sending it in clear in
> the initial handshake so I guess it's already game over!
> - You could configure a client to not sent password over wire by choosing
> another authentication algorithm, BUT if there's no encryption of the
> channel, any data can be intercepted and rewritten. And of course, the
> rogue server could actually run queries on behalf of the user...
> - if using Kerberos, things are a bit different: even if a MITM intercepts
> the token, only the server can decode it properly and set up encryption so
> that only client can use it (a proxy server would not be able to decode the
> traffic). So what you need to ensure is that you actually use Kerberos as
> the only authentication mechanism, and that the channel is encrypted (if
> channel is not encryted, see above). This is things you should do by
> configuring the client by not sending the password (no need to), only
> authorize Kerberos authentication, and verify that encryption is enabled
> (which is already done I believe).
>
> For comparison, HTTP protocol (using it since it is one of the most used
> public protocol) has no issue with client sending an authentication header
> to a remote 

[jira] [Created] (DRILL-5919) Add session option to allow json reader/writer to work with NaN,INF

[Volodymyr Tkach (JIRA)]

Volodymyr Tkach created DRILL-5919:
--

 Summary: Add session option to allow json reader/writer to work 
with NaN,INF
 Key: DRILL-5919
 URL: https://issues.apache.org/jira/browse/DRILL-5919
 Project: Apache Drill
  Issue Type: Improvement
  Components: Storage - JSON
Affects Versions: 1.12.0
Reporter: Volodymyr Tkach
Assignee: Volodymyr Tkach
Priority: Minor


Add session options to allow drill working with non standard json strings 
number literals like: NaN, Infinity, -Infinity. By default these options will 
be switched off, the user will be able to toggle them during working session.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[GitHub] drill issue #892: DRILL-5645: negation of expression causes null pointer exc...

[josiahyan]

Github user josiahyan commented on the issue:

https://github.com/apache/drill/pull/892
  
Yay! First commit!

Thanks for the review!


---

Re: gitbox?

[Arina Yelchiyeva]

+1 but as we have communicated on Hangouts committer should be responsible
for:

1. making sure that there is only one commit (otherwise ask developer to
squash the commits, unless several commits are required) and commit message
is according to the Drill standards.
2. all unit tests pass.
3. all functional and advanced tests pass (
https://github.com/mapr/drill-test-framework).

Kind regards
Arina

On Tue, Oct 31, 2017 at 11:53 PM, Charles Givre  wrote:

> I’d vote +1 for moving to gitbox.
> — C
>
> > On Oct 31, 2017, at 13:54, Parth Chandra  wrote:
> >
> > Bumping this thread up.
> >
> > Vlad brought this up in the hangout today and it sounds like we would
> like
> > to move to Gitbox. Thanks Vlad for the patient explanations!
> >
> > Committers, let's use this thread to vote on the the suggestion.
> >
> > I'm +1 on moving to gitbox.
> >
> > Also, I can work with Vlad and Paul on updating the merge process
> document.
> >
> >
> >
> > On Wed, Aug 30, 2017 at 1:34 PM, Vlad Rozov  wrote:
> >
> >> Hi,
> >>
> >> As I am new to Drill, I don't know if migration from "Git WiP" (
> >> https://git-wip-us.apache.org) to "Github Dual Master" (
> >> https://gitbox.apache.org) was already discussed by the community, but
> >> from my Apache Apex experience I would recommend to consider migrating
> >> Drill ASF repos to the gitbox. Such move will give committers write
> access
> >> to the Drill repository on Github with all the perks that Github
> provides.
> >>
> >> Thank you,
> >>
> >> Vlad
> >>
>
>