You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2021/04/26 20:49:42 UTC
[GitHub] [ozone] adoroszlai opened a new pull request #2186: HDDS-4729. Add token support for container admin operations
adoroszlai opened a new pull request #2186:
URL: https://github.com/apache/ozone/pull/2186
## What changes were proposed in this pull request?
This change introduces container tokens. These are similar to existing "block tokens", but applied and required for container operations. They are issued by SCM in response to the new `getContainerToken` request.
https://issues.apache.org/jira/browse/HDDS-4729
## How was this patch tested?
Added unit test. Updated existing integration tests (including fixing and enabling `TestSecureContainerServer`).
https://github.com/adoroszlai/hadoop-ozone/actions/runs/786346758
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xiaoyuyao commented on a change in pull request #2186: HDDS-4729. Add token support for container admin operations
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #2186:
URL: https://github.com/apache/ozone/pull/2186#discussion_r624215984
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/HddsDispatcher.java
##########
@@ -425,14 +423,14 @@ ContainerCommandResponseProto createContainer(
return handler.handle(requestBuilder.build(), null, null);
}
- private void validateBlockToken(
+ private void validateToken(
ContainerCommandRequestProto msg) throws IOException {
- if (isBlockTokenEnabled && tokenVerifier != null &&
- HddsUtils.requireBlockToken(msg.getCmdType())) {
+
+ if (tokenVerifier != null) {
Review comment:
Do we still need the null check with the NoopTokenVerifier?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #2186: HDDS-4729. Add token support for container admin operations
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2186:
URL: https://github.com/apache/ozone/pull/2186#discussion_r624443708
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/HddsDispatcher.java
##########
@@ -109,10 +109,8 @@ public HddsDispatcher(ConfigurationSource config, ContainerSet contSet,
this.containerCloseThreshold = conf.getFloat(
HddsConfigKeys.HDDS_CONTAINER_CLOSE_THRESHOLD,
HddsConfigKeys.HDDS_CONTAINER_CLOSE_THRESHOLD_DEFAULT);
- this.isBlockTokenEnabled = conf.getBoolean(
- HddsConfigKeys.HDDS_BLOCK_TOKEN_ENABLED,
- HddsConfigKeys.HDDS_BLOCK_TOKEN_ENABLED_DEFAULT);
- this.tokenVerifier = tokenVerifier;
+ this.tokenVerifier = tokenVerifier != null ? tokenVerifier
Review comment:
I think checking for `null` here is more defensive. Also, `HddsDispatcher` is created by several tests, I wanted to avoid changing those.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #2186: HDDS-4729. Add token support for container admin operations
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2186:
URL: https://github.com/apache/ozone/pull/2186#discussion_r624442234
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/x509/SecurityConfig.java
##########
@@ -330,6 +330,9 @@ public Duration getMaxCertificateDuration() {
return this.maxCertDuration;
}
+ /**
+ * Whether to require short-lived tokens for block and container operations.
+ */
public boolean isBlockTokenEnabled() {
Review comment:
I don't think introducing a new config property for this adds value. I'm not sure why we even have a config property for enabling block tokens. I think they should have been enabled in all secure clusters, ie. by `ozone.security.enabled=true`. But now we have `hdds.block.token.enabled`. Do we really need another config property just to force all users who want security to set this new config, too?
We would also need another one for container token token lifetime.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xiaoyuyao commented on a change in pull request #2186: HDDS-4729. Add token support for container admin operations
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #2186:
URL: https://github.com/apache/ozone/pull/2186#discussion_r622292174
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/token/ShortLivedTokenVerifier.java
##########
@@ -0,0 +1,135 @@
+/*
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.hadoop.hdds.security.token;
+
+import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos;
+import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerCommandRequestProto;
+import org.apache.hadoop.hdds.security.exception.SCMSecurityException;
+import org.apache.hadoop.hdds.security.x509.SecurityConfig;
+import org.apache.hadoop.hdds.security.x509.certificate.client.CertificateClient;
+import org.apache.hadoop.security.UserGroupInformation;
+import org.apache.hadoop.security.token.Token;
+
+import java.io.ByteArrayInputStream;
+import java.io.DataInputStream;
+import java.io.IOException;
+import java.security.cert.CertificateExpiredException;
+import java.security.cert.CertificateNotYetValidException;
+import java.security.cert.X509Certificate;
+import java.time.Instant;
+import java.util.Objects;
+
+/**
+ * Verifies short-lived token.
+ * @param <T> type of short-lived token
+ */
+public abstract class
+ ShortLivedTokenVerifier<T extends ShortLivedTokenIdentifier>
+ implements TokenVerifier {
+
+ private final CertificateClient caClient;
+ private final SecurityConfig conf;
+
+ protected ShortLivedTokenVerifier(SecurityConfig conf,
+ CertificateClient caClient) {
+ this.conf = conf;
+ this.caClient = caClient;
+ }
+
+ /** Whether the specific kind of token is required for {@code cmdType}. */
+ protected abstract boolean isTokenRequired(ContainerProtos.Type cmdType);
+
+ /** Create new empty token identifier, to be filled from Token. */
+ protected abstract T createTokenIdentifier();
+
+ /** Extract info on "service" being accessed by {@code cmd}. */
+ protected abstract Object getService(ContainerCommandRequestProto cmd);
+
+ /** Hook for further verification. */
+ protected void verify(T tokenId, ContainerCommandRequestProto cmd)
+ throws SCMSecurityException {
+ // NOP
+ }
+
+ @Override
+ public void verify(String user, Token<?> token,
+ ContainerCommandRequestProto cmd) throws SCMSecurityException {
+
+ if (!conf.isBlockTokenEnabled()) {
+ return;
+ }
+
+ if (!isTokenRequired(cmd.getCmdType())) {
+ return;
+ }
+
+ if (caClient == null) {
+ throw new SCMSecurityException("Certificate client not available " +
+ "to validate token");
+ }
+
+ T tokenId = createTokenIdentifier();
+ try {
+ tokenId.readFields(new DataInputStream(new ByteArrayInputStream(
+ token.getIdentifier())));
+ } catch (IOException ex) {
+ throw new BlockTokenException("Failed to decode token : " + token);
Review comment:
This is not block token specific, can we have a generic SCM token exception here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #2186: HDDS-4729. Add token support for container admin operations
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2186:
URL: https://github.com/apache/ozone/pull/2186#discussion_r622307901
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/token/ShortLivedTokenVerifier.java
##########
@@ -0,0 +1,135 @@
+/*
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.hadoop.hdds.security.token;
+
+import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos;
+import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerCommandRequestProto;
+import org.apache.hadoop.hdds.security.exception.SCMSecurityException;
+import org.apache.hadoop.hdds.security.x509.SecurityConfig;
+import org.apache.hadoop.hdds.security.x509.certificate.client.CertificateClient;
+import org.apache.hadoop.security.UserGroupInformation;
+import org.apache.hadoop.security.token.Token;
+
+import java.io.ByteArrayInputStream;
+import java.io.DataInputStream;
+import java.io.IOException;
+import java.security.cert.CertificateExpiredException;
+import java.security.cert.CertificateNotYetValidException;
+import java.security.cert.X509Certificate;
+import java.time.Instant;
+import java.util.Objects;
+
+/**
+ * Verifies short-lived token.
+ * @param <T> type of short-lived token
+ */
+public abstract class
+ ShortLivedTokenVerifier<T extends ShortLivedTokenIdentifier>
+ implements TokenVerifier {
+
+ private final CertificateClient caClient;
+ private final SecurityConfig conf;
+
+ protected ShortLivedTokenVerifier(SecurityConfig conf,
+ CertificateClient caClient) {
+ this.conf = conf;
+ this.caClient = caClient;
+ }
+
+ /** Whether the specific kind of token is required for {@code cmdType}. */
+ protected abstract boolean isTokenRequired(ContainerProtos.Type cmdType);
+
+ /** Create new empty token identifier, to be filled from Token. */
+ protected abstract T createTokenIdentifier();
+
+ /** Extract info on "service" being accessed by {@code cmd}. */
+ protected abstract Object getService(ContainerCommandRequestProto cmd);
+
+ /** Hook for further verification. */
+ protected void verify(T tokenId, ContainerCommandRequestProto cmd)
+ throws SCMSecurityException {
+ // NOP
+ }
+
+ @Override
+ public void verify(String user, Token<?> token,
+ ContainerCommandRequestProto cmd) throws SCMSecurityException {
+
+ if (!conf.isBlockTokenEnabled()) {
Review comment:
Thanks @xiaoyuyao for taking a look.
I thought having a single config item for both block token and container token would be enough. Is there a reason to enable only one of them, but not the other one?
Regarding the names (config item, exception), I kept "block token"-related ones for backward compatibility.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #2186: HDDS-4729. Add token support for container admin operations
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #2186:
URL: https://github.com/apache/ozone/pull/2186#issuecomment-840317467
Thanks @xiaoyuyao for the reviews.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xiaoyuyao commented on a change in pull request #2186: HDDS-4729. Add token support for container admin operations
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #2186:
URL: https://github.com/apache/ozone/pull/2186#discussion_r630548273
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
##########
@@ -790,6 +795,23 @@ public boolean getReplicationManagerStatus() {
.collect(Collectors.toList());
}
+ @Override
+ public Token<?> getContainerToken(ContainerID containerID)
+ throws IOException {
+ String remoteUser = getRemoteUserName();
+ getScm().checkAdminAccess(remoteUser);
+
+ if (!blockTokenEnabled) {
Review comment:
should we use the container token enabled key to determine whether return a valid container token or not?
##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerOperationClient.java
##########
@@ -86,6 +91,8 @@ public ContainerOperationClient(OzoneConfiguration conf) throws IOException {
replicationFactor = HddsProtos.ReplicationFactor.ONE;
replicationType = HddsProtos.ReplicationType.STAND_ALONE;
}
+ blockTokenEnabled = conf.getBoolean(HDDS_BLOCK_TOKEN_ENABLED,
Review comment:
same as above?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xiaoyuyao commented on a change in pull request #2186: HDDS-4729. Add token support for container admin operations
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #2186:
URL: https://github.com/apache/ozone/pull/2186#discussion_r624215005
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/HddsDispatcher.java
##########
@@ -109,10 +109,8 @@ public HddsDispatcher(ConfigurationSource config, ContainerSet contSet,
this.containerCloseThreshold = conf.getFloat(
HddsConfigKeys.HDDS_CONTAINER_CLOSE_THRESHOLD,
HddsConfigKeys.HDDS_CONTAINER_CLOSE_THRESHOLD_DEFAULT);
- this.isBlockTokenEnabled = conf.getBoolean(
- HddsConfigKeys.HDDS_BLOCK_TOKEN_ENABLED,
- HddsConfigKeys.HDDS_BLOCK_TOKEN_ENABLED_DEFAULT);
- this.tokenVerifier = tokenVerifier;
+ this.tokenVerifier = tokenVerifier != null ? tokenVerifier
Review comment:
NIT: can we move new NoopTokenVerifier() to the caller in OzoneContainer?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #2186: HDDS-4729. Add token support for container admin operations
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #2186:
URL: https://github.com/apache/ozone/pull/2186#issuecomment-837368878
Thanks @xiaoyuyao, I have updated the patch based on your comments. Can you please take another 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #2186: HDDS-4729. Add token support for container admin operations
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2186:
URL: https://github.com/apache/ozone/pull/2186#discussion_r630747865
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java
##########
@@ -790,6 +795,23 @@ public boolean getReplicationManagerStatus() {
.collect(Collectors.toList());
}
+ @Override
+ public Token<?> getContainerToken(ContainerID containerID)
+ throws IOException {
+ String remoteUser = getRemoteUserName();
+ getScm().checkAdminAccess(remoteUser);
+
+ if (!blockTokenEnabled) {
Review comment:
Thanks, fixed.
##########
File path: hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerOperationClient.java
##########
@@ -86,6 +91,8 @@ public ContainerOperationClient(OzoneConfiguration conf) throws IOException {
replicationFactor = HddsProtos.ReplicationFactor.ONE;
replicationType = HddsProtos.ReplicationType.STAND_ALONE;
}
+ blockTokenEnabled = conf.getBoolean(HDDS_BLOCK_TOKEN_ENABLED,
Review comment:
Thanks, fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xiaoyuyao merged pull request #2186: HDDS-4729. Add token support for container admin operations
Posted by GitBox <gi...@apache.org>.
xiaoyuyao merged pull request #2186:
URL: https://github.com/apache/ozone/pull/2186
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xiaoyuyao commented on a change in pull request #2186: HDDS-4729. Add token support for container admin operations
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #2186:
URL: https://github.com/apache/ozone/pull/2186#discussion_r624537752
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/HddsDispatcher.java
##########
@@ -109,10 +109,8 @@ public HddsDispatcher(ConfigurationSource config, ContainerSet contSet,
this.containerCloseThreshold = conf.getFloat(
HddsConfigKeys.HDDS_CONTAINER_CLOSE_THRESHOLD,
HddsConfigKeys.HDDS_CONTAINER_CLOSE_THRESHOLD_DEFAULT);
- this.isBlockTokenEnabled = conf.getBoolean(
- HddsConfigKeys.HDDS_BLOCK_TOKEN_ENABLED,
- HddsConfigKeys.HDDS_BLOCK_TOKEN_ENABLED_DEFAULT);
- this.tokenVerifier = tokenVerifier;
+ this.tokenVerifier = tokenVerifier != null ? tokenVerifier
Review comment:
I found that too from later code changes in tests. Good to be defensive here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xiaoyuyao commented on pull request #2186: HDDS-4729. Add token support for container admin operations
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on pull request #2186:
URL: https://github.com/apache/ozone/pull/2186#issuecomment-839931950
Thanks @adoroszlai for the update. Latest change LGTM, +1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #2186: HDDS-4729. Add token support for container admin operations
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2186:
URL: https://github.com/apache/ozone/pull/2186#discussion_r629693276
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/HddsDispatcher.java
##########
@@ -425,14 +423,14 @@ ContainerCommandResponseProto createContainer(
return handler.handle(requestBuilder.build(), null, null);
}
- private void validateBlockToken(
+ private void validateToken(
ContainerCommandRequestProto msg) throws IOException {
- if (isBlockTokenEnabled && tokenVerifier != null &&
- HddsUtils.requireBlockToken(msg.getCmdType())) {
+
+ if (tokenVerifier != null) {
Review comment:
Removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xiaoyuyao commented on a change in pull request #2186: HDDS-4729. Add token support for container admin operations
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #2186:
URL: https://github.com/apache/ozone/pull/2186#discussion_r624212701
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/x509/SecurityConfig.java
##########
@@ -330,6 +330,9 @@ public Duration getMaxCertificateDuration() {
return this.maxCertDuration;
}
+ /**
+ * Whether to require short-lived tokens for block and container operations.
+ */
public boolean isBlockTokenEnabled() {
Review comment:
Block token are container token are all validated on datanodes but issued by different services, can we define separate configurations to enable/disable them differently?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #2186: HDDS-4729. Add token support for container admin operations
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2186:
URL: https://github.com/apache/ozone/pull/2186#discussion_r629693503
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/CertificateClient.java
##########
@@ -278,4 +281,14 @@ void storeRootCACertificate(String pemEncodedCert, boolean force)
*/
long getLatestCrlId() throws IOException;
+ default void assertKeyPair() throws OzoneSecurityException {
+ try {
+ Objects.requireNonNull(getPublicKey());
+ Objects.requireNonNull(getPrivateKey());
+ Objects.requireNonNull(getCertificate());
Review comment:
Renamed to `assertValidKeysAndCertificate`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xiaoyuyao commented on a change in pull request #2186: HDDS-4729. Add token support for container admin operations
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #2186:
URL: https://github.com/apache/ozone/pull/2186#discussion_r622291740
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/token/ShortLivedTokenVerifier.java
##########
@@ -0,0 +1,135 @@
+/*
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.hadoop.hdds.security.token;
+
+import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos;
+import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerCommandRequestProto;
+import org.apache.hadoop.hdds.security.exception.SCMSecurityException;
+import org.apache.hadoop.hdds.security.x509.SecurityConfig;
+import org.apache.hadoop.hdds.security.x509.certificate.client.CertificateClient;
+import org.apache.hadoop.security.UserGroupInformation;
+import org.apache.hadoop.security.token.Token;
+
+import java.io.ByteArrayInputStream;
+import java.io.DataInputStream;
+import java.io.IOException;
+import java.security.cert.CertificateExpiredException;
+import java.security.cert.CertificateNotYetValidException;
+import java.security.cert.X509Certificate;
+import java.time.Instant;
+import java.util.Objects;
+
+/**
+ * Verifies short-lived token.
+ * @param <T> type of short-lived token
+ */
+public abstract class
+ ShortLivedTokenVerifier<T extends ShortLivedTokenIdentifier>
+ implements TokenVerifier {
+
+ private final CertificateClient caClient;
+ private final SecurityConfig conf;
+
+ protected ShortLivedTokenVerifier(SecurityConfig conf,
+ CertificateClient caClient) {
+ this.conf = conf;
+ this.caClient = caClient;
+ }
+
+ /** Whether the specific kind of token is required for {@code cmdType}. */
+ protected abstract boolean isTokenRequired(ContainerProtos.Type cmdType);
+
+ /** Create new empty token identifier, to be filled from Token. */
+ protected abstract T createTokenIdentifier();
+
+ /** Extract info on "service" being accessed by {@code cmd}. */
+ protected abstract Object getService(ContainerCommandRequestProto cmd);
+
+ /** Hook for further verification. */
+ protected void verify(T tokenId, ContainerCommandRequestProto cmd)
+ throws SCMSecurityException {
+ // NOP
+ }
+
+ @Override
+ public void verify(String user, Token<?> token,
+ ContainerCommandRequestProto cmd) throws SCMSecurityException {
+
+ if (!conf.isBlockTokenEnabled()) {
Review comment:
This is not block token specific, do we have a generic conf here for scm token?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #2186: HDDS-4729. Add token support for container admin operations
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #2186:
URL: https://github.com/apache/ozone/pull/2186#issuecomment-837899281
@xiaoyuyao Thanks for taking another look.
> I just have some questions added inline wrt. mixed usage of container token/block token configuration for the client code.
Can you please point me to these questions? I cannot find them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xiaoyuyao commented on a change in pull request #2186: HDDS-4729. Add token support for container admin operations
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #2186:
URL: https://github.com/apache/ozone/pull/2186#discussion_r624537355
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/x509/SecurityConfig.java
##########
@@ -330,6 +330,9 @@ public Duration getMaxCertificateDuration() {
return this.maxCertDuration;
}
+ /**
+ * Whether to require short-lived tokens for block and container operations.
+ */
public boolean isBlockTokenEnabled() {
Review comment:
Technical wise, I think that we could live without a separate config. But there are several reasons for having the token on datanode optional:
1. performance: the additional validation adds the cost for each block I/O
2. backward compatibility: old client will be broken without upgrade.
3. deployment with flexibility
bq. We would also need another one for container token token lifetime.
We can use a default one for the short lived tokens if customer does not need to specify. This is common in hadoop delegation tokens.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #2186: HDDS-4729. Add token support for container admin operations
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #2186:
URL: https://github.com/apache/ozone/pull/2186#issuecomment-839591872
Thanks @xiaoyuyao for spotting the enabled flag mismatch. Patch is updated, please take another 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xiaoyuyao commented on a change in pull request #2186: HDDS-4729. Add token support for container admin operations
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #2186:
URL: https://github.com/apache/ozone/pull/2186#discussion_r624303054
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/CertificateClient.java
##########
@@ -278,4 +281,14 @@ void storeRootCACertificate(String pemEncodedCert, boolean force)
*/
long getLatestCrlId() throws IOException;
+ default void assertKeyPair() throws OzoneSecurityException {
+ try {
+ Objects.requireNonNull(getPublicKey());
+ Objects.requireNonNull(getPrivateKey());
+ Objects.requireNonNull(getCertificate());
Review comment:
NIT: can we rename it to like assertValidKeyCerts to reflect the code?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org