You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "fapifta (via GitHub)" <gi...@apache.org> on 2023/02/22 01:47:40 UTC
[GitHub] [ozone] fapifta commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
fapifta commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1109174448
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/package-info.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.
+ */
+
+/**
+ * In secure mode, Ozone uses symmetric key algorithm to sign all its issued
+ * tokens, such as block or container tokens. These tokens are then verified
+ * by datanodes to ensure their authenticity and integrity.
+ * <p/>
+ *
+ * That process requires symmetric {@link javax.crypto.SecretKey} to be
+ * generated, managed, and distributed to different Ozone components.
+ * For example, the token signer (Ozone Manager and SCM) and the
+ * verifier (datanode) need to use the same SecretKey.
+ * <p/>
+ *
+ * This package encloses the logic to manage symmetric secret keys
+ * lifecycle. In details, it consists of the following components:
+ * <ul>
+ * <li>
+ * The definition of manage secret key which is shared between SCM,
+ * OM and datanodes, see
+ * {@link org.apache.hadoop.hdds.security.symmetric.ManagedSecretKey}.
+ * </li>
+ *
+ * <li>
+ * The definition of secret key states, which is designed to get replicated
+ * across all SCM instances, see
+ * {@link org.apache.hadoop.hdds.security.symmetric.SecretKeyState}
+ * </li>
+ *
+ * <li>
+ * The definition and implementation of secret key persistent storage, to
+ * help retain SecretKey after restarts, see
+ * {@link org.apache.hadoop.hdds.security.symmetric.SecretKeyStore} and
+ * {@link org.apache.hadoop.hdds.security.symmetric.LocalSecretKeyStore}.
+ * </li>
+ *
+ * <li>
+ * The basic logic to manage secret key lifecycle, see
+ * {@link org.apache.hadoop.hdds.security.symmetric.SecretKeyManager}
+ * </li>
+ * </ul>
+ *
+ * <p/>
+ * The overall design is documented <a href=http://surl.li/epvkc>here</a>.
Review Comment:
I think it would be better to export the document into a PDF, upload it to the JIRA, and link the JIRA ID from here instead.
##########
hadoop-hdds/common/src/main/resources/ozone-default.xml:
##########
@@ -3602,4 +3602,50 @@
history from compaction DAG. Uses millisecond by default when no time unit is specified.
</description>
</property>
+ <property>
+ <name>hdds.secret.key.file.name</name>
+ <value>secret_keys.json</value>
+ <tag>SCM, SECURITY</tag>
+ <description>
+ Name of file which stores symmetric secret keys for token signatures.
+ </description>
+ </property>
+ <property>
+ <name>hdds.secret.key.expiry.duration</name>
+ <value>7d</value>
+ <tag>SCM, SECURITY</tag>
+ <description>
+ The duration for which symmetric secret keys issued by SCM are valid.
+ This default value, in combination with hdds.secret.key.rotate.duration=1d, result in that 7 secret keys for the
+ last 7 days will are kept valid at any point of time.
+ </description>
+ </property>
+ <property>
+ <name>hdds.secret.key.rotate.duration</name>
+ <value>1d</value>
+ <tag>SCM, SECURITY</tag>
+ <description>
+ The duration that SCM periodically generate a new symmetric secret keys.
+ </description>
+ </property>
+ <property>
+ <name>hdds.secret.key.rotate.check.duration</name>
+ <value>10m</value>
+ <tag>SCM, SECURITY</tag>
+ <description>
+ The duration that SCM periodically checks if it's time to generate new symmetric secret keys.
+ This config has an impact on the practical correctness of secret key expiry and rotation period. For example,
+ if hdds.secret.key.rotate.duration=1d and hdds.secret.key.rotate.check.duration=10m, the actual key rotation
+ will happen each 1d +/- 10m.
+ </description>
+ </property>
+ <property>
+ <name>hdds.secret.key.algorithm</name>
+ <value>HmacSHA256</value>
+ <tag>SCM, SECURITY</tag>
+ <description>
+ The algorithm that SCM uses to generate symmetric secret keys.
+ The formats accepted are based on the ISO-8601 duration format PnDTnHnMn.nS
Review Comment:
I think this line should not be here, the algorithm should be named based on KeyGenerator's accepted names documented here: https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#KeyGenerator
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java:
##########
@@ -0,0 +1,171 @@
+/**
+ * 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.scm.security;
+
+import com.google.common.util.concurrent.ThreadFactoryBuilder;
+import org.apache.hadoop.hdds.conf.ConfigurationSource;
+import org.apache.hadoop.hdds.scm.ha.SCMContext;
+import org.apache.hadoop.hdds.scm.ha.SCMRatisServer;
+import org.apache.hadoop.hdds.scm.ha.SCMService;
+import org.apache.hadoop.hdds.security.symmetric.LocalSecretKeyStore;
+import org.apache.hadoop.hdds.security.symmetric.SecretKeyConfig;
+import org.apache.hadoop.hdds.security.symmetric.SecretKeyManager;
+import org.apache.hadoop.hdds.security.symmetric.SecretKeyState;
+import org.apache.hadoop.hdds.security.symmetric.SecretKeyStore;
+import org.apache.hadoop.hdds.security.x509.SecurityConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.time.Duration;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_SECRET_KEY_ROTATE_CHECK_DURATION;
+import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_SECRET_KEY_ROTATE_CHECK_DURATION_DEFAULT;
+import static org.apache.hadoop.ozone.OzoneConsts.SCM_CA_CERT_STORAGE_DIR;
+
+/**
+ * A background service running in SCM to maintain the SecretKeys lifecycle.
+ */
+public class SecretKeyManagerService implements SCMService, Runnable {
+ public static final Logger LOG =
+ LoggerFactory.getLogger(SecretKeyManagerService.class);
+
+ private final SCMContext scmContext;
+ private final SecretKeyManager secretKeyManager;
+
+
+ /**
+ * SCMService related variables.
+ */
+ private final Lock serviceLock = new ReentrantLock();
+ private ServiceStatus serviceStatus = ServiceStatus.PAUSING;
+
+ private final Duration rotationCheckDuration;
+ private final ScheduledExecutorService scheduler;
+
+ @SuppressWarnings("parameternumber")
+ public SecretKeyManagerService(SCMContext scmContext,
+ ConfigurationSource conf,
+ SCMRatisServer ratisServer) {
+ this.scmContext = scmContext;
+
+ SecretKeyConfig secretKeyConfig = new SecretKeyConfig(conf,
+ SCM_CA_CERT_STORAGE_DIR);
+ SecretKeyStore secretKeyStore = new LocalSecretKeyStore(
+ secretKeyConfig.getLocalSecretKeyFile());
+ SecretKeyState secretKeyState = new ScmSecretKeyStateBuilder()
+ .setSecretKeyStore(secretKeyStore)
+ .setRatisServer(ratisServer)
+ .build();
+ secretKeyManager = new SecretKeyManager(secretKeyState,
+ secretKeyStore, secretKeyConfig);
+
+ scheduler = Executors.newScheduledThreadPool(1,
+ new ThreadFactoryBuilder().setDaemon(true)
+ .setNameFormat(getServiceName())
+ .build());
+
+ long rotationCheckInMs = conf.getTimeDuration(
Review Comment:
Should this be part of the SecretKeyConfig?
Also, I think, as we expect a duration in the config, and we work with a duration here in the code, why don't we just read the config as a string and then construct a duration from that instead of converting it to a long than back to a duration?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java:
##########
@@ -0,0 +1,171 @@
+/**
+ * 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.scm.security;
+
+import com.google.common.util.concurrent.ThreadFactoryBuilder;
+import org.apache.hadoop.hdds.conf.ConfigurationSource;
+import org.apache.hadoop.hdds.scm.ha.SCMContext;
+import org.apache.hadoop.hdds.scm.ha.SCMRatisServer;
+import org.apache.hadoop.hdds.scm.ha.SCMService;
+import org.apache.hadoop.hdds.security.symmetric.LocalSecretKeyStore;
+import org.apache.hadoop.hdds.security.symmetric.SecretKeyConfig;
+import org.apache.hadoop.hdds.security.symmetric.SecretKeyManager;
+import org.apache.hadoop.hdds.security.symmetric.SecretKeyState;
+import org.apache.hadoop.hdds.security.symmetric.SecretKeyStore;
+import org.apache.hadoop.hdds.security.x509.SecurityConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.time.Duration;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_SECRET_KEY_ROTATE_CHECK_DURATION;
+import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_SECRET_KEY_ROTATE_CHECK_DURATION_DEFAULT;
+import static org.apache.hadoop.ozone.OzoneConsts.SCM_CA_CERT_STORAGE_DIR;
+
+/**
+ * A background service running in SCM to maintain the SecretKeys lifecycle.
+ */
+public class SecretKeyManagerService implements SCMService, Runnable {
+ public static final Logger LOG =
+ LoggerFactory.getLogger(SecretKeyManagerService.class);
+
+ private final SCMContext scmContext;
+ private final SecretKeyManager secretKeyManager;
+
+
+ /**
+ * SCMService related variables.
+ */
+ private final Lock serviceLock = new ReentrantLock();
+ private ServiceStatus serviceStatus = ServiceStatus.PAUSING;
+
+ private final Duration rotationCheckDuration;
+ private final ScheduledExecutorService scheduler;
+
+ @SuppressWarnings("parameternumber")
+ public SecretKeyManagerService(SCMContext scmContext,
+ ConfigurationSource conf,
+ SCMRatisServer ratisServer) {
+ this.scmContext = scmContext;
+
+ SecretKeyConfig secretKeyConfig = new SecretKeyConfig(conf,
+ SCM_CA_CERT_STORAGE_DIR);
+ SecretKeyStore secretKeyStore = new LocalSecretKeyStore(
+ secretKeyConfig.getLocalSecretKeyFile());
+ SecretKeyState secretKeyState = new ScmSecretKeyStateBuilder()
+ .setSecretKeyStore(secretKeyStore)
+ .setRatisServer(ratisServer)
+ .build();
+ secretKeyManager = new SecretKeyManager(secretKeyState,
+ secretKeyStore, secretKeyConfig);
+
+ scheduler = Executors.newScheduledThreadPool(1,
+ new ThreadFactoryBuilder().setDaemon(true)
+ .setNameFormat(getServiceName())
+ .build());
+
+ long rotationCheckInMs = conf.getTimeDuration(
+ HDDS_SECRET_KEY_ROTATE_CHECK_DURATION,
+ HDDS_SECRET_KEY_ROTATE_CHECK_DURATION_DEFAULT, TimeUnit.MILLISECONDS);
+ rotationCheckDuration = Duration.ofMillis(rotationCheckInMs);
+
+ start();
+ }
+
+ @Override
+ public void notifyStatusChanged() {
+ serviceLock.lock();
+ try {
+ if (scmContext.isLeaderReady()) {
+ // Asynchronously initialize SecretKeys for first time leader.
+ if (!secretKeyManager.isInitialized()) {
+ scheduler.schedule(() -> {
+ try {
+ secretKeyManager.checkAndInitialize();
+ } catch (TimeoutException e) {
+ throw new RuntimeException(
+ "Timeout replicating initialized state.", e);
+ }
+ }, 0, TimeUnit.SECONDS);
+ }
+
+ serviceStatus = ServiceStatus.RUNNING;
+ } else {
+ serviceStatus = ServiceStatus.PAUSING;
+ }
+ } finally {
+ serviceLock.unlock();
+ }
+ }
+
+ @Override
+ public boolean shouldRun() {
+ serviceLock.lock();
+ try {
+ return serviceStatus == ServiceStatus.RUNNING;
+ } finally {
+ serviceLock.unlock();
+ }
+ }
+
+ @Override
+ public void run() {
+ if (!shouldRun()) {
+ return;
+ }
+
+ try {
+ boolean rotated = secretKeyManager.checkAndRotate();
+ if (rotated) {
+ LOG.info("SecretKeys have been updated.");
+ } else {
+ LOG.info("SecretKeys have not been updated.");
Review Comment:
I am not sure if this one should be on info level, I think we will log this every 10 minutes throughout the day by default. Does that hold any meaningful information in the log file?
Also there is one log message from secretKeyManager once the new key is generated, so we might not need the logging here at all, what do you think?
--
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@ozone.apache.org
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