You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "otterc (via GitHub)" <gi...@apache.org> on 2023/12/16 04:17:00 UTC
[PR] [CELEBORN-1176] Server side support for Sasl Auth [incubator-celeborn]
otterc opened a new pull request, #2164:
URL: https://github.com/apache/incubator-celeborn/pull/2164
### What changes were proposed in this pull request?
This adds the server side Sasl authentication support in the transport layer. Most of this code is taken from Apache Spark.
### Why are the changes needed?
The changes are needed for adding authentication to Celeborn. See [CELEBORN-1011](https://issues.apache.org/jira/browse/CELEBORN-1011).
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Added UTs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1176] Server side support for Sasl Auth [incubator-celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2164:
URL: https://github.com/apache/incubator-celeborn/pull/2164#discussion_r1429229386
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/SaslUtils.java:
##########
@@ -35,17 +35,33 @@ public class SaslUtils {
/** Quality of protection value that does not include encryption. */
static final String QOP_AUTH = "auth";
+ /** Quality of protection value that includes encryption. */
+ static final String QOP_AUTH_CONF = "auth-conf";
Review Comment:
Removed "auth-conf"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1176] Server side support for Sasl Auth [incubator-celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2164:
URL: https://github.com/apache/incubator-celeborn/pull/2164#discussion_r1429229090
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/SecretRegistry.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * 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.celeborn.common.network.sasl;
+
+/** Interface for getting a secret key associated with some application. */
+public interface SecretRegistry {
+
+ /** Gets an appropriate SASL secret key for the given userName. */
+ String getSecretKey(String userName);
Review Comment:
Changed to appId.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1176] Server side support for Sasl Auth [incubator-celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2164:
URL: https://github.com/apache/incubator-celeborn/pull/2164#discussion_r1429056107
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/CelebornSaslServer.java:
##########
@@ -0,0 +1,162 @@
+/*
+ * 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.celeborn.common.network.sasl;
+
+import static org.apache.celeborn.common.network.sasl.SaslUtils.*;
+
+import java.util.Map;
+
+import javax.annotation.Nullable;
+import javax.security.auth.callback.Callback;
+import javax.security.auth.callback.CallbackHandler;
+import javax.security.auth.callback.NameCallback;
+import javax.security.auth.callback.PasswordCallback;
+import javax.security.auth.callback.UnsupportedCallbackException;
+import javax.security.sasl.AuthorizeCallback;
+import javax.security.sasl.RealmCallback;
+import javax.security.sasl.Sasl;
+import javax.security.sasl.SaslException;
+import javax.security.sasl.SaslServer;
+
+import com.google.common.base.Preconditions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A SASL Server for Celeborn which simply keeps track of the state of a single SASL session, from
+ * the initial state to the "authenticated" state. (It is not a server in the sense of accepting
+ * connections on some socket.)
+ */
+public class CelebornSaslServer {
+ private static final Logger logger = LoggerFactory.getLogger(CelebornSaslServer.class);
+
+ private SaslServer saslServer;
+
+ public CelebornSaslServer(
+ String saslMechanism,
+ @Nullable Map<String, String> saslProps,
+ @Nullable CallbackHandler callbackHandler) {
+ Preconditions.checkNotNull(saslMechanism);
+ try {
+ this.saslServer =
+ Sasl.createSaslServer(saslMechanism, null, DEFAULT_REALM, saslProps, callbackHandler);
+ } catch (SaslException e) {
+ throw new IllegalArgumentException(e);
+ }
+ }
+
+ /** Determines whether the authentication exchange has completed successfully. */
+ public synchronized boolean isComplete() {
+ return saslServer != null && saslServer.isComplete();
+ }
+
+ /** Returns the value of a negotiated property. */
+ public synchronized Object getNegotiatedProperty(String name) {
+ return saslServer.getNegotiatedProperty(name);
+ }
+
+ /**
+ * Used to respond to server SASL tokens.
+ *
+ * @param token Server's SASL token
+ * @return response to send back to the server.
+ */
+ public synchronized byte[] response(byte[] token) {
+ try {
+ return saslServer != null ? saslServer.evaluateResponse(token) : EMPTY_BYTE_ARRAY;
+ } catch (SaslException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ /**
+ * Disposes of any system resources or security-sensitive information the SaslServer might be
+ * using.
+ */
+ public synchronized void dispose() {
+ if (saslServer != null) {
+ try {
+ saslServer.dispose();
+ } catch (SaslException e) {
+ // ignore
+ } finally {
+ saslServer = null;
+ }
+ }
+ }
+
+ /**
+ * Implementation of javax.security.auth.callback.CallbackHandler for SASL DIGEST-MD5 mechanism.
+ */
+ static class DigestCallbackHandler implements CallbackHandler {
+ private final SaslUserNameNotifier userNameNotifier;
+ private final SecretRegistry secretKeyHolder;
+
+ private String userName = null;
+
+ DigestCallbackHandler(SaslUserNameNotifier userNameNotifier, SecretRegistry secretRegistry) {
+ this.userNameNotifier = Preconditions.checkNotNull(userNameNotifier);
+ this.secretKeyHolder = Preconditions.checkNotNull(secretRegistry);
+ }
+
+ @Override
+ public void handle(Callback[] callbacks) throws UnsupportedCallbackException, SaslException {
+ for (Callback callback : callbacks) {
+ if (callback instanceof NameCallback) {
+ logger.trace("SASL server callback: setting username");
+ NameCallback nc = (NameCallback) callback;
+ String encodedName = nc.getName() != null ? nc.getName() : nc.getDefaultName();
+ if (encodedName == null) {
+ throw new SaslException("No username provided by client");
+ }
+ userName = decodeIdentifier(encodedName);
+ userNameNotifier.notifySaslUserName(userName);
+ } else if (callback instanceof PasswordCallback) {
+ logger.trace("SASL server callback: setting password");
+ PasswordCallback pc = (PasswordCallback) callback;
+ String secret = secretKeyHolder.getSecretKey(userName);
+ if (secret == null) {
+ // TODO: CELEBORN-1179 Add support for fetching the secret from the Celeborn master.
+ throw new RuntimeException("Registration information not found for " + userName);
+ }
+ pc.setPassword(encodePassword(secret));
+ } else if (callback instanceof RealmCallback) {
+ logger.trace("SASL server callback: setting realm");
+ RealmCallback rc = (RealmCallback) callback;
+ rc.setText(rc.getDefaultText());
+ } else if (callback instanceof AuthorizeCallback) {
+ AuthorizeCallback ac = (AuthorizeCallback) callback;
+ String authId = ac.getAuthenticationID();
+ String authzId = ac.getAuthorizationID();
+ ac.setAuthorized(authId.equals(authzId));
+ if (ac.isAuthorized()) {
+ ac.setAuthorizedID(authzId);
+ }
+ logger.debug("SASL Authorization complete, authorized set to {}", ac.isAuthorized());
+ } else {
+ throw new UnsupportedCallbackException(callback, "Unrecognized callback: " + callback);
+ }
+ }
+ }
+ }
+
+ /** Notifies the registered {@link SaslUserNameNotifier} of the SASL user name. */
+ interface SaslUserNameNotifier {
+ void notifySaslUserName(String userName);
Review Comment:
`setClientId` is also invoked at the server in `SaslRpcHandler`.
Here is the Spark code where it is set. https://github.com/apache/spark/blob/14a933bbe2eb1c71988f475036735f07b2e1fa6a/common/network-common/src/main/java/org/apache/spark/network/sasl/SaslRpcHandler.java#L87
It's then used in the other handlers in the `checkAuth()` method to check whether the app is authorized to access the shuffle data.
So, I kept this in Celeborn as well. I though we may need it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1176] Server side support for Sasl Auth [incubator-celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2164:
URL: https://github.com/apache/incubator-celeborn/pull/2164#discussion_r1429071278
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/CelebornSaslServer.java:
##########
@@ -0,0 +1,162 @@
+/*
+ * 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.celeborn.common.network.sasl;
+
+import static org.apache.celeborn.common.network.sasl.SaslUtils.*;
+
+import java.util.Map;
+
+import javax.annotation.Nullable;
+import javax.security.auth.callback.Callback;
+import javax.security.auth.callback.CallbackHandler;
+import javax.security.auth.callback.NameCallback;
+import javax.security.auth.callback.PasswordCallback;
+import javax.security.auth.callback.UnsupportedCallbackException;
+import javax.security.sasl.AuthorizeCallback;
+import javax.security.sasl.RealmCallback;
+import javax.security.sasl.Sasl;
+import javax.security.sasl.SaslException;
+import javax.security.sasl.SaslServer;
+
+import com.google.common.base.Preconditions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A SASL Server for Celeborn which simply keeps track of the state of a single SASL session, from
+ * the initial state to the "authenticated" state. (It is not a server in the sense of accepting
+ * connections on some socket.)
+ */
+public class CelebornSaslServer {
+ private static final Logger logger = LoggerFactory.getLogger(CelebornSaslServer.class);
+
+ private SaslServer saslServer;
+
+ public CelebornSaslServer(
+ String saslMechanism,
+ @Nullable Map<String, String> saslProps,
+ @Nullable CallbackHandler callbackHandler) {
+ Preconditions.checkNotNull(saslMechanism);
+ try {
+ this.saslServer =
+ Sasl.createSaslServer(saslMechanism, null, DEFAULT_REALM, saslProps, callbackHandler);
+ } catch (SaslException e) {
+ throw new IllegalArgumentException(e);
+ }
+ }
+
+ /** Determines whether the authentication exchange has completed successfully. */
+ public synchronized boolean isComplete() {
+ return saslServer != null && saslServer.isComplete();
+ }
+
+ /** Returns the value of a negotiated property. */
+ public synchronized Object getNegotiatedProperty(String name) {
+ return saslServer.getNegotiatedProperty(name);
+ }
+
+ /**
+ * Used to respond to server SASL tokens.
+ *
+ * @param token Server's SASL token
+ * @return response to send back to the server.
+ */
+ public synchronized byte[] response(byte[] token) {
+ try {
+ return saslServer != null ? saslServer.evaluateResponse(token) : EMPTY_BYTE_ARRAY;
+ } catch (SaslException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ /**
+ * Disposes of any system resources or security-sensitive information the SaslServer might be
+ * using.
+ */
+ public synchronized void dispose() {
+ if (saslServer != null) {
+ try {
+ saslServer.dispose();
+ } catch (SaslException e) {
+ // ignore
+ } finally {
+ saslServer = null;
+ }
+ }
+ }
+
+ /**
+ * Implementation of javax.security.auth.callback.CallbackHandler for SASL DIGEST-MD5 mechanism.
+ */
+ static class DigestCallbackHandler implements CallbackHandler {
+ private final SaslUserNameNotifier userNameNotifier;
+ private final SecretRegistry secretKeyHolder;
+
+ private String userName = null;
+
+ DigestCallbackHandler(SaslUserNameNotifier userNameNotifier, SecretRegistry secretRegistry) {
+ this.userNameNotifier = Preconditions.checkNotNull(userNameNotifier);
+ this.secretKeyHolder = Preconditions.checkNotNull(secretRegistry);
+ }
+
+ @Override
+ public void handle(Callback[] callbacks) throws UnsupportedCallbackException, SaslException {
+ for (Callback callback : callbacks) {
+ if (callback instanceof NameCallback) {
+ logger.trace("SASL server callback: setting username");
+ NameCallback nc = (NameCallback) callback;
+ String encodedName = nc.getName() != null ? nc.getName() : nc.getDefaultName();
+ if (encodedName == null) {
+ throw new SaslException("No username provided by client");
+ }
+ userName = decodeIdentifier(encodedName);
+ userNameNotifier.notifySaslUserName(userName);
+ } else if (callback instanceof PasswordCallback) {
+ logger.trace("SASL server callback: setting password");
+ PasswordCallback pc = (PasswordCallback) callback;
+ String secret = secretKeyHolder.getSecretKey(userName);
+ if (secret == null) {
+ // TODO: CELEBORN-1179 Add support for fetching the secret from the Celeborn master.
+ throw new RuntimeException("Registration information not found for " + userName);
+ }
+ pc.setPassword(encodePassword(secret));
+ } else if (callback instanceof RealmCallback) {
+ logger.trace("SASL server callback: setting realm");
+ RealmCallback rc = (RealmCallback) callback;
+ rc.setText(rc.getDefaultText());
+ } else if (callback instanceof AuthorizeCallback) {
+ AuthorizeCallback ac = (AuthorizeCallback) callback;
+ String authId = ac.getAuthenticationID();
+ String authzId = ac.getAuthorizationID();
+ ac.setAuthorized(authId.equals(authzId));
+ if (ac.isAuthorized()) {
+ ac.setAuthorizedID(authzId);
+ }
+ logger.debug("SASL Authorization complete, authorized set to {}", ac.isAuthorized());
+ } else {
+ throw new UnsupportedCallbackException(callback, "Unrecognized callback: " + callback);
+ }
+ }
+ }
+ }
+
+ /** Notifies the registered {@link SaslUserNameNotifier} of the SASL user name. */
+ interface SaslUserNameNotifier {
+ void notifySaslUserName(String userName);
Review Comment:
How is Celeborn currently ensuring that all requests from a client are for its own application id ? Or is this something you are planning to add in a later PR ?
Agree, for this we would need setClientId - I would have preferred if we set it after authn completes though ... but the result is the same, so should not matter imo.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1176] Server side support for Sasl Auth [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture closed pull request #2164: [CELEBORN-1176] Server side support for Sasl Auth
URL: https://github.com/apache/incubator-celeborn/pull/2164
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1176] Server side support for Sasl Auth [incubator-celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2164:
URL: https://github.com/apache/incubator-celeborn/pull/2164#discussion_r1429218980
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/SaslRpcHandler.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.celeborn.common.network.sasl;
+
+import static org.apache.celeborn.common.network.sasl.SaslUtils.*;
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+
+import io.netty.channel.Channel;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.network.client.RpcResponseCallback;
+import org.apache.celeborn.common.network.client.TransportClient;
+import org.apache.celeborn.common.network.protocol.RequestMessage;
+import org.apache.celeborn.common.network.protocol.RpcRequest;
+import org.apache.celeborn.common.network.protocol.TransportMessage;
+import org.apache.celeborn.common.network.server.AbstractAuthRpcHandler;
+import org.apache.celeborn.common.network.server.BaseMessageHandler;
+import org.apache.celeborn.common.network.util.TransportConf;
+import org.apache.celeborn.common.protocol.PbSaslRequest;
+
+/**
+ * RPC Handler which performs SASL authentication before delegating to a child RPC handler. The
+ * delegate will only receive messages if the given connection has been successfully authenticated.
+ * A connection may be authenticated at most once.
+ *
+ * <p>Note that the authentication process consists of multiple challenge-response pairs, each of
+ * which are individual RPCs.
+ */
+public class SaslRpcHandler extends AbstractAuthRpcHandler {
+ private static final Logger logger = LoggerFactory.getLogger(SaslRpcHandler.class);
+
+ /** Transport configuration. */
+ private final TransportConf conf;
+
+ /** The client channel. */
+ private final Channel channel;
+
+ /** Class which provides secret keys which are shared by server and client on a per-app basis. */
+ private final SecretRegistry secretRegistry;
+
+ private CelebornSaslServer saslServer;
+
+ public SaslRpcHandler(
+ TransportConf conf,
+ Channel channel,
+ BaseMessageHandler delegate,
+ SecretRegistry secretRegistry) {
+ super(delegate);
+ this.conf = conf;
+ this.channel = channel;
+ this.secretRegistry = secretRegistry;
+ this.saslServer = null;
+ }
+
+ @Override
+ public boolean checkRegistered() {
+ return delegate.checkRegistered();
+ }
+
+ @Override
+ public boolean doAuthChallenge(
+ TransportClient client, RequestMessage message, RpcResponseCallback callback) {
+ if (saslServer == null || !saslServer.isComplete()) {
+ RpcRequest rpcRequest = (RpcRequest) message;
+ PbSaslRequest saslMessage;
+ try {
+ TransportMessage pbMsg = TransportMessage.fromByteBuffer(message.body().nioByteBuffer());
+ saslMessage = pbMsg.getParsedPayload();
+ } catch (IOException e) {
+ logger.error("Error while parsing Sasl Message with RPC id {}", rpcRequest.requestId, e);
+ callback.onFailure(e);
+ return false;
+ }
+ assert saslMessage != null;
Review Comment:
I will remove this. If the message is not a sasl message, then there will be an exception parsing the message which will be handled by the block above it. I may have added it while testing but it is really not needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1176] Server side support for Sasl Auth [incubator-celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2164:
URL: https://github.com/apache/incubator-celeborn/pull/2164#discussion_r1429071278
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/CelebornSaslServer.java:
##########
@@ -0,0 +1,162 @@
+/*
+ * 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.celeborn.common.network.sasl;
+
+import static org.apache.celeborn.common.network.sasl.SaslUtils.*;
+
+import java.util.Map;
+
+import javax.annotation.Nullable;
+import javax.security.auth.callback.Callback;
+import javax.security.auth.callback.CallbackHandler;
+import javax.security.auth.callback.NameCallback;
+import javax.security.auth.callback.PasswordCallback;
+import javax.security.auth.callback.UnsupportedCallbackException;
+import javax.security.sasl.AuthorizeCallback;
+import javax.security.sasl.RealmCallback;
+import javax.security.sasl.Sasl;
+import javax.security.sasl.SaslException;
+import javax.security.sasl.SaslServer;
+
+import com.google.common.base.Preconditions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A SASL Server for Celeborn which simply keeps track of the state of a single SASL session, from
+ * the initial state to the "authenticated" state. (It is not a server in the sense of accepting
+ * connections on some socket.)
+ */
+public class CelebornSaslServer {
+ private static final Logger logger = LoggerFactory.getLogger(CelebornSaslServer.class);
+
+ private SaslServer saslServer;
+
+ public CelebornSaslServer(
+ String saslMechanism,
+ @Nullable Map<String, String> saslProps,
+ @Nullable CallbackHandler callbackHandler) {
+ Preconditions.checkNotNull(saslMechanism);
+ try {
+ this.saslServer =
+ Sasl.createSaslServer(saslMechanism, null, DEFAULT_REALM, saslProps, callbackHandler);
+ } catch (SaslException e) {
+ throw new IllegalArgumentException(e);
+ }
+ }
+
+ /** Determines whether the authentication exchange has completed successfully. */
+ public synchronized boolean isComplete() {
+ return saslServer != null && saslServer.isComplete();
+ }
+
+ /** Returns the value of a negotiated property. */
+ public synchronized Object getNegotiatedProperty(String name) {
+ return saslServer.getNegotiatedProperty(name);
+ }
+
+ /**
+ * Used to respond to server SASL tokens.
+ *
+ * @param token Server's SASL token
+ * @return response to send back to the server.
+ */
+ public synchronized byte[] response(byte[] token) {
+ try {
+ return saslServer != null ? saslServer.evaluateResponse(token) : EMPTY_BYTE_ARRAY;
+ } catch (SaslException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ /**
+ * Disposes of any system resources or security-sensitive information the SaslServer might be
+ * using.
+ */
+ public synchronized void dispose() {
+ if (saslServer != null) {
+ try {
+ saslServer.dispose();
+ } catch (SaslException e) {
+ // ignore
+ } finally {
+ saslServer = null;
+ }
+ }
+ }
+
+ /**
+ * Implementation of javax.security.auth.callback.CallbackHandler for SASL DIGEST-MD5 mechanism.
+ */
+ static class DigestCallbackHandler implements CallbackHandler {
+ private final SaslUserNameNotifier userNameNotifier;
+ private final SecretRegistry secretKeyHolder;
+
+ private String userName = null;
+
+ DigestCallbackHandler(SaslUserNameNotifier userNameNotifier, SecretRegistry secretRegistry) {
+ this.userNameNotifier = Preconditions.checkNotNull(userNameNotifier);
+ this.secretKeyHolder = Preconditions.checkNotNull(secretRegistry);
+ }
+
+ @Override
+ public void handle(Callback[] callbacks) throws UnsupportedCallbackException, SaslException {
+ for (Callback callback : callbacks) {
+ if (callback instanceof NameCallback) {
+ logger.trace("SASL server callback: setting username");
+ NameCallback nc = (NameCallback) callback;
+ String encodedName = nc.getName() != null ? nc.getName() : nc.getDefaultName();
+ if (encodedName == null) {
+ throw new SaslException("No username provided by client");
+ }
+ userName = decodeIdentifier(encodedName);
+ userNameNotifier.notifySaslUserName(userName);
+ } else if (callback instanceof PasswordCallback) {
+ logger.trace("SASL server callback: setting password");
+ PasswordCallback pc = (PasswordCallback) callback;
+ String secret = secretKeyHolder.getSecretKey(userName);
+ if (secret == null) {
+ // TODO: CELEBORN-1179 Add support for fetching the secret from the Celeborn master.
+ throw new RuntimeException("Registration information not found for " + userName);
+ }
+ pc.setPassword(encodePassword(secret));
+ } else if (callback instanceof RealmCallback) {
+ logger.trace("SASL server callback: setting realm");
+ RealmCallback rc = (RealmCallback) callback;
+ rc.setText(rc.getDefaultText());
+ } else if (callback instanceof AuthorizeCallback) {
+ AuthorizeCallback ac = (AuthorizeCallback) callback;
+ String authId = ac.getAuthenticationID();
+ String authzId = ac.getAuthorizationID();
+ ac.setAuthorized(authId.equals(authzId));
+ if (ac.isAuthorized()) {
+ ac.setAuthorizedID(authzId);
+ }
+ logger.debug("SASL Authorization complete, authorized set to {}", ac.isAuthorized());
+ } else {
+ throw new UnsupportedCallbackException(callback, "Unrecognized callback: " + callback);
+ }
+ }
+ }
+ }
+
+ /** Notifies the registered {@link SaslUserNameNotifier} of the SASL user name. */
+ interface SaslUserNameNotifier {
+ void notifySaslUserName(String userName);
Review Comment:
How is Celeborn currently ensuring that all requests from a client are for its own application id ? Or is this something you are planning to add in a later PR ?
Agree, for this we would need setClientId - I would have preferred if we set it after authn completes though ...
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/CelebornSaslServer.java:
##########
@@ -0,0 +1,162 @@
+/*
+ * 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.celeborn.common.network.sasl;
+
+import static org.apache.celeborn.common.network.sasl.SaslUtils.*;
+
+import java.util.Map;
+
+import javax.annotation.Nullable;
+import javax.security.auth.callback.Callback;
+import javax.security.auth.callback.CallbackHandler;
+import javax.security.auth.callback.NameCallback;
+import javax.security.auth.callback.PasswordCallback;
+import javax.security.auth.callback.UnsupportedCallbackException;
+import javax.security.sasl.AuthorizeCallback;
+import javax.security.sasl.RealmCallback;
+import javax.security.sasl.Sasl;
+import javax.security.sasl.SaslException;
+import javax.security.sasl.SaslServer;
+
+import com.google.common.base.Preconditions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A SASL Server for Celeborn which simply keeps track of the state of a single SASL session, from
+ * the initial state to the "authenticated" state. (It is not a server in the sense of accepting
+ * connections on some socket.)
+ */
+public class CelebornSaslServer {
+ private static final Logger logger = LoggerFactory.getLogger(CelebornSaslServer.class);
+
+ private SaslServer saslServer;
+
+ public CelebornSaslServer(
+ String saslMechanism,
+ @Nullable Map<String, String> saslProps,
+ @Nullable CallbackHandler callbackHandler) {
+ Preconditions.checkNotNull(saslMechanism);
+ try {
+ this.saslServer =
+ Sasl.createSaslServer(saslMechanism, null, DEFAULT_REALM, saslProps, callbackHandler);
+ } catch (SaslException e) {
+ throw new IllegalArgumentException(e);
+ }
+ }
+
+ /** Determines whether the authentication exchange has completed successfully. */
+ public synchronized boolean isComplete() {
+ return saslServer != null && saslServer.isComplete();
+ }
+
+ /** Returns the value of a negotiated property. */
+ public synchronized Object getNegotiatedProperty(String name) {
+ return saslServer.getNegotiatedProperty(name);
+ }
+
+ /**
+ * Used to respond to server SASL tokens.
+ *
+ * @param token Server's SASL token
+ * @return response to send back to the server.
+ */
+ public synchronized byte[] response(byte[] token) {
+ try {
+ return saslServer != null ? saslServer.evaluateResponse(token) : EMPTY_BYTE_ARRAY;
+ } catch (SaslException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ /**
+ * Disposes of any system resources or security-sensitive information the SaslServer might be
+ * using.
+ */
+ public synchronized void dispose() {
+ if (saslServer != null) {
+ try {
+ saslServer.dispose();
+ } catch (SaslException e) {
+ // ignore
+ } finally {
+ saslServer = null;
+ }
+ }
+ }
+
+ /**
+ * Implementation of javax.security.auth.callback.CallbackHandler for SASL DIGEST-MD5 mechanism.
+ */
+ static class DigestCallbackHandler implements CallbackHandler {
+ private final SaslUserNameNotifier userNameNotifier;
+ private final SecretRegistry secretKeyHolder;
+
+ private String userName = null;
+
+ DigestCallbackHandler(SaslUserNameNotifier userNameNotifier, SecretRegistry secretRegistry) {
+ this.userNameNotifier = Preconditions.checkNotNull(userNameNotifier);
+ this.secretKeyHolder = Preconditions.checkNotNull(secretRegistry);
+ }
+
+ @Override
+ public void handle(Callback[] callbacks) throws UnsupportedCallbackException, SaslException {
+ for (Callback callback : callbacks) {
+ if (callback instanceof NameCallback) {
+ logger.trace("SASL server callback: setting username");
+ NameCallback nc = (NameCallback) callback;
+ String encodedName = nc.getName() != null ? nc.getName() : nc.getDefaultName();
+ if (encodedName == null) {
+ throw new SaslException("No username provided by client");
+ }
+ userName = decodeIdentifier(encodedName);
+ userNameNotifier.notifySaslUserName(userName);
+ } else if (callback instanceof PasswordCallback) {
+ logger.trace("SASL server callback: setting password");
+ PasswordCallback pc = (PasswordCallback) callback;
+ String secret = secretKeyHolder.getSecretKey(userName);
+ if (secret == null) {
+ // TODO: CELEBORN-1179 Add support for fetching the secret from the Celeborn master.
+ throw new RuntimeException("Registration information not found for " + userName);
+ }
+ pc.setPassword(encodePassword(secret));
+ } else if (callback instanceof RealmCallback) {
+ logger.trace("SASL server callback: setting realm");
+ RealmCallback rc = (RealmCallback) callback;
+ rc.setText(rc.getDefaultText());
+ } else if (callback instanceof AuthorizeCallback) {
+ AuthorizeCallback ac = (AuthorizeCallback) callback;
+ String authId = ac.getAuthenticationID();
+ String authzId = ac.getAuthorizationID();
+ ac.setAuthorized(authId.equals(authzId));
+ if (ac.isAuthorized()) {
+ ac.setAuthorizedID(authzId);
+ }
+ logger.debug("SASL Authorization complete, authorized set to {}", ac.isAuthorized());
+ } else {
+ throw new UnsupportedCallbackException(callback, "Unrecognized callback: " + callback);
+ }
+ }
+ }
+ }
+
+ /** Notifies the registered {@link SaslUserNameNotifier} of the SASL user name. */
+ interface SaslUserNameNotifier {
+ void notifySaslUserName(String userName);
Review Comment:
How is Celeborn currently ensuring that all requests from a client are for its own application id ? Or is this something you are planning to add in a later PR ?
Agree, for this we would need setClientId - I would have preferred if we set it after authn completes though ... but the result is the same, so does not matter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1176] Server side support for Sasl Auth [incubator-celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2164:
URL: https://github.com/apache/incubator-celeborn/pull/2164#discussion_r1429228792
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/CelebornSaslServer.java:
##########
@@ -0,0 +1,162 @@
+/*
+ * 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.celeborn.common.network.sasl;
+
+import static org.apache.celeborn.common.network.sasl.SaslUtils.*;
+
+import java.util.Map;
+
+import javax.annotation.Nullable;
+import javax.security.auth.callback.Callback;
+import javax.security.auth.callback.CallbackHandler;
+import javax.security.auth.callback.NameCallback;
+import javax.security.auth.callback.PasswordCallback;
+import javax.security.auth.callback.UnsupportedCallbackException;
+import javax.security.sasl.AuthorizeCallback;
+import javax.security.sasl.RealmCallback;
+import javax.security.sasl.Sasl;
+import javax.security.sasl.SaslException;
+import javax.security.sasl.SaslServer;
+
+import com.google.common.base.Preconditions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A SASL Server for Celeborn which simply keeps track of the state of a single SASL session, from
+ * the initial state to the "authenticated" state. (It is not a server in the sense of accepting
+ * connections on some socket.)
+ */
+public class CelebornSaslServer {
+ private static final Logger logger = LoggerFactory.getLogger(CelebornSaslServer.class);
+
+ private SaslServer saslServer;
+
+ public CelebornSaslServer(
+ String saslMechanism,
+ @Nullable Map<String, String> saslProps,
+ @Nullable CallbackHandler callbackHandler) {
+ Preconditions.checkNotNull(saslMechanism);
+ try {
+ this.saslServer =
+ Sasl.createSaslServer(saslMechanism, null, DEFAULT_REALM, saslProps, callbackHandler);
+ } catch (SaslException e) {
+ throw new IllegalArgumentException(e);
+ }
+ }
+
+ /** Determines whether the authentication exchange has completed successfully. */
+ public synchronized boolean isComplete() {
+ return saslServer != null && saslServer.isComplete();
+ }
+
+ /** Returns the value of a negotiated property. */
+ public synchronized Object getNegotiatedProperty(String name) {
+ return saslServer.getNegotiatedProperty(name);
+ }
+
+ /**
+ * Used to respond to server SASL tokens.
+ *
+ * @param token Server's SASL token
+ * @return response to send back to the server.
+ */
+ public synchronized byte[] response(byte[] token) {
+ try {
+ return saslServer != null ? saslServer.evaluateResponse(token) : EMPTY_BYTE_ARRAY;
+ } catch (SaslException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ /**
+ * Disposes of any system resources or security-sensitive information the SaslServer might be
+ * using.
+ */
+ public synchronized void dispose() {
+ if (saslServer != null) {
+ try {
+ saslServer.dispose();
+ } catch (SaslException e) {
+ // ignore
+ } finally {
+ saslServer = null;
+ }
+ }
+ }
+
+ /**
+ * Implementation of javax.security.auth.callback.CallbackHandler for SASL DIGEST-MD5 mechanism.
+ */
+ static class DigestCallbackHandler implements CallbackHandler {
+ private final SaslUserNameNotifier userNameNotifier;
+ private final SecretRegistry secretKeyHolder;
+
+ private String userName = null;
Review Comment:
Yes, added the comment and also verified this is the behavior with DigestMD5Server.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1176] Server side support for Sasl Auth [incubator-celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on PR #2164:
URL: https://github.com/apache/incubator-celeborn/pull/2164#issuecomment-1858718285
@waitinfuture @mridulm @RexXiong @FMX @pan3793 @akpatnam25 @rmcyang
Could you please take a look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1176] Server side support for Sasl Auth [incubator-celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2164:
URL: https://github.com/apache/incubator-celeborn/pull/2164#discussion_r1429038814
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/CelebornSaslServer.java:
##########
@@ -0,0 +1,162 @@
+/*
+ * 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.celeborn.common.network.sasl;
+
+import static org.apache.celeborn.common.network.sasl.SaslUtils.*;
+
+import java.util.Map;
+
+import javax.annotation.Nullable;
+import javax.security.auth.callback.Callback;
+import javax.security.auth.callback.CallbackHandler;
+import javax.security.auth.callback.NameCallback;
+import javax.security.auth.callback.PasswordCallback;
+import javax.security.auth.callback.UnsupportedCallbackException;
+import javax.security.sasl.AuthorizeCallback;
+import javax.security.sasl.RealmCallback;
+import javax.security.sasl.Sasl;
+import javax.security.sasl.SaslException;
+import javax.security.sasl.SaslServer;
+
+import com.google.common.base.Preconditions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A SASL Server for Celeborn which simply keeps track of the state of a single SASL session, from
+ * the initial state to the "authenticated" state. (It is not a server in the sense of accepting
+ * connections on some socket.)
+ */
+public class CelebornSaslServer {
+ private static final Logger logger = LoggerFactory.getLogger(CelebornSaslServer.class);
+
+ private SaslServer saslServer;
+
+ public CelebornSaslServer(
+ String saslMechanism,
+ @Nullable Map<String, String> saslProps,
+ @Nullable CallbackHandler callbackHandler) {
+ Preconditions.checkNotNull(saslMechanism);
+ try {
+ this.saslServer =
+ Sasl.createSaslServer(saslMechanism, null, DEFAULT_REALM, saslProps, callbackHandler);
+ } catch (SaslException e) {
+ throw new IllegalArgumentException(e);
+ }
+ }
+
+ /** Determines whether the authentication exchange has completed successfully. */
+ public synchronized boolean isComplete() {
+ return saslServer != null && saslServer.isComplete();
+ }
+
+ /** Returns the value of a negotiated property. */
+ public synchronized Object getNegotiatedProperty(String name) {
+ return saslServer.getNegotiatedProperty(name);
+ }
+
+ /**
+ * Used to respond to server SASL tokens.
+ *
+ * @param token Server's SASL token
+ * @return response to send back to the server.
+ */
+ public synchronized byte[] response(byte[] token) {
+ try {
+ return saslServer != null ? saslServer.evaluateResponse(token) : EMPTY_BYTE_ARRAY;
+ } catch (SaslException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ /**
+ * Disposes of any system resources or security-sensitive information the SaslServer might be
+ * using.
+ */
+ public synchronized void dispose() {
+ if (saslServer != null) {
+ try {
+ saslServer.dispose();
+ } catch (SaslException e) {
+ // ignore
+ } finally {
+ saslServer = null;
+ }
+ }
+ }
+
+ /**
+ * Implementation of javax.security.auth.callback.CallbackHandler for SASL DIGEST-MD5 mechanism.
+ */
+ static class DigestCallbackHandler implements CallbackHandler {
+ private final SaslUserNameNotifier userNameNotifier;
+ private final SecretRegistry secretKeyHolder;
+
+ private String userName = null;
Review Comment:
Can you add a comment that this does not need to be `volatile` since the `handle` invocation includes both `NameCallback` and `PasswordCallback` (with name before password) - and so in the same thread ?
A future/different `CallbackHandler` might not be able to make that assumption (for a different mechanism) - in case others use this as the basis for adding future sasl mechanisms.
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/SaslRpcHandler.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.celeborn.common.network.sasl;
+
+import static org.apache.celeborn.common.network.sasl.SaslUtils.*;
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+
+import io.netty.channel.Channel;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.network.client.RpcResponseCallback;
+import org.apache.celeborn.common.network.client.TransportClient;
+import org.apache.celeborn.common.network.protocol.RequestMessage;
+import org.apache.celeborn.common.network.protocol.RpcRequest;
+import org.apache.celeborn.common.network.protocol.TransportMessage;
+import org.apache.celeborn.common.network.server.AbstractAuthRpcHandler;
+import org.apache.celeborn.common.network.server.BaseMessageHandler;
+import org.apache.celeborn.common.network.util.TransportConf;
+import org.apache.celeborn.common.protocol.PbSaslRequest;
+
+/**
+ * RPC Handler which performs SASL authentication before delegating to a child RPC handler. The
+ * delegate will only receive messages if the given connection has been successfully authenticated.
+ * A connection may be authenticated at most once.
+ *
+ * <p>Note that the authentication process consists of multiple challenge-response pairs, each of
+ * which are individual RPCs.
+ */
+public class SaslRpcHandler extends AbstractAuthRpcHandler {
+ private static final Logger logger = LoggerFactory.getLogger(SaslRpcHandler.class);
+
+ /** Transport configuration. */
+ private final TransportConf conf;
+
+ /** The client channel. */
+ private final Channel channel;
+
+ /** Class which provides secret keys which are shared by server and client on a per-app basis. */
+ private final SecretRegistry secretRegistry;
+
+ private CelebornSaslServer saslServer;
+
+ public SaslRpcHandler(
+ TransportConf conf,
+ Channel channel,
+ BaseMessageHandler delegate,
+ SecretRegistry secretRegistry) {
+ super(delegate);
+ this.conf = conf;
+ this.channel = channel;
+ this.secretRegistry = secretRegistry;
+ this.saslServer = null;
+ }
+
+ @Override
+ public boolean checkRegistered() {
+ return delegate.checkRegistered();
+ }
+
+ @Override
+ public boolean doAuthChallenge(
+ TransportClient client, RequestMessage message, RpcResponseCallback callback) {
+ if (saslServer == null || !saslServer.isComplete()) {
+ RpcRequest rpcRequest = (RpcRequest) message;
+ PbSaslRequest saslMessage;
+ try {
+ TransportMessage pbMsg = TransportMessage.fromByteBuffer(message.body().nioByteBuffer());
+ saslMessage = pbMsg.getParsedPayload();
+ } catch (IOException e) {
+ logger.error("Error while parsing Sasl Message with RPC id {}", rpcRequest.requestId, e);
+ callback.onFailure(e);
+ return false;
+ }
+ assert saslMessage != null;
+ if (saslServer == null) {
+ CelebornSaslServer.DigestCallbackHandler digestCallbackHandler =
+ getDigestCallbackHandler(client);
+ saslServer =
+ new CelebornSaslServer(DIGEST_MD5, DEFAULT_SASL_SERVER_PROPS, digestCallbackHandler);
+ }
+
+ byte[] response = saslServer.response(saslMessage.getPayload().toByteArray());
+ callback.onSuccess(ByteBuffer.wrap(response));
+ }
+ if (saslServer.isComplete()) {
+ logger.debug("SASL authentication successful for channel {}", client);
+ complete();
+ return true;
+ }
+ return false;
+ }
+
+ private CelebornSaslServer.DigestCallbackHandler getDigestCallbackHandler(
+ TransportClient client) {
+ CelebornSaslServer.SaslUserNameNotifier userNameNotifier =
+ id -> {
+ if (client.getClientId() != null && !client.getClientId().equals(id)) {
+ throw new IllegalStateException(
+ "Client tried to change user ID from " + client.getClientId() + " to " + id);
+ }
+ if (client.getClientId() == null) {
+ client.setClientId(id);
+ }
+ };
+
+ return new CelebornSaslServer.DigestCallbackHandler(userNameNotifier, secretRegistry);
+ }
+
Review Comment:
Do we want to refactor `channelInactive` and `complete` ?
Something like:
```
channelInactive:
super.channelInactive()
cleanup()
complete:
cleanup()
cleanup:
if (null != saslServer) {
try {
saslServer.dispose();
} catch (Exception ex) {
// handle it
} finally {
saslServer = null;
}
```
##########
common/src/test/java/org/apache/celeborn/common/network/sasl/CelebornSaslSuiteJ.java:
##########
@@ -0,0 +1,209 @@
+/*
+ * 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.celeborn.common.network.sasl;
+
+import static org.apache.celeborn.common.network.sasl.SaslUtils.*;
+import static org.junit.Assert.*;
+import static org.mockito.Mockito.*;
+
+import java.nio.ByteBuffer;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.celeborn.common.CelebornConf;
+import org.apache.celeborn.common.network.TransportContext;
+import org.apache.celeborn.common.network.client.RpcResponseCallback;
+import org.apache.celeborn.common.network.client.TransportClient;
+import org.apache.celeborn.common.network.client.TransportClientBootstrap;
+import org.apache.celeborn.common.network.protocol.RequestMessage;
+import org.apache.celeborn.common.network.server.BaseMessageHandler;
+import org.apache.celeborn.common.network.server.TransportServer;
+import org.apache.celeborn.common.network.util.TransportConf;
+import org.apache.celeborn.common.util.JavaUtils;
+
+/**
+ * Jointly tests {@link CelebornSaslClient} and {@link CelebornSaslServer}, as both are black boxes.
+ */
+public class CelebornSaslSuiteJ {
+ private static final String TEST_USER = "appId";
+ private static final String TEST_SECRET = "secret";
+
+ @BeforeClass
+ public static void setup() {
+ SecretRegistryImpl.getInstance().register(TEST_USER, TEST_SECRET);
+ }
+
+ @Test
+ public void testDigestMatching() {
+ CelebornSaslClient client =
+ new CelebornSaslClient(
+ DIGEST_MD5,
+ DEFAULT_SASL_CLIENT_PROPS,
+ new CelebornSaslClient.ClientCallbackHandler(TEST_USER, TEST_SECRET));
+ CelebornSaslServer server =
+ new CelebornSaslServer(
+ DIGEST_MD5,
+ DEFAULT_SASL_SERVER_PROPS,
+ new CelebornSaslServer.DigestCallbackHandler(
+ userName -> assertEquals(TEST_USER, userName), SecretRegistryImpl.getInstance()));
+
+ assertFalse(client.isComplete());
+ assertFalse(server.isComplete());
+
+ byte[] clientMessage = client.firstToken();
+
+ while (!client.isComplete()) {
+ clientMessage = client.response(server.response(clientMessage));
+ }
+ assertTrue(server.isComplete());
+
+ // Disposal should invalidate
+ server.dispose();
+ assertFalse(server.isComplete());
+ client.dispose();
+ assertFalse(client.isComplete());
+ }
+
+ @Test
+ public void testDigestNonMatching() {
+ CelebornSaslClient client =
+ new CelebornSaslClient(
+ DIGEST_MD5,
+ DEFAULT_SASL_CLIENT_PROPS,
+ new CelebornSaslClient.ClientCallbackHandler(TEST_USER, "my-secret"));
Review Comment:
nit: to make it explicit - mutate `TEST_SECRET` so that it does not match. For example: `"invalid-" + TEST_SECRET` or some such ?
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/SecretRegistry.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * 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.celeborn.common.network.sasl;
+
+/** Interface for getting a secret key associated with some application. */
+public interface SecretRegistry {
+
+ /** Gets an appropriate SASL secret key for the given userName. */
+ String getSecretKey(String userName);
Review Comment:
We have `userName` here and `appId` in `isRegistered` - make both `appId` (perhaps preferable - to make intent clear) or `userName` ?
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/SaslRpcHandler.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.celeborn.common.network.sasl;
+
+import static org.apache.celeborn.common.network.sasl.SaslUtils.*;
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+
+import io.netty.channel.Channel;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.network.client.RpcResponseCallback;
+import org.apache.celeborn.common.network.client.TransportClient;
+import org.apache.celeborn.common.network.protocol.RequestMessage;
+import org.apache.celeborn.common.network.protocol.RpcRequest;
+import org.apache.celeborn.common.network.protocol.TransportMessage;
+import org.apache.celeborn.common.network.server.AbstractAuthRpcHandler;
+import org.apache.celeborn.common.network.server.BaseMessageHandler;
+import org.apache.celeborn.common.network.util.TransportConf;
+import org.apache.celeborn.common.protocol.PbSaslRequest;
+
+/**
+ * RPC Handler which performs SASL authentication before delegating to a child RPC handler. The
+ * delegate will only receive messages if the given connection has been successfully authenticated.
+ * A connection may be authenticated at most once.
+ *
+ * <p>Note that the authentication process consists of multiple challenge-response pairs, each of
+ * which are individual RPCs.
+ */
+public class SaslRpcHandler extends AbstractAuthRpcHandler {
+ private static final Logger logger = LoggerFactory.getLogger(SaslRpcHandler.class);
+
+ /** Transport configuration. */
+ private final TransportConf conf;
+
+ /** The client channel. */
+ private final Channel channel;
+
+ /** Class which provides secret keys which are shared by server and client on a per-app basis. */
+ private final SecretRegistry secretRegistry;
+
+ private CelebornSaslServer saslServer;
+
+ public SaslRpcHandler(
+ TransportConf conf,
+ Channel channel,
+ BaseMessageHandler delegate,
+ SecretRegistry secretRegistry) {
+ super(delegate);
+ this.conf = conf;
+ this.channel = channel;
+ this.secretRegistry = secretRegistry;
+ this.saslServer = null;
+ }
+
+ @Override
+ public boolean checkRegistered() {
+ return delegate.checkRegistered();
+ }
+
+ @Override
+ public boolean doAuthChallenge(
+ TransportClient client, RequestMessage message, RpcResponseCallback callback) {
+ if (saslServer == null || !saslServer.isComplete()) {
+ RpcRequest rpcRequest = (RpcRequest) message;
+ PbSaslRequest saslMessage;
+ try {
+ TransportMessage pbMsg = TransportMessage.fromByteBuffer(message.body().nioByteBuffer());
+ saslMessage = pbMsg.getParsedPayload();
+ } catch (IOException e) {
+ logger.error("Error while parsing Sasl Message with RPC id {}", rpcRequest.requestId, e);
+ callback.onFailure(e);
+ return false;
+ }
+ assert saslMessage != null;
Review Comment:
QQ: If this does become `null` - what is the result ? Will it fail the connection with an rpc error ?
Do we want to explicitly call `onFailure` instead ?
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/SaslUtils.java:
##########
@@ -35,17 +35,33 @@ public class SaslUtils {
/** Quality of protection value that does not include encryption. */
static final String QOP_AUTH = "auth";
+ /** Quality of protection value that includes encryption. */
+ static final String QOP_AUTH_CONF = "auth-conf";
Review Comment:
We are not relying on SASL to provide encryption support, why are we using `auth-conf` instead of `auth` ?
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/CelebornSaslServer.java:
##########
@@ -0,0 +1,162 @@
+/*
+ * 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.celeborn.common.network.sasl;
+
+import static org.apache.celeborn.common.network.sasl.SaslUtils.*;
+
+import java.util.Map;
+
+import javax.annotation.Nullable;
+import javax.security.auth.callback.Callback;
+import javax.security.auth.callback.CallbackHandler;
+import javax.security.auth.callback.NameCallback;
+import javax.security.auth.callback.PasswordCallback;
+import javax.security.auth.callback.UnsupportedCallbackException;
+import javax.security.sasl.AuthorizeCallback;
+import javax.security.sasl.RealmCallback;
+import javax.security.sasl.Sasl;
+import javax.security.sasl.SaslException;
+import javax.security.sasl.SaslServer;
+
+import com.google.common.base.Preconditions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A SASL Server for Celeborn which simply keeps track of the state of a single SASL session, from
+ * the initial state to the "authenticated" state. (It is not a server in the sense of accepting
+ * connections on some socket.)
+ */
+public class CelebornSaslServer {
+ private static final Logger logger = LoggerFactory.getLogger(CelebornSaslServer.class);
+
+ private SaslServer saslServer;
+
+ public CelebornSaslServer(
+ String saslMechanism,
+ @Nullable Map<String, String> saslProps,
+ @Nullable CallbackHandler callbackHandler) {
+ Preconditions.checkNotNull(saslMechanism);
+ try {
+ this.saslServer =
+ Sasl.createSaslServer(saslMechanism, null, DEFAULT_REALM, saslProps, callbackHandler);
+ } catch (SaslException e) {
+ throw new IllegalArgumentException(e);
+ }
+ }
+
+ /** Determines whether the authentication exchange has completed successfully. */
+ public synchronized boolean isComplete() {
+ return saslServer != null && saslServer.isComplete();
+ }
+
+ /** Returns the value of a negotiated property. */
+ public synchronized Object getNegotiatedProperty(String name) {
+ return saslServer.getNegotiatedProperty(name);
+ }
+
+ /**
+ * Used to respond to server SASL tokens.
+ *
+ * @param token Server's SASL token
+ * @return response to send back to the server.
+ */
+ public synchronized byte[] response(byte[] token) {
+ try {
+ return saslServer != null ? saslServer.evaluateResponse(token) : EMPTY_BYTE_ARRAY;
+ } catch (SaslException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ /**
+ * Disposes of any system resources or security-sensitive information the SaslServer might be
+ * using.
+ */
+ public synchronized void dispose() {
+ if (saslServer != null) {
+ try {
+ saslServer.dispose();
+ } catch (SaslException e) {
+ // ignore
+ } finally {
+ saslServer = null;
+ }
+ }
+ }
+
+ /**
+ * Implementation of javax.security.auth.callback.CallbackHandler for SASL DIGEST-MD5 mechanism.
+ */
+ static class DigestCallbackHandler implements CallbackHandler {
+ private final SaslUserNameNotifier userNameNotifier;
+ private final SecretRegistry secretKeyHolder;
+
+ private String userName = null;
+
+ DigestCallbackHandler(SaslUserNameNotifier userNameNotifier, SecretRegistry secretRegistry) {
+ this.userNameNotifier = Preconditions.checkNotNull(userNameNotifier);
+ this.secretKeyHolder = Preconditions.checkNotNull(secretRegistry);
+ }
+
+ @Override
+ public void handle(Callback[] callbacks) throws UnsupportedCallbackException, SaslException {
+ for (Callback callback : callbacks) {
+ if (callback instanceof NameCallback) {
+ logger.trace("SASL server callback: setting username");
+ NameCallback nc = (NameCallback) callback;
+ String encodedName = nc.getName() != null ? nc.getName() : nc.getDefaultName();
+ if (encodedName == null) {
+ throw new SaslException("No username provided by client");
+ }
+ userName = decodeIdentifier(encodedName);
+ userNameNotifier.notifySaslUserName(userName);
+ } else if (callback instanceof PasswordCallback) {
+ logger.trace("SASL server callback: setting password");
+ PasswordCallback pc = (PasswordCallback) callback;
+ String secret = secretKeyHolder.getSecretKey(userName);
+ if (secret == null) {
+ // TODO: CELEBORN-1179 Add support for fetching the secret from the Celeborn master.
+ throw new RuntimeException("Registration information not found for " + userName);
+ }
+ pc.setPassword(encodePassword(secret));
+ } else if (callback instanceof RealmCallback) {
+ logger.trace("SASL server callback: setting realm");
+ RealmCallback rc = (RealmCallback) callback;
+ rc.setText(rc.getDefaultText());
+ } else if (callback instanceof AuthorizeCallback) {
+ AuthorizeCallback ac = (AuthorizeCallback) callback;
+ String authId = ac.getAuthenticationID();
+ String authzId = ac.getAuthorizationID();
+ ac.setAuthorized(authId.equals(authzId));
+ if (ac.isAuthorized()) {
+ ac.setAuthorizedID(authzId);
+ }
+ logger.debug("SASL Authorization complete, authorized set to {}", ac.isAuthorized());
+ } else {
+ throw new UnsupportedCallbackException(callback, "Unrecognized callback: " + callback);
+ }
+ }
+ }
+ }
+
+ /** Notifies the registered {@link SaslUserNameNotifier} of the SASL user name. */
+ interface SaslUserNameNotifier {
+ void notifySaslUserName(String userName);
Review Comment:
Is this mainly for use in `CELEBORN-1179` ? Or is this for future evolution to support s2s connection sasl ?
Currently, `setClientId` is invoked only at client, not server
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1176] Server side support for Sasl Auth [incubator-celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2164:
URL: https://github.com/apache/incubator-celeborn/pull/2164#issuecomment-1858715578
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2164?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `37 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`41df4eb`)](https://app.codecov.io/gh/apache/incubator-celeborn/commit/41df4ebbea7963c8ad7b87b1ce1307e5979a4a5c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 46.03% compared to head [(`ecfa567`)](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2164?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 47.07%.
> Report is 2 commits behind head on main.
| [Files](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...e/celeborn/common/network/sasl/SaslRpcHandler.java](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9jZWxlYm9ybi9jb21tb24vbmV0d29yay9zYXNsL1Nhc2xScGNIYW5kbGVyLmphdmE=) | 68.75% | [9 Missing and 6 partials :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...leborn/common/network/sasl/CelebornSaslServer.java](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9jZWxlYm9ybi9jb21tb24vbmV0d29yay9zYXNsL0NlbGVib3JuU2FzbFNlcnZlci5qYXZh) | 74.55% | [7 Missing and 7 partials :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../common/network/server/AbstractAuthRpcHandler.java](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9jZWxlYm9ybi9jb21tb24vbmV0d29yay9zZXJ2ZXIvQWJzdHJhY3RBdXRoUnBjSGFuZGxlci5qYXZh) | 75.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...leborn/common/network/sasl/SecretRegistryImpl.java](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9jZWxlYm9ybi9jb21tb24vbmV0d29yay9zYXNsL1NlY3JldFJlZ2lzdHJ5SW1wbC5qYXZh) | 70.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2164?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #2164 +/- ##
==========================================
+ Coverage 46.03% 47.07% +1.05%
==========================================
Files 180 185 +5
Lines 11492 11662 +170
Branches 1027 1042 +15
==========================================
+ Hits 5289 5489 +200
+ Misses 5852 5803 -49
- Partials 351 370 +19
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2164?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1176] Server side support for Sasl Auth [incubator-celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2164:
URL: https://github.com/apache/incubator-celeborn/pull/2164#discussion_r1429040236
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/SaslRpcHandler.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.celeborn.common.network.sasl;
+
+import static org.apache.celeborn.common.network.sasl.SaslUtils.*;
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+
+import io.netty.channel.Channel;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.network.client.RpcResponseCallback;
+import org.apache.celeborn.common.network.client.TransportClient;
+import org.apache.celeborn.common.network.protocol.RequestMessage;
+import org.apache.celeborn.common.network.protocol.RpcRequest;
+import org.apache.celeborn.common.network.protocol.TransportMessage;
+import org.apache.celeborn.common.network.server.AbstractAuthRpcHandler;
+import org.apache.celeborn.common.network.server.BaseMessageHandler;
+import org.apache.celeborn.common.network.util.TransportConf;
+import org.apache.celeborn.common.protocol.PbSaslRequest;
+
+/**
+ * RPC Handler which performs SASL authentication before delegating to a child RPC handler. The
+ * delegate will only receive messages if the given connection has been successfully authenticated.
+ * A connection may be authenticated at most once.
+ *
+ * <p>Note that the authentication process consists of multiple challenge-response pairs, each of
+ * which are individual RPCs.
+ */
+public class SaslRpcHandler extends AbstractAuthRpcHandler {
+ private static final Logger logger = LoggerFactory.getLogger(SaslRpcHandler.class);
+
+ /** Transport configuration. */
+ private final TransportConf conf;
+
+ /** The client channel. */
+ private final Channel channel;
+
+ /** Class which provides secret keys which are shared by server and client on a per-app basis. */
+ private final SecretRegistry secretRegistry;
+
+ private CelebornSaslServer saslServer;
+
+ public SaslRpcHandler(
+ TransportConf conf,
+ Channel channel,
+ BaseMessageHandler delegate,
+ SecretRegistry secretRegistry) {
+ super(delegate);
+ this.conf = conf;
+ this.channel = channel;
+ this.secretRegistry = secretRegistry;
+ this.saslServer = null;
+ }
+
+ @Override
+ public boolean checkRegistered() {
+ return delegate.checkRegistered();
+ }
+
+ @Override
+ public boolean doAuthChallenge(
+ TransportClient client, RequestMessage message, RpcResponseCallback callback) {
+ if (saslServer == null || !saslServer.isComplete()) {
+ RpcRequest rpcRequest = (RpcRequest) message;
+ PbSaslRequest saslMessage;
+ try {
+ TransportMessage pbMsg = TransportMessage.fromByteBuffer(message.body().nioByteBuffer());
+ saslMessage = pbMsg.getParsedPayload();
+ } catch (IOException e) {
+ logger.error("Error while parsing Sasl Message with RPC id {}", rpcRequest.requestId, e);
+ callback.onFailure(e);
+ return false;
+ }
+ assert saslMessage != null;
Review Comment:
QQ: If this does become `null` - what is the result ? Will it fail the connection with an rpc error ?
Do we want to explicitly return error/fail instead ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1176] Server side support for Sasl Auth [incubator-celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2164:
URL: https://github.com/apache/incubator-celeborn/pull/2164#discussion_r1429112425
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/CelebornSaslServer.java:
##########
@@ -0,0 +1,162 @@
+/*
+ * 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.celeborn.common.network.sasl;
+
+import static org.apache.celeborn.common.network.sasl.SaslUtils.*;
+
+import java.util.Map;
+
+import javax.annotation.Nullable;
+import javax.security.auth.callback.Callback;
+import javax.security.auth.callback.CallbackHandler;
+import javax.security.auth.callback.NameCallback;
+import javax.security.auth.callback.PasswordCallback;
+import javax.security.auth.callback.UnsupportedCallbackException;
+import javax.security.sasl.AuthorizeCallback;
+import javax.security.sasl.RealmCallback;
+import javax.security.sasl.Sasl;
+import javax.security.sasl.SaslException;
+import javax.security.sasl.SaslServer;
+
+import com.google.common.base.Preconditions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A SASL Server for Celeborn which simply keeps track of the state of a single SASL session, from
+ * the initial state to the "authenticated" state. (It is not a server in the sense of accepting
+ * connections on some socket.)
+ */
+public class CelebornSaslServer {
+ private static final Logger logger = LoggerFactory.getLogger(CelebornSaslServer.class);
+
+ private SaslServer saslServer;
+
+ public CelebornSaslServer(
+ String saslMechanism,
+ @Nullable Map<String, String> saslProps,
+ @Nullable CallbackHandler callbackHandler) {
+ Preconditions.checkNotNull(saslMechanism);
+ try {
+ this.saslServer =
+ Sasl.createSaslServer(saslMechanism, null, DEFAULT_REALM, saslProps, callbackHandler);
+ } catch (SaslException e) {
+ throw new IllegalArgumentException(e);
+ }
+ }
+
+ /** Determines whether the authentication exchange has completed successfully. */
+ public synchronized boolean isComplete() {
+ return saslServer != null && saslServer.isComplete();
+ }
+
+ /** Returns the value of a negotiated property. */
+ public synchronized Object getNegotiatedProperty(String name) {
+ return saslServer.getNegotiatedProperty(name);
+ }
+
+ /**
+ * Used to respond to server SASL tokens.
+ *
+ * @param token Server's SASL token
+ * @return response to send back to the server.
+ */
+ public synchronized byte[] response(byte[] token) {
+ try {
+ return saslServer != null ? saslServer.evaluateResponse(token) : EMPTY_BYTE_ARRAY;
+ } catch (SaslException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ /**
+ * Disposes of any system resources or security-sensitive information the SaslServer might be
+ * using.
+ */
+ public synchronized void dispose() {
+ if (saslServer != null) {
+ try {
+ saslServer.dispose();
+ } catch (SaslException e) {
+ // ignore
+ } finally {
+ saslServer = null;
+ }
+ }
+ }
+
+ /**
+ * Implementation of javax.security.auth.callback.CallbackHandler for SASL DIGEST-MD5 mechanism.
+ */
+ static class DigestCallbackHandler implements CallbackHandler {
+ private final SaslUserNameNotifier userNameNotifier;
+ private final SecretRegistry secretKeyHolder;
+
+ private String userName = null;
+
+ DigestCallbackHandler(SaslUserNameNotifier userNameNotifier, SecretRegistry secretRegistry) {
+ this.userNameNotifier = Preconditions.checkNotNull(userNameNotifier);
+ this.secretKeyHolder = Preconditions.checkNotNull(secretRegistry);
+ }
+
+ @Override
+ public void handle(Callback[] callbacks) throws UnsupportedCallbackException, SaslException {
+ for (Callback callback : callbacks) {
+ if (callback instanceof NameCallback) {
+ logger.trace("SASL server callback: setting username");
+ NameCallback nc = (NameCallback) callback;
+ String encodedName = nc.getName() != null ? nc.getName() : nc.getDefaultName();
+ if (encodedName == null) {
+ throw new SaslException("No username provided by client");
+ }
+ userName = decodeIdentifier(encodedName);
+ userNameNotifier.notifySaslUserName(userName);
+ } else if (callback instanceof PasswordCallback) {
+ logger.trace("SASL server callback: setting password");
+ PasswordCallback pc = (PasswordCallback) callback;
+ String secret = secretKeyHolder.getSecretKey(userName);
+ if (secret == null) {
+ // TODO: CELEBORN-1179 Add support for fetching the secret from the Celeborn master.
+ throw new RuntimeException("Registration information not found for " + userName);
+ }
+ pc.setPassword(encodePassword(secret));
+ } else if (callback instanceof RealmCallback) {
+ logger.trace("SASL server callback: setting realm");
+ RealmCallback rc = (RealmCallback) callback;
+ rc.setText(rc.getDefaultText());
+ } else if (callback instanceof AuthorizeCallback) {
+ AuthorizeCallback ac = (AuthorizeCallback) callback;
+ String authId = ac.getAuthenticationID();
+ String authzId = ac.getAuthorizationID();
+ ac.setAuthorized(authId.equals(authzId));
+ if (ac.isAuthorized()) {
+ ac.setAuthorizedID(authzId);
+ }
+ logger.debug("SASL Authorization complete, authorized set to {}", ac.isAuthorized());
+ } else {
+ throw new UnsupportedCallbackException(callback, "Unrecognized callback: " + callback);
+ }
+ }
+ }
+ }
+
+ /** Notifies the registered {@link SaslUserNameNotifier} of the SASL user name. */
+ interface SaslUserNameNotifier {
+ void notifySaslUserName(String userName);
Review Comment:
I'll remove it for now. It can be added when it is needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org