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 2023/01/20 01:01:21 UTC
[GitHub] [ozone] duongkame opened a new pull request, #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM (Draft)
duongkame opened a new pull request, #4194:
URL: https://github.com/apache/ozone/pull/4194
## What changes were proposed in this pull request?
Draft implementation of symmetric SecretKeys lifescycle management in SCM, with SecretKeys synchronization between SCMs.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7734
## How was this patch tested?
Unit-tested.
--
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
[GitHub] [ozone] duongkame commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1090250875
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java:
##########
@@ -0,0 +1,165 @@
+/*
+ * 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.symmetric;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.crypto.KeyGenerator;
+import java.security.NoSuchAlgorithmException;
+import java.time.Duration;
+import java.time.Instant;
+import java.util.List;
+import java.util.UUID;
+import java.util.concurrent.TimeoutException;
+
+import static java.time.Duration.between;
+import static java.util.Comparator.comparing;
+import static java.util.Objects.requireNonNull;
+import static java.util.stream.Collectors.toList;
+
+/**
+ * This component manages symmetric SecretKey life-cycle, including generation,
+ * rotation and destruction.
+ */
+public class SecretKeyManager {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(SecretKeyManager.class);
+
+ private final SecretKeyState state;
+ private boolean pendingInititializedState = false;
+ private final Duration rotationDuration;
+ private final Duration validityDuration;
+ private final SecretKeyStore keyStore;
+
+ private final KeyGenerator keyGenerator;
+
+ public SecretKeyManager(SecretKeyState state,
+ SecretKeyStore keyStore,
+ Duration rotationDuration,
+ Duration validityDuration,
+ String algorithm) {
+ this.state = requireNonNull(state);
+ this.rotationDuration = requireNonNull(rotationDuration);
+ this.validityDuration = requireNonNull(validityDuration);
+ this.keyStore = requireNonNull(keyStore);
+ this.keyGenerator = createKeyGenerator(algorithm);
+ }
+
+ public SecretKeyManager(SecretKeyState state,
+ SecretKeyStore keyStore,
+ SecretKeyConfig config) {
+ this(state, keyStore, config.getRotateDuration(),
+ config.getExpiryDuration(), config.getAlgorithm());
+ }
+
+ /**
+ * Initialize the state from by loading SecretKeys from local file, or
+ * generate new keys if the file doesn't exist.
+ *
+ * @throws TimeoutException can possibly occur when replicating the state.
+ */
+ public synchronized boolean initialize() {
+ if (state.getCurrentKey() != null) {
+ return false;
+ }
+
+ List<ManagedSecretKey> sortedKeys = keyStore.load()
+ .stream()
+ .filter(x -> !x.isExpired())
+ .sorted(comparing(ManagedSecretKey::getCreationTime))
+ .collect(toList());
+
+ ManagedSecretKey currentKey;
+ if (sortedKeys.isEmpty()) {
+ // First start, generate new key as the current key.
+ currentKey = generateSecretKey();
+ sortedKeys.add(currentKey);
+ LOG.info("No keys is loaded, generated new key: {}", currentKey);
+ } else {
+ // For restarts, reload allKeys and take the latest one as current.
+ currentKey = sortedKeys.get(sortedKeys.size() - 1);
+ LOG.info("Key reloaded, current key: {}, all keys: {}", currentKey,
+ sortedKeys);
+ }
+
+ // First, update the SecretKey state to make it visible immediately on the
+ // current instance.
+ state.updateKeysInternal(currentKey, sortedKeys);
Review Comment:
Yup. As discussed offline, the keys should be agreed upon by all SCM parties before being visible to clients. A change has been made.
--
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
[GitHub] [ozone] duongkame commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1113785533
##########
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:
Originally I wanted to keep `SecretKeyConfig` enclosing configs used by the `org.apache.hadoop.hdds.security.symmetric` only. Yet, eventually it is just a config parser for secret key related logic. Moved.
Regarding reading the duration as a String and converting to `Duration` directly, I think it's a matter of format. `Duration` supports ISO-8601 format, i.e the configured string has to be like `PT1D` or `P1H`... While the standard Hadoop time duration reader supports a custom "in-house" format like `1d`, `2h`...
Most of the time duration configs (except some configs related to X509 cerificate) are in the Hadoop format and I think we'd prefer to keep it consistent that way.
--
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
[GitHub] [ozone] duongkame commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1090228151
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java:
##########
@@ -0,0 +1,138 @@
+/*
+ * 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.symmetric;
+
+import com.google.common.collect.ImmutableList;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import javax.crypto.KeyGenerator;
+import javax.crypto.SecretKey;
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermission;
+import java.time.Duration;
+import java.time.Instant;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+import java.util.stream.Stream;
+
+import static com.google.common.collect.Lists.newArrayList;
+import static com.google.common.collect.Sets.newHashSet;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Test cases for {@link LocalSecretKeyStore}.
+ */
+public class LocalSecretKeyStoreTest {
+ private SecretKeyStore secretKeyStore;
+ private Path testSecretFile;
+
+ @BeforeEach
+ private void setup() throws Exception {
+ testSecretFile = Files.createTempFile("key-strore-test", ".json");
+ secretKeyStore = new LocalSecretKeyStore(testSecretFile);
+ }
+
+ public static Stream<Arguments> saveAndLoadTestCases() throws Exception {
+ return Stream.of(
+ // empty
+ Arguments.of(ImmutableList.of()),
+ // single secret keys.
+ Arguments.of(newArrayList(
+ generateKey("HmacSHA256")
+ )),
+ // multiple secret keys.
+ Arguments.of(newArrayList(
+ generateKey("HmacSHA1"),
+ generateKey("HmacSHA256")
+ ))
+ );
+ }
+
+ @ParameterizedTest
+ @MethodSource("saveAndLoadTestCases")
+ public void testSaveAndLoad(List<ManagedSecretKey> keys) throws IOException {
Review Comment:
Good point, compatible test has been added.
--
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
[GitHub] [ozone] kerneltime commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1088504179
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsConfigKeys.java:
##########
@@ -216,6 +216,28 @@ public final class HddsConfigKeys {
public static final String HDDS_X509_ROOTCA_PRIVATE_KEY_FILE_DEFAULT =
"";
+ public static final String HDDS_SECRET_KEY_FILE =
Review Comment:
To make sure I understand the configurations. Keys expire in `HDDS_SECRET_KEY_EXPIRY_DURATION`; new keys are generated every `HDDS_SECRET_KEY_ROTATE_DURATION` and are checked for rotation every `HDDS_SECRET_KEY_ROTATE_CHECK_DURATION`? In that case, the defaults say keys are checked for rotation every 10 minutes, are valid for 7 days and a new one is generated daily. Thus, seven keys are floating around that are valid at any given time. Could you add some comments explaining this?
##########
hadoop-hdds/common/src/main/resources/ozone-default.xml:
##########
@@ -3523,4 +3523,49 @@
Interval in MINUTES by Recon to request SCM DB Snapshot.
</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>P7D</value>
+ <tag>SCM, SECURITY</tag>
+ <description>
+ The duration for which symmetric secret keys issued by SCM are valid.
+ The formats accepted are based on the ISO-8601 duration format PnDTnHnMn.nS
+ </description>
+ </property>
+ <property>
+ <name>hdds.secret.key.rotate.duration</name>
+ <value>P1D</value>
+ <tag>SCM, SECURITY</tag>
+ <description>
+ The duration that SCM periodically generate a new symmetric secret keys.
+ The formats accepted are based on the ISO-8601 duration format PnDTnHnMn.nS
+ </description>
+ </property>
+ <property>
+ <name>hdds.secret.key.rotate.check.duration</name>
+ <value>PT10M</value>
Review Comment:
This could be in hours considering a day is the default.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java:
##########
@@ -0,0 +1,227 @@
+/*
+ * 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.symmetric;
+
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.MappingIterator;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.SequenceWriter;
+import com.fasterxml.jackson.databind.SerializationFeature;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import com.fasterxml.jackson.databind.deser.std.StdDeserializer;
+import com.fasterxml.jackson.databind.ser.std.StdSerializer;
+import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.crypto.SecretKey;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermission;
+import java.time.Instant;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+
+import static com.google.common.collect.Sets.newHashSet;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE;
+import static java.util.stream.Collectors.toList;
+
+/**
+ * A {@link SecretKeyStore} that saves and loads SecretKeys from/to a
+ * JSON file on local file system.
+ */
+public class LocalSecretKeyStore implements SecretKeyStore {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(LocalSecretKeyStore.class);
+
+ private final Path secretKeysFile;
+ private final ObjectMapper mapper;
+
+ public LocalSecretKeyStore(Path secretKeysFile) {
+ this.secretKeysFile = secretKeysFile;
+ this.mapper = new ObjectMapper()
+ .registerModule(new JavaTimeModule())
+ .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false);
+ }
+
+ @Override
+ public synchronized List<ManagedSecretKey> load() {
+ if (!secretKeysFile.toFile().exists()) {
+ return Collections.emptyList();
+ }
+
+ ObjectReader reader = mapper.readerFor(ManagedSecretKeyDto.class);
+ try (MappingIterator<ManagedSecretKeyDto> iterator =
+ reader.readValues(secretKeysFile.toFile())) {
+ List<ManagedSecretKeyDto> dtos = iterator.readAll();
+ List<ManagedSecretKey> result = dtos.stream()
+ .map(ManagedSecretKeyDto::toObject)
+ .collect(toList());
+ LOG.info("Loaded {} from {}", result, secretKeysFile);
+ return result;
+ } catch (IOException e) {
+ throw new IllegalStateException("Error reading SecretKeys from "
+ + secretKeysFile, e);
+ }
+ }
+
+ @Override
+ public synchronized void save(Collection<ManagedSecretKey> secretKeys) {
+ setFileOwnerPermissions();
+
+ List<ManagedSecretKeyDto> dtos = secretKeys.stream()
+ .map(ManagedSecretKeyDto::new)
+ .collect(toList());
+
+ try (SequenceWriter writer =
+ mapper.writer().writeValues(secretKeysFile.toFile())) {
+ writer.init(true);
+ for (ManagedSecretKeyDto dto : dtos) {
+ writer.write(dto);
+ }
+ } catch (IOException e) {
+ throw new IllegalStateException("Error saving SecretKeys to file "
+ + secretKeysFile, e);
+ }
+ LOG.info("Saved {} to file {}", secretKeys, secretKeysFile);
+ }
+
+ private void setFileOwnerPermissions() {
+ Set<PosixFilePermission> permissions = newHashSet(OWNER_READ, OWNER_WRITE);
Review Comment:
This can be a static unmodifiable set.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java:
##########
@@ -0,0 +1,227 @@
+/*
+ * 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.symmetric;
+
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.MappingIterator;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.SequenceWriter;
+import com.fasterxml.jackson.databind.SerializationFeature;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import com.fasterxml.jackson.databind.deser.std.StdDeserializer;
+import com.fasterxml.jackson.databind.ser.std.StdSerializer;
+import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.crypto.SecretKey;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermission;
+import java.time.Instant;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+
+import static com.google.common.collect.Sets.newHashSet;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE;
+import static java.util.stream.Collectors.toList;
+
+/**
+ * A {@link SecretKeyStore} that saves and loads SecretKeys from/to a
+ * JSON file on local file system.
+ */
+public class LocalSecretKeyStore implements SecretKeyStore {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(LocalSecretKeyStore.class);
+
+ private final Path secretKeysFile;
+ private final ObjectMapper mapper;
+
+ public LocalSecretKeyStore(Path secretKeysFile) {
+ this.secretKeysFile = secretKeysFile;
+ this.mapper = new ObjectMapper()
+ .registerModule(new JavaTimeModule())
+ .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false);
+ }
+
+ @Override
+ public synchronized List<ManagedSecretKey> load() {
+ if (!secretKeysFile.toFile().exists()) {
+ return Collections.emptyList();
+ }
+
+ ObjectReader reader = mapper.readerFor(ManagedSecretKeyDto.class);
+ try (MappingIterator<ManagedSecretKeyDto> iterator =
+ reader.readValues(secretKeysFile.toFile())) {
+ List<ManagedSecretKeyDto> dtos = iterator.readAll();
+ List<ManagedSecretKey> result = dtos.stream()
+ .map(ManagedSecretKeyDto::toObject)
+ .collect(toList());
+ LOG.info("Loaded {} from {}", result, secretKeysFile);
+ return result;
+ } catch (IOException e) {
+ throw new IllegalStateException("Error reading SecretKeys from "
+ + secretKeysFile, e);
+ }
+ }
+
+ @Override
+ public synchronized void save(Collection<ManagedSecretKey> secretKeys) {
+ setFileOwnerPermissions();
Review Comment:
I think this can be changed to createFile which also sets permissions. Right now this method is responsible to create the file and set the permission.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java:
##########
@@ -0,0 +1,165 @@
+/*
+ * 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.symmetric;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.crypto.KeyGenerator;
+import java.security.NoSuchAlgorithmException;
+import java.time.Duration;
+import java.time.Instant;
+import java.util.List;
+import java.util.UUID;
+import java.util.concurrent.TimeoutException;
+
+import static java.time.Duration.between;
+import static java.util.Comparator.comparing;
+import static java.util.Objects.requireNonNull;
+import static java.util.stream.Collectors.toList;
+
+/**
+ * This component manages symmetric SecretKey life-cycle, including generation,
+ * rotation and destruction.
+ */
+public class SecretKeyManager {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(SecretKeyManager.class);
+
+ private final SecretKeyState state;
+ private boolean pendingInititializedState = false;
+ private final Duration rotationDuration;
+ private final Duration validityDuration;
+ private final SecretKeyStore keyStore;
+
+ private final KeyGenerator keyGenerator;
+
+ public SecretKeyManager(SecretKeyState state,
+ SecretKeyStore keyStore,
+ Duration rotationDuration,
+ Duration validityDuration,
+ String algorithm) {
+ this.state = requireNonNull(state);
+ this.rotationDuration = requireNonNull(rotationDuration);
+ this.validityDuration = requireNonNull(validityDuration);
+ this.keyStore = requireNonNull(keyStore);
+ this.keyGenerator = createKeyGenerator(algorithm);
+ }
+
+ public SecretKeyManager(SecretKeyState state,
+ SecretKeyStore keyStore,
+ SecretKeyConfig config) {
+ this(state, keyStore, config.getRotateDuration(),
+ config.getExpiryDuration(), config.getAlgorithm());
+ }
+
+ /**
+ * Initialize the state from by loading SecretKeys from local file, or
+ * generate new keys if the file doesn't exist.
+ *
+ * @throws TimeoutException can possibly occur when replicating the state.
+ */
+ public synchronized boolean initialize() {
+ if (state.getCurrentKey() != null) {
+ return false;
+ }
+
+ List<ManagedSecretKey> sortedKeys = keyStore.load()
+ .stream()
+ .filter(x -> !x.isExpired())
+ .sorted(comparing(ManagedSecretKey::getCreationTime))
+ .collect(toList());
+
+ ManagedSecretKey currentKey;
+ if (sortedKeys.isEmpty()) {
+ // First start, generate new key as the current key.
+ currentKey = generateSecretKey();
+ sortedKeys.add(currentKey);
+ LOG.info("No keys is loaded, generated new key: {}", currentKey);
+ } else {
+ // For restarts, reload allKeys and take the latest one as current.
+ currentKey = sortedKeys.get(sortedKeys.size() - 1);
+ LOG.info("Key reloaded, current key: {}, all keys: {}", currentKey,
+ sortedKeys);
+ }
+
+ // First, update the SecretKey state to make it visible immediately on the
+ // current instance.
+ state.updateKeysInternal(currentKey, sortedKeys);
Review Comment:
Should the keys be agreed upon before serving them out?
##########
hadoop-hdds/common/src/main/resources/ozone-default.xml:
##########
@@ -3523,4 +3523,49 @@
Interval in MINUTES by Recon to request SCM DB Snapshot.
</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>P7D</value>
Review Comment:
Nit: We follow a different convention in other places, it might make sense to stick with `TimeDurationUtil.java` which is used in the same config file for other durations.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java:
##########
@@ -0,0 +1,227 @@
+/*
+ * 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.symmetric;
+
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.MappingIterator;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.SequenceWriter;
+import com.fasterxml.jackson.databind.SerializationFeature;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import com.fasterxml.jackson.databind.deser.std.StdDeserializer;
+import com.fasterxml.jackson.databind.ser.std.StdSerializer;
+import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.crypto.SecretKey;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermission;
+import java.time.Instant;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+
+import static com.google.common.collect.Sets.newHashSet;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE;
+import static java.util.stream.Collectors.toList;
+
+/**
+ * A {@link SecretKeyStore} that saves and loads SecretKeys from/to a
+ * JSON file on local file system.
+ */
+public class LocalSecretKeyStore implements SecretKeyStore {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(LocalSecretKeyStore.class);
+
+ private final Path secretKeysFile;
+ private final ObjectMapper mapper;
+
+ public LocalSecretKeyStore(Path secretKeysFile) {
+ this.secretKeysFile = secretKeysFile;
+ this.mapper = new ObjectMapper()
+ .registerModule(new JavaTimeModule())
+ .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false);
+ }
+
+ @Override
+ public synchronized List<ManagedSecretKey> load() {
+ if (!secretKeysFile.toFile().exists()) {
+ return Collections.emptyList();
+ }
+
+ ObjectReader reader = mapper.readerFor(ManagedSecretKeyDto.class);
+ try (MappingIterator<ManagedSecretKeyDto> iterator =
+ reader.readValues(secretKeysFile.toFile())) {
+ List<ManagedSecretKeyDto> dtos = iterator.readAll();
+ List<ManagedSecretKey> result = dtos.stream()
+ .map(ManagedSecretKeyDto::toObject)
+ .collect(toList());
+ LOG.info("Loaded {} from {}", result, secretKeysFile);
+ return result;
+ } catch (IOException e) {
+ throw new IllegalStateException("Error reading SecretKeys from "
+ + secretKeysFile, e);
+ }
+ }
+
+ @Override
+ public synchronized void save(Collection<ManagedSecretKey> secretKeys) {
+ setFileOwnerPermissions();
+
+ List<ManagedSecretKeyDto> dtos = secretKeys.stream()
+ .map(ManagedSecretKeyDto::new)
+ .collect(toList());
+
+ try (SequenceWriter writer =
+ mapper.writer().writeValues(secretKeysFile.toFile())) {
+ writer.init(true);
+ for (ManagedSecretKeyDto dto : dtos) {
+ writer.write(dto);
+ }
+ } catch (IOException e) {
+ throw new IllegalStateException("Error saving SecretKeys to file "
+ + secretKeysFile, e);
+ }
+ LOG.info("Saved {} to file {}", secretKeys, secretKeysFile);
+ }
+
+ private void setFileOwnerPermissions() {
Review Comment:
```suggestion
private void createSecretKeysFile() {
```
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java:
##########
@@ -0,0 +1,165 @@
+/*
+ * 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.symmetric;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.crypto.KeyGenerator;
+import java.security.NoSuchAlgorithmException;
+import java.time.Duration;
+import java.time.Instant;
+import java.util.List;
+import java.util.UUID;
+import java.util.concurrent.TimeoutException;
+
+import static java.time.Duration.between;
+import static java.util.Comparator.comparing;
+import static java.util.Objects.requireNonNull;
+import static java.util.stream.Collectors.toList;
+
+/**
+ * This component manages symmetric SecretKey life-cycle, including generation,
+ * rotation and destruction.
+ */
+public class SecretKeyManager {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(SecretKeyManager.class);
+
+ private final SecretKeyState state;
+ private boolean pendingInititializedState = false;
+ private final Duration rotationDuration;
+ private final Duration validityDuration;
+ private final SecretKeyStore keyStore;
+
+ private final KeyGenerator keyGenerator;
+
+ public SecretKeyManager(SecretKeyState state,
+ SecretKeyStore keyStore,
+ Duration rotationDuration,
+ Duration validityDuration,
+ String algorithm) {
+ this.state = requireNonNull(state);
+ this.rotationDuration = requireNonNull(rotationDuration);
+ this.validityDuration = requireNonNull(validityDuration);
+ this.keyStore = requireNonNull(keyStore);
+ this.keyGenerator = createKeyGenerator(algorithm);
+ }
+
+ public SecretKeyManager(SecretKeyState state,
+ SecretKeyStore keyStore,
+ SecretKeyConfig config) {
+ this(state, keyStore, config.getRotateDuration(),
+ config.getExpiryDuration(), config.getAlgorithm());
+ }
+
+ /**
+ * Initialize the state from by loading SecretKeys from local file, or
+ * generate new keys if the file doesn't exist.
+ *
+ * @throws TimeoutException can possibly occur when replicating the state.
+ */
+ public synchronized boolean initialize() {
+ if (state.getCurrentKey() != null) {
+ return false;
+ }
+
+ List<ManagedSecretKey> sortedKeys = keyStore.load()
+ .stream()
+ .filter(x -> !x.isExpired())
+ .sorted(comparing(ManagedSecretKey::getCreationTime))
+ .collect(toList());
+
+ ManagedSecretKey currentKey;
+ if (sortedKeys.isEmpty()) {
+ // First start, generate new key as the current key.
+ currentKey = generateSecretKey();
+ sortedKeys.add(currentKey);
+ LOG.info("No keys is loaded, generated new key: {}", currentKey);
+ } else {
+ // For restarts, reload allKeys and take the latest one as current.
+ currentKey = sortedKeys.get(sortedKeys.size() - 1);
+ LOG.info("Key reloaded, current key: {}, all keys: {}", currentKey,
+ sortedKeys);
+ }
+
+ // First, update the SecretKey state to make it visible immediately on the
+ // current instance.
+ state.updateKeysInternal(currentKey, sortedKeys);
+ // Then, remember to replicate SecretKey states to all instances.
+ pendingInititializedState = true;
+ return true;
+ }
+
+ public synchronized void flushInitializedState() throws TimeoutException {
Review Comment:
Add a javadoc for what this method is expected to do
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java:
##########
@@ -0,0 +1,227 @@
+/*
+ * 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.symmetric;
+
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.MappingIterator;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.SequenceWriter;
+import com.fasterxml.jackson.databind.SerializationFeature;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import com.fasterxml.jackson.databind.deser.std.StdDeserializer;
+import com.fasterxml.jackson.databind.ser.std.StdSerializer;
+import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.crypto.SecretKey;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermission;
+import java.time.Instant;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+
+import static com.google.common.collect.Sets.newHashSet;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE;
+import static java.util.stream.Collectors.toList;
+
+/**
+ * A {@link SecretKeyStore} that saves and loads SecretKeys from/to a
+ * JSON file on local file system.
+ */
+public class LocalSecretKeyStore implements SecretKeyStore {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(LocalSecretKeyStore.class);
+
+ private final Path secretKeysFile;
+ private final ObjectMapper mapper;
+
+ public LocalSecretKeyStore(Path secretKeysFile) {
+ this.secretKeysFile = secretKeysFile;
+ this.mapper = new ObjectMapper()
+ .registerModule(new JavaTimeModule())
+ .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false);
+ }
+
+ @Override
+ public synchronized List<ManagedSecretKey> load() {
+ if (!secretKeysFile.toFile().exists()) {
+ return Collections.emptyList();
+ }
+
+ ObjectReader reader = mapper.readerFor(ManagedSecretKeyDto.class);
+ try (MappingIterator<ManagedSecretKeyDto> iterator =
+ reader.readValues(secretKeysFile.toFile())) {
+ List<ManagedSecretKeyDto> dtos = iterator.readAll();
+ List<ManagedSecretKey> result = dtos.stream()
+ .map(ManagedSecretKeyDto::toObject)
+ .collect(toList());
+ LOG.info("Loaded {} from {}", result, secretKeysFile);
+ return result;
+ } catch (IOException e) {
+ throw new IllegalStateException("Error reading SecretKeys from "
+ + secretKeysFile, e);
+ }
+ }
+
+ @Override
+ public synchronized void save(Collection<ManagedSecretKey> secretKeys) {
+ setFileOwnerPermissions();
+
+ List<ManagedSecretKeyDto> dtos = secretKeys.stream()
+ .map(ManagedSecretKeyDto::new)
+ .collect(toList());
+
+ try (SequenceWriter writer =
+ mapper.writer().writeValues(secretKeysFile.toFile())) {
+ writer.init(true);
+ for (ManagedSecretKeyDto dto : dtos) {
+ writer.write(dto);
+ }
+ } catch (IOException e) {
+ throw new IllegalStateException("Error saving SecretKeys to file "
+ + secretKeysFile, e);
+ }
+ LOG.info("Saved {} to file {}", secretKeys, secretKeysFile);
+ }
+
+ private void setFileOwnerPermissions() {
+ Set<PosixFilePermission> permissions = newHashSet(OWNER_READ, OWNER_WRITE);
+ try {
+ if (!Files.exists(secretKeysFile)) {
+ if (!Files.exists(secretKeysFile.getParent())) {
+ Files.createDirectories(secretKeysFile.getParent());
+ }
+ Files.createFile(secretKeysFile);
+ }
+ Files.setPosixFilePermissions(secretKeysFile, permissions);
+ } catch (IOException e) {
Review Comment:
Unlikely but include `FileAlreadyExistsException`
##########
hadoop-hdds/common/src/main/resources/ozone-default.xml:
##########
@@ -3523,4 +3523,49 @@
Interval in MINUTES by Recon to request SCM DB Snapshot.
</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>P7D</value>
+ <tag>SCM, SECURITY</tag>
+ <description>
+ The duration for which symmetric secret keys issued by SCM are valid.
+ The formats accepted are based on the ISO-8601 duration format PnDTnHnMn.nS
+ </description>
+ </property>
+ <property>
+ <name>hdds.secret.key.rotate.duration</name>
+ <value>P1D</value>
+ <tag>SCM, SECURITY</tag>
+ <description>
+ The duration that SCM periodically generate a new symmetric secret keys.
+ The formats accepted are based on the ISO-8601 duration format PnDTnHnMn.nS
+ </description>
+ </property>
+ <property>
+ <name>hdds.secret.key.rotate.check.duration</name>
+ <value>PT10M</value>
+ <tag>SCM, SECURITY</tag>
+ <description>
+ The duration that SCM periodically checks if it's time to generate new symmetric secret keys.
+ This must be smaller than hdds.secret.key.rotate.duration.
+ The formats accepted are based on the ISO-8601 duration format PnDTnHnMn.nS
+ </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:
```suggestion
The algorithm that SCM uses to generate symmetric secret keys.
```
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java:
##########
@@ -0,0 +1,227 @@
+/*
+ * 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.symmetric;
+
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.MappingIterator;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.SequenceWriter;
+import com.fasterxml.jackson.databind.SerializationFeature;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import com.fasterxml.jackson.databind.deser.std.StdDeserializer;
+import com.fasterxml.jackson.databind.ser.std.StdSerializer;
+import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.crypto.SecretKey;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermission;
+import java.time.Instant;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+
+import static com.google.common.collect.Sets.newHashSet;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE;
+import static java.util.stream.Collectors.toList;
+
+/**
+ * A {@link SecretKeyStore} that saves and loads SecretKeys from/to a
+ * JSON file on local file system.
+ */
+public class LocalSecretKeyStore implements SecretKeyStore {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(LocalSecretKeyStore.class);
+
+ private final Path secretKeysFile;
+ private final ObjectMapper mapper;
+
+ public LocalSecretKeyStore(Path secretKeysFile) {
+ this.secretKeysFile = secretKeysFile;
+ this.mapper = new ObjectMapper()
+ .registerModule(new JavaTimeModule())
+ .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false);
+ }
+
+ @Override
+ public synchronized List<ManagedSecretKey> load() {
+ if (!secretKeysFile.toFile().exists()) {
+ return Collections.emptyList();
+ }
+
+ ObjectReader reader = mapper.readerFor(ManagedSecretKeyDto.class);
+ try (MappingIterator<ManagedSecretKeyDto> iterator =
+ reader.readValues(secretKeysFile.toFile())) {
+ List<ManagedSecretKeyDto> dtos = iterator.readAll();
+ List<ManagedSecretKey> result = dtos.stream()
+ .map(ManagedSecretKeyDto::toObject)
+ .collect(toList());
+ LOG.info("Loaded {} from {}", result, secretKeysFile);
+ return result;
+ } catch (IOException e) {
+ throw new IllegalStateException("Error reading SecretKeys from "
+ + secretKeysFile, e);
+ }
+ }
+
+ @Override
+ public synchronized void save(Collection<ManagedSecretKey> secretKeys) {
+ setFileOwnerPermissions();
+
+ List<ManagedSecretKeyDto> dtos = secretKeys.stream()
+ .map(ManagedSecretKeyDto::new)
+ .collect(toList());
+
+ try (SequenceWriter writer =
+ mapper.writer().writeValues(secretKeysFile.toFile())) {
+ writer.init(true);
+ for (ManagedSecretKeyDto dto : dtos) {
+ writer.write(dto);
+ }
+ } catch (IOException e) {
+ throw new IllegalStateException("Error saving SecretKeys to file "
+ + secretKeysFile, e);
+ }
+ LOG.info("Saved {} to file {}", secretKeys, secretKeysFile);
+ }
+
+ private void setFileOwnerPermissions() {
+ Set<PosixFilePermission> permissions = newHashSet(OWNER_READ, OWNER_WRITE);
+ try {
+ if (!Files.exists(secretKeysFile)) {
+ if (!Files.exists(secretKeysFile.getParent())) {
+ Files.createDirectories(secretKeysFile.getParent());
+ }
+ Files.createFile(secretKeysFile);
+ }
+ Files.setPosixFilePermissions(secretKeysFile, permissions);
+ } catch (IOException e) {
+ throw new IllegalStateException("Error setting secret keys file" +
+ " permission: " + secretKeysFile, e);
+ }
+ }
+
+ private static class ManagedSecretKeyDto {
+ private UUID id;
Review Comment:
Add a monotonically increasing count to avoid depending on wallclock time to sort the keys.
--
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
[GitHub] [ozone] fapifta commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1115568917
##########
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:
Ah I see, I haven't think about that... alright, let's leave it this way.
--
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
[GitHub] [ozone] duongkame commented on pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on PR #4194:
URL: https://github.com/apache/ozone/pull/4194#issuecomment-1409846648
> @duongkame thank you for working on this one, the code and the structure of the code looks well thought out.
>
> As we discussed with @jojochuang we should not use Serializable, instead we probably should serialize and deserialize the things with protobuf as we do in other parts of the code.
>
> Besides this please find my comments inline.
Thanks for the review @fapifta. I visited all the comments and took action.
--
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
[GitHub] [ozone] duongkame commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1095195787
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java:
##########
@@ -0,0 +1,173 @@
+/**
+ * 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 ScmSecretKeyState.Builder()
+ .setSecretKeyStore(secretKeyStore)
+ .setRatisServer(ratisServer)
+ .build();
+ secretKeyManager = new SecretKeyManager(secretKeyState,
+ secretKeyStore, secretKeyConfig);
+
+ scheduler = Executors.newScheduledThreadPool(1,
+ new ThreadFactoryBuilder().setDaemon(true)
+ .setNameFormat(getServiceName())
+ .build());
+
+ String rotationCheckDurationStr = conf.get(
+ HDDS_SECRET_KEY_ROTATE_CHECK_DURATION,
+ HDDS_SECRET_KEY_ROTATE_CHECK_DURATION_DEFAULT);
+ rotationCheckDuration = Duration.parse(rotationCheckDurationStr);
+
+ start();
+ }
+
+ @Override
+ public void notifyStatusChanged() {
+ serviceLock.lock();
+ try {
+ if (scmContext.isLeaderReady()) {
+
+ // Initialize SecretKeys if for first time leader.
+ if (secretKeyManager.initialize()) {
+ // replicate the initialized SecretKeys to followers.
+ scheduler.schedule(() -> {
+ try {
+ secretKeyManager.flushInitializedState();
+ } catch (TimeoutException e) {
+ throw new RuntimeException(
Review Comment:
A `TimeoutException` translated to `RuntimeException` would crash the scheduled task, not SCM.
And a `TimeoutException` doesn't indicate a failure, it just means the wait is over, and the ratis update flow would be still happening asynchronously.
The next rotation check will check if the state is initialized by ratis, otherwise, it'll retry.
Thay may, however, leave a window in which SecretKey state may not have been initialized and tokens can't be generated. For now, the rotation check is configured to run every 10m.
--
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
[GitHub] [ozone] fapifta commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1088139365
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java:
##########
@@ -0,0 +1,227 @@
+/*
+ * 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.symmetric;
+
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.MappingIterator;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.SequenceWriter;
+import com.fasterxml.jackson.databind.SerializationFeature;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import com.fasterxml.jackson.databind.deser.std.StdDeserializer;
+import com.fasterxml.jackson.databind.ser.std.StdSerializer;
+import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.crypto.SecretKey;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermission;
+import java.time.Instant;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+
+import static com.google.common.collect.Sets.newHashSet;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE;
+import static java.util.stream.Collectors.toList;
+
+/**
+ * A {@link SecretKeyStore} that saves and loads SecretKeys from/to a
+ * JSON file on local file system.
+ */
+public class LocalSecretKeyStore implements SecretKeyStore {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(LocalSecretKeyStore.class);
+
+ private final Path secretKeysFile;
+ private final ObjectMapper mapper;
+
+ public LocalSecretKeyStore(Path secretKeysFile) {
+ this.secretKeysFile = secretKeysFile;
+ this.mapper = new ObjectMapper()
+ .registerModule(new JavaTimeModule())
+ .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false);
+ }
+
+ @Override
+ public synchronized List<ManagedSecretKey> load() {
+ if (!secretKeysFile.toFile().exists()) {
+ return Collections.emptyList();
+ }
+
+ ObjectReader reader = mapper.readerFor(ManagedSecretKeyDto.class);
+ try (MappingIterator<ManagedSecretKeyDto> iterator =
+ reader.readValues(secretKeysFile.toFile())) {
+ List<ManagedSecretKeyDto> dtos = iterator.readAll();
+ List<ManagedSecretKey> result = dtos.stream()
+ .map(ManagedSecretKeyDto::toObject)
+ .collect(toList());
+ LOG.info("Loaded {} from {}", result, secretKeysFile);
+ return result;
+ } catch (IOException e) {
+ throw new IllegalStateException("Error reading SecretKeys from "
+ + secretKeysFile, e);
+ }
+ }
+
+ @Override
+ public synchronized void save(Collection<ManagedSecretKey> secretKeys) {
+ setFileOwnerPermissions(secretKeysFile);
+
+ List<ManagedSecretKeyDto> dtos = secretKeys.stream()
+ .map(ManagedSecretKeyDto::new)
+ .collect(toList());
+
+ try (SequenceWriter writer =
+ mapper.writer().writeValues(secretKeysFile.toFile())) {
+ writer.init(true);
+ for (ManagedSecretKeyDto dto : dtos) {
Review Comment:
I know it is nitpicking, but we can use dtos.foreach here, to harmonize with the rest of the code that uses functionals.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java:
##########
@@ -0,0 +1,164 @@
+/*
+ * 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.symmetric;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.crypto.KeyGenerator;
+import java.security.NoSuchAlgorithmException;
+import java.time.Duration;
+import java.time.Instant;
+import java.util.List;
+import java.util.UUID;
+import java.util.concurrent.TimeoutException;
+
+import static java.time.Duration.between;
+import static java.util.Comparator.comparing;
+import static java.util.Objects.requireNonNull;
+import static java.util.stream.Collectors.toList;
+
+/**
+ * This component manages symmetric SecretKey life-cycle, including generation,
+ * rotation and destruction.
+ */
+public class SecretKeyManager {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(SecretKeyManager.class);
+
+ private final SecretKeyState state;
+ private boolean pendingInititializedState = false;
+ private final Duration rotationDuration;
+ private final Duration validityDuration;
+ private final SecretKeyStore keyStore;
+
+ private final KeyGenerator keyGenerator;
+
+ public SecretKeyManager(SecretKeyState state,
+ SecretKeyStore keyStore,
+ Duration rotationDuration,
+ Duration validityDuration,
+ String algorithm) {
+ this.state = requireNonNull(state);
+ this.rotationDuration = requireNonNull(rotationDuration);
+ this.validityDuration = requireNonNull(validityDuration);
+ this.keyStore = requireNonNull(keyStore);
+ this.keyGenerator = createKeyGenerator(algorithm);
+ }
+
+ public SecretKeyManager(SecretKeyState state,
+ SecretKeyStore keyStore,
+ SecretKeyConfig config) {
+ this(state, keyStore, config.getRotateDuration(),
+ config.getExpiryDuration(), config.getAlgorithm());
+ }
+
+ /**
+ * Initialize the state from by loading SecretKeys from local file, or
+ * generate new keys if the file doesn't exist.
+ *
+ * @throws TimeoutException can possibly occur when replicating the state.
+ */
+ public synchronized boolean initialize() {
+ if (state.getCurrentKey() != null) {
+ return false;
+ }
+
+ List<ManagedSecretKey> sortedKeys = keyStore.load()
+ .stream()
+ .filter(x -> !x.isExpired())
+ .sorted(comparing(ManagedSecretKey::getCreationTime))
+ .collect(toList());
+
+ ManagedSecretKey currentKey;
+ if (sortedKeys.isEmpty()) {
+ // First start, generate new key as the current key.
+ currentKey = generateSecretKey();
+ sortedKeys.add(currentKey);
+ LOG.info("No keys is loaded, generated new key: {}", currentKey);
Review Comment:
nit: are
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/package-info.java:
##########
@@ -0,0 +1,22 @@
+/*
+ * 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.
+ */
+
+/**
+ * Encapsulate classes dealing with symmetric key algorithms.
+ */
+package org.apache.hadoop.hdds.security.symmetric;
Review Comment:
At the end of the day, can we add a more comprehensive doc on the package, to summarize what we have here in more detail?
Best would be to give at least some detail why the package exists, what is the fundamental logic behind storing things, what are the config options that are affecting how the code here behaves, and such?
I also would love to have a readme or some kind of a doc linked that summarizes the design. As a first step the design doc might work, but as after implementation we know more about what we ended up with and why, we might write down the original options we discussed, and then the final architecture and the decisions that led to the result.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java:
##########
@@ -0,0 +1,227 @@
+/*
+ * 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.symmetric;
+
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.MappingIterator;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.SequenceWriter;
+import com.fasterxml.jackson.databind.SerializationFeature;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import com.fasterxml.jackson.databind.deser.std.StdDeserializer;
+import com.fasterxml.jackson.databind.ser.std.StdSerializer;
+import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.crypto.SecretKey;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermission;
+import java.time.Instant;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+
+import static com.google.common.collect.Sets.newHashSet;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE;
+import static java.util.stream.Collectors.toList;
+
+/**
+ * A {@link SecretKeyStore} that saves and loads SecretKeys from/to a
+ * JSON file on local file system.
+ */
+public class LocalSecretKeyStore implements SecretKeyStore {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(LocalSecretKeyStore.class);
+
+ private final Path secretKeysFile;
+ private final ObjectMapper mapper;
+
+ public LocalSecretKeyStore(Path secretKeysFile) {
+ this.secretKeysFile = secretKeysFile;
+ this.mapper = new ObjectMapper()
+ .registerModule(new JavaTimeModule())
+ .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false);
+ }
+
+ @Override
+ public synchronized List<ManagedSecretKey> load() {
+ if (!secretKeysFile.toFile().exists()) {
+ return Collections.emptyList();
+ }
+
+ ObjectReader reader = mapper.readerFor(ManagedSecretKeyDto.class);
+ try (MappingIterator<ManagedSecretKeyDto> iterator =
+ reader.readValues(secretKeysFile.toFile())) {
+ List<ManagedSecretKeyDto> dtos = iterator.readAll();
+ List<ManagedSecretKey> result = dtos.stream()
+ .map(ManagedSecretKeyDto::toObject)
+ .collect(toList());
+ LOG.info("Loaded {} from {}", result, secretKeysFile);
+ return result;
+ } catch (IOException e) {
+ throw new IllegalStateException("Error reading SecretKeys from "
+ + secretKeysFile, e);
+ }
+ }
+
+ @Override
+ public synchronized void save(Collection<ManagedSecretKey> secretKeys) {
+ setFileOwnerPermissions(secretKeysFile);
+
+ List<ManagedSecretKeyDto> dtos = secretKeys.stream()
+ .map(ManagedSecretKeyDto::new)
+ .collect(toList());
+
+ try (SequenceWriter writer =
+ mapper.writer().writeValues(secretKeysFile.toFile())) {
+ writer.init(true);
+ for (ManagedSecretKeyDto dto : dtos) {
+ writer.write(dto);
+ }
+ } catch (IOException e) {
+ throw new IllegalStateException("Error saving SecretKeys to file "
+ + secretKeysFile, e);
+ }
+ LOG.info("Saved {} to file {}", secretKeys, secretKeysFile);
+ }
+
+ private void setFileOwnerPermissions(Path path) {
Review Comment:
Again a little bit of nitpicking, but will we use this method with any other path then the secretKeysFile? If not, we can omit the parameter.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SerializableCodec.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.
+ */
+
+package org.apache.hadoop.hdds.scm.security;
Review Comment:
Shouldn't this be in the o.a.h.hdds.scm.ha.io.codec package as the other codec implementations?
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java:
##########
@@ -0,0 +1,138 @@
+/*
+ * 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.symmetric;
+
+import com.google.common.collect.ImmutableList;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import javax.crypto.KeyGenerator;
+import javax.crypto.SecretKey;
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermission;
+import java.time.Duration;
+import java.time.Instant;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+import java.util.stream.Stream;
+
+import static com.google.common.collect.Lists.newArrayList;
+import static com.google.common.collect.Sets.newHashSet;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Test cases for {@link LocalSecretKeyStore}.
+ */
+public class LocalSecretKeyStoreTest {
+ private SecretKeyStore secretKeyStore;
+ private Path testSecretFile;
+
+ @BeforeEach
+ private void setup() throws Exception {
+ testSecretFile = Files.createTempFile("key-strore-test", ".json");
+ secretKeyStore = new LocalSecretKeyStore(testSecretFile);
+ }
+
+ public static Stream<Arguments> saveAndLoadTestCases() throws Exception {
+ return Stream.of(
+ // empty
+ Arguments.of(ImmutableList.of()),
+ // single secret keys.
+ Arguments.of(newArrayList(
+ generateKey("HmacSHA256")
+ )),
+ // multiple secret keys.
+ Arguments.of(newArrayList(
+ generateKey("HmacSHA1"),
+ generateKey("HmacSHA256")
+ ))
+ );
+ }
+
+ @ParameterizedTest
+ @MethodSource("saveAndLoadTestCases")
+ public void testSaveAndLoad(List<ManagedSecretKey> keys) throws IOException {
Review Comment:
Here we are testing whether what we save we can load, and the two will be identical.
It would be nice to have a file saved with the patch, that serves as an etalon, so we have a test case that will check if we can load the same data with the current code that we saved with a previous version of the code, with that we can catch cases where we make an incompatible change in the serialization.
--
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
[GitHub] [ozone] duongkame commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1090225970
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SerializableCodec.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.
+ */
+
+package org.apache.hadoop.hdds.scm.security;
+
+import com.google.protobuf.ByteString;
+import com.google.protobuf.InvalidProtocolBufferException;
+import org.apache.hadoop.hdds.scm.ha.io.Codec;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+
+/**
+ * A general codec for {@link java.io.Serializable} objects.
+ */
+public class SerializableCodec implements Codec {
+ @Override
+ public ByteString serialize(Object object)
+ throws InvalidProtocolBufferException {
+ try {
+ ByteArrayOutputStream bos = new ByteArrayOutputStream();
Review Comment:
Make sense, I've updated to use protobuf.
--
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
[GitHub] [ozone] kerneltime commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1122557301
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java:
##########
@@ -0,0 +1,173 @@
+/**
+ * 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 ScmSecretKeyState.Builder()
+ .setSecretKeyStore(secretKeyStore)
+ .setRatisServer(ratisServer)
+ .build();
+ secretKeyManager = new SecretKeyManager(secretKeyState,
+ secretKeyStore, secretKeyConfig);
+
+ scheduler = Executors.newScheduledThreadPool(1,
+ new ThreadFactoryBuilder().setDaemon(true)
+ .setNameFormat(getServiceName())
+ .build());
+
+ String rotationCheckDurationStr = conf.get(
+ HDDS_SECRET_KEY_ROTATE_CHECK_DURATION,
+ HDDS_SECRET_KEY_ROTATE_CHECK_DURATION_DEFAULT);
+ rotationCheckDuration = Duration.parse(rotationCheckDurationStr);
+
+ start();
+ }
+
+ @Override
+ public void notifyStatusChanged() {
+ serviceLock.lock();
+ try {
+ if (scmContext.isLeaderReady()) {
+
+ // Initialize SecretKeys if for first time leader.
+ if (secretKeyManager.initialize()) {
+ // replicate the initialized SecretKeys to followers.
+ scheduler.schedule(() -> {
+ try {
+ secretKeyManager.flushInitializedState();
+ } catch (TimeoutException e) {
+ throw new RuntimeException(
Review Comment:
We should look into uncaught exception handling. Cashing SCM in the handler makes sense if keys cannot be managed.
--
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
[GitHub] [ozone] duongkame commented on pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on PR #4194:
URL: https://github.com/apache/ozone/pull/4194#issuecomment-1449441116
Thanks, @fapifta. Totally agree that anything replicated by Ratis should be a part of Ratis snapshot, including SecretKeys.
Also, I'll clean up the design doc and create a final immutable version (pdf).
Just one more thing, can you indeed approve the PR?
--
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
[GitHub] [ozone] duongkame commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1090238755
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java:
##########
@@ -0,0 +1,227 @@
+/*
+ * 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.symmetric;
+
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.MappingIterator;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.SequenceWriter;
+import com.fasterxml.jackson.databind.SerializationFeature;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import com.fasterxml.jackson.databind.deser.std.StdDeserializer;
+import com.fasterxml.jackson.databind.ser.std.StdSerializer;
+import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.crypto.SecretKey;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermission;
+import java.time.Instant;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+
+import static com.google.common.collect.Sets.newHashSet;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE;
+import static java.util.stream.Collectors.toList;
+
+/**
+ * A {@link SecretKeyStore} that saves and loads SecretKeys from/to a
+ * JSON file on local file system.
+ */
+public class LocalSecretKeyStore implements SecretKeyStore {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(LocalSecretKeyStore.class);
+
+ private final Path secretKeysFile;
+ private final ObjectMapper mapper;
+
+ public LocalSecretKeyStore(Path secretKeysFile) {
+ this.secretKeysFile = secretKeysFile;
+ this.mapper = new ObjectMapper()
+ .registerModule(new JavaTimeModule())
+ .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false);
+ }
+
+ @Override
+ public synchronized List<ManagedSecretKey> load() {
+ if (!secretKeysFile.toFile().exists()) {
+ return Collections.emptyList();
+ }
+
+ ObjectReader reader = mapper.readerFor(ManagedSecretKeyDto.class);
+ try (MappingIterator<ManagedSecretKeyDto> iterator =
+ reader.readValues(secretKeysFile.toFile())) {
+ List<ManagedSecretKeyDto> dtos = iterator.readAll();
+ List<ManagedSecretKey> result = dtos.stream()
+ .map(ManagedSecretKeyDto::toObject)
+ .collect(toList());
+ LOG.info("Loaded {} from {}", result, secretKeysFile);
+ return result;
+ } catch (IOException e) {
+ throw new IllegalStateException("Error reading SecretKeys from "
+ + secretKeysFile, e);
+ }
+ }
+
+ @Override
+ public synchronized void save(Collection<ManagedSecretKey> secretKeys) {
+ setFileOwnerPermissions();
+
+ List<ManagedSecretKeyDto> dtos = secretKeys.stream()
+ .map(ManagedSecretKeyDto::new)
+ .collect(toList());
+
+ try (SequenceWriter writer =
+ mapper.writer().writeValues(secretKeysFile.toFile())) {
+ writer.init(true);
+ for (ManagedSecretKeyDto dto : dtos) {
+ writer.write(dto);
+ }
+ } catch (IOException e) {
+ throw new IllegalStateException("Error saving SecretKeys to file "
+ + secretKeysFile, e);
+ }
+ LOG.info("Saved {} to file {}", secretKeys, secretKeysFile);
+ }
+
+ private void setFileOwnerPermissions() {
+ Set<PosixFilePermission> permissions = newHashSet(OWNER_READ, OWNER_WRITE);
+ try {
+ if (!Files.exists(secretKeysFile)) {
+ if (!Files.exists(secretKeysFile.getParent())) {
+ Files.createDirectories(secretKeysFile.getParent());
+ }
+ Files.createFile(secretKeysFile);
+ }
+ Files.setPosixFilePermissions(secretKeysFile, permissions);
+ } catch (IOException e) {
Review Comment:
`FileAlreadyExistsException` is a subclass of `IOException`, maybe we don't need to include both?
--
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
[GitHub] [ozone] jojochuang commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1088398091
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SerializableCodec.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.
+ */
+
+package org.apache.hadoop.hdds.scm.security;
+
+import com.google.protobuf.ByteString;
+import com.google.protobuf.InvalidProtocolBufferException;
+import org.apache.hadoop.hdds.scm.ha.io.Codec;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+
+/**
+ * A general codec for {@link java.io.Serializable} objects.
+ */
+public class SerializableCodec implements Codec {
+ @Override
+ public ByteString serialize(Object object)
+ throws InvalidProtocolBufferException {
+ try {
+ ByteArrayOutputStream bos = new ByteArrayOutputStream();
Review Comment:
We should stay away from ByteArrayOutputStream and ObjectOutputStream.
(1) Potential vulnerability due to code injection.
(2) No guarantee of 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.
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
[GitHub] [ozone] duongkame commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1113780852
##########
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:
Good point. I pinned the link to the jira containing the design doc.
--
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
[GitHub] [ozone] duongkame commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1095124161
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java:
##########
@@ -0,0 +1,227 @@
+/*
+ * 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.symmetric;
+
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.MappingIterator;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.SequenceWriter;
+import com.fasterxml.jackson.databind.SerializationFeature;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import com.fasterxml.jackson.databind.deser.std.StdDeserializer;
+import com.fasterxml.jackson.databind.ser.std.StdSerializer;
+import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.crypto.SecretKey;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermission;
+import java.time.Instant;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+
+import static com.google.common.collect.Sets.newHashSet;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE;
+import static java.util.stream.Collectors.toList;
+
+/**
+ * A {@link SecretKeyStore} that saves and loads SecretKeys from/to a
+ * JSON file on local file system.
+ */
+public class LocalSecretKeyStore implements SecretKeyStore {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(LocalSecretKeyStore.class);
+
+ private final Path secretKeysFile;
+ private final ObjectMapper mapper;
+
+ public LocalSecretKeyStore(Path secretKeysFile) {
+ this.secretKeysFile = secretKeysFile;
+ this.mapper = new ObjectMapper()
+ .registerModule(new JavaTimeModule())
+ .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false);
+ }
+
+ @Override
+ public synchronized List<ManagedSecretKey> load() {
+ if (!secretKeysFile.toFile().exists()) {
+ return Collections.emptyList();
+ }
+
+ ObjectReader reader = mapper.readerFor(ManagedSecretKeyDto.class);
+ try (MappingIterator<ManagedSecretKeyDto> iterator =
+ reader.readValues(secretKeysFile.toFile())) {
+ List<ManagedSecretKeyDto> dtos = iterator.readAll();
+ List<ManagedSecretKey> result = dtos.stream()
+ .map(ManagedSecretKeyDto::toObject)
+ .collect(toList());
+ LOG.info("Loaded {} from {}", result, secretKeysFile);
+ return result;
+ } catch (IOException e) {
+ throw new IllegalStateException("Error reading SecretKeys from "
+ + secretKeysFile, e);
+ }
+ }
+
+ @Override
+ public synchronized void save(Collection<ManagedSecretKey> secretKeys) {
+ setFileOwnerPermissions();
+
+ List<ManagedSecretKeyDto> dtos = secretKeys.stream()
+ .map(ManagedSecretKeyDto::new)
+ .collect(toList());
+
+ try (SequenceWriter writer =
+ mapper.writer().writeValues(secretKeysFile.toFile())) {
+ writer.init(true);
+ for (ManagedSecretKeyDto dto : dtos) {
+ writer.write(dto);
+ }
+ } catch (IOException e) {
+ throw new IllegalStateException("Error saving SecretKeys to file "
+ + secretKeysFile, e);
+ }
+ LOG.info("Saved {} to file {}", secretKeys, secretKeysFile);
+ }
+
+ private void setFileOwnerPermissions() {
+ Set<PosixFilePermission> permissions = newHashSet(OWNER_READ, OWNER_WRITE);
+ try {
+ if (!Files.exists(secretKeysFile)) {
+ if (!Files.exists(secretKeysFile.getParent())) {
+ Files.createDirectories(secretKeysFile.getParent());
+ }
+ Files.createFile(secretKeysFile);
+ }
+ Files.setPosixFilePermissions(secretKeysFile, permissions);
+ } catch (IOException e) {
Review Comment:
Any `IOException` will result in SCM crash because the secret keys cannot be saved to files as a reaction to a Ratis log entry.
As we check for the file existence before creating them, the window of error is small (as someone may create the same file at the very same time). Crashing SCM for this unlikely event is just fine to me.
--
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
[GitHub] [ozone] duongkame commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1095118376
##########
hadoop-hdds/common/src/main/resources/ozone-default.xml:
##########
@@ -3523,4 +3523,49 @@
Interval in MINUTES by Recon to request SCM DB Snapshot.
</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>P7D</value>
+ <tag>SCM, SECURITY</tag>
+ <description>
+ The duration for which symmetric secret keys issued by SCM are valid.
+ The formats accepted are based on the ISO-8601 duration format PnDTnHnMn.nS
+ </description>
+ </property>
+ <property>
+ <name>hdds.secret.key.rotate.duration</name>
+ <value>P1D</value>
+ <tag>SCM, SECURITY</tag>
+ <description>
+ The duration that SCM periodically generate a new symmetric secret keys.
+ The formats accepted are based on the ISO-8601 duration format PnDTnHnMn.nS
+ </description>
+ </property>
+ <property>
+ <name>hdds.secret.key.rotate.check.duration</name>
+ <value>PT10M</value>
+ <tag>SCM, SECURITY</tag>
+ <description>
+ The duration that SCM periodically checks if it's time to generate new symmetric secret keys.
+ This must be smaller than hdds.secret.key.rotate.duration.
+ The formats accepted are based on the ISO-8601 duration format PnDTnHnMn.nS
+ </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:
It's updated.
--
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
[GitHub] [ozone] hemantk-12 commented on pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on PR #4194:
URL: https://github.com/apache/ozone/pull/4194#issuecomment-1419985384
@jojochuang and @fapifta Can you please look at the latest revision? Thanks.
--
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
[GitHub] [ozone] duongkame commented on pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on PR #4194:
URL: https://github.com/apache/ozone/pull/4194#issuecomment-1453828735
Thanks @fapifta @kerneltime @jojochuang for the review.
--
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
[GitHub] [ozone] fapifta commented on pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta commented on PR #4194:
URL: https://github.com/apache/ozone/pull/4194#issuecomment-1442141428
You're welcome!
In the meantime one thought does bother me a bit, and would like to understand the intent for that...
In the SecretKeyManager and the SecretKeyState, I have some doubts around the mechanism that generates a new key and does the rotation... it is hard to tell what exactly, it is just a bad feeling so far, so let me think out loud here:
If I understand correctly, SecretKeyManager will decide it should rotate, once the current key is older than 1 day.
Once the rotation happens eventually after 1d, the SecretKeyManager#checkAndRotate() method creates a new list of secret keys, it filters the oldest expired key from the list, and SecretKeyState#updateKeys is called with the keys in a sorted order, where the newest key is the first item in the list.
This call to updateKeys is replicated to the other SCMs, so all SCMs eventually will have the new set of keys.
So far this is clean on its own, the confusion on my side comes after this. The way of implementation suggests that the second proposal the pull model will be used on the DNs.
How this will look like?
I guess if the DN runs into a token, that is newer then the newest key it has, it will pull the list of the keys from SCM. While OMs will pull periodically from SCM.
If my understanding is correct, then operations wise we are good, and probably my mind stuck with the push model and the pre-generated and distributed next key.
During the thought experiment around this, I started to feel that it might not be correct to have a SecretKeyState that gets its internal representation from outside, and also misses proper encapsulation... With the current code this might not be a problem, but here are the concerns:
1. the internal list is guarded by locking, but its reference is leaking via the getSortedKeys() method.
2. updateKeys can override the full internal state
1. is easy to fix, and probably it is just some automatism where usually I don't think about this concern either for the first sight
2. on the other hand there might be a more interesting question... Let's do some thought experiment:
Let's say we have one SCM (SCM1) that is stopped for an extended period of time, and its raft logs are so old that when it gets back in service it downloads a new snapshot, and since that snapshot there was no rotation happening. At this time SCM2 is the leader, but after SCM1 comes up and syncronizes its data and becomes a proper follower, SCM2 goes down before there was a rotation of secret keys, and during election SCM1 becomes the elected leader.
In this scenario will SCM1 have the secretKeys properly? As based on the code my assumption is that in this case SCM1 will have 1 secret key in its list generated at startup (let's assume every key that was persisted has expired already), with that it will issue tokens, and DNs will be able to validate those tokens, but once keys are rotated, the list of old keys will be overridden in all SCMs.
After this has happened, let's assume a DN goes down, and comes up due to a restart for any reason. In this case if there is a client that has a token issued by SCM2 back when it was the leader, was not be able to read data from the restarted DN, as that DN will not be able to fetch the formerly valid token from anywhere, while those DNs that have the token in memory will work fine further causing intermittent read failures.
Do I miss something, or this is a legit scenario with this approach of managing the secret keys?
I see this happening because the keys are managed in a file, not in rocksDB, and so when SCM1 downloads the snapshot of the data to replay logs from that snapshot, it might not replay the transaction that updates the file, and the file is not in the snapshot, but putting the secret keys to rocksDB with that to the snapshot will hurt us later when it comes to conforming with FISMA for example.
--
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
[GitHub] [ozone] fapifta commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
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
[GitHub] [ozone] fapifta commented on pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta commented on PR #4194:
URL: https://github.com/apache/ozone/pull/4194#issuecomment-1446058899
Hi @duongkame,
Thank you for your answers, I haven't noticed HDDS-7945, and that I believe is entitled to solve the consistency problem I tried to describe. The problem is when SCM2 takes on leadership, and does not get the previous state because it was not part of the snapshot, and the updates are not there during log replay, so after that it would overwrite the keys used with the next rotation. This problem should be solved, if the keys are syncronized within the snapshot as well in between the SCMs.
The first one with the unmodifiable list is perfectly fine this way, I just left the thought there, but did not wanted to propose any change just wanted to discuss and understand how you think about it, thank you for sharing the thought process of yours.
From my side, I think I am fine with the changes. +1.
The only thing remained, that is not to be fixed within the PR itself, but we should get rid of the internal JIRA link in the design doc added to the main JIRA ;)
--
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
[GitHub] [ozone] kerneltime commented on pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on PR #4194:
URL: https://github.com/apache/ozone/pull/4194#issuecomment-1451309371
We can merge this in once @fapifta has approved it.
Since we have a developer branch now, we can merge more eagerly and file follow up jiras for work identified.
--
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
[GitHub] [ozone] duongkame commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1090237105
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java:
##########
@@ -0,0 +1,227 @@
+/*
+ * 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.symmetric;
+
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.MappingIterator;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.SequenceWriter;
+import com.fasterxml.jackson.databind.SerializationFeature;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import com.fasterxml.jackson.databind.deser.std.StdDeserializer;
+import com.fasterxml.jackson.databind.ser.std.StdSerializer;
+import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.crypto.SecretKey;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermission;
+import java.time.Instant;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+
+import static com.google.common.collect.Sets.newHashSet;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE;
+import static java.util.stream.Collectors.toList;
+
+/**
+ * A {@link SecretKeyStore} that saves and loads SecretKeys from/to a
+ * JSON file on local file system.
+ */
+public class LocalSecretKeyStore implements SecretKeyStore {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(LocalSecretKeyStore.class);
+
+ private final Path secretKeysFile;
+ private final ObjectMapper mapper;
+
+ public LocalSecretKeyStore(Path secretKeysFile) {
+ this.secretKeysFile = secretKeysFile;
+ this.mapper = new ObjectMapper()
+ .registerModule(new JavaTimeModule())
+ .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false);
+ }
+
+ @Override
+ public synchronized List<ManagedSecretKey> load() {
+ if (!secretKeysFile.toFile().exists()) {
+ return Collections.emptyList();
+ }
+
+ ObjectReader reader = mapper.readerFor(ManagedSecretKeyDto.class);
+ try (MappingIterator<ManagedSecretKeyDto> iterator =
+ reader.readValues(secretKeysFile.toFile())) {
+ List<ManagedSecretKeyDto> dtos = iterator.readAll();
+ List<ManagedSecretKey> result = dtos.stream()
+ .map(ManagedSecretKeyDto::toObject)
+ .collect(toList());
+ LOG.info("Loaded {} from {}", result, secretKeysFile);
+ return result;
+ } catch (IOException e) {
+ throw new IllegalStateException("Error reading SecretKeys from "
+ + secretKeysFile, e);
+ }
+ }
+
+ @Override
+ public synchronized void save(Collection<ManagedSecretKey> secretKeys) {
+ setFileOwnerPermissions();
+
+ List<ManagedSecretKeyDto> dtos = secretKeys.stream()
+ .map(ManagedSecretKeyDto::new)
+ .collect(toList());
+
+ try (SequenceWriter writer =
+ mapper.writer().writeValues(secretKeysFile.toFile())) {
+ writer.init(true);
+ for (ManagedSecretKeyDto dto : dtos) {
+ writer.write(dto);
+ }
+ } catch (IOException e) {
+ throw new IllegalStateException("Error saving SecretKeys to file "
+ + secretKeysFile, e);
+ }
+ LOG.info("Saved {} to file {}", secretKeys, secretKeysFile);
+ }
+
+ private void setFileOwnerPermissions() {
+ Set<PosixFilePermission> permissions = newHashSet(OWNER_READ, OWNER_WRITE);
+ try {
+ if (!Files.exists(secretKeysFile)) {
+ if (!Files.exists(secretKeysFile.getParent())) {
+ Files.createDirectories(secretKeysFile.getParent());
+ }
+ Files.createFile(secretKeysFile);
+ }
+ Files.setPosixFilePermissions(secretKeysFile, permissions);
+ } catch (IOException e) {
+ throw new IllegalStateException("Error setting secret keys file" +
+ " permission: " + secretKeysFile, e);
+ }
+ }
+
+ private static class ManagedSecretKeyDto {
+ private UUID id;
Review Comment:
I believe all the time durations like rotation or expiry should be much longer than the accepted time skew.
Plus, a SecretKey is always generated on top of the existing agreed ones (we use the latest key timestamp to determine if it's time to generate a new key). And that way, the time order between SecretKey is always protected.
--
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
[GitHub] [ozone] kerneltime commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1090242541
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java:
##########
@@ -0,0 +1,227 @@
+/*
+ * 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.symmetric;
+
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.MappingIterator;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.SequenceWriter;
+import com.fasterxml.jackson.databind.SerializationFeature;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import com.fasterxml.jackson.databind.deser.std.StdDeserializer;
+import com.fasterxml.jackson.databind.ser.std.StdSerializer;
+import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.crypto.SecretKey;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermission;
+import java.time.Instant;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+
+import static com.google.common.collect.Sets.newHashSet;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE;
+import static java.util.stream.Collectors.toList;
+
+/**
+ * A {@link SecretKeyStore} that saves and loads SecretKeys from/to a
+ * JSON file on local file system.
+ */
+public class LocalSecretKeyStore implements SecretKeyStore {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(LocalSecretKeyStore.class);
+
+ private final Path secretKeysFile;
+ private final ObjectMapper mapper;
+
+ public LocalSecretKeyStore(Path secretKeysFile) {
+ this.secretKeysFile = secretKeysFile;
+ this.mapper = new ObjectMapper()
+ .registerModule(new JavaTimeModule())
+ .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false);
+ }
+
+ @Override
+ public synchronized List<ManagedSecretKey> load() {
+ if (!secretKeysFile.toFile().exists()) {
+ return Collections.emptyList();
+ }
+
+ ObjectReader reader = mapper.readerFor(ManagedSecretKeyDto.class);
+ try (MappingIterator<ManagedSecretKeyDto> iterator =
+ reader.readValues(secretKeysFile.toFile())) {
+ List<ManagedSecretKeyDto> dtos = iterator.readAll();
+ List<ManagedSecretKey> result = dtos.stream()
+ .map(ManagedSecretKeyDto::toObject)
+ .collect(toList());
+ LOG.info("Loaded {} from {}", result, secretKeysFile);
+ return result;
+ } catch (IOException e) {
+ throw new IllegalStateException("Error reading SecretKeys from "
+ + secretKeysFile, e);
+ }
+ }
+
+ @Override
+ public synchronized void save(Collection<ManagedSecretKey> secretKeys) {
+ setFileOwnerPermissions();
+
+ List<ManagedSecretKeyDto> dtos = secretKeys.stream()
+ .map(ManagedSecretKeyDto::new)
+ .collect(toList());
+
+ try (SequenceWriter writer =
+ mapper.writer().writeValues(secretKeysFile.toFile())) {
+ writer.init(true);
+ for (ManagedSecretKeyDto dto : dtos) {
+ writer.write(dto);
+ }
+ } catch (IOException e) {
+ throw new IllegalStateException("Error saving SecretKeys to file "
+ + secretKeysFile, e);
+ }
+ LOG.info("Saved {} to file {}", secretKeys, secretKeysFile);
+ }
+
+ private void setFileOwnerPermissions() {
+ Set<PosixFilePermission> permissions = newHashSet(OWNER_READ, OWNER_WRITE);
+ try {
+ if (!Files.exists(secretKeysFile)) {
+ if (!Files.exists(secretKeysFile.getParent())) {
+ Files.createDirectories(secretKeysFile.getParent());
+ }
+ Files.createFile(secretKeysFile);
+ }
+ Files.setPosixFilePermissions(secretKeysFile, permissions);
+ } catch (IOException e) {
Review Comment:
Makes sense unless we want to differentiate. What would the error handling be if this step fails? Will go over it again.
--
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
[GitHub] [ozone] duongkame commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1090227710
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/package-info.java:
##########
@@ -0,0 +1,22 @@
+/*
+ * 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.
+ */
+
+/**
+ * Encapsulate classes dealing with symmetric key algorithms.
+ */
+package org.apache.hadoop.hdds.security.symmetric;
Review Comment:
Totally, I've updated it with more detailed documentation. Would be great if you can have 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.
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
[GitHub] [ozone] duongkame commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1089543317
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java:
##########
@@ -0,0 +1,227 @@
+/*
+ * 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.symmetric;
+
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.MappingIterator;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.SequenceWriter;
+import com.fasterxml.jackson.databind.SerializationFeature;
+import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize;
+import com.fasterxml.jackson.databind.deser.std.StdDeserializer;
+import com.fasterxml.jackson.databind.ser.std.StdSerializer;
+import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.crypto.SecretKey;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermission;
+import java.time.Instant;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+
+import static com.google.common.collect.Sets.newHashSet;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
+import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE;
+import static java.util.stream.Collectors.toList;
+
+/**
+ * A {@link SecretKeyStore} that saves and loads SecretKeys from/to a
+ * JSON file on local file system.
+ */
+public class LocalSecretKeyStore implements SecretKeyStore {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(LocalSecretKeyStore.class);
+
+ private final Path secretKeysFile;
+ private final ObjectMapper mapper;
+
+ public LocalSecretKeyStore(Path secretKeysFile) {
+ this.secretKeysFile = secretKeysFile;
+ this.mapper = new ObjectMapper()
+ .registerModule(new JavaTimeModule())
+ .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false);
+ }
+
+ @Override
+ public synchronized List<ManagedSecretKey> load() {
+ if (!secretKeysFile.toFile().exists()) {
+ return Collections.emptyList();
+ }
+
+ ObjectReader reader = mapper.readerFor(ManagedSecretKeyDto.class);
+ try (MappingIterator<ManagedSecretKeyDto> iterator =
+ reader.readValues(secretKeysFile.toFile())) {
+ List<ManagedSecretKeyDto> dtos = iterator.readAll();
+ List<ManagedSecretKey> result = dtos.stream()
+ .map(ManagedSecretKeyDto::toObject)
+ .collect(toList());
+ LOG.info("Loaded {} from {}", result, secretKeysFile);
+ return result;
+ } catch (IOException e) {
+ throw new IllegalStateException("Error reading SecretKeys from "
+ + secretKeysFile, e);
+ }
+ }
+
+ @Override
+ public synchronized void save(Collection<ManagedSecretKey> secretKeys) {
+ setFileOwnerPermissions(secretKeysFile);
+
+ List<ManagedSecretKeyDto> dtos = secretKeys.stream()
+ .map(ManagedSecretKeyDto::new)
+ .collect(toList());
+
+ try (SequenceWriter writer =
+ mapper.writer().writeValues(secretKeysFile.toFile())) {
+ writer.init(true);
+ for (ManagedSecretKeyDto dto : dtos) {
Review Comment:
I like using all functionals too, something like`dto.forEach(writer::write)`, yet, the `write` method throws `IOException`, and the checked exception is not "functional" friendly. A wrapper to turn it into RuntimeException can work, but it looks like an unnecessary complication.
--
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
[GitHub] [ozone] duongkame commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1113787415
##########
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:
Even though the logs don't really indicate anything meaningful, I wanted to keep them as the thread "heartbeat".
Yet, I guess you're right. I removed them, and will add a few metrics instead to help monitor SecretKeyService (HDDS-8004).
--
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
[GitHub] [ozone] duongkame commented on pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on PR #4194:
URL: https://github.com/apache/ozone/pull/4194#issuecomment-1439386593
>
> Hi @duongkame thank you for the continued work on this one, in general it is getting better and I like the approach and most of the code, though I have added some minor comments inline, please take a look.
Thanks again for the very thorough review, @fapifta. I've looked into them and made the necessary changes.
--
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
[GitHub] [ozone] jojochuang commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1088416628
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java:
##########
@@ -0,0 +1,173 @@
+/**
+ * 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 ScmSecretKeyState.Builder()
+ .setSecretKeyStore(secretKeyStore)
+ .setRatisServer(ratisServer)
+ .build();
+ secretKeyManager = new SecretKeyManager(secretKeyState,
+ secretKeyStore, secretKeyConfig);
+
+ scheduler = Executors.newScheduledThreadPool(1,
+ new ThreadFactoryBuilder().setDaemon(true)
+ .setNameFormat(getServiceName())
+ .build());
+
+ String rotationCheckDurationStr = conf.get(
+ HDDS_SECRET_KEY_ROTATE_CHECK_DURATION,
+ HDDS_SECRET_KEY_ROTATE_CHECK_DURATION_DEFAULT);
+ rotationCheckDuration = Duration.parse(rotationCheckDurationStr);
+
+ start();
+ }
+
+ @Override
+ public void notifyStatusChanged() {
+ serviceLock.lock();
+ try {
+ if (scmContext.isLeaderReady()) {
+
+ // Initialize SecretKeys if for first time leader.
+ if (secretKeyManager.initialize()) {
+ // replicate the initialized SecretKeys to followers.
+ scheduler.schedule(() -> {
+ try {
+ secretKeyManager.flushInitializedState();
+ } catch (TimeoutException e) {
+ throw new RuntimeException(
Review Comment:
should it retry if time out? RuntimeException would crash the SCM. Probably makes sense for such a critical service.
--
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
[GitHub] [ozone] duongkame commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1113781091
##########
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:
My bad. Updated the description.
--
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
[GitHub] [ozone] fapifta commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1116009439
##########
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:
One thing I noticed in the document linked to the JIRA, it has a link to a Cloudera internal JIRA ticket ID in the first paragraph, that is not visible for others, we might edit that part of the upstream design doc to give the information what is hidden for non-Cloudera folks.
--
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
[GitHub] [ozone] duongkame commented on pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on PR #4194:
URL: https://github.com/apache/ozone/pull/4194#issuecomment-1408081382
> Currently, the code is differentiating current keys and all keys and has a lot of code to make the distinction between them. Would keeping the keys to always be a sorted list be cleaner? The index 0 or n-1 is always the current key and we copy and move the entire list around in the code as needed. Half way done will take a deeper look at ratis integration and tests later.
Thanks for the suggestion, @kerneltime. It was started in the most natural way which apparently not the smartest one. All the code related to the distinction has been cleaned up.
--
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
[GitHub] [ozone] duongkame commented on a diff in pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on code in PR #4194:
URL: https://github.com/apache/ozone/pull/4194#discussion_r1095118111
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsConfigKeys.java:
##########
@@ -216,6 +216,28 @@ public final class HddsConfigKeys {
public static final String HDDS_X509_ROOTCA_PRIVATE_KEY_FILE_DEFAULT =
"";
+ public static final String HDDS_SECRET_KEY_FILE =
Review Comment:
I added an explanation in `ozone-site.xml`
--
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
[GitHub] [ozone] duongkame commented on pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on PR #4194:
URL: https://github.com/apache/ozone/pull/4194#issuecomment-1442884341
> You're welcome! In the meantime one thought does bother me a bit, and would like to understand the intent for that...
>
> In the SecretKeyManager and the SecretKeyState, I have some doubts around the mechanism that generates a new key and does the rotation... it is hard to tell what exactly, it is just a bad feeling so far, so let me think out loud here: If I understand correctly, SecretKeyManager will decide it should rotate, once the current key is older than 1 day. Once the rotation happens eventually after 1d, the SecretKeyManager#checkAndRotate() method creates a new list of secret keys, it filters the oldest expired key from the list, and SecretKeyState#updateKeys is called with the keys in a sorted order, where the newest key is the first item in the list. This call to updateKeys is replicated to the other SCMs, so all SCMs eventually will have the new set of keys.
>
> So far this is clean on its own, the confusion on my side comes after this. The way of implementation suggests that the second proposal the pull model will be used on the DNs. How this will look like? I guess if the DN runs into a token, that is newer then the newest key it has, it will pull the list of the keys from SCM. While OMs will pull periodically from SCM. If my understanding is correct, then operations wise we are good, and probably my mind stuck with the push model and the pre-generated and distributed next key.
>
> During the thought experiment around this, I started to feel that it might not be correct to have a SecretKeyState that gets its internal representation from outside, and also misses proper encapsulation... With the current code this might not be a problem, but here are the concerns:
>
> 1. the internal list is guarded by locking, but its reference is leaking via the getSortedKeys() method. Though the actual implementation used is an unmodifiable list as of now.
> 2. updateKeys can override the full internal state
>
> The first one might not be a problem at all at the end of the day, it might be, if someone carelessly change the internal representation from unmodifiable list to a mutable list impementation. Second one on the other hand there might be a more interesting question... Let's do some thought experiment: Let's say we have one SCM (SCM1) that is stopped for an extended period of time, and its raft logs are so old that when it gets back in service it downloads a new snapshot, and since that snapshot there was no rotation happening. At this time SCM2 is the leader, but after SCM1 comes up and syncronizes its data and becomes a proper follower, SCM2 goes down before there was a rotation of secret keys, and during election SCM1 becomes the elected leader.
>
> In this scenario will SCM1 have the secretKeys properly? As based on the code my assumption is that in this case SCM1 will have 1 secret key in its list generated at startup (let's assume every key that was persisted has expired already), with that it will issue tokens, and DNs will be able to validate those tokens, but once keys are rotated, the list of old keys will be overridden in all SCMs.
>
> After this has happened, let's assume a DN goes down, and comes up due to a restart for any reason. In this case if there is a client that has a token issued by SCM2 back when it was the leader, was not be able to read data from the restarted DN, as that DN will not be able to fetch the formerly valid token from anywhere, while those DNs that have the token in memory will work fine further causing intermittent read failures.
>
> Do I miss something, or this is a legit scenario with this approach of managing the secret keys? I see this happening because the keys are managed in a file, not in rocksDB, and so when SCM1 downloads the snapshot of the data to replay logs from that snapshot, it might not replay the transaction that updates the file, and the file is not in the snapshot, but putting the secret keys to rocksDB with that to the snapshot will hurt us later when it comes to conforming with FISMA for example.
Thanks again for being thorough, @fapifta.
#1 is just a design choice. I was considering between a) creating a copy list to serve a query, or b) leveraging immutability. I think in Ozone we're so easy to go with a) and that's something I notice when doing a memory scaling analysis in SCM. Today, buffers, e.g. pipeline datanodes, are just copied over and that seems unnecessary. Immutability is a good alternative to deal with concurrency and should be used where possible. (Yes, I know, it's not that easy to spread that awareness in Java, i.e. there's no good implicit type for immutable data, which would be easier in the functional world).
--
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
[GitHub] [ozone] duongkame merged pull request #4194: HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame merged PR #4194:
URL: https://github.com/apache/ozone/pull/4194
--
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