You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@spark.apache.org by vanzin <gi...@git.apache.org> on 2017/01/09 22:01:33 UTC

[GitHub] spark pull request #16521: [SPARK-19139][core] New auth mechanism for transp...

GitHub user vanzin opened a pull request:

    https://github.com/apache/spark/pull/16521

    [SPARK-19139][core] New auth mechanism for transport library.

    This change introduces a new auth mechanism to the transport library,
    to be used when users enable strong encryption. This auth mechanism
    has better security than the currently used DIGEST-MD5.
    
    The new protocol uses symmetric key encryption to mutually authenticate
    the endpoints, and is very loosely based on ISO/IEC 9798.
    
    The new protocol falls back to SASL when it thinks the remote end is old.
    Because SASL does not support asking the server for multiple auth protocols,
    which would mean we could re-use the existing SASL code by just adding a
    new SASL provider, the protocol is implemented outside of the SASL API
    to avoid the boilerplate of adding a new provider.
    
    Details of the auth protocol are discussed in the included README.md
    file.
    
    This change partly undos the changes added in SPARK-13331; AES encryption
    is now decoupled from SASL authentication. The encryption code itself,
    though, has been re-used as part of this change.
    
    ## How was this patch tested?
    
    - Unit tests
    - Tested Spark 2.2 against Spark 1.6 shuffle service with SASL enabled
    - Tested Spark 2.2 against Spark 2.2 shuffle service with SASL fallback disabled


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

    $ git pull https://github.com/vanzin/spark SPARK-19139

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

    https://github.com/apache/spark/pull/16521.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 #16521
    
----
commit e219c8e1ffe634e44e6d6a44e02c21843b518241
Author: Marcelo Vanzin <va...@cloudera.com>
Date:   2016-12-16T00:11:51Z

    [SPARK-19139][core] New auth mechanism for transport library.
    
    This change introduces a new auth mechanism to the transport library,
    to be used when users enable strong encryption. This auth mechanism
    has better security than the currently used DIGEST-MD5.
    
    The new protocol uses symmetric key encryption to mutually authenticate
    the endpoints, and is very loosely based on ISO/IEC 9798.
    
    The new protocol falls back to SASL when it thinks the remote end is old.
    Because SASL does not support asking the server for multiple auth protocols,
    which would mean we could re-use the existing SASL code by just adding a
    new SASL provider, the protocol is implemented outside of the SASL API
    to avoid the boilerplate of adding a new provider.
    
    Details of the auth protocol are discussed in the included README.md
    file.
    
    This change partly undos the changes added in SPARK-13331; AES encryption
    is now decoupled from SASL authentication. The encryption code itself,
    though, has been re-used as part of this change.

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Merged build finished. Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    **[Test build #71869 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/71869/testReport)** for PR 16521 at commit [`718247e`](https://github.com/apache/spark/commit/718247e85d2a9767840bda57117c0dfc79041451).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    **[Test build #71870 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/71870/testReport)** for PR 16521 at commit [`718247e`](https://github.com/apache/spark/commit/718247e85d2a9767840bda57117c0dfc79041451).
     * This patch **fails to build**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    **[Test build #71095 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/71095/testReport)** for PR 16521 at commit [`e219c8e`](https://github.com/apache/spark/commit/e219c8e1ffe634e44e6d6a44e02c21843b518241).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #16521: [SPARK-19139][core] New auth mechanism for transp...

Posted by asfgit <gi...@git.apache.org>.

Github user asfgit closed the pull request at:

    https://github.com/apache/spark/pull/16521


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Merged build finished. Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #16521: [SPARK-19139][core] New auth mechanism for transp...

Posted by zsxwing <gi...@git.apache.org>.

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

https://github.com/apache/spark/pull/16521#discussion_r97184574

--- Diff: common/network-common/src/main/java/org/apache/spark/network/crypto/README.md ---
@@ -0,0 +1,158 @@
+Spark Auth Protocol and AES Encryption Support
+==============================================
+
+This file describes an auth protocol used by Spark as a more secure alternative to DIGEST-MD5. This
+protocol is built on symmetric key encryption, based on the assumption that the two endpoints being
+authenticated share a common secret, which is how Spark authentication currently works. The protocol
+provides mutual authentication, meaning that after the negotiation both parties know that the remote
+side knows the shared secret. The protocol is influenced by the ISO/IEC 9798 protocol, although it's
+not an implementation of it.
+
+This protocol could be replaced with TLS PSK, except no PSK ciphers are available in the currently
+released JREs.
+
+The protocol aims at solving the following shortcomings in Spark's current usage of DIGEST-MD5:
+
+- MD5 is an aging hash algorithm with known weaknesses, and a more secure alternative is desired.
+- DIGEST-MD5 has a pre-defined set of ciphers for which it can generate keys. The only
+ viable, supported cipher these days is 3DES, and a more modern alternative is desired.
+- Encrypting AES session keys with 3DES doesn't solve the issue, since the weakest link
+ in the negotiation would still be MD5 and 3DES.
+
+The protocol assumes that the shared secret is generated and distributed in a secure manner.
+
+The protocol always negotiates encryption keys. If encryption is not desired, the existing
+SASL-based authentication, or no authentication at all, can be chosen instead.
+
+When messages are described below, it's expected that the implementation should support
+arbitrary sizes for fields that don't have a fixed size.
+
+Client Challenge
+----------------
+
+The auth negotiation is started by the client. The client starts by generating an encryption
+key based on the application's shared secret, and a nonce.
+
+ KEY = KDF(SECRET, SALT, KEY_LENGTH)
+
+Where:
+- KDF(): a key derivation function that takes a secret, a salt, a configurable number of
+ iterations, and a configurable key length.
+- SALT: a byte sequence used to salt the key derivation function.
+- KEY_LENGTH: length of the encryption key to generate.
+
+
+The client generates a message with the following content:
+
+ CLIENT_CHALLENGE = (
+ APP_ID,
+ KDF,
+ ITERATIONS,
+ CIPHER,
+ KEY_LENGTH,
+ ANONCE,
+ ENC(APP_ID || ANONCE || CHALLENGE))
+
+Where:
+
+- APP_ID: the application ID which the server uses to identify the shared secret.
+- KDF: the key derivation function described above.
+- ITERATIONS: number of iterations to run the KDF when generating keys.
+- CIPHER: the cipher used to encrypt data.
+- KEY_LENGTH: length of the encryption keys to generate, in bits.
+- ANONCE: the nonce used as the salt when generating the auth key.
+- ENC(): an encryption function that uses the cipher and the generated key. This function
+ will also be used in the definition of other messages below.
+- CHALLENGE: a byte sequence used as a challenge to the server.
+- ||: concatenation operator.
+
+When strings are used where byte arrays are expected, the UTF-8 representation of the string
+is assumed.
+
+To respond to the challenge, the server should consider the byte array as representing an
+arbitrary-length integer, and respond with the value of the integer plus one.
+
+
+Server Response And Challenge
+-----------------------------
+
+Once the client challenge is received, the server will generate the same auth key by
+using the same algorithm the client has used. It will then verify the client challenge:
+if the APP_ID and ANONCE fields match, the server knows that the client has the shared
+secret. The server then creates a response to the client challenge, to prove that it also
+has the secret key, and provides parameters to be used when creating the session key.
+
+The following describes the response from the server:
+
+ SERVER_CHALLENGE = (
+ ENC(APP_ID || ANONCE || RESPONSE),
+ ENC(SNONCE),
+ ENC(INIV),
+ ENC(OUTIV))
+
+Where:
+
+- RESPONSE: the server's response to the client challenge.
+- SNONCE: a nonce to be used as salt when generating the session key.
+- INIV: initialization vector used to initialize the input channel of the client.
+- OUTIV: initialization vector used to initialize the output channel of the client.
+
+At this point the server considers the client to be authenticated, and will try to
+decrypt any data further sent by the client using the session key.
+
+
+Default Algorithms
+------------------
+
+Configuration options are available for the KDF and cipher algorithms to use.
+
+The default KDF is "PBKDF2WithHmacSHA1". Users should be able to select any algorithm
+from those supported by the `javax.crypto.SecretKeyFactory` class, as long as they support
+PBEKeySpec when generating keys. The default number of iterations was chosen to take a
+reasonable amount of time on modern CPUs. See the documentation in TransportConf for more
+details.
+
+The default cipher algorithm is "AES/CTR/NoPadding". Users should be able to select any
+algorithm supported by the commons-crypto library. It should allow the cipher to operate
+in stream mode.
+
+The default key length is 128 (bits).
+
+
+Implementation Details
+----------------------
+
+The commons-crypto library currently only supports AES ciphers, and requires an initialization
+vector (IV). This first version of the protocol does not explicitly include the IV in the client
+challenge message. Instead, the IV should be derived from the nonce, including the needed bytes, and
+padding the IV with zeroes in case the nonce is not long enough.
+
+Future versions of the protocol might add support for new ciphers and explicitly include needed
+configuration parameters in the messages.
+
+
+Threat Assessment
+-----------------
+
+The protocol is secure against different forms of attack:
+
+* Eavesdropping: the protocol is built on the assumption that it's computationally infeasible
+ to calculate the original secret from the encrypted messages. Neither the secret nor any
+ encryption keys are transmitted on the wire, encrypted or not.
+
+* Man-in-the-middle: because the protocol performs mutual authentication, both ends need to
+ know the shared secret to be able to decrypt session data. Even if an attacker is able to insert a
+ malicious "proxy" between endpoints, the attacker won't be able to read any of the data exchanged
+ between client and server, nor insert arbitrary commands for the server to execute.
+
+* Replay attacks: the use of nonces when generating keys prevents an attacker from being able to
--- End diff --

The server doesn't verify a nonce was used or not, so it don't prevents replay attacks. Right?

---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #16521: [SPARK-19139][core] New auth mechanism for transp...

Posted by zsxwing <gi...@git.apache.org>.

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

    https://github.com/apache/spark/pull/16521#discussion_r97185594
  
    --- Diff: common/network-common/src/test/java/org/apache/spark/network/crypto/AuthIntegrationSuite.java ---
    @@ -0,0 +1,211 @@
    +/*
    + * 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.spark.network.crypto;
    +
    +import java.nio.ByteBuffer;
    +import java.util.List;
    +import java.util.Map;
    +
    +import com.google.common.collect.ImmutableMap;
    +import com.google.common.collect.Lists;
    +import io.netty.channel.Channel;
    +import org.junit.After;
    +import org.junit.Test;
    +import static org.junit.Assert.*;
    +import static org.mockito.Mockito.*;
    +
    +import org.apache.spark.network.TestUtils;
    +import org.apache.spark.network.TransportContext;
    +import org.apache.spark.network.client.RpcResponseCallback;
    +import org.apache.spark.network.client.TransportClient;
    +import org.apache.spark.network.client.TransportClientBootstrap;
    +import org.apache.spark.network.sasl.SaslServerBootstrap;
    +import org.apache.spark.network.sasl.SecretKeyHolder;
    +import org.apache.spark.network.server.RpcHandler;
    +import org.apache.spark.network.server.StreamManager;
    +import org.apache.spark.network.server.TransportServer;
    +import org.apache.spark.network.server.TransportServerBootstrap;
    +import org.apache.spark.network.util.JavaUtils;
    +import org.apache.spark.network.util.MapConfigProvider;
    +import org.apache.spark.network.util.TransportConf;
    +
    +public class AuthIntegrationSuite {
    +
    +  private AuthTestCtx ctx;
    +
    +  @After
    +  public void cleanUp() throws Exception {
    +    if (ctx != null) {
    +      ctx.close();
    +    }
    +    ctx = null;
    +  }
    +
    +  @Test
    +  public void testNewAuth() throws Exception {
    +    ctx = new AuthTestCtx();
    +    ctx.createServer("secret");
    +    ctx.createClient("secret");
    +
    +    ByteBuffer reply = ctx.client.sendRpcSync(JavaUtils.stringToBytes("Ping"), 5000);
    +    assertEquals("Pong", JavaUtils.bytesToString(reply));
    +    assertTrue(ctx.authRpcHandler.doDelegate);
    +  }
    +
    +  @Test
    +  public void testAuthFailure() throws Exception {
    +    ctx = new AuthTestCtx();
    +    ctx.createServer("server");
    +
    +    try {
    +      ctx.createClient("client");
    +      fail("Should have failed to create client.");
    +    } catch (Exception e) {
    +      assertFalse(ctx.authRpcHandler.doDelegate);
    +      assertFalse(ctx.serverChannel.isActive());
    +    }
    +  }
    +
    +  @Test
    +  public void testSaslServerFallback() throws Exception {
    +    ctx = new AuthTestCtx();
    +    ctx.createServer("secret", true);
    +    ctx.createClient("secret", false);
    +
    +    ByteBuffer reply = ctx.client.sendRpcSync(JavaUtils.stringToBytes("Ping"), 5000);
    +    assertEquals("Pong", JavaUtils.bytesToString(reply));
    +  }
    +
    +  @Test
    +  public void testSaslClientFallback() throws Exception {
    +    ctx = new AuthTestCtx();
    +    ctx.createServer("secret", false);
    +    ctx.createClient("secret", true);
    +
    +    ByteBuffer reply = ctx.client.sendRpcSync(JavaUtils.stringToBytes("Ping"), 5000);
    +    assertEquals("Pong", JavaUtils.bytesToString(reply));
    +  }
    +
    +  @Test
    +  public void testAuthReplay() throws Exception {
    +    // This test covers the case where an attacker replays a challenge message sniffed from the
    +    // network, but doesn't know the actual secret. The server should close the connection as
    +    // soon as a message is sent after authentication is performed. This is emulated by removing
    +    // the client encryption handler after authentication.
    +    ctx = new AuthTestCtx();
    +    ctx.createServer("secret");
    +    ctx.createClient("secret");
    +
    +    assertNotNull(ctx.client.getChannel().pipeline()
    +      .remove(TransportCipher.ENCRYPTION_HANDLER_NAME));
    +
    +    try {
    +      ctx.client.sendRpcSync(JavaUtils.stringToBytes("Ping"), 5000);
    +      fail("Should have failed unencrypted RPC.");
    +    } catch (Exception e) {
    +      assertTrue(ctx.authRpcHandler.doDelegate);
    +    }
    +  }
    +
    +  private class AuthTestCtx {
    +
    +    private final String appId = "testAppId";
    +    private final TransportConf conf;
    +    private final TransportContext ctx;
    +
    +    TransportClient client;
    +    TransportServer server;
    +    Channel serverChannel;
    +    AuthRpcHandler authRpcHandler;
    --- End diff --
    
    nit: `volatile`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/71355/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #16521: [SPARK-19139][core] New auth mechanism for transp...

Posted by zsxwing <gi...@git.apache.org>.

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

    https://github.com/apache/spark/pull/16521#discussion_r97179088
  
    --- Diff: common/network-common/src/main/java/org/apache/spark/network/crypto/ClientChallenge.java ---
    @@ -0,0 +1,101 @@
    +/*
    + * 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.spark.network.crypto;
    +
    +import java.nio.ByteBuffer;
    +
    +import io.netty.buffer.ByteBuf;
    +import io.netty.buffer.Unpooled;
    +
    +import org.apache.spark.network.protocol.Encodable;
    +import org.apache.spark.network.protocol.Encoders;
    +
    +/**
    + * The client challenge message, used to initiate authentication.
    + *
    + * @see README.md
    + */
    +public class ClientChallenge implements Encodable {
    +  /** Serialization tag used to catch incorrect payloads. */
    +  private static final byte TAG_BYTE = (byte) 0xFA;
    +
    +  public final String appId;
    +  public final String kdf;
    +  public int iterations;
    --- End diff --
    
    nit: miss `final`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by zsxwing <gi...@git.apache.org>.

Github user zsxwing commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Made one pass. Looks good overall. Just some nits.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    **[Test build #71095 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/71095/testReport)** for PR 16521 at commit [`e219c8e`](https://github.com/apache/spark/commit/e219c8e1ffe634e44e6d6a44e02c21843b518241).
     * This patch **fails Spark unit tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    **[Test build #71347 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/71347/testReport)** for PR 16521 at commit [`ee9d232`](https://github.com/apache/spark/commit/ee9d232b7220aa6a48e80706431670ea1a913ede).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    **[Test build #71104 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/71104/testReport)** for PR 16521 at commit [`a2b3ff6`](https://github.com/apache/spark/commit/a2b3ff6175716f2509774a8ffbb2bf0f30b860cb).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    **[Test build #71104 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/71104/testReport)** for PR 16521 at commit [`a2b3ff6`](https://github.com/apache/spark/commit/a2b3ff6175716f2509774a8ffbb2bf0f30b860cb).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #16521: [SPARK-19139][core] New auth mechanism for transp...

Posted by zsxwing <gi...@git.apache.org>.

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

    https://github.com/apache/spark/pull/16521#discussion_r97185042
  
    --- Diff: common/network-common/src/main/java/org/apache/spark/network/util/TransportConf.java ---
    @@ -162,40 +163,95 @@ public int portMaxRetries() {
       }
     
       /**
    -   * Maximum number of bytes to be encrypted at a time when SASL encryption is enabled.
    +   * Enables strong encryption. Also enables the new auth protocol, used to negotiate keys.
        */
    -  public int maxSaslEncryptedBlockSize() {
    -    return Ints.checkedCast(JavaUtils.byteStringAsBytes(
    -      conf.get("spark.network.sasl.maxEncryptedBlockSize", "64k")));
    +  public boolean aesEncryptionEnabled() {
    --- End diff --
    
    nit: rename this method to a general name?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #16521: [SPARK-19139][core] New auth mechanism for transp...

Posted by vanzin <gi...@git.apache.org>.

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

https://github.com/apache/spark/pull/16521#discussion_r97391080

This is explained in the paragraph after the bullet list. The server always generates new nonces for sessions, so replaying the challenge will not allow an attacker to establish a session.

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    **[Test build #71871 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/71871/testReport)** for PR 16521 at commit [`39df4b3`](https://github.com/apache/spark/commit/39df4b312997e4c2f9bacfb9da2d5adaad8af509).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by yhuai <gi...@git.apache.org>.

Github user yhuai commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    @vanzin I have not reviewed this PR yet. Just have two level questions. Is there any change to existing behaviors and settings (compared with Spark 2.1)? Also, does our doc have enough contents to explain how to set those confs and how those work? Thanks!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    **[Test build #71356 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/71356/testReport)** for PR 16521 at commit [`ee9d232`](https://github.com/apache/spark/commit/ee9d232b7220aa6a48e80706431670ea1a913ede).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/71356/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #16521: [SPARK-19139][core] New auth mechanism for transp...

Posted by zsxwing <gi...@git.apache.org>.

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

    https://github.com/apache/spark/pull/16521#discussion_r97174570
  
    --- Diff: core/src/main/scala/org/apache/spark/SparkConf.scala ---
    @@ -607,6 +607,10 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
               "\"client\".")
           }
         }
    +
    +    val encryptionEnabled = get(NETWORK_ENCRYPTION_ENABLED) || get(SASL_ENCRYPTION_ENABLED)
    +    require(!encryptionEnabled || get(NETWORK_AUTH_ENABLED),
    +      s"${NETWORK_AUTH_ENABLED.key} must be enabled when enabling encryption.")
    --- End diff --
    
    It's unclear in the doc that what will be used when both `spark.network.crypto.enabled` and `spark.authenticate.enableSaslEncryption` are true. It's better to just disable this case. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #16521: [SPARK-19139][core] New auth mechanism for transp...

Posted by zsxwing <gi...@git.apache.org>.

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

    https://github.com/apache/spark/pull/16521#discussion_r97425988
  
    --- Diff: core/src/main/scala/org/apache/spark/SparkConf.scala ---
    @@ -607,6 +607,10 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
               "\"client\".")
           }
         }
    +
    +    val encryptionEnabled = get(NETWORK_ENCRYPTION_ENABLED) || get(SASL_ENCRYPTION_ENABLED)
    +    require(!encryptionEnabled || get(NETWORK_AUTH_ENABLED),
    +      s"${NETWORK_AUTH_ENABLED.key} must be enabled when enabling encryption.")
    --- End diff --
    
    Makes sense.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    **[Test build #71356 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/71356/testReport)** for PR 16521 at commit [`ee9d232`](https://github.com/apache/spark/commit/ee9d232b7220aa6a48e80706431670ea1a913ede).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #16521: [SPARK-19139][core] New auth mechanism for transp...

Posted by jsoltren <gi...@git.apache.org>.

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

    https://github.com/apache/spark/pull/16521#discussion_r96719170
  
    --- Diff: common/network-common/src/main/java/org/apache/spark/network/crypto/README.md ---
    @@ -0,0 +1,158 @@
    +Spark Auth Protocol and AES Encryption Support
    +==============================================
    +
    +This file describes an auth protocol used by Spark as a more secure alternative to DIGEST-MD5. This
    +protocol is built on symmetric key encryption, based on the assumption that the two endpoints being
    +authenticated share a common secret, which is how Spark authentication currently works. The protocol
    +provides mutual authentication, meaning that after the negotiation both parties know that the remote
    +side knows the shared secret. The protocol is influenced by the ISO/IEC 9798 protocol, although it's
    +not an implementation of it.
    +
    +This protocol could be replaced with TLS PSK, except no PSK ciphers are available in the currently
    +released JREs.
    +
    +The protocol aims at solving the following shortcomings in Spark's current usage of DIGEST-MD5:
    +
    +- MD5 is an aging hash algorithm with known weaknesses, and a more secure alternative is desired.
    +- DIGEST-MD5 has a pre-defined set of ciphers for which it can generate keys. The only
    +  viable, supported cipher these days is 3DES, and a more modern alternative is desired.
    +- Encrypting AES session keys with 3DES doesn't solve the issue, since the weakest link
    +  in the negotiation would still be MD5 and 3DES.
    +
    +The protocol assumes that the shared secret is generated and distributed in a secure manner.
    --- End diff --
    
    It might be helpful to mention the current shared secret generation and distribution mechanisms to drive the point that these are, hopefully, stronger than DIGEST-MD5 or possibly even AES.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #16521: [SPARK-19139][core] New auth mechanism for transp...

Posted by zsxwing <gi...@git.apache.org>.

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

    https://github.com/apache/spark/pull/16521#discussion_r97185583
  
    --- Diff: common/network-common/src/test/java/org/apache/spark/network/crypto/AuthIntegrationSuite.java ---
    @@ -0,0 +1,211 @@
    +/*
    + * 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.spark.network.crypto;
    +
    +import java.nio.ByteBuffer;
    +import java.util.List;
    +import java.util.Map;
    +
    +import com.google.common.collect.ImmutableMap;
    +import com.google.common.collect.Lists;
    +import io.netty.channel.Channel;
    +import org.junit.After;
    +import org.junit.Test;
    +import static org.junit.Assert.*;
    +import static org.mockito.Mockito.*;
    +
    +import org.apache.spark.network.TestUtils;
    +import org.apache.spark.network.TransportContext;
    +import org.apache.spark.network.client.RpcResponseCallback;
    +import org.apache.spark.network.client.TransportClient;
    +import org.apache.spark.network.client.TransportClientBootstrap;
    +import org.apache.spark.network.sasl.SaslServerBootstrap;
    +import org.apache.spark.network.sasl.SecretKeyHolder;
    +import org.apache.spark.network.server.RpcHandler;
    +import org.apache.spark.network.server.StreamManager;
    +import org.apache.spark.network.server.TransportServer;
    +import org.apache.spark.network.server.TransportServerBootstrap;
    +import org.apache.spark.network.util.JavaUtils;
    +import org.apache.spark.network.util.MapConfigProvider;
    +import org.apache.spark.network.util.TransportConf;
    +
    +public class AuthIntegrationSuite {
    +
    +  private AuthTestCtx ctx;
    +
    +  @After
    +  public void cleanUp() throws Exception {
    +    if (ctx != null) {
    +      ctx.close();
    +    }
    +    ctx = null;
    +  }
    +
    +  @Test
    +  public void testNewAuth() throws Exception {
    +    ctx = new AuthTestCtx();
    +    ctx.createServer("secret");
    +    ctx.createClient("secret");
    +
    +    ByteBuffer reply = ctx.client.sendRpcSync(JavaUtils.stringToBytes("Ping"), 5000);
    +    assertEquals("Pong", JavaUtils.bytesToString(reply));
    +    assertTrue(ctx.authRpcHandler.doDelegate);
    +  }
    +
    +  @Test
    +  public void testAuthFailure() throws Exception {
    +    ctx = new AuthTestCtx();
    +    ctx.createServer("server");
    +
    +    try {
    +      ctx.createClient("client");
    +      fail("Should have failed to create client.");
    +    } catch (Exception e) {
    +      assertFalse(ctx.authRpcHandler.doDelegate);
    +      assertFalse(ctx.serverChannel.isActive());
    +    }
    +  }
    +
    +  @Test
    +  public void testSaslServerFallback() throws Exception {
    +    ctx = new AuthTestCtx();
    +    ctx.createServer("secret", true);
    +    ctx.createClient("secret", false);
    +
    +    ByteBuffer reply = ctx.client.sendRpcSync(JavaUtils.stringToBytes("Ping"), 5000);
    +    assertEquals("Pong", JavaUtils.bytesToString(reply));
    +  }
    +
    +  @Test
    +  public void testSaslClientFallback() throws Exception {
    +    ctx = new AuthTestCtx();
    +    ctx.createServer("secret", false);
    +    ctx.createClient("secret", true);
    +
    +    ByteBuffer reply = ctx.client.sendRpcSync(JavaUtils.stringToBytes("Ping"), 5000);
    +    assertEquals("Pong", JavaUtils.bytesToString(reply));
    +  }
    +
    +  @Test
    +  public void testAuthReplay() throws Exception {
    +    // This test covers the case where an attacker replays a challenge message sniffed from the
    +    // network, but doesn't know the actual secret. The server should close the connection as
    +    // soon as a message is sent after authentication is performed. This is emulated by removing
    +    // the client encryption handler after authentication.
    +    ctx = new AuthTestCtx();
    +    ctx.createServer("secret");
    +    ctx.createClient("secret");
    +
    +    assertNotNull(ctx.client.getChannel().pipeline()
    +      .remove(TransportCipher.ENCRYPTION_HANDLER_NAME));
    +
    +    try {
    +      ctx.client.sendRpcSync(JavaUtils.stringToBytes("Ping"), 5000);
    +      fail("Should have failed unencrypted RPC.");
    +    } catch (Exception e) {
    +      assertTrue(ctx.authRpcHandler.doDelegate);
    +    }
    +  }
    +
    +  private class AuthTestCtx {
    +
    +    private final String appId = "testAppId";
    +    private final TransportConf conf;
    +    private final TransportContext ctx;
    +
    +    TransportClient client;
    +    TransportServer server;
    +    Channel serverChannel;
    --- End diff --
    
    nit: `volatile`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    **[Test build #71167 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/71167/testReport)** for PR 16521 at commit [`3894a02`](https://github.com/apache/spark/commit/3894a02bbc9f7e918b958a7aeb8a9964033bd1e1).
     * This patch **fails Spark unit tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by vanzin <gi...@git.apache.org>.

Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    /cc @zsxwing @squito @andrewor14 (in case he's still looking at things)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by vanzin <gi...@git.apache.org>.

Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    need to rebase to current master...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by vanzin <gi...@git.apache.org>.

Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    retest this please


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #16521: [SPARK-19139][core] New auth mechanism for transp...

Posted by vanzin <gi...@git.apache.org>.

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

    https://github.com/apache/spark/pull/16521#discussion_r96064676
  
    --- Diff: docs/configuration.md ---
    @@ -1625,40 +1625,40 @@ Apart from these, the following properties are also available, and may be useful
       </td>
     </tr>
     <tr>
    -  <td><code>spark.authenticate.enableSaslEncryption</code></td>
    +  <td><code>spark.network.crypto.enabled</code></td>
       <td>false</td>
       <td>
    -    Enable encrypted communication when authentication is
    -    enabled. This is supported by the block transfer service and the
    -    RPC endpoints.
    +    Enable encryption using the commons-crypto library for RPC and block transfer service.
    +    Requires <code>spark.authenticate</code> to be enabled.
    --- End diff --
    
    I added some for validating that in SparkConf; right now the config keys are scattered all over the code, I'll file a separate bug for cleaning those up.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/71869/
    Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    **[Test build #71871 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/71871/testReport)** for PR 16521 at commit [`39df4b3`](https://github.com/apache/spark/commit/39df4b312997e4c2f9bacfb9da2d5adaad8af509).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    **[Test build #71167 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/71167/testReport)** for PR 16521 at commit [`3894a02`](https://github.com/apache/spark/commit/3894a02bbc9f7e918b958a7aeb8a9964033bd1e1).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/71104/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    **[Test build #71355 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/71355/testReport)** for PR 16521 at commit [`ee9d232`](https://github.com/apache/spark/commit/ee9d232b7220aa6a48e80706431670ea1a913ede).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    **[Test build #71355 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/71355/testReport)** for PR 16521 at commit [`ee9d232`](https://github.com/apache/spark/commit/ee9d232b7220aa6a48e80706431670ea1a913ede).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #16521: [SPARK-19139][core] New auth mechanism for transp...

Posted by squito <gi...@git.apache.org>.

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

https://github.com/apache/spark/pull/16521#discussion_r95624898

typo: CHALLENGE

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    **[Test build #71870 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/71870/testReport)** for PR 16521 at commit [`718247e`](https://github.com/apache/spark/commit/718247e85d2a9767840bda57117c0dfc79041451).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #16521: [SPARK-19139][core] New auth mechanism for transp...

Posted by zsxwing <gi...@git.apache.org>.

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

    https://github.com/apache/spark/pull/16521#discussion_r97175304
  
    --- Diff: common/network-common/src/main/java/org/apache/spark/network/crypto/AuthClientBootstrap.java ---
    @@ -0,0 +1,128 @@
    +/*
    + * 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.spark.network.crypto;
    +
    +import java.io.IOException;
    +import java.nio.ByteBuffer;
    +import java.security.GeneralSecurityException;
    +import java.security.Key;
    +import javax.crypto.KeyGenerator;
    +import javax.crypto.Mac;
    +import static java.nio.charset.StandardCharsets.UTF_8;
    +
    +import com.google.common.base.Preconditions;
    +import com.google.common.base.Throwables;
    +import io.netty.buffer.ByteBuf;
    +import io.netty.buffer.Unpooled;
    +import io.netty.channel.Channel;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import org.apache.spark.network.client.TransportClient;
    +import org.apache.spark.network.client.TransportClientBootstrap;
    +import org.apache.spark.network.sasl.SaslClientBootstrap;
    +import org.apache.spark.network.sasl.SecretKeyHolder;
    +import org.apache.spark.network.util.JavaUtils;
    +import org.apache.spark.network.util.TransportConf;
    +
    +/**
    + * Bootstraps a {@link TransportClient} by performing authentication using Spark's auth protocol.
    + *
    + * This bootstrap falls back to using the SASL bootstrap if the server throws an error during
    + * authentication, and the configuration allows it. This is used for backwards compatibility
    + * with external shuffle services that do not support the new protocol.
    + *
    + * It also automatically falls back to SASL if the new encryption backend is disabled, so that
    + * callers only need to install this bootstrap when authentication is enabled.
    + */
    +public class AuthClientBootstrap implements TransportClientBootstrap {
    +
    +  private static final Logger LOG = LoggerFactory.getLogger(AuthClientBootstrap.class);
    +
    +  private final TransportConf conf;
    +  private final String appId;
    +  private final String authUser;
    +  private final SecretKeyHolder secretKeyHolder;
    +
    +  public AuthClientBootstrap(
    +      TransportConf conf,
    +      String appId,
    +      SecretKeyHolder secretKeyHolder) {
    +    this.conf = conf;
    +    // TODO: right now this behaves like the SASL backend, because when executors start up
    +    // they don't necessarily know the app ID. So they send a hardcoded "user" that is defined
    +    // in the SecurityManager, which will also always return the same secret (regardless of the
    +    // user name). All that's needed here is for this "user" to match on both sides, since that's
    +    // required by the protocol. At some point, though, it would be better for the actual app ID
    +    // to be provided here.
    +    this.appId = appId;
    +    this.authUser = secretKeyHolder.getSaslUser(appId);
    +    this.secretKeyHolder = secretKeyHolder;
    +  }
    +
    +  @Override
    +  public void doBootstrap(TransportClient client, Channel channel) {
    +    if (!conf.aesEncryptionEnabled()) {
    +      LOG.debug("AES encryption disabled, using old auth protocol.");
    +      doSaslAuth(client, channel);
    +      return;
    +    }
    +
    +    try {
    +      doSparkAuth(client, channel);
    +    } catch (GeneralSecurityException | IOException e) {
    +      throw Throwables.propagate(e);
    +    } catch (RuntimeException e) {
    +      // There isn't a good exception that can be caught here to know whether it's really
    +      // OK to switch back to SASL (because the server doesn't speak the new protocol). So
    +      // try it anyway, and in the worst case things will fail again.
    +      if (conf.saslFallback()) {
    +        LOG.debug("New auth protocol failed, trying SASL.", e);
    --- End diff --
    
    nit: sometimes, it's just because the server config is wrong and a warning is better to help the user find out it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/71347/
    Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #16521: [SPARK-19139][core] New auth mechanism for transp...

Posted by squito <gi...@git.apache.org>.

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

https://github.com/apache/spark/pull/16521#discussion_r95625434

typo: RESPONSE

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/71871/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    **[Test build #71347 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/71347/testReport)** for PR 16521 at commit [`ee9d232`](https://github.com/apache/spark/commit/ee9d232b7220aa6a48e80706431670ea1a913ede).
     * This patch **fails from timeout after a configured wait of \`250m\`**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #16521: [SPARK-19139][core] New auth mechanism for transp...

Posted by vanzin <gi...@git.apache.org>.

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

    https://github.com/apache/spark/pull/16521#discussion_r96720969
  
    --- Diff: common/network-common/src/main/java/org/apache/spark/network/crypto/README.md ---
    @@ -0,0 +1,158 @@
    +Spark Auth Protocol and AES Encryption Support
    +==============================================
    +
    +This file describes an auth protocol used by Spark as a more secure alternative to DIGEST-MD5. This
    +protocol is built on symmetric key encryption, based on the assumption that the two endpoints being
    +authenticated share a common secret, which is how Spark authentication currently works. The protocol
    +provides mutual authentication, meaning that after the negotiation both parties know that the remote
    +side knows the shared secret. The protocol is influenced by the ISO/IEC 9798 protocol, although it's
    +not an implementation of it.
    +
    +This protocol could be replaced with TLS PSK, except no PSK ciphers are available in the currently
    +released JREs.
    +
    +The protocol aims at solving the following shortcomings in Spark's current usage of DIGEST-MD5:
    +
    +- MD5 is an aging hash algorithm with known weaknesses, and a more secure alternative is desired.
    +- DIGEST-MD5 has a pre-defined set of ciphers for which it can generate keys. The only
    +  viable, supported cipher these days is 3DES, and a more modern alternative is desired.
    +- Encrypting AES session keys with 3DES doesn't solve the issue, since the weakest link
    +  in the negotiation would still be MD5 and 3DES.
    +
    +The protocol assumes that the shared secret is generated and distributed in a secure manner.
    --- End diff --
    
    That is discussed in `SecurityManager.scala`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #16521: [SPARK-19139][core] New auth mechanism for transp...

Posted by zsxwing <gi...@git.apache.org>.

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

    https://github.com/apache/spark/pull/16521#discussion_r97185757
  
    --- Diff: common/network-common/src/test/java/org/apache/spark/network/crypto/AuthIntegrationSuite.java ---
    @@ -0,0 +1,211 @@
    +/*
    + * 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.spark.network.crypto;
    +
    +import java.nio.ByteBuffer;
    +import java.util.List;
    +import java.util.Map;
    +
    +import com.google.common.collect.ImmutableMap;
    +import com.google.common.collect.Lists;
    +import io.netty.channel.Channel;
    +import org.junit.After;
    +import org.junit.Test;
    +import static org.junit.Assert.*;
    +import static org.mockito.Mockito.*;
    +
    +import org.apache.spark.network.TestUtils;
    +import org.apache.spark.network.TransportContext;
    +import org.apache.spark.network.client.RpcResponseCallback;
    +import org.apache.spark.network.client.TransportClient;
    +import org.apache.spark.network.client.TransportClientBootstrap;
    +import org.apache.spark.network.sasl.SaslServerBootstrap;
    +import org.apache.spark.network.sasl.SecretKeyHolder;
    +import org.apache.spark.network.server.RpcHandler;
    +import org.apache.spark.network.server.StreamManager;
    +import org.apache.spark.network.server.TransportServer;
    +import org.apache.spark.network.server.TransportServerBootstrap;
    +import org.apache.spark.network.util.JavaUtils;
    +import org.apache.spark.network.util.MapConfigProvider;
    +import org.apache.spark.network.util.TransportConf;
    +
    +public class AuthIntegrationSuite {
    +
    +  private AuthTestCtx ctx;
    +
    +  @After
    +  public void cleanUp() throws Exception {
    +    if (ctx != null) {
    +      ctx.close();
    +    }
    +    ctx = null;
    +  }
    +
    +  @Test
    +  public void testNewAuth() throws Exception {
    +    ctx = new AuthTestCtx();
    +    ctx.createServer("secret");
    +    ctx.createClient("secret");
    +
    +    ByteBuffer reply = ctx.client.sendRpcSync(JavaUtils.stringToBytes("Ping"), 5000);
    +    assertEquals("Pong", JavaUtils.bytesToString(reply));
    +    assertTrue(ctx.authRpcHandler.doDelegate);
    --- End diff --
    
    nit: please also check `delegate` type to ensure it doesn't use `sasl`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Merged build finished. Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by zsxwing <gi...@git.apache.org>.

Github user zsxwing commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Sorry for the delay. Looking at it now.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by vanzin <gi...@git.apache.org>.

Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    > Is there any change to existing behaviors and settings (compared with Spark 2.1)?
    
    No. I added some new config names that replace old ones (to have more generic names), but the old names still work.
    
    > Also, does our doc have enough contents to explain how to set those confs and how those work? 
    
    I think so.  I added docs for the new configs, the important old ones were already documented.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #16521: [SPARK-19139][core] New auth mechanism for transp...

Posted by squito <gi...@git.apache.org>.

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

https://github.com/apache/spark/pull/16521#discussion_r95625631

--- Diff: common/network-common/src/main/java/org/apache/spark/network/crypto/README.md ---
@@ -0,0 +1,158 @@
+Spark Auth Protocol and AES Encryption Support
+==============================================
+
+This file describes an auth protocol used by Spark as a more secure alternative to DIGEST-MD5. This
+protocol is built on symmetric key encryption, based on the assumption that the two endpoints being
+authenticated share a common secret, which is how Spark authentication currently works. The protocol
+provides mutual authentication, meaning that after the negotiation both parties know that the remote
+side knows the shared secret. The protocol is influenced by the ISO/IEC 9798 protocol, although it's
+not an implementation of it.
+
+This protocol could be replaced with TLS PSK, except no PSK ciphers are available in the currently
+released JREs.
+
+The protocol aims at solving the following shortcomings in Spark's current usage of DIGEST-MD5:
+
+- MD5 is an aging hash algorithm with known weaknesses, and a more secure alternative is desired.
+- DIGEST-MD5 has a pre-defined set of ciphers for which it can generate keys. The only
+ viable, supported cipher these days is 3DES, and a more modern alternative is desired.
+- Encrypting AES session keys with 3DES doesn't solve the issue, since the weakest link
+ in the negotiation would still be MD5 and 3DES.
+
+The protocol assumes that the shared secret is generated and distributed in a secure manner.
+
+The protocol always negotiates encryption keys. If encryption is not desired, the existing
+SASL-based authentication, or no authentication at all, can be chosen instead.
+
+When messages are described below, it's expected that the implementation should support
+arbitrary sizes for fields that don't have a fixed size.
+
+Client Challenge
+----------------
+
+The auth negotiation is started by the client. The client starts by generating an encryption
+key based on the application's shared secret, and a nonce.
+
+ KEY = KDF(SECRET, SALT, KEY_LENGTH)
+
+Where:
+- KDF(): a key derivation function that takes a secret, a salt, a configurable number of
+ iterations, and a configurable key length.
+- SALT: a byte sequence used to salt the key derivation function.
+- KEY_LENGTH: length of the encryption key to generate.
+
+
+The client generates a message with the following content:
+
+ CLIENT_CHALLENGE = (
+ APP_ID,
+ KDF,
+ ITERATIONS,
+ CIPHER,
+ KEY_LENGTH,
+ ANONCE,
+ ENC(APP_ID || ANONCE || CHALLENGE))
+
+Where:
+
+- APP_ID: the application ID which the server uses to identify the shared secret.
+- KDF: the key derivation function described above.
+- ITERATIONS: number of iterations to run the KDF when generating keys.
+- CIPHER: the cipher used to encrypt data.
+- KEY_LENGTH: length of the encryption keys to generate, in bits.
+- ANONCE: the nonce used as the salt when generating the auth key.
+- ENC(): an encryption function that uses the cipher and the generated key. This function
+ will also be used in the definition of other messages below.
+- CCHALLENGE: a byte sequence used as a challenge to the server.
+- ||: concatenation operator.
+
+When strings are used where byte arrays are expected, the UTF-8 representation of the string
+is assumed.
+
+To respond to the challenge, the server should consider the byte array as representing an
+arbitrary-length integer, and respond with the value of the integer plus one.
+
+
+Server Response And Challenge
+-----------------------------
+
+Once the client challenge is received, the server will generate the same auth key by
+using the same algorithm the client has used. It will then verify the client challenge:
+if the APP_ID and ANONCE fields match, the server knows that the client has the shared
+secret. The server then creates a response to the client challenge, to prove that it also
+has the secret key, and provides parameters to be used when creating the session key.
+
+The following describes the response from the server:
+
+ SERVER_CHALLENGE = (
+ ENC(APP_ID || ANONCE || RESPONSE),
+ ENC(SNONCE),
+ ENC(INIV),
+ ENC(OUTIV))
+
+Where:
+
+- CRESPONSE: the server's response to the client challenge.
+- SNONCE: a nonce to be used as salt when generating the session key.
+- INIV: initialization vector used to initialize the input channel of the client.
+- OUTIV: initialization vector used to initialize the output channel of the client.
+
+At this point the server considers the client to be authenticated, and will try to
+decrypt any data further sent by the client using the session key.
+
+
+Default Algorithms
+------------------
+
+Configuration options are available for the KDF and cipher algorithms to use.
+
+The default KDF is "PBKDF2WithHmacSHA1". Users should be able to select any algorithm
+from those supported by the `javax.crypto.SecretKeyFactory` class, as long as they support
+PBEKeySpec when generating keys. The default number of iterations is calculated to take a
+resonable amount of time on modern CPUs. See the documentation in TransportConf for more
--- End diff --

reasonable

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by zsxwing <gi...@git.apache.org>.

Github user zsxwing commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    LGTM pending tests


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #16521: [SPARK-19139][core] New auth mechanism for transp...

Posted by zsxwing <gi...@git.apache.org>.

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

https://github.com/apache/spark/pull/16521#discussion_r97425895

Yeah. I didn't read the codes correctly.

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by vanzin <gi...@git.apache.org>.

Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Hmm, my final cleanup broke some tests, let me fix those...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #16521: [SPARK-19139][core] New auth mechanism for transp...

Posted by vanzin <gi...@git.apache.org>.

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

    https://github.com/apache/spark/pull/16521#discussion_r97390391
  
    --- Diff: core/src/main/scala/org/apache/spark/SparkConf.scala ---
    @@ -607,6 +607,10 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Seria
               "\"client\".")
           }
         }
    +
    +    val encryptionEnabled = get(NETWORK_ENCRYPTION_ENABLED) || get(SASL_ENCRYPTION_ENABLED)
    +    require(!encryptionEnabled || get(NETWORK_AUTH_ENABLED),
    +      s"${NETWORK_AUTH_ENABLED.key} must be enabled when enabling encryption.")
    --- End diff --
    
    Disable what case?
    
    You need to be able to configure them separately, and if for some weird reason you want RPC encryption but don't want shuffle encryption when talking to an old shuffle service, these settings allow that.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Merged build finished. Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #16521: [SPARK-19139][core] New auth mechanism for transp...

Posted by squito <gi...@git.apache.org>.

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

    https://github.com/apache/spark/pull/16521#discussion_r95661186
  
    --- Diff: docs/configuration.md ---
    @@ -1625,40 +1625,40 @@ Apart from these, the following properties are also available, and may be useful
       </td>
     </tr>
     <tr>
    -  <td><code>spark.authenticate.enableSaslEncryption</code></td>
    +  <td><code>spark.network.crypto.enabled</code></td>
       <td>false</td>
       <td>
    -    Enable encrypted communication when authentication is
    -    enabled. This is supported by the block transfer service and the
    -    RPC endpoints.
    +    Enable encryption using the commons-crypto library for RPC and block transfer service.
    +    Requires <code>spark.authenticate</code> to be enabled.
    --- End diff --
    
    if `spark.authenticate=false`, what happens if this is `true`?  It looks like it is just ignored, I think fail-fast would be ideal.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #16521: [SPARK-19139][core] New auth mechanism for transp...

Posted by zsxwing <gi...@git.apache.org>.

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

    https://github.com/apache/spark/pull/16521#discussion_r97176557
  
    --- Diff: common/network-common/src/main/java/org/apache/spark/network/crypto/AuthRpcHandler.java ---
    @@ -0,0 +1,169 @@
    +/*
    + * 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.spark.network.crypto;
    +
    +import java.io.IOException;
    +import java.nio.ByteBuffer;
    +import javax.security.sasl.Sasl;
    +
    +import com.google.common.annotations.VisibleForTesting;
    +import com.google.common.base.Throwables;
    +import io.netty.buffer.ByteBuf;
    +import io.netty.buffer.Unpooled;
    +import io.netty.channel.Channel;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import org.apache.spark.network.client.RpcResponseCallback;
    +import org.apache.spark.network.client.TransportClient;
    +import org.apache.spark.network.sasl.SecretKeyHolder;
    +import org.apache.spark.network.sasl.SaslRpcHandler;
    +import org.apache.spark.network.server.RpcHandler;
    +import org.apache.spark.network.server.StreamManager;
    +import org.apache.spark.network.util.JavaUtils;
    +import org.apache.spark.network.util.TransportConf;
    +
    +/**
    + * RPC Handler which performs authentication using Spark's auth protocol before delegating to a
    + * child RPC handler. If the configuration allows, this handler will delegate messages to a SASL
    + * RPC handler for further authentication, to support for clients that do not support Spark's
    + * protocol.
    + *
    + * The delegate will only receive messages if the given connection has been successfully
    + * authenticated. A connection may be authenticated at most once.
    + */
    +class AuthRpcHandler extends RpcHandler {
    +  private static final Logger LOG = LoggerFactory.getLogger(AuthRpcHandler.class);
    +
    +  /** Transport configuration. */
    +  private final TransportConf conf;
    +
    +  /** The client channel. */
    +  private final Channel channel;
    +
    +  /**
    +   * RpcHandler we will delegate to for authenticated connections. When falling back to SASL
    +   * this will be replaced with the SASL RPC handler.
    +   */
    +  private RpcHandler delegate;
    +
    +  /** Class which provides secret keys which are shared by server and client on a per-app basis. */
    +  private final SecretKeyHolder secretKeyHolder;
    +
    +  /** Whether auth is done and future calls should be delegated. */
    +  @VisibleForTesting
    +  boolean doDelegate;
    +
    +  AuthRpcHandler(
    +      TransportConf conf,
    +      Channel channel,
    +      RpcHandler delegate,
    +      SecretKeyHolder secretKeyHolder) {
    +    this.conf = conf;
    +    this.channel = channel;
    +    this.delegate = delegate;
    +    this.secretKeyHolder = secretKeyHolder;
    +  }
    +
    +  @Override
    +  public void receive(TransportClient client, ByteBuffer message, RpcResponseCallback callback) {
    +    if (doDelegate) {
    +      delegate.receive(client, message, callback);
    +      return;
    +    }
    +
    +    int position = message.position();
    +    int limit = message.limit();
    +
    +    ClientChallenge challenge;
    +    try {
    +      challenge = ClientChallenge.decodeMessage(message);
    +      LOG.debug("Received new auth challenge for client {}.", channel.remoteAddress());
    +    } catch (RuntimeException e) {
    +      if (conf.saslFallback()) {
    +        LOG.debug("Failed to parse new auth challenge, reverting to SASL for client {}.",
    --- End diff --
    
    nit: debug -> warn


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Merged build finished. Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/71095/
    Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/71167/
    Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by zsxwing <gi...@git.apache.org>.

Github user zsxwing commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Thanks. Merging to master.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/71870/
    Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by vanzin <gi...@git.apache.org>.

Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Weird error, the code is there... retest this please


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #16521: [SPARK-19139][core] New auth mechanism for transp...

Posted by zsxwing <gi...@git.apache.org>.

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

    https://github.com/apache/spark/pull/16521#discussion_r97178959
  
    --- Diff: common/network-common/src/main/java/org/apache/spark/network/crypto/ServerResponse.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.spark.network.crypto;
    +
    +import java.nio.ByteBuffer;
    +
    +import io.netty.buffer.ByteBuf;
    +import io.netty.buffer.Unpooled;
    +
    +import org.apache.spark.network.protocol.Encodable;
    +import org.apache.spark.network.protocol.Encoders;
    +
    +/**
    + * Server's response to client's challenge.
    + *
    + * @see README.md
    + */
    +public class ServerResponse implements Encodable {
    +  /** Serialization tag used to catch incorrect payloads. */
    +  private static final byte TAG_BYTE = (byte) 0xFB;
    +
    +  public final byte[] response;
    +  public final byte[] nonce;
    +  public final byte[] inputIv;
    +  public final byte[] outputIv;
    +
    +  public ServerResponse(
    +      byte[] response,
    +      byte[] nonce,
    +      byte[] inputIv,
    +      byte[] outputIv) {
    +    this.response = response;
    +    this.nonce = nonce;
    +    this.inputIv = inputIv;
    +    this.outputIv = outputIv;
    +  }
    +
    +  @Override
    +  public int encodedLength() {
    +    return 1 +
    +      Encoders.ByteArrays.encodedLength(response) +
    +      Encoders.ByteArrays.encodedLength(nonce) +
    +      Encoders.ByteArrays.encodedLength(inputIv) +
    +      Encoders.ByteArrays.encodedLength(outputIv);
    +  }
    +
    +  @Override
    +  public void encode(ByteBuf buf) {
    +    buf.writeByte(TAG_BYTE);
    +    Encoders.ByteArrays.encode(buf, response);
    +    Encoders.ByteArrays.encode(buf, nonce);
    +    Encoders.ByteArrays.encode(buf, inputIv);
    +    Encoders.ByteArrays.encode(buf, outputIv);
    +  }
    +
    +  public static ServerResponse decodeMessage(ByteBuffer buffer) {
    +    ByteBuf buf = Unpooled.wrappedBuffer(buffer);
    +
    +    if (buf.readByte() != TAG_BYTE) {
    +      throw new IllegalArgumentException("Expected ServerChallenge, received something else.");
    --- End diff --
    
    nit: ServerChallenge  -> ServerResponse


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    **[Test build #71869 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/71869/testReport)** for PR 16521 at commit [`718247e`](https://github.com/apache/spark/commit/718247e85d2a9767840bda57117c0dfc79041451).
     * This patch **fails to build**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by vanzin <gi...@git.apache.org>.

Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Will leave this open for a couple more days, but would appreciate more eyes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #16521: [SPARK-19139][core] New auth mechanism for transport lib...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/16521
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org