From af81814a4fedc69350114a4fd07bd7e511b436ca Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Thu, 12 Jan 2023 09:57:26 -0800 Subject: [PATCH 01/27] HDDS-7734: Implement symmetric SecretKeys lifescycle management in SCM. - Bootstrap logic for symmetric SecretKeys lifescycle management. --- .../symmetric/LocalSecretKeyStore.java | 218 ++++++++++++++++++ .../security/symmetric/ManagedSecretKey.java | 78 +++++++ .../security/symmetric/SecretKeyManager.java | 59 +++++ .../symmetric/SecretKeyManagerImpl.java | 166 +++++++++++++ .../security/symmetric/SecretKeyStore.java | 31 +++ .../hdds/security/symmetric/package-info.java | 22 ++ .../symmetric/LocalSecretKeyStoreTest.java | 140 +++++++++++ .../symmetric/SecretKeyManagerTest.java | 212 +++++++++++++++++ 8 files changed, 926 insertions(+) create mode 100644 hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java create mode 100644 hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/ManagedSecretKey.java create mode 100644 hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java create mode 100644 hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerImpl.java create mode 100644 hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyStore.java create mode 100644 hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/package-info.java create mode 100644 hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java create mode 100644 hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java new file mode 100644 index 000000000000..567500eecdff --- /dev/null +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java @@ -0,0 +1,218 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.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 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.Paths; +import java.nio.file.attribute.PosixFilePermission; +import java.security.KeyStore; +import java.time.Duration; +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 final Path secretKeysFile; + private final ObjectMapper mapper; + + public LocalSecretKeyStore(String keyDirectory, String secretKeyFileName) { + this.secretKeysFile = Paths.get(keyDirectory, secretKeyFileName); + this.mapper = new ObjectMapper() + .registerModule(new JavaTimeModule()) + .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false); + } + + @Override + public synchronized List load() { + if (!secretKeysFile.toFile().exists()) { + return Collections.emptyList(); + } + + ObjectReader reader = mapper.readerFor(ManagedSecretKeyDto.class); + try (MappingIterator iterator = + reader.readValues(secretKeysFile.toFile())) { + List dtos = iterator.readAll(); + return dtos.stream() + .map(ManagedSecretKeyDto::toObject) + .collect(toList()); + } catch (IOException e) { + throw new IllegalStateException("Error reading SecretKeys from " + + secretKeysFile, e); + } + } + + @Override + public synchronized void save(Collection secretKeys) { + setFileOwnerPermissions(secretKeysFile); + + List 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); + } + } + + private void setFileOwnerPermissions(Path path) { + Set permissions = newHashSet(OWNER_READ, OWNER_WRITE); + try { + if (!Files.exists(path)) { + Files.createFile(path); + } + Files.setPosixFilePermissions(path, permissions); + } catch (IOException e) { + throw new IllegalStateException("Error setting secret keys file" + + " permission: " + secretKeysFile, e); + } + } + + private static class ManagedSecretKeyDto { + private UUID id; + private Instant creationTime; + private Instant expiryTime; + private SecretKey secretKey; + + /** + * Used by Jackson when deserializing. + */ + ManagedSecretKeyDto() { + } + + ManagedSecretKeyDto(ManagedSecretKey object) { + id = object.getId(); + creationTime = object.getCreationTime(); + expiryTime = object.getExpiryTime(); + secretKey = object.getSecretKey(); + } + + public ManagedSecretKey toObject() { + return new ManagedSecretKey(id, creationTime, + expiryTime, secretKey); + } + + public UUID getId() { + return id; + } + + public void setId(UUID id) { + this.id = id; + } + + public Instant getCreationTime() { + return creationTime; + } + + public void setCreationTime(Instant creationTime) { + this.creationTime = creationTime; + } + + public Instant getExpiryTime() { + return expiryTime; + } + + public void setExpiryTime(Instant expiryTime) { + this.expiryTime = expiryTime; + } + + @JsonSerialize(using = SecretKeySerializer.class) + @JsonDeserialize(using = SecretKeyDeserializer.class) + public SecretKey getSecretKey() { + return secretKey; + } + + public void setSecretKey(SecretKey secretKey) { + this.secretKey = secretKey; + } + } + + private static class SecretKeySerializer extends StdSerializer { + SecretKeySerializer() { + super(SecretKey.class); + } + + @Override + public void serialize(SecretKey value, JsonGenerator gen, + SerializerProvider provider) throws IOException { + ByteArrayOutputStream bos = new ByteArrayOutputStream(); + ObjectOutputStream oos = new ObjectOutputStream(bos); + oos.writeObject(value); + gen.writeBinary(bos.toByteArray()); + } + } + + private static class SecretKeyDeserializer extends + StdDeserializer { + SecretKeyDeserializer() { + super(SecretKey.class); + } + + @Override + public SecretKey deserialize(JsonParser p, DeserializationContext ctxt) + throws IOException { + ByteArrayInputStream bis = new ByteArrayInputStream(p.getBinaryValue()); + ObjectInputStream ois = new ObjectInputStream(bis); + try { + return (SecretKey) ois.readObject(); + } catch (ClassNotFoundException e) { + throw new IllegalStateException("Error reading SecretKey", e); + } + } + } +} diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/ManagedSecretKey.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/ManagedSecretKey.java new file mode 100644 index 000000000000..715f143ce778 --- /dev/null +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/ManagedSecretKey.java @@ -0,0 +1,78 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.hdds.security.symmetric; + +import javax.crypto.SecretKey; +import java.time.Instant; +import java.util.UUID; + +/** + * Enclosed a symmetric {@link SecretKey} with additional data for life-cycle + * management. + */ +public class ManagedSecretKey { + private final UUID id; + private final Instant creationTime; + private final Instant expiryTime; + private final SecretKey secretKey; + + public ManagedSecretKey(UUID id, + Instant creationTime, + Instant expiryTime, + SecretKey secretKey) { + this.id = id; + this.creationTime = creationTime; + this.expiryTime = expiryTime; + this.secretKey = secretKey; + } + + public boolean isExpired() { + return expiryTime.isBefore(Instant.now()); + } + + public UUID getId() { + return id; + } + + public SecretKey getSecretKey() { + return secretKey; + } + + public Instant getCreationTime() { + return creationTime; + } + + public Instant getExpiryTime() { + return expiryTime; + } + + @Override + public int hashCode() { + return id.hashCode(); + } + + @Override + public boolean equals(Object obj) { + if (!(obj instanceof ManagedSecretKey)) { + return false; + } + ManagedSecretKey that = (ManagedSecretKey) obj; + return this.id.equals(that.id); + } +} diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java new file mode 100644 index 000000000000..d85ab97aab91 --- /dev/null +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java @@ -0,0 +1,59 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.hdds.security.symmetric; + +import org.apache.hadoop.hdds.scm.metadata.Replicate; + +import javax.annotation.Nonnull; +import java.util.Set; +import java.util.concurrent.TimeoutException; + +/** + * This component manages symmetric SecretKey life-cycle, including generation, + * rotation and destruction. + */ +public interface SecretKeyManager { + + /** + * @return the current active key, which is used for signing tokens. + */ + @Nonnull + ManagedSecretKey getCurrentKey(); + + /** + * @return all the keys that managed by this manager. + */ + Set getAllKeys(); + + /** + * Check and rotate the keys. + * + * @return true if rotation actually happens, false if it doesn't. + */ + boolean checkAndRotate(); + + /** + * Update the SecretKeys. + * This is a short-hand for replicating SecretKeys across all SCM instances + * after each rotation, typically invoked by checkAndRotate + */ + @Replicate + void updateKeys(ManagedSecretKey currentKey, + Set allKeys) throws TimeoutException; +} diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerImpl.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerImpl.java new file mode 100644 index 000000000000..47d95b24a088 --- /dev/null +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerImpl.java @@ -0,0 +1,166 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.hdds.security.symmetric; + +import javax.annotation.Nonnull; +import javax.crypto.KeyGenerator; +import java.security.NoSuchAlgorithmException; +import java.time.Duration; +import java.time.Instant; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.UUID; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.stream.Collectors; + +import static java.time.Duration.between; +import static java.util.Comparator.comparing; +import static java.util.Objects.requireNonNull; + +public class SecretKeyManagerImpl implements SecretKeyManager { + private final ReadWriteLock lock = new ReentrantReadWriteLock(); + private ManagedSecretKey currentKey; + private final Map allKeys = new HashMap<>(); + + private final SecretKeyStore keyStore; + private final Duration validityDuration; + private final Duration rotationDuration; + + private final KeyGenerator keyGenerator; + + SecretKeyManagerImpl(SecretKeyStore keyStore, Duration validityDuration, + Duration rotationDuration, String algorithm) { + this.keyStore = keyStore; + this.validityDuration = requireNonNull(validityDuration); + this.rotationDuration = requireNonNull(rotationDuration); + this.keyGenerator = createKeyGenerator(algorithm); + + reloadKeys(); + } + + private void reloadKeys() { + List sortedKeys = keyStore.load() + .stream() + .filter(x -> !x.isExpired()) + .sorted(comparing(ManagedSecretKey::getCreationTime)) + .collect(Collectors.toList()); + + if (sortedKeys.isEmpty()) { + // First start, generate new key as the current key. + currentKey = generateSecretKey(); + allKeys.put(currentKey.getId(), currentKey); + } else { + // For restarts, reload allKeys and take the latest one as current. + currentKey = sortedKeys.get(sortedKeys.size() - 1); + sortedKeys.forEach(x -> allKeys.put(x.getId(), x)); + } + } + + @Override + @Nonnull + public ManagedSecretKey getCurrentKey() { + lock.readLock().lock(); + try { + return requireNonNull(currentKey); + } finally { + lock.readLock().unlock(); + } + } + + @Override + public Set getAllKeys() { + lock.readLock().lock(); + try { + return new HashSet<>(allKeys.values()); + } finally { + lock.readLock().unlock(); + } + } + + @Override + public boolean checkAndRotate() { + ManagedSecretKey newCurrentKey = null; + Set updatedKeys = null; + + lock.readLock().lock(); + try { + if (shouldRotate()) { + newCurrentKey = generateSecretKey(); + updatedKeys = allKeys.values() + .stream().filter(x -> !x.isExpired()) + .collect(Collectors.toSet()); + updatedKeys.add(newCurrentKey); + } + } finally { + lock.readLock().unlock(); + } + + if (newCurrentKey != null) { + updateKeys(newCurrentKey, updatedKeys); + return true; + } + + return false; + } + + @Override + public void updateKeys(ManagedSecretKey newCurrentKey, + Set newAllKeys) { + lock.writeLock().lock(); + try { + currentKey = newCurrentKey; + allKeys.clear(); + for (ManagedSecretKey secretKey : newAllKeys) { + allKeys.put(secretKey.getId(), secretKey); + } + keyStore.save(allKeys.values()); + } finally { + lock.writeLock().unlock(); + } + } + + private boolean shouldRotate() { + Duration established = between(currentKey.getCreationTime(), Instant.now()); + return established.compareTo(rotationDuration) >= 0; + } + + private KeyGenerator createKeyGenerator(String algorithm) { + try { + return KeyGenerator.getInstance(algorithm); + } catch (NoSuchAlgorithmException e) { + throw new IllegalArgumentException("Error creating KeyGenerator for " + + "algorithm " + algorithm, e); + } + } + + private ManagedSecretKey generateSecretKey() { + Instant now = Instant.now(); + return new ManagedSecretKey( + UUID.randomUUID(), + now, + now.plus(validityDuration), + keyGenerator.generateKey() + ); + } + +} diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyStore.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyStore.java new file mode 100644 index 000000000000..aa4184fc9b32 --- /dev/null +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyStore.java @@ -0,0 +1,31 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.hdds.security.symmetric; + +import java.util.Collection; +import java.util.List; + +/** + * Interface for SecretKey storage component. + */ +public interface SecretKeyStore { + List load(); + + void save(Collection secretKeys); +} diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/package-info.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/package-info.java new file mode 100644 index 000000000000..10b4dc884294 --- /dev/null +++ b/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 + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * Encapsulate classes dealing with symmetric key algorithms. + */ +package org.apache.hadoop.hdds.security.symmetric; diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java new file mode 100644 index 000000000000..e0c4545d36dd --- /dev/null +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java @@ -0,0 +1,140 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.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 keyDir; + private String secretKeysFileName = "test.json"; + + @BeforeEach + private void setup() throws Exception { + keyDir = Files.createTempDirectory("key-store-test"); + secretKeyStore = new LocalSecretKeyStore(keyDir.toFile().getAbsolutePath(), + secretKeysFileName); + } + + public static Stream 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 keys) throws IOException { + secretKeyStore.save(keys); + + // Ensure the intended file exists and is readable and writeable to + // file owner only. + File file = new File(keyDir.toFile(), secretKeysFileName); + assertTrue(file.exists()); + Set permissions = + Files.getPosixFilePermissions(file.toPath()); + assertEquals(newHashSet(OWNER_READ, OWNER_WRITE), permissions); + + List reloadedKeys = secretKeyStore.load(); + assertEqualKeys(keys, reloadedKeys); + } + + @Test + public void testOverwrite() throws Exception { + List initialKeys = + newArrayList(generateKey("HmacSHA256")); + secretKeyStore.save(initialKeys); + + List updatedKeys = newArrayList( + generateKey("HmacSHA1"), + generateKey("HmacSHA256") + ); + secretKeyStore.save(updatedKeys); + + assertEqualKeys(updatedKeys, secretKeyStore.load()); + } + + private void assertEqualKeys(List keys, + List reloadedKeys) { + assertEquals(keys.size(), reloadedKeys.size()); + for (int i = 0; i < keys.size(); i++) { + ManagedSecretKey key = keys.get(i); + ManagedSecretKey reloadedKey = reloadedKeys.get(i); + + assertEquals(key.getId(), reloadedKey.getId()); + assertEquals(key.getCreationTime().toEpochMilli(), + reloadedKey.getCreationTime().toEpochMilli()); + assertEquals(key.getExpiryTime(), + reloadedKey.getExpiryTime()); + assertEquals(key.getSecretKey(), reloadedKey.getSecretKey()); + } + } + + private static ManagedSecretKey generateKey(String algorithm) + throws Exception { + KeyGenerator keyGen = KeyGenerator.getInstance(algorithm); + SecretKey secretKey = keyGen.generateKey(); + Instant now = Instant.now(); + return new ManagedSecretKey( + UUID.randomUUID(), + now, + now.plus(Duration.ofHours(1)), + secretKey + ); + } +} diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java new file mode 100644 index 000000000000..43f4adac0f40 --- /dev/null +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java @@ -0,0 +1,212 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.hdds.security.symmetric; + +import com.google.common.collect.ImmutableList; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.mockito.ArgumentCaptor; +import org.mockito.Mockito; + +import javax.crypto.KeyGenerator; +import javax.crypto.SecretKey; +import java.time.Duration; +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 java.util.concurrent.TimeoutException; +import java.util.stream.Stream; + +import static com.google.common.collect.Lists.newArrayList; +import static com.google.common.collect.Sets.newHashSet; +import static java.time.Instant.now; +import static java.time.temporal.ChronoUnit.DAYS; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.params.provider.Arguments.of; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Tests cases for {@link SecretKeyManager} implementation. + */ +public class SecretKeyManagerTest { + private final static Duration VALIDITY_DURATION = Duration.ofDays(3); + private final static Duration ROTATION_DURATION = Duration.ofDays(1); + private final static String ALGORITHM = "HmacSHA256"; + + private SecretKeyStore mockedKeyStore; + + @BeforeEach + private void setup() { + mockedKeyStore = Mockito.mock(SecretKeyStore.class); + } + + public static Stream loadSecretKeysTestCases() throws Exception { + ManagedSecretKey k0 = generateKey(now()); + ManagedSecretKey k1 = generateKey(now().minus(1, DAYS)); + ManagedSecretKey k2 = generateKey(now().minus(2, DAYS)); + ManagedSecretKey k3 = generateKey(now().minus(3, DAYS)); + ManagedSecretKey k4 = generateKey(now().minus(4, DAYS)); + ManagedSecretKey k5 = generateKey(now().minus(5, DAYS)); + return Stream.of( + // first start + of(ImmutableList.of(), null, null), + + // restart => nothing is filtered + of(newArrayList(k0, k1, k2), k0, newArrayList(k0, k1, k2)), + + // stop 1 day and start + of(newArrayList(k1, k2, k3), k1, newArrayList(k1, k2)), + + // stop 2 day and start => expired keys are filtered + of(newArrayList(k2, k3, k4), k2, newArrayList(k2)), + + // stop 3 day and start, all saved keys are filtered + of(newArrayList(k3, k4, k5), null, null) + ); + } + + /** + * Verify how SecretKeyManager initializes its keys under different scenarios, + * e.g. with or without the present of saved keys. + */ + @ParameterizedTest + @MethodSource("loadSecretKeysTestCases") + public void testLoadSecretKeys(List savedSecretKey, + ManagedSecretKey expectedCurrentKey, + List expectedLoadedKeys) { + when(mockedKeyStore.load()).thenReturn(savedSecretKey); + + SecretKeyManager secretKeyManager = new SecretKeyManagerImpl(mockedKeyStore, + VALIDITY_DURATION, ROTATION_DURATION, ALGORITHM); + + if (expectedCurrentKey != null) { + assertEquals(secretKeyManager.getCurrentKey(), expectedCurrentKey); + Set allKeys = secretKeyManager.getAllKeys(); + assertSameKeys(expectedLoadedKeys, allKeys); + } else { + // expect the current key is newly generated. + assertFalse(savedSecretKey.contains(secretKeyManager.getCurrentKey())); + assertEquals(1, secretKeyManager.getAllKeys().size()); + assertTrue(secretKeyManager.getAllKeys().contains( + secretKeyManager.getCurrentKey())); + } + } + + private static void assertSameKeys(Collection expected, + Collection actual) { + assertEquals(expected.size(), actual.size()); + for (ManagedSecretKey expectedKey : expected) { + assertTrue(actual.contains(expectedKey)); + } + } + + public static Stream rotationTestCases() throws Exception { + ManagedSecretKey k0 = generateKey(now()); + ManagedSecretKey k1 = generateKey(now().minus(1, DAYS)); + ManagedSecretKey k2 = generateKey(now().minus(2, DAYS)); + ManagedSecretKey k3 = generateKey(now().minus(3, DAYS)); + ManagedSecretKey k4 = generateKey(now().minus(4, DAYS)); + return Stream.of( + + // Currentkey is new, not rotate. + of(newHashSet(k0, k1, k2), k0, false, null), + + // Current key just exceeds the rotation period. + of(newHashSet(k1, k2, k3), k1, true, newHashSet(k1, k2)), + + // Current key exceeds the rotation period for a significant time (2d). + of(newHashSet(k2, k3, k4), k2, true, newHashSet(k2)) + ); + } + + /** + * Verify rotation behavior under different scenarios. + */ + @ParameterizedTest + @MethodSource("rotationTestCases") + public void testRotate(Set initialKeys, + ManagedSecretKey initialCurrentKey, + boolean expectRotate, + Set expectedRetainedKeys) throws TimeoutException { + + when(mockedKeyStore.load()).thenReturn(Collections.emptyList()); + SecretKeyManager secretKeyManager = new SecretKeyManagerImpl(mockedKeyStore, + VALIDITY_DURATION, ROTATION_DURATION, ALGORITHM); + + // Set the initial state. + secretKeyManager.updateKeys(initialCurrentKey, initialKeys); + Mockito.reset(mockedKeyStore); + + assertEquals(secretKeyManager.checkAndRotate(), expectRotate); + + if (expectRotate) { + // Verify rotation behavior. + + // 1. A new key is generated as current key. + ManagedSecretKey currentKey = secretKeyManager.getCurrentKey(); + assertNotEquals(initialCurrentKey, currentKey); + assertFalse(initialKeys.contains(currentKey)); + + // 2. keys are correctly rotated, expired ones are excluded. + Set expectedAllKeys = expectedRetainedKeys; + expectedAllKeys.add(currentKey); + assertSameKeys(expectedAllKeys, secretKeyManager.getAllKeys()); + + // 3. All keys are stored. + ArgumentCaptor> storedKeyCaptor = + ArgumentCaptor.forClass(Collection.class); + verify(mockedKeyStore).save(storedKeyCaptor.capture()); + assertSameKeys(expectedAllKeys, storedKeyCaptor.getValue()); + + // 4. The new generated key has correct data. + assertEquals(ALGORITHM, currentKey.getSecretKey().getAlgorithm()); + assertEquals(0, + Duration.between(currentKey.getCreationTime(), now()).toMinutes()); + Instant expectedExpiryTime = now().plus(VALIDITY_DURATION); + assertEquals(0, + Duration.between(currentKey.getExpiryTime(), + expectedExpiryTime).toMinutes()); + } else { + assertEquals(initialCurrentKey, secretKeyManager.getCurrentKey()); + assertSameKeys(initialKeys, secretKeyManager.getAllKeys()); + } + } + + private static ManagedSecretKey generateKey(Instant creationTime) + throws Exception { + KeyGenerator keyGen = KeyGenerator.getInstance(ALGORITHM); + SecretKey secretKey = keyGen.generateKey(); + return new ManagedSecretKey( + UUID.randomUUID(), + creationTime, + creationTime.plus(VALIDITY_DURATION), + secretKey + ); + } + +} From 099d176ab8c77934282e512f5281782402c2207e Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Tue, 17 Jan 2023 09:40:39 -0800 Subject: [PATCH 02/27] HDDS-7734: Implement symmetric SecretKeys lifescycle management in SCM. wip impl of SecretKey replication. --- .../apache/hadoop/hdds/HddsConfigKeys.java | 22 +++ .../symmetric/LocalSecretKeyStore.java | 22 ++- .../security/symmetric/ManagedSecretKey.java | 9 +- .../security/symmetric/SecretKeyConfig.java | 63 +++++++ .../security/symmetric/SecretKeyManager.java | 153 ++++++++++++---- .../symmetric/SecretKeyManagerImpl.java | 166 ------------------ .../security/symmetric/SecretKeyState.java | 52 ++++++ .../symmetric/LocalSecretKeyStoreTest.java | 10 +- .../symmetric/SecretKeyManagerTest.java | 93 +++++++--- .../src/main/proto/SCMRatisProtocol.proto | 1 + .../hadoop/hdds/scm/ha/io/CodecFactory.java | 3 + .../hadoop/hdds/scm/ha/io/ListCodec.java | 6 +- .../hdds/scm/security/ScmSecretKeyState.java | 128 ++++++++++++++ .../scm/security/SecretKeyManagerService.java | 152 ++++++++++++++++ .../hdds/scm/security/SerializableCodec.java | 41 +++++ .../scm/server/StorageContainerManager.java | 6 + 16 files changed, 681 insertions(+), 246 deletions(-) create mode 100644 hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java delete mode 100644 hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerImpl.java create mode 100644 hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java create mode 100644 hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyState.java create mode 100644 hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java create mode 100644 hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SerializableCodec.java diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsConfigKeys.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsConfigKeys.java index cb258dfa74dc..850a4fe0c2eb 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsConfigKeys.java +++ b/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 = + "hdds.secret.key.file.name"; + public static final String HDDS_SECRET_KEY_FILE_DEFAULT = "secret_keys.json"; + + public static final String HDDS_SECRET_KEY_EXPIRY_DURATION = + "hdds.secret.key.expiry.duration"; + public static final String HDDS_SECRET_KEY_EXPIRY_DURATION_DEFAULT = "P7D"; + + public static final String HDDS_SECRET_KEY_ROTATE_DURATION = + "hdds.secret.key.rotate.duration"; + public static final String HDDS_SECRET_KEY_ROTATE_DURATION_DEFAULT = "P1D"; + + public static final String HDDS_SECRET_KEY_ALGORITHM = + "hdds.secret.key.algorithm"; + public static final String HDDS_SECRET_KEY_ALGORITHM_DEFAULT = + "HmacSHA256"; + + public static final String HDDS_SECRET_KEY_ROTATE_CHECK_DURATION = + "hdds.secret.key.rotate.check.duration"; + public static final String HDDS_SECRET_KEY_ROTATE_CHECK_DURATION_DEFAULT + = "PT10M"; + /** * Do not instantiate. */ diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java index 567500eecdff..46543c38b1c3 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java @@ -32,6 +32,8 @@ 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; @@ -41,10 +43,7 @@ import java.io.ObjectOutputStream; import java.nio.file.Files; import java.nio.file.Path; -import java.nio.file.Paths; import java.nio.file.attribute.PosixFilePermission; -import java.security.KeyStore; -import java.time.Duration; import java.time.Instant; import java.util.Collection; import java.util.Collections; @@ -62,11 +61,13 @@ * 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(String keyDirectory, String secretKeyFileName) { - this.secretKeysFile = Paths.get(keyDirectory, secretKeyFileName); + public LocalSecretKeyStore(Path secretKeysFile) { + this.secretKeysFile = secretKeysFile; this.mapper = new ObjectMapper() .registerModule(new JavaTimeModule()) .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false); @@ -82,9 +83,11 @@ public synchronized List load() { try (MappingIterator iterator = reader.readValues(secretKeysFile.toFile())) { List dtos = iterator.readAll(); - return dtos.stream() + List 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); @@ -99,7 +102,8 @@ public synchronized void save(Collection secretKeys) { .map(ManagedSecretKeyDto::new) .collect(toList()); - try (SequenceWriter writer = mapper.writer().writeValues(secretKeysFile.toFile())) { + try (SequenceWriter writer = + mapper.writer().writeValues(secretKeysFile.toFile())) { writer.init(true); for (ManagedSecretKeyDto dto : dtos) { writer.write(dto); @@ -108,12 +112,16 @@ public synchronized void save(Collection secretKeys) { throw new IllegalStateException("Error saving SecretKeys to file " + secretKeysFile, e); } + LOG.info("Saved {} to file {}", secretKeys, secretKeysFile); } private void setFileOwnerPermissions(Path path) { Set permissions = newHashSet(OWNER_READ, OWNER_WRITE); try { if (!Files.exists(path)) { + if (!Files.exists(path.getParent())) { + Files.createDirectories(path.getParent()); + } Files.createFile(path); } Files.setPosixFilePermissions(path, permissions); diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/ManagedSecretKey.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/ManagedSecretKey.java index 715f143ce778..cbb47df27b28 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/ManagedSecretKey.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/ManagedSecretKey.java @@ -19,6 +19,7 @@ package org.apache.hadoop.hdds.security.symmetric; import javax.crypto.SecretKey; +import java.io.Serializable; import java.time.Instant; import java.util.UUID; @@ -26,7 +27,7 @@ * Enclosed a symmetric {@link SecretKey} with additional data for life-cycle * management. */ -public class ManagedSecretKey { +public final class ManagedSecretKey implements Serializable { private final UUID id; private final Instant creationTime; private final Instant expiryTime; @@ -75,4 +76,10 @@ public boolean equals(Object obj) { ManagedSecretKey that = (ManagedSecretKey) obj; return this.id.equals(that.id); } + + @Override + public String toString() { + return "SecretKey(id = " + id + ", creation at: " + + creationTime + ", expire at: " + expiryTime + ")"; + } } diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java new file mode 100644 index 000000000000..3083ed62b44f --- /dev/null +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java @@ -0,0 +1,63 @@ +package org.apache.hadoop.hdds.security.symmetric; + +import org.apache.hadoop.hdds.conf.ConfigurationSource; + +import java.nio.file.Path; +import java.nio.file.Paths; +import java.time.Duration; + +import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_KEY_DIR_NAME; +import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_KEY_DIR_NAME_DEFAULT; +import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_METADATA_DIR_NAME; +import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_SECRET_KEY_ALGORITHM; +import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_SECRET_KEY_ALGORITHM_DEFAULT; +import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_SECRET_KEY_EXPIRY_DURATION; +import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_SECRET_KEY_EXPIRY_DURATION_DEFAULT; +import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_SECRET_KEY_FILE; +import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_SECRET_KEY_FILE_DEFAULT; +import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_SECRET_KEY_ROTATE_DURATION; +import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_SECRET_KEY_ROTATE_DURATION_DEFAULT; +import static org.apache.hadoop.hdds.HddsConfigKeys.OZONE_METADATA_DIRS; + +public class SecretKeyConfig { + private final Path localSecretKeyFile; + private final Duration rotateDuration; + private final Duration expiryDuration; + private final String algorithm; + + public SecretKeyConfig(ConfigurationSource conf, String component) { + String metadataDir = conf.get(HDDS_METADATA_DIR_NAME, + conf.get(OZONE_METADATA_DIRS)); + String keyDir = conf.get(HDDS_KEY_DIR_NAME, HDDS_KEY_DIR_NAME_DEFAULT); + String fileName = conf.get(HDDS_SECRET_KEY_FILE, + HDDS_SECRET_KEY_FILE_DEFAULT); + localSecretKeyFile = Paths.get(metadataDir, component, keyDir, fileName); + + String rotateDuration = conf.get(HDDS_SECRET_KEY_ROTATE_DURATION, + HDDS_SECRET_KEY_ROTATE_DURATION_DEFAULT); + this.rotateDuration = Duration.parse(rotateDuration); + + String expiryDuration = conf.get(HDDS_SECRET_KEY_EXPIRY_DURATION, + HDDS_SECRET_KEY_EXPIRY_DURATION_DEFAULT); + this.expiryDuration = Duration.parse(expiryDuration); + + this.algorithm = conf.get(HDDS_SECRET_KEY_ALGORITHM, + HDDS_SECRET_KEY_ALGORITHM_DEFAULT); + } + + public Path getLocalSecretKeyFile() { + return localSecretKeyFile; + } + + public Duration getRotateDuration() { + return rotateDuration; + } + + public Duration getExpiryDuration() { + return expiryDuration; + } + + public String getAlgorithm() { + return algorithm; + } +} diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java index d85ab97aab91..77d9881a1030 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java @@ -1,59 +1,136 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - *

- * http://www.apache.org/licenses/LICENSE-2.0 - *

- * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - package org.apache.hadoop.hdds.security.symmetric; -import org.apache.hadoop.hdds.scm.metadata.Replicate; +import org.apache.hadoop.hdds.HddsUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; -import javax.annotation.Nonnull; -import java.util.Set; +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 interface SecretKeyManager { +public class SecretKeyManager { + private static final Logger LOG = LoggerFactory.getLogger(SecretKeyManager.class); - /** - * @return the current active key, which is used for signing tokens. - */ - @Nonnull - ManagedSecretKey getCurrentKey(); + private final SecretKeyState state; + 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()); + } /** - * @return all the keys that managed by this manager. + * 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. */ - Set getAllKeys(); + public synchronized void initialize() throws TimeoutException { + if (state.getCurrentKey() != null) { + return; + } + + List 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); + } + + state.updateKeys(currentKey, sortedKeys); + } /** * Check and rotate the keys. * * @return true if rotation actually happens, false if it doesn't. */ - boolean checkAndRotate(); + public synchronized boolean checkAndRotate() throws TimeoutException { + ManagedSecretKey newCurrentKey = null; + List updatedKeys = null; - /** - * Update the SecretKeys. - * This is a short-hand for replicating SecretKeys across all SCM instances - * after each rotation, typically invoked by checkAndRotate - */ - @Replicate - void updateKeys(ManagedSecretKey currentKey, - Set allKeys) throws TimeoutException; + ManagedSecretKey currentKey = state.getCurrentKey(); + if (shouldRotate(currentKey)) { + newCurrentKey = generateSecretKey(); + updatedKeys = state.getAllKeys() + .stream().filter(x -> !x.isExpired()) + .collect(toList()); + updatedKeys.add(newCurrentKey); + } + + if (newCurrentKey != null) { + LOG.info("SecretKey rotation is happening, new key generated {}", + newCurrentKey); + state.updateKeys(newCurrentKey, updatedKeys); + return true; + } + return false; + } + + private boolean shouldRotate(ManagedSecretKey currentKey) { + Duration established = between(currentKey.getCreationTime(), Instant.now()); + return established.compareTo(rotationDuration) >= 0; + } + + private ManagedSecretKey generateSecretKey() { + Instant now = Instant.now(); + return new ManagedSecretKey( + UUID.randomUUID(), + now, + now.plus(validityDuration), + keyGenerator.generateKey() + ); + } + + private KeyGenerator createKeyGenerator(String algorithm) { + try { + return KeyGenerator.getInstance(algorithm); + } catch (NoSuchAlgorithmException e) { + throw new IllegalArgumentException("Error creating KeyGenerator for " + + "algorithm " + algorithm, e); + } + } } diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerImpl.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerImpl.java deleted file mode 100644 index 47d95b24a088..000000000000 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerImpl.java +++ /dev/null @@ -1,166 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - *

- * http://www.apache.org/licenses/LICENSE-2.0 - *

- * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.apache.hadoop.hdds.security.symmetric; - -import javax.annotation.Nonnull; -import javax.crypto.KeyGenerator; -import java.security.NoSuchAlgorithmException; -import java.time.Duration; -import java.time.Instant; -import java.util.HashMap; -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.Set; -import java.util.UUID; -import java.util.concurrent.locks.ReadWriteLock; -import java.util.concurrent.locks.ReentrantReadWriteLock; -import java.util.stream.Collectors; - -import static java.time.Duration.between; -import static java.util.Comparator.comparing; -import static java.util.Objects.requireNonNull; - -public class SecretKeyManagerImpl implements SecretKeyManager { - private final ReadWriteLock lock = new ReentrantReadWriteLock(); - private ManagedSecretKey currentKey; - private final Map allKeys = new HashMap<>(); - - private final SecretKeyStore keyStore; - private final Duration validityDuration; - private final Duration rotationDuration; - - private final KeyGenerator keyGenerator; - - SecretKeyManagerImpl(SecretKeyStore keyStore, Duration validityDuration, - Duration rotationDuration, String algorithm) { - this.keyStore = keyStore; - this.validityDuration = requireNonNull(validityDuration); - this.rotationDuration = requireNonNull(rotationDuration); - this.keyGenerator = createKeyGenerator(algorithm); - - reloadKeys(); - } - - private void reloadKeys() { - List sortedKeys = keyStore.load() - .stream() - .filter(x -> !x.isExpired()) - .sorted(comparing(ManagedSecretKey::getCreationTime)) - .collect(Collectors.toList()); - - if (sortedKeys.isEmpty()) { - // First start, generate new key as the current key. - currentKey = generateSecretKey(); - allKeys.put(currentKey.getId(), currentKey); - } else { - // For restarts, reload allKeys and take the latest one as current. - currentKey = sortedKeys.get(sortedKeys.size() - 1); - sortedKeys.forEach(x -> allKeys.put(x.getId(), x)); - } - } - - @Override - @Nonnull - public ManagedSecretKey getCurrentKey() { - lock.readLock().lock(); - try { - return requireNonNull(currentKey); - } finally { - lock.readLock().unlock(); - } - } - - @Override - public Set getAllKeys() { - lock.readLock().lock(); - try { - return new HashSet<>(allKeys.values()); - } finally { - lock.readLock().unlock(); - } - } - - @Override - public boolean checkAndRotate() { - ManagedSecretKey newCurrentKey = null; - Set updatedKeys = null; - - lock.readLock().lock(); - try { - if (shouldRotate()) { - newCurrentKey = generateSecretKey(); - updatedKeys = allKeys.values() - .stream().filter(x -> !x.isExpired()) - .collect(Collectors.toSet()); - updatedKeys.add(newCurrentKey); - } - } finally { - lock.readLock().unlock(); - } - - if (newCurrentKey != null) { - updateKeys(newCurrentKey, updatedKeys); - return true; - } - - return false; - } - - @Override - public void updateKeys(ManagedSecretKey newCurrentKey, - Set newAllKeys) { - lock.writeLock().lock(); - try { - currentKey = newCurrentKey; - allKeys.clear(); - for (ManagedSecretKey secretKey : newAllKeys) { - allKeys.put(secretKey.getId(), secretKey); - } - keyStore.save(allKeys.values()); - } finally { - lock.writeLock().unlock(); - } - } - - private boolean shouldRotate() { - Duration established = between(currentKey.getCreationTime(), Instant.now()); - return established.compareTo(rotationDuration) >= 0; - } - - private KeyGenerator createKeyGenerator(String algorithm) { - try { - return KeyGenerator.getInstance(algorithm); - } catch (NoSuchAlgorithmException e) { - throw new IllegalArgumentException("Error creating KeyGenerator for " + - "algorithm " + algorithm, e); - } - } - - private ManagedSecretKey generateSecretKey() { - Instant now = Instant.now(); - return new ManagedSecretKey( - UUID.randomUUID(), - now, - now.plus(validityDuration), - keyGenerator.generateKey() - ); - } - -} diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java new file mode 100644 index 000000000000..20507b782a1c --- /dev/null +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java @@ -0,0 +1,52 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.hdds.security.symmetric; + +import org.apache.hadoop.hdds.scm.metadata.Replicate; + +import javax.annotation.Nonnull; +import java.util.Collection; +import java.util.List; +import java.util.Set; +import java.util.concurrent.TimeoutException; + +/** + * This component holds the state of managed SecretKeys, including the + * current key and all active keys. + */ +public interface SecretKeyState { + /** + * @return the current active key, which is used for signing tokens. + */ + ManagedSecretKey getCurrentKey(); + + /** + * @return all the keys that managed by this manager. + */ + Set getAllKeys(); + + /** + * Update the SecretKeys. + * This is a short-hand for replicating SecretKeys across all SCM instances + * after each rotation. + */ + @Replicate + void updateKeys(ManagedSecretKey currentKey, + List allKeys) throws TimeoutException; +} diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java index e0c4545d36dd..4b6d26d7340d 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java @@ -51,14 +51,12 @@ */ public class LocalSecretKeyStoreTest { private SecretKeyStore secretKeyStore; - private Path keyDir; - private String secretKeysFileName = "test.json"; + private Path testSecretFile; @BeforeEach private void setup() throws Exception { - keyDir = Files.createTempDirectory("key-store-test"); - secretKeyStore = new LocalSecretKeyStore(keyDir.toFile().getAbsolutePath(), - secretKeysFileName); + testSecretFile = Files.createTempFile("key-strore-test", ".json"); + secretKeyStore = new LocalSecretKeyStore(testSecretFile); } public static Stream saveAndLoadTestCases() throws Exception { @@ -84,7 +82,7 @@ public void testSaveAndLoad(List keys) throws IOException { // Ensure the intended file exists and is readable and writeable to // file owner only. - File file = new File(keyDir.toFile(), secretKeysFileName); + File file = testSecretFile.toFile(); assertTrue(file.exists()); Set permissions = Files.getPosixFilePermissions(file.toPath()); diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java index 43f4adac0f40..8ab486b82b6d 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java @@ -31,8 +31,9 @@ import java.time.Duration; import java.time.Instant; import java.util.Collection; -import java.util.Collections; +import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.UUID; import java.util.concurrent.TimeoutException; @@ -42,6 +43,8 @@ import static com.google.common.collect.Sets.newHashSet; import static java.time.Instant.now; import static java.time.temporal.ChronoUnit.DAYS; +import static java.util.function.Function.identity; +import static java.util.stream.Collectors.toMap; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotEquals; @@ -51,7 +54,7 @@ import static org.mockito.Mockito.when; /** - * Tests cases for {@link SecretKeyManager} implementation. + * Tests cases for {@link SecretKeyState} implementation. */ public class SecretKeyManagerTest { private final static Duration VALIDITY_DURATION = Duration.ofDays(3); @@ -98,22 +101,26 @@ public static Stream loadSecretKeysTestCases() throws Exception { @MethodSource("loadSecretKeysTestCases") public void testLoadSecretKeys(List savedSecretKey, ManagedSecretKey expectedCurrentKey, - List expectedLoadedKeys) { - when(mockedKeyStore.load()).thenReturn(savedSecretKey); + List expectedLoadedKeys) + throws TimeoutException { + SecretKeyState state = new TestSecretKeyState(mockedKeyStore); + SecretKeyManager lifeCycleManager = + new SecretKeyManager(state, mockedKeyStore, + ROTATION_DURATION, VALIDITY_DURATION, ALGORITHM); - SecretKeyManager secretKeyManager = new SecretKeyManagerImpl(mockedKeyStore, - VALIDITY_DURATION, ROTATION_DURATION, ALGORITHM); + when(mockedKeyStore.load()).thenReturn(savedSecretKey); + lifeCycleManager.initialize(); if (expectedCurrentKey != null) { - assertEquals(secretKeyManager.getCurrentKey(), expectedCurrentKey); - Set allKeys = secretKeyManager.getAllKeys(); + assertEquals(state.getCurrentKey(), expectedCurrentKey); + Set allKeys = state.getAllKeys(); assertSameKeys(expectedLoadedKeys, allKeys); } else { // expect the current key is newly generated. - assertFalse(savedSecretKey.contains(secretKeyManager.getCurrentKey())); - assertEquals(1, secretKeyManager.getAllKeys().size()); - assertTrue(secretKeyManager.getAllKeys().contains( - secretKeyManager.getCurrentKey())); + assertFalse(savedSecretKey.contains(state.getCurrentKey())); + assertEquals(1, state.getAllKeys().size()); + assertTrue(state.getAllKeys().contains( + state.getCurrentKey())); } } @@ -134,13 +141,13 @@ public static Stream rotationTestCases() throws Exception { return Stream.of( // Currentkey is new, not rotate. - of(newHashSet(k0, k1, k2), k0, false, null), + of(newArrayList(k0, k1, k2), k0, false, null), // Current key just exceeds the rotation period. - of(newHashSet(k1, k2, k3), k1, true, newHashSet(k1, k2)), + of(newArrayList(k1, k2, k3), k1, true, newArrayList(k1, k2)), // Current key exceeds the rotation period for a significant time (2d). - of(newHashSet(k2, k3, k4), k2, true, newHashSet(k2)) + of(newArrayList(k2, k3, k4), k2, true, newArrayList(k2)) ); } @@ -149,33 +156,36 @@ public static Stream rotationTestCases() throws Exception { */ @ParameterizedTest @MethodSource("rotationTestCases") - public void testRotate(Set initialKeys, + public void testRotate(List initialKeys, ManagedSecretKey initialCurrentKey, boolean expectRotate, - Set expectedRetainedKeys) throws TimeoutException { + List expectedRetainedKeys) + throws TimeoutException { - when(mockedKeyStore.load()).thenReturn(Collections.emptyList()); - SecretKeyManager secretKeyManager = new SecretKeyManagerImpl(mockedKeyStore, - VALIDITY_DURATION, ROTATION_DURATION, ALGORITHM); + SecretKeyState state = new TestSecretKeyState(mockedKeyStore); + + SecretKeyManager lifeCycleManager = + new SecretKeyManager(state, mockedKeyStore, + ROTATION_DURATION, VALIDITY_DURATION, ALGORITHM); // Set the initial state. - secretKeyManager.updateKeys(initialCurrentKey, initialKeys); + state.updateKeys(initialCurrentKey, initialKeys); Mockito.reset(mockedKeyStore); - assertEquals(secretKeyManager.checkAndRotate(), expectRotate); + assertEquals(lifeCycleManager.checkAndRotate(), expectRotate); if (expectRotate) { // Verify rotation behavior. // 1. A new key is generated as current key. - ManagedSecretKey currentKey = secretKeyManager.getCurrentKey(); + ManagedSecretKey currentKey = state.getCurrentKey(); assertNotEquals(initialCurrentKey, currentKey); assertFalse(initialKeys.contains(currentKey)); // 2. keys are correctly rotated, expired ones are excluded. - Set expectedAllKeys = expectedRetainedKeys; + List expectedAllKeys = expectedRetainedKeys; expectedAllKeys.add(currentKey); - assertSameKeys(expectedAllKeys, secretKeyManager.getAllKeys()); + assertSameKeys(expectedAllKeys, state.getAllKeys()); // 3. All keys are stored. ArgumentCaptor> storedKeyCaptor = @@ -192,8 +202,8 @@ public void testRotate(Set initialKeys, Duration.between(currentKey.getExpiryTime(), expectedExpiryTime).toMinutes()); } else { - assertEquals(initialCurrentKey, secretKeyManager.getCurrentKey()); - assertSameKeys(initialKeys, secretKeyManager.getAllKeys()); + assertEquals(initialCurrentKey, state.getCurrentKey()); + assertSameKeys(initialKeys, state.getAllKeys()); } } @@ -209,4 +219,33 @@ private static ManagedSecretKey generateKey(Instant creationTime) ); } + private static class TestSecretKeyState implements SecretKeyState { + private ManagedSecretKey currentKey; + private Map allKeys; + private SecretKeyStore keyStore; + + public TestSecretKeyState(SecretKeyStore keyStore) { + this.keyStore = keyStore; + } + + @Override + public ManagedSecretKey getCurrentKey() { + return currentKey; + } + + @Override + public Set getAllKeys() { + return new HashSet<>(allKeys.values()); + } + + @Override + public void updateKeys(ManagedSecretKey currentKey, + List allKeys) { + this.currentKey = currentKey; + this.allKeys = allKeys.stream().collect( + toMap(ManagedSecretKey::getId, identity())); + keyStore.save(allKeys); + } + } + } diff --git a/hadoop-hdds/interface-server/src/main/proto/SCMRatisProtocol.proto b/hadoop-hdds/interface-server/src/main/proto/SCMRatisProtocol.proto index 41da6a5468f8..4fb0737b3925 100644 --- a/hadoop-hdds/interface-server/src/main/proto/SCMRatisProtocol.proto +++ b/hadoop-hdds/interface-server/src/main/proto/SCMRatisProtocol.proto @@ -29,6 +29,7 @@ enum RequestType { MOVE = 6; STATEFUL_SERVICE_CONFIG = 7; FINALIZE = 8; + SECRET_KEY = 9; } message Method { diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/CodecFactory.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/CodecFactory.java index 6c75593be14c..5e8e9b19bc8c 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/CodecFactory.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/CodecFactory.java @@ -23,6 +23,8 @@ import com.google.protobuf.ProtocolMessageEnum; import org.apache.commons.lang3.ClassUtils; +import org.apache.hadoop.hdds.scm.security.SerializableCodec; +import org.apache.hadoop.hdds.security.symmetric.ManagedSecretKey; import java.math.BigInteger; import java.security.cert.X509Certificate; @@ -49,6 +51,7 @@ public final class CodecFactory { codecs.put(BigInteger.class, new BigIntegerCodec()); codecs.put(X509Certificate.class, new X509CertificateCodec()); codecs.put(ByteString.class, new ByteStringCodec()); + codecs.put(ManagedSecretKey.class, new SerializableCodec()); } private CodecFactory() { } diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ListCodec.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ListCodec.java index 0667b8776f11..67d8d5522794 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ListCodec.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ListCodec.java @@ -23,6 +23,7 @@ import org.apache.hadoop.hdds.scm.ha.ReflectionUtil; import java.lang.reflect.InvocationTargetException; +import java.util.ArrayList; import java.util.List; /** @@ -51,8 +52,11 @@ public ByteString serialize(Object object) public Object deserialize(Class type, ByteString value) throws InvalidProtocolBufferException { try { + // If argument type is the generic interface, then determine a + // concrete implementation. + Class concreteType = (type == List.class) ? ArrayList.class : type; - List result = (List) type.newInstance(); + List result = (List) concreteType.newInstance(); final ListArgument listArgs = (ListArgument) ReflectionUtil .getMethod(ListArgument.class, "parseFrom", byte[].class) .invoke(null, (Object) value.toByteArray()); diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyState.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyState.java new file mode 100644 index 000000000000..ead5512bf70b --- /dev/null +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyState.java @@ -0,0 +1,128 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.hdds.scm.security; + +import org.apache.hadoop.hdds.protocol.proto.SCMRatisProtocol; +import org.apache.hadoop.hdds.scm.ha.SCMHAInvocationHandler; +import org.apache.hadoop.hdds.scm.ha.SCMRatisServer; +import org.apache.hadoop.hdds.security.symmetric.ManagedSecretKey; +import org.apache.hadoop.hdds.security.symmetric.SecretKeyState; +import org.apache.hadoop.hdds.security.symmetric.SecretKeyStore; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import javax.annotation.Nonnull; +import java.lang.reflect.Proxy; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.UUID; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; + +import static java.util.Objects.requireNonNull; + +public class ScmSecretKeyState implements SecretKeyState { + private static final Logger LOG = + LoggerFactory.getLogger(ScmSecretKeyState.class); + + private final ReadWriteLock lock = new ReentrantReadWriteLock(); + private ManagedSecretKey currentKey; + private final Map allKeys = new HashMap<>(); + + private final SecretKeyStore keyStore; + + + private ScmSecretKeyState(SecretKeyStore keyStore) { + this.keyStore = requireNonNull(keyStore); + } + + @Override + @Nonnull + public ManagedSecretKey getCurrentKey() { + lock.readLock().lock(); + try { + return currentKey; + } finally { + lock.readLock().unlock(); + } + } + + @Override + public Set getAllKeys() { + lock.readLock().lock(); + try { + return new HashSet<>(allKeys.values()); + } finally { + lock.readLock().unlock(); + } + } + + @Override + public void updateKeys(ManagedSecretKey newCurrentKey, + List newAllKeys) { + LOG.info("Updating keys with currentKey={}, all keys={}", newCurrentKey, + newAllKeys); + lock.writeLock().lock(); + try { + currentKey = newCurrentKey; + allKeys.clear(); + for (ManagedSecretKey secretKey : newAllKeys) { + allKeys.put(secretKey.getId(), secretKey); + } + keyStore.save(allKeys.values()); + } finally { + lock.writeLock().unlock(); + } + } + + public static class Builder { + + private SecretKeyStore secretKeyStore; + private SCMRatisServer scmRatisServer; + + + public Builder setSecretKeyStore(SecretKeyStore secretKeyStore) { + this.secretKeyStore = secretKeyStore; + return this; + } + + public Builder setRatisServer(final SCMRatisServer ratisServer) { + scmRatisServer = ratisServer; + return this; + } + + public SecretKeyState build() { + final ScmSecretKeyState impl = + new ScmSecretKeyState(secretKeyStore); + + final SCMHAInvocationHandler scmhaInvocationHandler = + new SCMHAInvocationHandler(SCMRatisProtocol.RequestType.SECRET_KEY, + impl, scmRatisServer); + + return (SecretKeyState) Proxy.newProxyInstance( + SCMHAInvocationHandler.class.getClassLoader(), + new Class[]{SecretKeyState.class}, scmhaInvocationHandler); + + } + } + +} diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java new file mode 100644 index 000000000000..15ea3a4647f6 --- /dev/null +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java @@ -0,0 +1,152 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package org.apache.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.scm.ha.SCMServiceException; +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.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 SecretKet life-cycle. + */ +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 Duration rotationCheckDuration; + + private 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); + } + + @Override + public void notifyStatusChanged() { + serviceLock.lock(); + try { + if (scmContext.isLeaderReady()) { + try { + secretKeyManager.initialize(); + } catch (TimeoutException e) { + // todo: handle properly + LOG.error("Timeout updating SecretKeys to raft.", e); + } + 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() { + try { + boolean rotated = secretKeyManager.checkAndRotate(); + if (rotated) { + LOG.info("SecretKeys have been updated."); + } else { + LOG.info("SecretKeys have not been updated."); + } + } catch (TimeoutException e) { + LOG.error("Error occurred when updating SecretKeys", e); + } + } + + @Override + public String getServiceName() { + return SecretKeyManagerService.class.getSimpleName(); + } + + @Override + public void start() throws SCMServiceException { + scheduler.scheduleAtFixedRate(this,0, rotationCheckDuration.toMillis(), + TimeUnit.MILLISECONDS); + } + + @Override + public void stop() { + scheduler.shutdownNow(); + } +} diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SerializableCodec.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SerializableCodec.java new file mode 100644 index 000000000000..368c48155f9b --- /dev/null +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SerializableCodec.java @@ -0,0 +1,41 @@ +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; + +public class SerializableCodec implements Codec { + @Override + public ByteString serialize(Object object) + throws InvalidProtocolBufferException { + try { + ByteArrayOutputStream bos = new ByteArrayOutputStream(); + ObjectOutputStream oos = new ObjectOutputStream(bos); + oos.writeObject(object); + oos.flush(); + return ByteString.copyFrom(bos.toByteArray()); + } catch (IOException e) { + throw new RuntimeException("Error serializing " + + object.getClass().getCanonicalName(), e); + } + } + + @Override + public Object deserialize(Class type, ByteString value) + throws InvalidProtocolBufferException { + try { + ByteArrayInputStream bis = new ByteArrayInputStream(value.toByteArray()); + ObjectInputStream ois = new ObjectInputStream(bis); + return ois.readObject(); + } catch (IOException | ClassNotFoundException e) { + throw new RuntimeException("Error deserializing " + + type.getCanonicalName(), e); + } + } +} diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java index f5ceeb0295bb..facf2f27f7d3 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java @@ -62,6 +62,7 @@ import org.apache.hadoop.hdds.scm.ha.SequenceIdGenerator; import org.apache.hadoop.hdds.scm.ScmInfo; import org.apache.hadoop.hdds.scm.node.NodeAddressUpdateHandler; +import org.apache.hadoop.hdds.scm.security.SecretKeyManagerService; import org.apache.hadoop.hdds.scm.server.upgrade.FinalizationManager; import org.apache.hadoop.hdds.scm.server.upgrade.FinalizationManagerImpl; import org.apache.hadoop.hdds.scm.node.CommandQueueReportHandler; @@ -716,6 +717,11 @@ private void initializeSystemManagers(OzoneConfiguration conf, serviceManager.register(expiredContainerReplicaOpScrubber); + SecretKeyManagerService secretKeyManagerService = + new SecretKeyManagerService(scmContext, conf, + scmHAManager.getRatisServer()); + serviceManager.register(secretKeyManagerService); + if (configurator.getContainerManager() != null) { containerManager = configurator.getContainerManager(); } else { From 76a69e98b7e06dd6ecaa8b5014d5417daede65c1 Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Mon, 23 Jan 2023 16:32:09 -0800 Subject: [PATCH 03/27] Update WIP. --- .../src/main/resources/ozone-default.xml | 45 +++++++++++++++++++ .../security/symmetric/SecretKeyConfig.java | 21 +++++++++ .../security/symmetric/SecretKeyManager.java | 18 ++++---- .../security/symmetric/SecretKeyState.java | 16 +++++-- .../symmetric/SecretKeyManagerTest.java | 14 +++++- .../hdds/scm/security/ScmSecretKeyState.java | 16 +++++++ .../scm/security/SecretKeyManagerService.java | 16 +++++-- 7 files changed, 128 insertions(+), 18 deletions(-) diff --git a/hadoop-hdds/common/src/main/resources/ozone-default.xml b/hadoop-hdds/common/src/main/resources/ozone-default.xml index d26da2a2ca0d..dde6ce9f3faa 100644 --- a/hadoop-hdds/common/src/main/resources/ozone-default.xml +++ b/hadoop-hdds/common/src/main/resources/ozone-default.xml @@ -3602,4 +3602,49 @@ history from compaction DAG. Uses millisecond by default when no time unit is specified. + + hdds.secret.key.file.name + secret_keys.json + SCM, SECURITY + + Name of file which stores symmetric secret keys for token signatures. + + + + hdds.secret.key.expiry.duration + P7D + SCM, SECURITY + + 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 + + + + hdds.secret.key.rotate.duration + P1D + SCM, SECURITY + + The duration that SCM periodically generate a new symmetric secret keys. + The formats accepted are based on the ISO-8601 duration format PnDTnHnMn.nS + + + + hdds.secret.key.rotate.check.duration + PT10M + SCM, SECURITY + + 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 + + + + hdds.secret.key.algorithm + HmacSHA256 + SCM, SECURITY + + The algorithm that SCM uses to generate symmetric secret keys. + The formats accepted are based on the ISO-8601 duration format PnDTnHnMn.nS + + diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java index 3083ed62b44f..c29096517c99 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java @@ -1,3 +1,21 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package org.apache.hadoop.hdds.security.symmetric; import org.apache.hadoop.hdds.conf.ConfigurationSource; @@ -19,6 +37,9 @@ import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_SECRET_KEY_ROTATE_DURATION_DEFAULT; import static org.apache.hadoop.hdds.HddsConfigKeys.OZONE_METADATA_DIRS; +/** + * Configurations related to SecretKeys lifecycle management. + */ public class SecretKeyConfig { private final Path localSecretKeyFile; private final Duration rotateDuration; diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java index 77d9881a1030..45ac1041881c 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java @@ -22,7 +22,8 @@ * rotation and destruction. */ public class SecretKeyManager { - private static final Logger LOG = LoggerFactory.getLogger(SecretKeyManager.class); + private static final Logger LOG = + LoggerFactory.getLogger(SecretKeyManager.class); private final SecretKeyState state; private final Duration rotationDuration; @@ -80,6 +81,10 @@ public synchronized void initialize() throws TimeoutException { sortedKeys); } + // First, update the SecretKey state to make it visible immediately on the + // current instance. + state.updateKeysInternal(currentKey, sortedKeys); + // Then replicate the SecretKey state in all instances. state.updateKeys(currentKey, sortedKeys); } @@ -89,23 +94,18 @@ public synchronized void initialize() throws TimeoutException { * @return true if rotation actually happens, false if it doesn't. */ public synchronized boolean checkAndRotate() throws TimeoutException { - ManagedSecretKey newCurrentKey = null; - List updatedKeys = null; - ManagedSecretKey currentKey = state.getCurrentKey(); if (shouldRotate(currentKey)) { - newCurrentKey = generateSecretKey(); - updatedKeys = state.getAllKeys() + + ManagedSecretKey newCurrentKey = generateSecretKey(); + List updatedKeys = state.getAllKeys() .stream().filter(x -> !x.isExpired()) .collect(toList()); updatedKeys.add(newCurrentKey); - } - if (newCurrentKey != null) { LOG.info("SecretKey rotation is happening, new key generated {}", newCurrentKey); state.updateKeys(newCurrentKey, updatedKeys); - return true; } return false; } diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java index 20507b782a1c..b8b62e7a9d27 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java @@ -20,10 +20,9 @@ import org.apache.hadoop.hdds.scm.metadata.Replicate; -import javax.annotation.Nonnull; -import java.util.Collection; import java.util.List; import java.util.Set; +import java.util.UUID; import java.util.concurrent.TimeoutException; /** @@ -41,12 +40,23 @@ public interface SecretKeyState { */ Set getAllKeys(); + /** + * Get SecretKey by id. + */ + ManagedSecretKey getKeyById(UUID id); + /** * Update the SecretKeys. - * This is a short-hand for replicating SecretKeys across all SCM instances + * This method replicates SecretKeys across all SCM instances * after each rotation. */ @Replicate void updateKeys(ManagedSecretKey currentKey, List allKeys) throws TimeoutException; + + /** + * Update the SecretKeys on this instance only. + */ + void updateKeysInternal(ManagedSecretKey currentKey, + List allKeys); } diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java index 8ab486b82b6d..c7828c21dc3f 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java @@ -40,7 +40,6 @@ import java.util.stream.Stream; import static com.google.common.collect.Lists.newArrayList; -import static com.google.common.collect.Sets.newHashSet; import static java.time.Instant.now; import static java.time.temporal.ChronoUnit.DAYS; import static java.util.function.Function.identity; @@ -54,7 +53,7 @@ import static org.mockito.Mockito.when; /** - * Tests cases for {@link SecretKeyState} implementation. + * Tests cases for {@link SecretKeyManager} implementation. */ public class SecretKeyManagerTest { private final static Duration VALIDITY_DURATION = Duration.ofDays(3); @@ -238,6 +237,11 @@ public Set getAllKeys() { return new HashSet<>(allKeys.values()); } + @Override + public ManagedSecretKey getKeyById(UUID id) { + return allKeys.get(id); + } + @Override public void updateKeys(ManagedSecretKey currentKey, List allKeys) { @@ -246,6 +250,12 @@ public void updateKeys(ManagedSecretKey currentKey, toMap(ManagedSecretKey::getId, identity())); keyStore.save(allKeys); } + + @Override + public void updateKeysInternal(ManagedSecretKey currentKey, + List sortedKeys) { + updateKeys(currentKey, sortedKeys); + } } } diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyState.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyState.java index ead5512bf70b..a8b23a7f05e4 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyState.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyState.java @@ -76,9 +76,25 @@ public Set getAllKeys() { } } + @Override + public ManagedSecretKey getKeyById(UUID id) { + lock.readLock().lock(); + try { + return allKeys.get(id); + } finally { + lock.readLock().unlock(); + } + } + @Override public void updateKeys(ManagedSecretKey newCurrentKey, List newAllKeys) { + updateKeysInternal(newCurrentKey, newAllKeys); + } + + @Override + public void updateKeysInternal(ManagedSecretKey newCurrentKey, + List newAllKeys) { LOG.info("Updating keys with currentKey={}, all keys={}", newCurrentKey, newAllKeys); lock.writeLock().lock(); diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java index 15ea3a4647f6..fcd8bf0a6443 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java @@ -43,7 +43,7 @@ import static org.apache.hadoop.ozone.OzoneConsts.SCM_CA_CERT_STORAGE_DIR; /** - * A background service running in SCM to maintain the SecretKet life-cycle. + * A background service running in SCM to maintain the SecretKeys lifecycle. */ public class SecretKeyManagerService implements SCMService, Runnable { public static final Logger LOG = @@ -58,9 +58,9 @@ public class SecretKeyManagerService implements SCMService, Runnable { */ private final Lock serviceLock = new ReentrantLock(); private ServiceStatus serviceStatus = ServiceStatus.PAUSING; - private Duration rotationCheckDuration; - private ScheduledExecutorService scheduler; + private final Duration rotationCheckDuration; + private final ScheduledExecutorService scheduler; @SuppressWarnings("parameternumber") public SecretKeyManagerService(SCMContext scmContext, @@ -88,6 +88,8 @@ public SecretKeyManagerService(SCMContext scmContext, HDDS_SECRET_KEY_ROTATE_CHECK_DURATION, HDDS_SECRET_KEY_ROTATE_CHECK_DURATION_DEFAULT); rotationCheckDuration = Duration.parse(rotationCheckDurationStr); + + start(); } @Override @@ -122,6 +124,10 @@ public boolean shouldRun() { @Override public void run() { + if (!shouldRun()) { + return; + } + try { boolean rotated = secretKeyManager.checkAndRotate(); if (rotated) { @@ -140,7 +146,9 @@ public String getServiceName() { } @Override - public void start() throws SCMServiceException { + public void start() { + LOG.info("Scheduling rotation checker with interval {} seconds", + rotationCheckDuration.toMillis() / 1000); scheduler.scheduleAtFixedRate(this,0, rotationCheckDuration.toMillis(), TimeUnit.MILLISECONDS); } From 4e6bce4756637c6740ffd9000682cede909c1e10 Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Mon, 23 Jan 2023 16:59:47 -0800 Subject: [PATCH 04/27] Enable SecretKeyManager when security and block & container token is enabled. --- .../hdds/security/symmetric/SecretKeyManager.java | 1 - .../hdds/scm/security/SecretKeyManagerService.java | 13 +++++++++---- .../hdds/scm/server/StorageContainerManager.java | 12 ++++++++---- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java index 45ac1041881c..f43680caa750 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java @@ -96,7 +96,6 @@ public synchronized void initialize() throws TimeoutException { public synchronized boolean checkAndRotate() throws TimeoutException { ManagedSecretKey currentKey = state.getCurrentKey(); if (shouldRotate(currentKey)) { - ManagedSecretKey newCurrentKey = generateSecretKey(); List updatedKeys = state.getAllKeys() .stream().filter(x -> !x.isExpired()) diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java index fcd8bf0a6443..d03524bb3f28 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java @@ -5,9 +5,9 @@ * licenses this file to you under the Apache License, Version 2.0 (the * "License"); you may not use this file except in compliance with the License. * You may obtain a copy of the License at - * + *

* http://www.apache.org/licenses/LICENSE-2.0 - * + *

* Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the @@ -21,12 +21,12 @@ 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.scm.ha.SCMServiceException; 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; @@ -149,7 +149,7 @@ public String getServiceName() { public void start() { LOG.info("Scheduling rotation checker with interval {} seconds", rotationCheckDuration.toMillis() / 1000); - scheduler.scheduleAtFixedRate(this,0, rotationCheckDuration.toMillis(), + scheduler.scheduleAtFixedRate(this, 0, rotationCheckDuration.toMillis(), TimeUnit.MILLISECONDS); } @@ -157,4 +157,9 @@ public void start() { public void stop() { scheduler.shutdownNow(); } + + public static boolean isSecretKeyEnable(SecurityConfig conf) { + return conf.isSecurityEnabled() && + conf.isBlockTokenEnabled() && conf.isContainerTokenEnabled(); + } } diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java index facf2f27f7d3..2535e21dcfb0 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java @@ -182,6 +182,7 @@ import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_EVENT_REPORT_EXEC_WAIT_THRESHOLD_DEFAULT; import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_EVENT_REPORT_QUEUE_WAIT_THRESHOLD_DEFAULT; +import static org.apache.hadoop.hdds.scm.security.SecretKeyManagerService.isSecretKeyEnable; import static org.apache.hadoop.hdds.security.x509.certificate.authority.CertificateStore.CertType.VALID_CERTS; import static org.apache.hadoop.ozone.OzoneConsts.CRL_SEQUENCE_ID_KEY; import static org.apache.hadoop.ozone.OzoneConsts.SCM_SUB_CA_PREFIX; @@ -717,10 +718,13 @@ private void initializeSystemManagers(OzoneConfiguration conf, serviceManager.register(expiredContainerReplicaOpScrubber); - SecretKeyManagerService secretKeyManagerService = - new SecretKeyManagerService(scmContext, conf, - scmHAManager.getRatisServer()); - serviceManager.register(secretKeyManagerService); + if (isSecretKeyEnable(securityConfig)) { + LOG.info("Enabling SecretKeys management."); + SecretKeyManagerService secretKeyManagerService = + new SecretKeyManagerService(scmContext, conf, + scmHAManager.getRatisServer()); + serviceManager.register(secretKeyManagerService); + } if (configurator.getContainerManager() != null) { containerManager = configurator.getContainerManager(); From f891cbdf3c0c0c1c6b433567bcec09c53552f2c4 Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Mon, 23 Jan 2023 17:11:55 -0800 Subject: [PATCH 05/27] Correct condition to enable symetric secret keys --- .../hadoop/hdds/scm/security/SecretKeyManagerService.java | 2 +- .../apache/hadoop/hdds/scm/server/StorageContainerManager.java | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java index d03524bb3f28..5558b8eec526 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java @@ -160,6 +160,6 @@ public void stop() { public static boolean isSecretKeyEnable(SecurityConfig conf) { return conf.isSecurityEnabled() && - conf.isBlockTokenEnabled() && conf.isContainerTokenEnabled(); + (conf.isBlockTokenEnabled() || conf.isContainerTokenEnabled()); } } diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java index 2535e21dcfb0..736156dcc46c 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java @@ -719,7 +719,6 @@ private void initializeSystemManagers(OzoneConfiguration conf, serviceManager.register(expiredContainerReplicaOpScrubber); if (isSecretKeyEnable(securityConfig)) { - LOG.info("Enabling SecretKeys management."); SecretKeyManagerService secretKeyManagerService = new SecretKeyManagerService(scmContext, conf, scmHAManager.getRatisServer()); From 3d544ac92f2f83bced4f70c583e729e45d148437 Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Tue, 24 Jan 2023 16:47:51 -0800 Subject: [PATCH 06/27] Fix deadlock in 1st leader election. --- .../security/symmetric/SecretKeyManager.java | 21 ++++++++++++++----- .../security/symmetric/SecretKeyState.java | 2 +- .../symmetric/SecretKeyManagerTest.java | 9 ++++---- .../hdds/scm/security/ScmSecretKeyState.java | 7 +++---- .../scm/security/SecretKeyManagerService.java | 18 +++++++++++----- 5 files changed, 37 insertions(+), 20 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java index f43680caa750..dfaeef4414a4 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java @@ -1,6 +1,5 @@ package org.apache.hadoop.hdds.security.symmetric; -import org.apache.hadoop.hdds.HddsUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -26,6 +25,7 @@ public class SecretKeyManager { LoggerFactory.getLogger(SecretKeyManager.class); private final SecretKeyState state; + private boolean pendingInititializedState = false; private final Duration rotationDuration; private final Duration validityDuration; private final SecretKeyStore keyStore; @@ -57,9 +57,9 @@ public SecretKeyManager(SecretKeyState state, * * @throws TimeoutException can possibly occur when replicating the state. */ - public synchronized void initialize() throws TimeoutException { + public synchronized boolean initialize() { if (state.getCurrentKey() != null) { - return; + return false; } List sortedKeys = keyStore.load() @@ -84,8 +84,17 @@ public synchronized void initialize() throws TimeoutException { // First, update the SecretKey state to make it visible immediately on the // current instance. state.updateKeysInternal(currentKey, sortedKeys); - // Then replicate the SecretKey state in all instances. - state.updateKeys(currentKey, sortedKeys); + // Then, remember to replicate SecretKey states to all instances. + pendingInititializedState = true; + return true; + } + + public synchronized void flushInitializedState() throws TimeoutException { + if (pendingInititializedState) { + LOG.info("Replicating initialized state."); + state.updateKeys(state.getCurrentKey(), state.getAllKeys()); + pendingInititializedState = false; + } } /** @@ -94,6 +103,8 @@ public synchronized void initialize() throws TimeoutException { * @return true if rotation actually happens, false if it doesn't. */ public synchronized boolean checkAndRotate() throws TimeoutException { + flushInitializedState(); + ManagedSecretKey currentKey = state.getCurrentKey(); if (shouldRotate(currentKey)) { ManagedSecretKey newCurrentKey = generateSecretKey(); diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java index b8b62e7a9d27..a108c81e470b 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java @@ -38,7 +38,7 @@ public interface SecretKeyState { /** * @return all the keys that managed by this manager. */ - Set getAllKeys(); + List getAllKeys(); /** * Get SecretKey by id. diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java index c7828c21dc3f..fa939e6500eb 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java @@ -30,11 +30,10 @@ import javax.crypto.SecretKey; import java.time.Duration; import java.time.Instant; +import java.util.ArrayList; import java.util.Collection; -import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.UUID; import java.util.concurrent.TimeoutException; import java.util.stream.Stream; @@ -112,7 +111,7 @@ public void testLoadSecretKeys(List savedSecretKey, if (expectedCurrentKey != null) { assertEquals(state.getCurrentKey(), expectedCurrentKey); - Set allKeys = state.getAllKeys(); + List allKeys = state.getAllKeys(); assertSameKeys(expectedLoadedKeys, allKeys); } else { // expect the current key is newly generated. @@ -233,8 +232,8 @@ public ManagedSecretKey getCurrentKey() { } @Override - public Set getAllKeys() { - return new HashSet<>(allKeys.values()); + public List getAllKeys() { + return new ArrayList<>(allKeys.values()); } @Override diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyState.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyState.java index a8b23a7f05e4..38e08c2b0655 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyState.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyState.java @@ -29,11 +29,10 @@ import javax.annotation.Nonnull; import java.lang.reflect.Proxy; +import java.util.ArrayList; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.UUID; import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; @@ -67,10 +66,10 @@ public ManagedSecretKey getCurrentKey() { } @Override - public Set getAllKeys() { + public List getAllKeys() { lock.readLock().lock(); try { - return new HashSet<>(allKeys.values()); + return new ArrayList<>(allKeys.values()); } finally { lock.readLock().unlock(); } diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java index 5558b8eec526..3c4070589df0 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java @@ -97,12 +97,20 @@ public void notifyStatusChanged() { serviceLock.lock(); try { if (scmContext.isLeaderReady()) { - try { - secretKeyManager.initialize(); - } catch (TimeoutException e) { - // todo: handle properly - LOG.error("Timeout updating SecretKeys to raft.", e); + + // 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( + "Timeout replicating initialized state.", e); + } + }, 0, TimeUnit.SECONDS); } + serviceStatus = ServiceStatus.RUNNING; } else { serviceStatus = ServiceStatus.PAUSING; From 0d902daceab2956c427a6cc5f8869393a4fa52a3 Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Wed, 25 Jan 2023 10:19:41 -0800 Subject: [PATCH 07/27] Fix checkstyle. --- .../symmetric/LocalSecretKeyStore.java | 3 ++- .../security/symmetric/SecretKeyConfig.java | 8 +++---- .../security/symmetric/SecretKeyManager.java | 18 ++++++++++++++ .../security/symmetric/SecretKeyState.java | 1 - .../symmetric/LocalSecretKeyStoreTest.java | 2 +- .../symmetric/SecretKeyManagerTest.java | 24 +++++++++---------- .../hdds/scm/security/ScmSecretKeyState.java | 8 ++++++- .../hdds/scm/security/SerializableCodec.java | 21 ++++++++++++++++ 8 files changed, 65 insertions(+), 20 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java index 46543c38b1c3..cfc5872c452e 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java @@ -61,7 +61,8 @@ * JSON file on local file system. */ public class LocalSecretKeyStore implements SecretKeyStore { - private static final Logger LOG = LoggerFactory.getLogger(LocalSecretKeyStore.class); + private static final Logger LOG = + LoggerFactory.getLogger(LocalSecretKeyStore.class); private final Path secretKeysFile; private final ObjectMapper mapper; diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java index c29096517c99..cbca9dc2b2a1 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java @@ -54,13 +54,13 @@ public SecretKeyConfig(ConfigurationSource conf, String component) { HDDS_SECRET_KEY_FILE_DEFAULT); localSecretKeyFile = Paths.get(metadataDir, component, keyDir, fileName); - String rotateDuration = conf.get(HDDS_SECRET_KEY_ROTATE_DURATION, + String rotateDurationStr = conf.get(HDDS_SECRET_KEY_ROTATE_DURATION, HDDS_SECRET_KEY_ROTATE_DURATION_DEFAULT); - this.rotateDuration = Duration.parse(rotateDuration); + this.rotateDuration = Duration.parse(rotateDurationStr); - String expiryDuration = conf.get(HDDS_SECRET_KEY_EXPIRY_DURATION, + String expiryDurationStr = conf.get(HDDS_SECRET_KEY_EXPIRY_DURATION, HDDS_SECRET_KEY_EXPIRY_DURATION_DEFAULT); - this.expiryDuration = Duration.parse(expiryDuration); + this.expiryDuration = Duration.parse(expiryDurationStr); this.algorithm = conf.get(HDDS_SECRET_KEY_ALGORITHM, HDDS_SECRET_KEY_ALGORITHM_DEFAULT); diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java index dfaeef4414a4..3ee764ff9ddf 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java @@ -1,3 +1,21 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package org.apache.hadoop.hdds.security.symmetric; import org.slf4j.Logger; diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java index a108c81e470b..18b92dd41be6 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java @@ -21,7 +21,6 @@ import org.apache.hadoop.hdds.scm.metadata.Replicate; import java.util.List; -import java.util.Set; import java.util.UUID; import java.util.concurrent.TimeoutException; diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java index 4b6d26d7340d..194e38cfe7b9 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java @@ -47,7 +47,7 @@ import static org.junit.jupiter.api.Assertions.assertTrue; /** - * Test cases for {@link LocalSecretKeyStore} + * Test cases for {@link LocalSecretKeyStore}. */ public class LocalSecretKeyStoreTest { private SecretKeyStore secretKeyStore; diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java index fa939e6500eb..c2991dc59bf2 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java @@ -55,9 +55,9 @@ * Tests cases for {@link SecretKeyManager} implementation. */ public class SecretKeyManagerTest { - private final static Duration VALIDITY_DURATION = Duration.ofDays(3); - private final static Duration ROTATION_DURATION = Duration.ofDays(1); - private final static String ALGORITHM = "HmacSHA256"; + private static final Duration VALIDITY_DURATION = Duration.ofDays(3); + private static final Duration ROTATION_DURATION = Duration.ofDays(1); + private static final String ALGORITHM = "HmacSHA256"; private SecretKeyStore mockedKeyStore; @@ -222,7 +222,7 @@ private static class TestSecretKeyState implements SecretKeyState { private Map allKeys; private SecretKeyStore keyStore; - public TestSecretKeyState(SecretKeyStore keyStore) { + TestSecretKeyState(SecretKeyStore keyStore) { this.keyStore = keyStore; } @@ -242,18 +242,18 @@ public ManagedSecretKey getKeyById(UUID id) { } @Override - public void updateKeys(ManagedSecretKey currentKey, - List allKeys) { - this.currentKey = currentKey; - this.allKeys = allKeys.stream().collect( + public void updateKeys(ManagedSecretKey newCurrentKey, + List newAllKeys) { + this.currentKey = newCurrentKey; + this.allKeys = newAllKeys.stream().collect( toMap(ManagedSecretKey::getId, identity())); - keyStore.save(allKeys); + keyStore.save(newAllKeys); } @Override - public void updateKeysInternal(ManagedSecretKey currentKey, - List sortedKeys) { - updateKeys(currentKey, sortedKeys); + public void updateKeysInternal(ManagedSecretKey newCurrentKey, + List newAllKeys) { + updateKeys(newCurrentKey, newAllKeys); } } diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyState.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyState.java index 38e08c2b0655..36cb8815abe3 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyState.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyState.java @@ -39,7 +39,10 @@ import static java.util.Objects.requireNonNull; -public class ScmSecretKeyState implements SecretKeyState { +/** + * SCM implementation of {@link SecretKeyState}. + */ +public final class ScmSecretKeyState implements SecretKeyState { private static final Logger LOG = LoggerFactory.getLogger(ScmSecretKeyState.class); @@ -109,6 +112,9 @@ public void updateKeysInternal(ManagedSecretKey newCurrentKey, } } + /** + * Builder for {@link ScmSecretKeyState}. + */ public static class Builder { private SecretKeyStore secretKeyStore; diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SerializableCodec.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SerializableCodec.java index 368c48155f9b..059ed7b38455 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SerializableCodec.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SerializableCodec.java @@ -1,3 +1,21 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package org.apache.hadoop.hdds.scm.security; import com.google.protobuf.ByteString; @@ -10,6 +28,9 @@ 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) From 7655c717fde5f6c91ec9d8a2bc67d85391f3cc73 Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Wed, 25 Jan 2023 10:31:53 -0800 Subject: [PATCH 08/27] Fix checkstyle. --- .../hdds/scm/security/package-info.java | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/package-info.java diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/package-info.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/package-info.java new file mode 100644 index 000000000000..d60ecff9f90f --- /dev/null +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/package-info.java @@ -0,0 +1,26 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * Encapsulate classes dealing with symmetric key algorithms. + */ + +/** + * Encapsulate classes dealing with security concern in SCM. + */ +package org.apache.hadoop.hdds.scm.security; From 54923c83b06249796b0d44f202d5f3b78e5374d5 Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Thu, 26 Jan 2023 09:38:43 -0800 Subject: [PATCH 09/27] Config for docker testing. --- .../dist/src/main/compose/ozonesecure-ha/docker-config | 4 ++++ hadoop-ozone/dist/src/main/compose/ozonesecure/docker-config | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/hadoop-ozone/dist/src/main/compose/ozonesecure-ha/docker-config b/hadoop-ozone/dist/src/main/compose/ozonesecure-ha/docker-config index c9577874aa4e..5c6f516898d4 100644 --- a/hadoop-ozone/dist/src/main/compose/ozonesecure-ha/docker-config +++ b/hadoop-ozone/dist/src/main/compose/ozonesecure-ha/docker-config @@ -138,3 +138,7 @@ OZONE_CONF_DIR=/etc/hadoop OZONE_LOG_DIR=/var/log/hadoop no_proxy=om,scm,recon,s3g,kdc,localhost,127.0.0.1 + +OZONE-SITE.XML_hdds.secret.key.rotate.duration=PT5M +OZONE-SITE.XML_hdds.secret.key.rotate.check.duration=PT1M +OZONE-SITE.XML_hdds.secret.key.expiry.duration=PT1H \ No newline at end of file diff --git a/hadoop-ozone/dist/src/main/compose/ozonesecure/docker-config b/hadoop-ozone/dist/src/main/compose/ozonesecure/docker-config index c9df04e92275..c71311915db3 100644 --- a/hadoop-ozone/dist/src/main/compose/ozonesecure/docker-config +++ b/hadoop-ozone/dist/src/main/compose/ozonesecure/docker-config @@ -169,3 +169,7 @@ OZONE-SITE.XML_ozone.om.multitenancy.ranger.sync.timeout=10s # change or let all OMs write to AccessController if this dev flag is set. # OZONE-SITE.XML_ozone.om.tenant.dev.skip.ranger=true + +OZONE-SITE.XML_hdds.secret.key.rotate.duration=PT5M +OZONE-SITE.XML_hdds.secret.key.rotate.check.duration=PT1M +OZONE-SITE.XML_hdds.secret.key.expiry.duration=PT1H From b5bbc73d767e90da7de299de01d93d3aabb242a9 Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Thu, 26 Jan 2023 09:48:51 -0800 Subject: [PATCH 10/27] correct javadoc. --- .../apache/hadoop/hdds/security/symmetric/SecretKeyState.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java index 18b92dd41be6..087c6ac962a8 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java @@ -46,8 +46,7 @@ public interface SecretKeyState { /** * Update the SecretKeys. - * This method replicates SecretKeys across all SCM instances - * after each rotation. + * This method replicates SecretKeys across all SCM instances. */ @Replicate void updateKeys(ManagedSecretKey currentKey, From ff4d4f253c6cf70d69a3ac105dee7590a75a9e89 Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Thu, 26 Jan 2023 10:17:25 -0800 Subject: [PATCH 11/27] Fix failing uts. --- .../hadoop/hdds/security/symmetric/SecretKeyManager.java | 1 + .../hadoop/hdds/security/symmetric/SecretKeyManagerTest.java | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java index 3ee764ff9ddf..15b4eb220f9e 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java @@ -134,6 +134,7 @@ public synchronized boolean checkAndRotate() throws TimeoutException { LOG.info("SecretKey rotation is happening, new key generated {}", newCurrentKey); state.updateKeys(newCurrentKey, updatedKeys); + return true; } return false; } diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java index c2991dc59bf2..f2c5b0cf8da0 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java @@ -167,10 +167,10 @@ public void testRotate(List initialKeys, ROTATION_DURATION, VALIDITY_DURATION, ALGORITHM); // Set the initial state. - state.updateKeys(initialCurrentKey, initialKeys); + state.updateKeysInternal(initialCurrentKey, initialKeys); Mockito.reset(mockedKeyStore); - assertEquals(lifeCycleManager.checkAndRotate(), expectRotate); + assertEquals(expectRotate, lifeCycleManager.checkAndRotate()); if (expectRotate) { // Verify rotation behavior. From 4ff22c3391a92329019ae20fedba6ef1bd19209e Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Thu, 26 Jan 2023 16:54:11 -0800 Subject: [PATCH 12/27] Fixing findbug. --- .../security/symmetric/LocalSecretKeyStore.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java index cfc5872c452e..ca7c7d735dcc 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java @@ -97,7 +97,7 @@ public synchronized List load() { @Override public synchronized void save(Collection secretKeys) { - setFileOwnerPermissions(secretKeysFile); + setFileOwnerPermissions(); List dtos = secretKeys.stream() .map(ManagedSecretKeyDto::new) @@ -116,16 +116,16 @@ public synchronized void save(Collection secretKeys) { LOG.info("Saved {} to file {}", secretKeys, secretKeysFile); } - private void setFileOwnerPermissions(Path path) { + private void setFileOwnerPermissions() { Set permissions = newHashSet(OWNER_READ, OWNER_WRITE); try { - if (!Files.exists(path)) { - if (!Files.exists(path.getParent())) { - Files.createDirectories(path.getParent()); + if (!Files.exists(secretKeysFile)) { + if (!Files.exists(secretKeysFile.getParent())) { + Files.createDirectories(secretKeysFile.getParent()); } - Files.createFile(path); + Files.createFile(secretKeysFile); } - Files.setPosixFilePermissions(path, permissions); + Files.setPosixFilePermissions(secretKeysFile, permissions); } catch (IOException e) { throw new IllegalStateException("Error setting secret keys file" + " permission: " + secretKeysFile, e); From 5b2ec9015abdddc0b72923473baa30204adfe53c Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Fri, 27 Jan 2023 15:19:09 -0800 Subject: [PATCH 13/27] Add package documentation. --- .../hdds/security/symmetric/package-info.java | 43 ++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/package-info.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/package-info.java index 10b4dc884294..6e88c3445189 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/package-info.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/package-info.java @@ -17,6 +17,47 @@ */ /** - * Encapsulate classes dealing with symmetric key algorithms. + * 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. + *

+ * + * 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. + *

+ * + * This package encloses the logic to manage symmetric secret keys + * lifecycle. In details, it consists of the following components: + *

    + *
  • + * The definition of manage secret key which is shared between SCM, + * OM and datanodes, see + * {@link org.apache.hadoop.hdds.security.symmetric.ManagedSecretKey}. + *
    • + * + *
  • + * 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} + *
    • + * + *
  • + * 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}. + *
    • + * + *
  • + * The basic logic to manage secret key lifecycle, see + * {@link org.apache.hadoop.hdds.security.symmetric.SecretKeyManager} + *
    • + *
+ * + *

+ * The overall design is documented + * here. */ package org.apache.hadoop.hdds.security.symmetric; From 112e69db90d5b6a8d5b4d1ee8f868caa6ba7bd12 Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Fri, 27 Jan 2023 15:20:15 -0800 Subject: [PATCH 14/27] Use protobuf to serialize data to ratis. --- .../security/symmetric/ManagedSecretKey.java | 37 ++++++++++++++++++ .../proto/ScmServerSecurityProtocol.proto | 9 +++++ .../hadoop/hdds/scm/ha/io/CodecFactory.java | 3 +- .../io/ManagedSecretKeyCodec.java} | 38 +++++-------------- 4 files changed, 57 insertions(+), 30 deletions(-) rename hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/{security/SerializableCodec.java => ha/io/ManagedSecretKeyCodec.java} (51%) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/ManagedSecretKey.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/ManagedSecretKey.java index cbb47df27b28..b38dccef23fc 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/ManagedSecretKey.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/ManagedSecretKey.java @@ -18,7 +18,12 @@ package org.apache.hadoop.hdds.security.symmetric; +import com.google.protobuf.ByteString; +import org.apache.hadoop.hdds.protocol.proto.HddsProtos; +import org.apache.hadoop.hdds.protocol.proto.SCMSecurityProtocolProtos; + import javax.crypto.SecretKey; +import javax.crypto.spec.SecretKeySpec; import java.io.Serializable; import java.time.Instant; import java.util.UUID; @@ -82,4 +87,36 @@ public String toString() { return "SecretKey(id = " + id + ", creation at: " + creationTime + ", expire at: " + expiryTime + ")"; } + + /** + * @return the protobuf message to deserialize this object. + */ + public SCMSecurityProtocolProtos.ManagedSecretKey toProtobuf() { + HddsProtos.UUID id = HddsProtos.UUID.newBuilder() + .setMostSigBits(this.id.getMostSignificantBits()) + .setLeastSigBits(this.id.getLeastSignificantBits()) + .build(); + + return SCMSecurityProtocolProtos.ManagedSecretKey.newBuilder() + .setId(id) + .setCreationTime(this.creationTime.toEpochMilli()) + .setExpiryTime(this.expiryTime.toEpochMilli()) + .setAlgorithm(this.secretKey.getAlgorithm()) + .setEncoded(ByteString.copyFrom(this.secretKey.getEncoded())) + .build(); + } + + /** + * Create a {@link ManagedSecretKey} from a given protobuf message. + */ + public static ManagedSecretKey fromProtobuf( + SCMSecurityProtocolProtos.ManagedSecretKey message) { + UUID id = new UUID(message.getId().getMostSigBits(), + message.getId().getLeastSigBits()); + Instant creationTime = Instant.ofEpochMilli(message.getCreationTime()); + Instant expiryTime = Instant.ofEpochMilli(message.getExpiryTime()); + SecretKey secretKey = new SecretKeySpec(message.getEncoded().toByteArray(), + message.getAlgorithm()); + return new ManagedSecretKey(id, creationTime, expiryTime, secretKey); + } } diff --git a/hadoop-hdds/interface-server/src/main/proto/ScmServerSecurityProtocol.proto b/hadoop-hdds/interface-server/src/main/proto/ScmServerSecurityProtocol.proto index dc6bcf986c3d..3621018fa8e3 100644 --- a/hadoop-hdds/interface-server/src/main/proto/ScmServerSecurityProtocol.proto +++ b/hadoop-hdds/interface-server/src/main/proto/ScmServerSecurityProtocol.proto @@ -249,3 +249,12 @@ message SCMRevokeCertificatesResponseProto { service SCMSecurityProtocolService { rpc submitRequest (SCMSecurityRequest) returns (SCMSecurityResponse); } + +message ManagedSecretKey { + required UUID id = 1; + required uint64 creationTime = 2; + required uint64 expiryTime = 3; + required string algorithm = 4; + required bytes encoded = 5; +} + diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/CodecFactory.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/CodecFactory.java index 5e8e9b19bc8c..af7705150984 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/CodecFactory.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/CodecFactory.java @@ -23,7 +23,6 @@ import com.google.protobuf.ProtocolMessageEnum; import org.apache.commons.lang3.ClassUtils; -import org.apache.hadoop.hdds.scm.security.SerializableCodec; import org.apache.hadoop.hdds.security.symmetric.ManagedSecretKey; import java.math.BigInteger; @@ -51,7 +50,7 @@ public final class CodecFactory { codecs.put(BigInteger.class, new BigIntegerCodec()); codecs.put(X509Certificate.class, new X509CertificateCodec()); codecs.put(ByteString.class, new ByteStringCodec()); - codecs.put(ManagedSecretKey.class, new SerializableCodec()); + codecs.put(ManagedSecretKey.class, new ManagedSecretKeyCodec()); } private CodecFactory() { } diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SerializableCodec.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ManagedSecretKeyCodec.java similarity index 51% rename from hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SerializableCodec.java rename to hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ManagedSecretKeyCodec.java index 059ed7b38455..384d81876293 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SerializableCodec.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ManagedSecretKeyCodec.java @@ -16,47 +16,29 @@ * limitations under the License. */ -package org.apache.hadoop.hdds.scm.security; +package org.apache.hadoop.hdds.scm.ha.io; 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; +import org.apache.hadoop.hdds.protocol.proto.SCMSecurityProtocolProtos; +import org.apache.hadoop.hdds.security.symmetric.ManagedSecretKey; /** - * A general codec for {@link java.io.Serializable} objects. + * A codec for {@link ManagedSecretKey} objects. */ -public class SerializableCodec implements Codec { +public class ManagedSecretKeyCodec implements Codec { @Override public ByteString serialize(Object object) throws InvalidProtocolBufferException { - try { - ByteArrayOutputStream bos = new ByteArrayOutputStream(); - ObjectOutputStream oos = new ObjectOutputStream(bos); - oos.writeObject(object); - oos.flush(); - return ByteString.copyFrom(bos.toByteArray()); - } catch (IOException e) { - throw new RuntimeException("Error serializing " - + object.getClass().getCanonicalName(), e); - } + ManagedSecretKey secretKey = (ManagedSecretKey) object; + return ByteString.copyFrom(secretKey.toProtobuf().toByteArray()); } @Override public Object deserialize(Class type, ByteString value) throws InvalidProtocolBufferException { - try { - ByteArrayInputStream bis = new ByteArrayInputStream(value.toByteArray()); - ObjectInputStream ois = new ObjectInputStream(bis); - return ois.readObject(); - } catch (IOException | ClassNotFoundException e) { - throw new RuntimeException("Error deserializing " - + type.getCanonicalName(), e); - } + SCMSecurityProtocolProtos.ManagedSecretKey message = + SCMSecurityProtocolProtos.ManagedSecretKey.parseFrom(value); + return ManagedSecretKey.fromProtobuf(message); } } From 48578f92e649ff5e901ae7dcba90922cc0de60c8 Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Fri, 27 Jan 2023 15:20:36 -0800 Subject: [PATCH 15/27] Update documentation. --- .../apache/hadoop/hdds/security/symmetric/SecretKeyStore.java | 4 +++- .../org/apache/hadoop/hdds/scm/security/package-info.java | 4 ---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyStore.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyStore.java index aa4184fc9b32..3d6bf63b711a 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyStore.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyStore.java @@ -22,7 +22,9 @@ import java.util.List; /** - * Interface for SecretKey storage component. + * Interface for SecretKey storage component, which is responsible for saving + * the SecretKeys states persistently to ensure they're not lost during + * restarts. */ public interface SecretKeyStore { List load(); diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/package-info.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/package-info.java index d60ecff9f90f..296e7f0883ab 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/package-info.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/package-info.java @@ -16,10 +16,6 @@ * limitations under the License. */ -/** - * Encapsulate classes dealing with symmetric key algorithms. - */ - /** * Encapsulate classes dealing with security concern in SCM. */ From 7c558ef7706998c9e4a09f5cef7095edcb876f3a Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Fri, 27 Jan 2023 15:58:19 -0800 Subject: [PATCH 16/27] Avoid using Serializable to store SecretKey. --- .../symmetric/LocalSecretKeyStore.java | 64 ++++--------------- 1 file changed, 14 insertions(+), 50 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java index ca7c7d735dcc..d7af31cea62f 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java @@ -18,29 +18,18 @@ 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 javax.crypto.spec.SecretKeySpec; 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; @@ -136,7 +125,8 @@ private static class ManagedSecretKeyDto { private UUID id; private Instant creationTime; private Instant expiryTime; - private SecretKey secretKey; + private String algorithm; + private byte[] encoded; /** * Used by Jackson when deserializing. @@ -148,10 +138,12 @@ private static class ManagedSecretKeyDto { id = object.getId(); creationTime = object.getCreationTime(); expiryTime = object.getExpiryTime(); - secretKey = object.getSecretKey(); + algorithm = object.getSecretKey().getAlgorithm(); + encoded = object.getSecretKey().getEncoded(); } public ManagedSecretKey toObject() { + SecretKey secretKey = new SecretKeySpec(this.encoded, this.algorithm); return new ManagedSecretKey(id, creationTime, expiryTime, secretKey); } @@ -180,48 +172,20 @@ public void setExpiryTime(Instant expiryTime) { this.expiryTime = expiryTime; } - @JsonSerialize(using = SecretKeySerializer.class) - @JsonDeserialize(using = SecretKeyDeserializer.class) - public SecretKey getSecretKey() { - return secretKey; + public String getAlgorithm() { + return algorithm; } - public void setSecretKey(SecretKey secretKey) { - this.secretKey = secretKey; + public void setAlgorithm(String algorithm) { + this.algorithm = algorithm; } - } - private static class SecretKeySerializer extends StdSerializer { - SecretKeySerializer() { - super(SecretKey.class); + public byte[] getEncoded() { + return encoded; } - @Override - public void serialize(SecretKey value, JsonGenerator gen, - SerializerProvider provider) throws IOException { - ByteArrayOutputStream bos = new ByteArrayOutputStream(); - ObjectOutputStream oos = new ObjectOutputStream(bos); - oos.writeObject(value); - gen.writeBinary(bos.toByteArray()); - } - } - - private static class SecretKeyDeserializer extends - StdDeserializer { - SecretKeyDeserializer() { - super(SecretKey.class); - } - - @Override - public SecretKey deserialize(JsonParser p, DeserializationContext ctxt) - throws IOException { - ByteArrayInputStream bis = new ByteArrayInputStream(p.getBinaryValue()); - ObjectInputStream ois = new ObjectInputStream(bis); - try { - return (SecretKey) ois.readObject(); - } catch (ClassNotFoundException e) { - throw new IllegalStateException("Error reading SecretKey", e); - } + public void setEncoded(byte[] encoded) { + this.encoded = encoded; } } } From 1fce17d7df81bf8c335ab53163bada5520674943 Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Fri, 27 Jan 2023 17:03:17 -0800 Subject: [PATCH 17/27] Add test to verify compatibility of future change. --- .../symmetric/LocalSecretKeyStoreTest.java | 77 +++++++++++++++---- .../src/test/resources/test_secret_keys.json | 9 +++ 2 files changed, 70 insertions(+), 16 deletions(-) create mode 100644 hadoop-hdds/framework/src/test/resources/test_secret_keys.json diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java index 194e38cfe7b9..30f6c1d17bb8 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java @@ -27,13 +27,17 @@ import javax.crypto.KeyGenerator; import javax.crypto.SecretKey; +import javax.crypto.spec.SecretKeySpec; import java.io.File; import java.io.IOException; +import java.io.InputStream; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.StandardCopyOption; import java.nio.file.attribute.PosixFilePermission; import java.time.Duration; import java.time.Instant; +import java.util.Base64; import java.util.List; import java.util.Set; import java.util.UUID; @@ -92,6 +96,9 @@ public void testSaveAndLoad(List keys) throws IOException { assertEqualKeys(keys, reloadedKeys); } + /** + * Verifies that secret keys are overwritten by subsequent writes. + */ @Test public void testOverwrite() throws Exception { List initialKeys = @@ -107,31 +114,69 @@ public void testOverwrite() throws Exception { assertEqualKeys(updatedKeys, secretKeyStore.load()); } - private void assertEqualKeys(List keys, - List reloadedKeys) { - assertEquals(keys.size(), reloadedKeys.size()); - for (int i = 0; i < keys.size(); i++) { - ManagedSecretKey key = keys.get(i); - ManagedSecretKey reloadedKey = reloadedKeys.get(i); - - assertEquals(key.getId(), reloadedKey.getId()); - assertEquals(key.getCreationTime().toEpochMilli(), - reloadedKey.getCreationTime().toEpochMilli()); - assertEquals(key.getExpiryTime(), - reloadedKey.getExpiryTime()); - assertEquals(key.getSecretKey(), reloadedKey.getSecretKey()); + /** + * This scenario verifies if an existing secret keys file can be loaded. + * The intention of this is to ensure a saved file can be loaded after + * future changes to {@link ManagedSecretKey} schema. + * + * Please don't just change the content of test_secret_keys.json if this + * test fails, instead, analyse the backward-compatibility of the change. + */ + @Test + public void testLoadExistingFile() throws Exception { + // copy test file content to the backing file. + ClassLoader cl = Thread.currentThread().getContextClassLoader(); + try ( + InputStream is = cl.getResourceAsStream("test_secret_keys.json") + ) { + Files.copy(is, testSecretFile, StandardCopyOption.REPLACE_EXISTING); + } + + Instant date = Instant.parse("2007-12-03T10:15:30.00Z"); + ManagedSecretKey secretKey = new ManagedSecretKey( + UUID.fromString("78864cfb-793b-4157-8ad6-714c9f950a16"), + date, + date.plus(Duration.ofHours(1)), + new SecretKeySpec( + Base64.getDecoder().decode( + "YSeCdJRB4RclxoeE69ENmTe2Cv8ybyKhHP3mq4M1r8o="), + "HmacSHA256" + )); + + List expectedKeys = newArrayList(secretKey); + assertEqualKeys(expectedKeys, secretKeyStore.load()); + } + + private void assertEqualKeys(List expected, + List actual) { + assertEquals(expected.size(), actual.size()); + for (int i = 0; i < expected.size(); i++) { + ManagedSecretKey expectedKey = expected.get(i); + ManagedSecretKey actualKey = actual.get(i); + + assertEquals(expectedKey.getId(), actualKey.getId()); + assertEquals(expectedKey.getCreationTime().toEpochMilli(), + actualKey.getCreationTime().toEpochMilli()); + assertEquals(expectedKey.getExpiryTime(), + actualKey.getExpiryTime()); + assertEquals(expectedKey.getSecretKey(), actualKey.getSecretKey()); } } private static ManagedSecretKey generateKey(String algorithm) throws Exception { + return generateKey(algorithm, Instant.now()); + } + + private static ManagedSecretKey generateKey(String algorithm, + Instant creationTime) + throws Exception { KeyGenerator keyGen = KeyGenerator.getInstance(algorithm); SecretKey secretKey = keyGen.generateKey(); - Instant now = Instant.now(); return new ManagedSecretKey( UUID.randomUUID(), - now, - now.plus(Duration.ofHours(1)), + creationTime, + creationTime.plus(Duration.ofHours(1)), secretKey ); } diff --git a/hadoop-hdds/framework/src/test/resources/test_secret_keys.json b/hadoop-hdds/framework/src/test/resources/test_secret_keys.json new file mode 100644 index 000000000000..87e1307869ab --- /dev/null +++ b/hadoop-hdds/framework/src/test/resources/test_secret_keys.json @@ -0,0 +1,9 @@ +[ + { + "id":"78864cfb-793b-4157-8ad6-714c9f950a16", + "creationTime":"2007-12-03T10:15:30Z", + "expiryTime":"2007-12-03T11:15:30Z", + "algorithm":"HmacSHA256", + "encoded":"YSeCdJRB4RclxoeE69ENmTe2Cv8ybyKhHP3mq4M1r8o=" + } +] \ No newline at end of file From 9686d59b0f34147593fbbcc246b9f9c38d76b478 Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Sun, 29 Jan 2023 22:10:08 -0800 Subject: [PATCH 18/27] Remove the separation of currentKey and allKeys --- .../symmetric/LocalSecretKeyStore.java | 8 +- .../security/symmetric/SecretKeyManager.java | 33 ++-- .../security/symmetric/SecretKeyState.java | 39 +---- .../symmetric/SecretKeyStateImpl.java | 111 +++++++++++++ .../security/symmetric/SecretKeyStore.java | 2 + .../symmetric/SecretKeyManagerTest.java | 72 ++------- .../hdds/scm/security/ScmSecretKeyState.java | 149 ------------------ .../security/ScmSecretKeyStateBuilder.java | 43 +++++ .../scm/security/SecretKeyManagerService.java | 2 +- 9 files changed, 198 insertions(+), 261 deletions(-) create mode 100644 hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyStateImpl.java delete mode 100644 hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyState.java create mode 100644 hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyStateBuilder.java diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java index d7af31cea62f..44816946d4b2 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java @@ -95,9 +95,7 @@ public synchronized void save(Collection secretKeys) { try (SequenceWriter writer = mapper.writer().writeValues(secretKeysFile.toFile())) { writer.init(true); - for (ManagedSecretKeyDto dto : dtos) { - writer.write(dto); - } + writer.writeAll(dtos); } catch (IOException e) { throw new IllegalStateException("Error saving SecretKeys to file " + secretKeysFile, e); @@ -121,6 +119,10 @@ private void setFileOwnerPermissions() { } } + /** + * Just a simple DTO that allows serializing/deserializing the immutable + * {@link ManagedSecretKey} objects. + */ private static class ManagedSecretKeyDto { private UUID id; private Instant creationTime; diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java index 15b4eb220f9e..43385352351a 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java @@ -72,36 +72,33 @@ public SecretKeyManager(SecretKeyState state, /** * 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 sortedKeys = keyStore.load() + // Load and filter expired keys. + List allKeys = 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); + if (allKeys.isEmpty()) { + // if no valid key present , generate new key as the current key. + // This happens at first start or restart after being down for + // a significant time. + ManagedSecretKey newKey = generateSecretKey(); + allKeys.add(newKey); + LOG.info("No valid keys has been loaded, " + + "a new key is generated: {}", newKey); } 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); + LOG.info("Keys reloaded: {}", allKeys); } // First, update the SecretKey state to make it visible immediately on the // current instance. - state.updateKeysInternal(currentKey, sortedKeys); + state.updateKeysInternal(allKeys); // Then, remember to replicate SecretKey states to all instances. pendingInititializedState = true; return true; @@ -110,7 +107,7 @@ public synchronized boolean initialize() { public synchronized void flushInitializedState() throws TimeoutException { if (pendingInititializedState) { LOG.info("Replicating initialized state."); - state.updateKeys(state.getCurrentKey(), state.getAllKeys()); + state.updateKeys(state.getSortedKeys()); pendingInititializedState = false; } } @@ -126,14 +123,14 @@ public synchronized boolean checkAndRotate() throws TimeoutException { ManagedSecretKey currentKey = state.getCurrentKey(); if (shouldRotate(currentKey)) { ManagedSecretKey newCurrentKey = generateSecretKey(); - List updatedKeys = state.getAllKeys() + List updatedKeys = state.getSortedKeys() .stream().filter(x -> !x.isExpired()) .collect(toList()); updatedKeys.add(newCurrentKey); LOG.info("SecretKey rotation is happening, new key generated {}", newCurrentKey); - state.updateKeys(newCurrentKey, updatedKeys); + state.updateKeys(updatedKeys); return true; } return false; diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java index 087c6ac962a8..ef0f0e5e1406 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java @@ -1,27 +1,8 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - *

- * http://www.apache.org/licenses/LICENSE-2.0 - *

- * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - package org.apache.hadoop.hdds.security.symmetric; import org.apache.hadoop.hdds.scm.metadata.Replicate; import java.util.List; -import java.util.UUID; import java.util.concurrent.TimeoutException; /** @@ -30,31 +11,27 @@ */ public interface SecretKeyState { /** - * @return the current active key, which is used for signing tokens. + * Get the current active key, which is used for signing tokens. This is + * also the latest key managed by this state. */ ManagedSecretKey getCurrentKey(); /** - * @return all the keys that managed by this manager. - */ - List getAllKeys(); - - /** - * Get SecretKey by id. + * Get the keys that managed by this manager. + * The returned keys are sorted by creation time, in the order of latest + * to oldest. */ - ManagedSecretKey getKeyById(UUID id); + List getSortedKeys(); /** * Update the SecretKeys. * This method replicates SecretKeys across all SCM instances. */ @Replicate - void updateKeys(ManagedSecretKey currentKey, - List allKeys) throws TimeoutException; + void updateKeys(List newKeys) throws TimeoutException; /** * Update the SecretKeys on this instance only. */ - void updateKeysInternal(ManagedSecretKey currentKey, - List allKeys); + void updateKeysInternal(List newKeys); } diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyStateImpl.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyStateImpl.java new file mode 100644 index 000000000000..6fc6fa4472d2 --- /dev/null +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyStateImpl.java @@ -0,0 +1,111 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.hdds.security.symmetric; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.Collections; +import java.util.List; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; + +import static java.util.Comparator.comparing; +import static java.util.Objects.requireNonNull; +import static java.util.stream.Collectors.toList; + +/** + * Default implementation of {@link SecretKeyState}. + */ +public final class SecretKeyStateImpl implements SecretKeyState { + private static final Logger LOG = + LoggerFactory.getLogger(SecretKeyStateImpl.class); + + private final ReadWriteLock lock = new ReentrantReadWriteLock(); + + private List sortedKeys; + private ManagedSecretKey currentKey; + + private final SecretKeyStore keyStore; + + /** + * Instantiate a state with no keys. This state object needs to be backed by + * a proper replication proxy so that the @Replication method works. + */ + public SecretKeyStateImpl(SecretKeyStore keyStore) { + this.keyStore = requireNonNull(keyStore); + } + + /** + * Get the current active key, which is used for signing tokens. This is + * also the latest key managed by this state. + */ + public ManagedSecretKey getCurrentKey() { + lock.readLock().lock(); + try { + return currentKey; + } finally { + lock.readLock().unlock(); + } + } + + /** + * Get the keys that managed by this manager. + * The returned keys are sorted by creation time, in the order of latest + * to oldest. + */ + public List getSortedKeys() { + lock.readLock().lock(); + try { + return sortedKeys; + } finally { + lock.readLock().unlock(); + } + } + + /** + * Update the SecretKeys. + * This method replicates SecretKeys across all SCM instances. + */ + public void updateKeys(List newKeys) { + updateKeysInternal(newKeys); + } + + /** + * Update the SecretKeys on this instance only. + */ + public void updateKeysInternal(List newKeys) { + LOG.info("Updating keys with {}", newKeys); + lock.writeLock().lock(); + try { + // Store sorted keys in order of latest to oldest and make it + // immutable so that can be used to answer queries directly. + sortedKeys = Collections.unmodifiableList( + newKeys.stream() + .sorted(comparing(ManagedSecretKey::getCreationTime).reversed()) + .collect(toList()) + ); + currentKey = sortedKeys.get(0); + LOG.info("Current key updated {}", currentKey); + keyStore.save(sortedKeys); + } finally { + lock.writeLock().unlock(); + } + } +} \ No newline at end of file diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyStore.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyStore.java index 3d6bf63b711a..c851c3683d33 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyStore.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyStore.java @@ -25,6 +25,8 @@ * Interface for SecretKey storage component, which is responsible for saving * the SecretKeys states persistently to ensure they're not lost during * restarts. + * + * This interface allows new persistent storage to be plugged in easily. */ public interface SecretKeyStore { List load(); diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java index f2c5b0cf8da0..825968d90e4e 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java @@ -30,10 +30,8 @@ import javax.crypto.SecretKey; import java.time.Duration; import java.time.Instant; -import java.util.ArrayList; import java.util.Collection; import java.util.List; -import java.util.Map; import java.util.UUID; import java.util.concurrent.TimeoutException; import java.util.stream.Stream; @@ -41,8 +39,6 @@ import static com.google.common.collect.Lists.newArrayList; import static java.time.Instant.now; import static java.time.temporal.ChronoUnit.DAYS; -import static java.util.function.Function.identity; -import static java.util.stream.Collectors.toMap; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotEquals; @@ -99,9 +95,8 @@ public static Stream loadSecretKeysTestCases() throws Exception { @MethodSource("loadSecretKeysTestCases") public void testLoadSecretKeys(List savedSecretKey, ManagedSecretKey expectedCurrentKey, - List expectedLoadedKeys) - throws TimeoutException { - SecretKeyState state = new TestSecretKeyState(mockedKeyStore); + List expectedLoadedKeys) { + SecretKeyState state = new SecretKeyStateImpl(mockedKeyStore); SecretKeyManager lifeCycleManager = new SecretKeyManager(state, mockedKeyStore, ROTATION_DURATION, VALIDITY_DURATION, ALGORITHM); @@ -111,13 +106,13 @@ public void testLoadSecretKeys(List savedSecretKey, if (expectedCurrentKey != null) { assertEquals(state.getCurrentKey(), expectedCurrentKey); - List allKeys = state.getAllKeys(); + List allKeys = state.getSortedKeys(); assertSameKeys(expectedLoadedKeys, allKeys); } else { // expect the current key is newly generated. assertFalse(savedSecretKey.contains(state.getCurrentKey())); - assertEquals(1, state.getAllKeys().size()); - assertTrue(state.getAllKeys().contains( + assertEquals(1, state.getSortedKeys().size()); + assertTrue(state.getSortedKeys().contains( state.getCurrentKey())); } } @@ -139,13 +134,13 @@ public static Stream rotationTestCases() throws Exception { return Stream.of( // Currentkey is new, not rotate. - of(newArrayList(k0, k1, k2), k0, false, null), + of(newArrayList(k0, k1, k2), false, null), // Current key just exceeds the rotation period. - of(newArrayList(k1, k2, k3), k1, true, newArrayList(k1, k2)), + of(newArrayList(k1, k2, k3), true, newArrayList(k1, k2)), // Current key exceeds the rotation period for a significant time (2d). - of(newArrayList(k2, k3, k4), k2, true, newArrayList(k2)) + of(newArrayList(k2, k3, k4), true, newArrayList(k2)) ); } @@ -155,19 +150,19 @@ public static Stream rotationTestCases() throws Exception { @ParameterizedTest @MethodSource("rotationTestCases") public void testRotate(List initialKeys, - ManagedSecretKey initialCurrentKey, boolean expectRotate, List expectedRetainedKeys) throws TimeoutException { - SecretKeyState state = new TestSecretKeyState(mockedKeyStore); + SecretKeyState state = new SecretKeyStateImpl(mockedKeyStore); SecretKeyManager lifeCycleManager = new SecretKeyManager(state, mockedKeyStore, ROTATION_DURATION, VALIDITY_DURATION, ALGORITHM); // Set the initial state. - state.updateKeysInternal(initialCurrentKey, initialKeys); + state.updateKeysInternal(initialKeys); + ManagedSecretKey initialCurrentKey = state.getCurrentKey(); Mockito.reset(mockedKeyStore); assertEquals(expectRotate, lifeCycleManager.checkAndRotate()); @@ -183,7 +178,7 @@ public void testRotate(List initialKeys, // 2. keys are correctly rotated, expired ones are excluded. List expectedAllKeys = expectedRetainedKeys; expectedAllKeys.add(currentKey); - assertSameKeys(expectedAllKeys, state.getAllKeys()); + assertSameKeys(expectedAllKeys, state.getSortedKeys()); // 3. All keys are stored. ArgumentCaptor> storedKeyCaptor = @@ -201,7 +196,7 @@ public void testRotate(List initialKeys, expectedExpiryTime).toMinutes()); } else { assertEquals(initialCurrentKey, state.getCurrentKey()); - assertSameKeys(initialKeys, state.getAllKeys()); + assertSameKeys(initialKeys, state.getSortedKeys()); } } @@ -216,45 +211,4 @@ private static ManagedSecretKey generateKey(Instant creationTime) secretKey ); } - - private static class TestSecretKeyState implements SecretKeyState { - private ManagedSecretKey currentKey; - private Map allKeys; - private SecretKeyStore keyStore; - - TestSecretKeyState(SecretKeyStore keyStore) { - this.keyStore = keyStore; - } - - @Override - public ManagedSecretKey getCurrentKey() { - return currentKey; - } - - @Override - public List getAllKeys() { - return new ArrayList<>(allKeys.values()); - } - - @Override - public ManagedSecretKey getKeyById(UUID id) { - return allKeys.get(id); - } - - @Override - public void updateKeys(ManagedSecretKey newCurrentKey, - List newAllKeys) { - this.currentKey = newCurrentKey; - this.allKeys = newAllKeys.stream().collect( - toMap(ManagedSecretKey::getId, identity())); - keyStore.save(newAllKeys); - } - - @Override - public void updateKeysInternal(ManagedSecretKey newCurrentKey, - List newAllKeys) { - updateKeys(newCurrentKey, newAllKeys); - } - } - } diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyState.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyState.java deleted file mode 100644 index 36cb8815abe3..000000000000 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyState.java +++ /dev/null @@ -1,149 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - *

- * http://www.apache.org/licenses/LICENSE-2.0 - *

- * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.apache.hadoop.hdds.scm.security; - -import org.apache.hadoop.hdds.protocol.proto.SCMRatisProtocol; -import org.apache.hadoop.hdds.scm.ha.SCMHAInvocationHandler; -import org.apache.hadoop.hdds.scm.ha.SCMRatisServer; -import org.apache.hadoop.hdds.security.symmetric.ManagedSecretKey; -import org.apache.hadoop.hdds.security.symmetric.SecretKeyState; -import org.apache.hadoop.hdds.security.symmetric.SecretKeyStore; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -import javax.annotation.Nonnull; -import java.lang.reflect.Proxy; -import java.util.ArrayList; -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import java.util.UUID; -import java.util.concurrent.locks.ReadWriteLock; -import java.util.concurrent.locks.ReentrantReadWriteLock; - -import static java.util.Objects.requireNonNull; - -/** - * SCM implementation of {@link SecretKeyState}. - */ -public final class ScmSecretKeyState implements SecretKeyState { - private static final Logger LOG = - LoggerFactory.getLogger(ScmSecretKeyState.class); - - private final ReadWriteLock lock = new ReentrantReadWriteLock(); - private ManagedSecretKey currentKey; - private final Map allKeys = new HashMap<>(); - - private final SecretKeyStore keyStore; - - - private ScmSecretKeyState(SecretKeyStore keyStore) { - this.keyStore = requireNonNull(keyStore); - } - - @Override - @Nonnull - public ManagedSecretKey getCurrentKey() { - lock.readLock().lock(); - try { - return currentKey; - } finally { - lock.readLock().unlock(); - } - } - - @Override - public List getAllKeys() { - lock.readLock().lock(); - try { - return new ArrayList<>(allKeys.values()); - } finally { - lock.readLock().unlock(); - } - } - - @Override - public ManagedSecretKey getKeyById(UUID id) { - lock.readLock().lock(); - try { - return allKeys.get(id); - } finally { - lock.readLock().unlock(); - } - } - - @Override - public void updateKeys(ManagedSecretKey newCurrentKey, - List newAllKeys) { - updateKeysInternal(newCurrentKey, newAllKeys); - } - - @Override - public void updateKeysInternal(ManagedSecretKey newCurrentKey, - List newAllKeys) { - LOG.info("Updating keys with currentKey={}, all keys={}", newCurrentKey, - newAllKeys); - lock.writeLock().lock(); - try { - currentKey = newCurrentKey; - allKeys.clear(); - for (ManagedSecretKey secretKey : newAllKeys) { - allKeys.put(secretKey.getId(), secretKey); - } - keyStore.save(allKeys.values()); - } finally { - lock.writeLock().unlock(); - } - } - - /** - * Builder for {@link ScmSecretKeyState}. - */ - public static class Builder { - - private SecretKeyStore secretKeyStore; - private SCMRatisServer scmRatisServer; - - - public Builder setSecretKeyStore(SecretKeyStore secretKeyStore) { - this.secretKeyStore = secretKeyStore; - return this; - } - - public Builder setRatisServer(final SCMRatisServer ratisServer) { - scmRatisServer = ratisServer; - return this; - } - - public SecretKeyState build() { - final ScmSecretKeyState impl = - new ScmSecretKeyState(secretKeyStore); - - final SCMHAInvocationHandler scmhaInvocationHandler = - new SCMHAInvocationHandler(SCMRatisProtocol.RequestType.SECRET_KEY, - impl, scmRatisServer); - - return (SecretKeyState) Proxy.newProxyInstance( - SCMHAInvocationHandler.class.getClassLoader(), - new Class[]{SecretKeyState.class}, scmhaInvocationHandler); - - } - } - -} diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyStateBuilder.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyStateBuilder.java new file mode 100644 index 000000000000..358b994d18b3 --- /dev/null +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyStateBuilder.java @@ -0,0 +1,43 @@ +package org.apache.hadoop.hdds.scm.security; + +import org.apache.hadoop.hdds.protocol.proto.SCMRatisProtocol; +import org.apache.hadoop.hdds.scm.ha.SCMHAInvocationHandler; +import org.apache.hadoop.hdds.scm.ha.SCMRatisServer; +import org.apache.hadoop.hdds.security.symmetric.SecretKeyState; +import org.apache.hadoop.hdds.security.symmetric.SecretKeyStateImpl; +import org.apache.hadoop.hdds.security.symmetric.SecretKeyStore; + +import java.lang.reflect.Proxy; + +/** + * Builder for {@link SecretKeyState} with a proper proxy to make @Replicate + * happen. + */ +public class ScmSecretKeyStateBuilder { + private SecretKeyStore secretKeyStore; + private SCMRatisServer scmRatisServer; + + public ScmSecretKeyStateBuilder setSecretKeyStore( + SecretKeyStore secretKeyStore) { + this.secretKeyStore = secretKeyStore; + return this; + } + + public ScmSecretKeyStateBuilder setRatisServer( + final SCMRatisServer ratisServer) { + scmRatisServer = ratisServer; + return this; + } + + public SecretKeyState build() { + final SecretKeyState impl = new SecretKeyStateImpl(secretKeyStore); + + final SCMHAInvocationHandler scmhaInvocationHandler = + new SCMHAInvocationHandler(SCMRatisProtocol.RequestType.SECRET_KEY, + impl, scmRatisServer); + + return (SecretKeyState) Proxy.newProxyInstance( + SCMHAInvocationHandler.class.getClassLoader(), + new Class[]{SecretKeyState.class}, scmhaInvocationHandler); + } +} diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java index 3c4070589df0..f71ae24d1543 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java @@ -72,7 +72,7 @@ public SecretKeyManagerService(SCMContext scmContext, SCM_CA_CERT_STORAGE_DIR); SecretKeyStore secretKeyStore = new LocalSecretKeyStore( secretKeyConfig.getLocalSecretKeyFile()); - SecretKeyState secretKeyState = new ScmSecretKeyState.Builder() + SecretKeyState secretKeyState = new ScmSecretKeyStateBuilder() .setSecretKeyStore(secretKeyStore) .setRatisServer(ratisServer) .build(); From 42b52e1df4cef1b761d57ad61f3b2de5d9b1a3de Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Sun, 29 Jan 2023 22:41:41 -0800 Subject: [PATCH 19/27] All SCMs agree on the same keys at first elected leader. --- .../security/symmetric/SecretKeyManager.java | 36 ++++++++----------- .../security/symmetric/SecretKeyState.java | 7 ++-- .../symmetric/SecretKeyStateImpl.java | 11 +++--- .../symmetric/SecretKeyManagerTest.java | 7 ++-- .../scm/security/SecretKeyManagerService.java | 7 ++-- 5 files changed, 28 insertions(+), 40 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java index 43385352351a..8892c4c07bc6 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java @@ -43,7 +43,6 @@ public class SecretKeyManager { LoggerFactory.getLogger(SecretKeyManager.class); private final SecretKeyState state; - private boolean pendingInititializedState = false; private final Duration rotationDuration; private final Duration validityDuration; private final SecretKeyStore keyStore; @@ -70,14 +69,17 @@ public SecretKeyManager(SecretKeyState state, } /** - * Initialize the state from by loading SecretKeys from local file, or - * generate new keys if the file doesn't exist. + * If the SecretKey state is not initialized, initialize it from by loading + * SecretKeys from local file, or generate new keys if the file doesn't + * exist. */ - public synchronized boolean initialize() { - if (state.getCurrentKey() != null) { - return false; + public synchronized void checkAndInitialize() throws TimeoutException { + if (isInitialized()) { + return; } + LOG.info("Initializing SecretKeys."); + // Load and filter expired keys. List allKeys = keyStore.load() .stream() @@ -90,26 +92,17 @@ public synchronized boolean initialize() { // a significant time. ManagedSecretKey newKey = generateSecretKey(); allKeys.add(newKey); - LOG.info("No valid keys has been loaded, " + - "a new key is generated: {}", newKey); + LOG.info("No valid key has been loaded. " + + "A new key is generated: {}", newKey); } else { LOG.info("Keys reloaded: {}", allKeys); } - // First, update the SecretKey state to make it visible immediately on the - // current instance. - state.updateKeysInternal(allKeys); - // Then, remember to replicate SecretKey states to all instances. - pendingInititializedState = true; - return true; + state.updateKeys(allKeys); } - public synchronized void flushInitializedState() throws TimeoutException { - if (pendingInititializedState) { - LOG.info("Replicating initialized state."); - state.updateKeys(state.getSortedKeys()); - pendingInititializedState = false; - } + public boolean isInitialized() { + return state.getCurrentKey() != null; } /** @@ -118,7 +111,8 @@ public synchronized void flushInitializedState() throws TimeoutException { * @return true if rotation actually happens, false if it doesn't. */ public synchronized boolean checkAndRotate() throws TimeoutException { - flushInitializedState(); + // Initialize the state if it's not initialized already. + checkAndInitialize(); ManagedSecretKey currentKey = state.getCurrentKey(); if (shouldRotate(currentKey)) { diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java index ef0f0e5e1406..3200b141163a 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java @@ -13,6 +13,8 @@ public interface SecretKeyState { /** * Get the current active key, which is used for signing tokens. This is * also the latest key managed by this state. + * + * @return the current active key, or null if the state is not initialized. */ ManagedSecretKey getCurrentKey(); @@ -29,9 +31,4 @@ public interface SecretKeyState { */ @Replicate void updateKeys(List newKeys) throws TimeoutException; - - /** - * Update the SecretKeys on this instance only. - */ - void updateKeysInternal(List newKeys); } diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyStateImpl.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyStateImpl.java index 6fc6fa4472d2..d5c886fd99e6 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyStateImpl.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyStateImpl.java @@ -56,6 +56,7 @@ public SecretKeyStateImpl(SecretKeyStore keyStore) { * Get the current active key, which is used for signing tokens. This is * also the latest key managed by this state. */ + @Override public ManagedSecretKey getCurrentKey() { lock.readLock().lock(); try { @@ -70,6 +71,7 @@ public ManagedSecretKey getCurrentKey() { * The returned keys are sorted by creation time, in the order of latest * to oldest. */ + @Override public List getSortedKeys() { lock.readLock().lock(); try { @@ -83,14 +85,8 @@ public List getSortedKeys() { * Update the SecretKeys. * This method replicates SecretKeys across all SCM instances. */ + @Override public void updateKeys(List newKeys) { - updateKeysInternal(newKeys); - } - - /** - * Update the SecretKeys on this instance only. - */ - public void updateKeysInternal(List newKeys) { LOG.info("Updating keys with {}", newKeys); lock.writeLock().lock(); try { @@ -108,4 +104,5 @@ public void updateKeysInternal(List newKeys) { lock.writeLock().unlock(); } } + } \ No newline at end of file diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java index 825968d90e4e..053148e28dc2 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManagerTest.java @@ -95,14 +95,15 @@ public static Stream loadSecretKeysTestCases() throws Exception { @MethodSource("loadSecretKeysTestCases") public void testLoadSecretKeys(List savedSecretKey, ManagedSecretKey expectedCurrentKey, - List expectedLoadedKeys) { + List expectedLoadedKeys) + throws Exception { SecretKeyState state = new SecretKeyStateImpl(mockedKeyStore); SecretKeyManager lifeCycleManager = new SecretKeyManager(state, mockedKeyStore, ROTATION_DURATION, VALIDITY_DURATION, ALGORITHM); when(mockedKeyStore.load()).thenReturn(savedSecretKey); - lifeCycleManager.initialize(); + lifeCycleManager.checkAndInitialize(); if (expectedCurrentKey != null) { assertEquals(state.getCurrentKey(), expectedCurrentKey); @@ -161,7 +162,7 @@ public void testRotate(List initialKeys, ROTATION_DURATION, VALIDITY_DURATION, ALGORITHM); // Set the initial state. - state.updateKeysInternal(initialKeys); + state.updateKeys(initialKeys); ManagedSecretKey initialCurrentKey = state.getCurrentKey(); Mockito.reset(mockedKeyStore); diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java index f71ae24d1543..5445889a3c1e 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java @@ -97,13 +97,12 @@ public void notifyStatusChanged() { serviceLock.lock(); try { if (scmContext.isLeaderReady()) { - - // Initialize SecretKeys if for first time leader. - if (secretKeyManager.initialize()) { + // Asynchronously initialize SecretKeys for first time leader. + if (secretKeyManager.isInitialized()) { // replicate the initialized SecretKeys to followers. scheduler.schedule(() -> { try { - secretKeyManager.flushInitializedState(); + secretKeyManager.checkAndInitialize(); } catch (TimeoutException e) { throw new RuntimeException( "Timeout replicating initialized state.", e); From cb30215d82d5b4e5aa927c6525540f9d9fe6d7ae Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Sun, 29 Jan 2023 23:25:11 -0800 Subject: [PATCH 20/27] misc changes for checkstyle and docs. --- .../symmetric/LocalSecretKeyStore.java | 9 +++++---- .../security/symmetric/ManagedSecretKey.java | 4 ++-- .../security/symmetric/SecretKeyManager.java | 1 - .../security/symmetric/SecretKeyState.java | 18 ++++++++++++++++++ .../hdds/security/symmetric/package-info.java | 3 +-- .../scm/security/ScmSecretKeyStateBuilder.java | 17 +++++++++++++++++ .../scm/security/SecretKeyManagerService.java | 1 - 7 files changed, 43 insertions(+), 10 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java index 44816946d4b2..41635e808ab9 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java @@ -50,6 +50,8 @@ * JSON file on local file system. */ public class LocalSecretKeyStore implements SecretKeyStore { + public static final Set SECRET_KEYS_PERMISSIONS = + newHashSet(OWNER_READ, OWNER_WRITE); private static final Logger LOG = LoggerFactory.getLogger(LocalSecretKeyStore.class); @@ -86,7 +88,7 @@ public synchronized List load() { @Override public synchronized void save(Collection secretKeys) { - setFileOwnerPermissions(); + createSecretKeyFiles(); List dtos = secretKeys.stream() .map(ManagedSecretKeyDto::new) @@ -103,8 +105,7 @@ public synchronized void save(Collection secretKeys) { LOG.info("Saved {} to file {}", secretKeys, secretKeysFile); } - private void setFileOwnerPermissions() { - Set permissions = newHashSet(OWNER_READ, OWNER_WRITE); + private void createSecretKeyFiles() { try { if (!Files.exists(secretKeysFile)) { if (!Files.exists(secretKeysFile.getParent())) { @@ -112,7 +113,7 @@ private void setFileOwnerPermissions() { } Files.createFile(secretKeysFile); } - Files.setPosixFilePermissions(secretKeysFile, permissions); + Files.setPosixFilePermissions(secretKeysFile, SECRET_KEYS_PERMISSIONS); } catch (IOException e) { throw new IllegalStateException("Error setting secret keys file" + " permission: " + secretKeysFile, e); diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/ManagedSecretKey.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/ManagedSecretKey.java index b38dccef23fc..7e8aaacb4871 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/ManagedSecretKey.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/ManagedSecretKey.java @@ -92,13 +92,13 @@ public String toString() { * @return the protobuf message to deserialize this object. */ public SCMSecurityProtocolProtos.ManagedSecretKey toProtobuf() { - HddsProtos.UUID id = HddsProtos.UUID.newBuilder() + HddsProtos.UUID uuid = HddsProtos.UUID.newBuilder() .setMostSigBits(this.id.getMostSignificantBits()) .setLeastSigBits(this.id.getLeastSignificantBits()) .build(); return SCMSecurityProtocolProtos.ManagedSecretKey.newBuilder() - .setId(id) + .setId(uuid) .setCreationTime(this.creationTime.toEpochMilli()) .setExpiryTime(this.expiryTime.toEpochMilli()) .setAlgorithm(this.secretKey.getAlgorithm()) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java index 8892c4c07bc6..0dc5bf89023f 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyManager.java @@ -30,7 +30,6 @@ 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; diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java index 3200b141163a..7be70b4b029b 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyState.java @@ -1,3 +1,21 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package org.apache.hadoop.hdds.security.symmetric; import org.apache.hadoop.hdds.scm.metadata.Replicate; diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/package-info.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/package-info.java index 6e88c3445189..ea7465df7c79 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/package-info.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/package-info.java @@ -57,7 +57,6 @@ * * *

- * The overall design is documented - * here. + * The overall design is documented here. */ package org.apache.hadoop.hdds.security.symmetric; diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyStateBuilder.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyStateBuilder.java index 358b994d18b3..c689fd2db39d 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyStateBuilder.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/ScmSecretKeyStateBuilder.java @@ -1,3 +1,20 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + package org.apache.hadoop.hdds.scm.security; import org.apache.hadoop.hdds.protocol.proto.SCMRatisProtocol; diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java index 5445889a3c1e..9f2bcbea5bf1 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java @@ -99,7 +99,6 @@ public void notifyStatusChanged() { if (scmContext.isLeaderReady()) { // Asynchronously initialize SecretKeys for first time leader. if (secretKeyManager.isInitialized()) { - // replicate the initialized SecretKeys to followers. scheduler.schedule(() -> { try { secretKeyManager.checkAndInitialize(); From b3a438240fa8c2f0ccf93e971ce6a12560a43358 Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Mon, 30 Jan 2023 09:03:57 -0800 Subject: [PATCH 21/27] Use json as string constant to fix rat violate in the json file. --- .../symmetric/LocalSecretKeyStoreTest.java | 21 ++++++++++++------- .../src/test/resources/test_secret_keys.json | 9 -------- 2 files changed, 14 insertions(+), 16 deletions(-) delete mode 100644 hadoop-hdds/framework/src/test/resources/test_secret_keys.json diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java index 30f6c1d17bb8..d573c6fe6781 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java @@ -34,10 +34,12 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.StandardCopyOption; +import java.nio.file.StandardOpenOption; import java.nio.file.attribute.PosixFilePermission; import java.time.Duration; import java.time.Instant; import java.util.Base64; +import java.util.Collections; import java.util.List; import java.util.Set; import java.util.UUID; @@ -119,18 +121,23 @@ public void testOverwrite() throws Exception { * The intention of this is to ensure a saved file can be loaded after * future changes to {@link ManagedSecretKey} schema. * - * Please don't just change the content of test_secret_keys.json if this + * Please don't just change the content of test json if this * test fails, instead, analyse the backward-compatibility of the change. */ @Test public void testLoadExistingFile() throws Exception { // copy test file content to the backing file. - ClassLoader cl = Thread.currentThread().getContextClassLoader(); - try ( - InputStream is = cl.getResourceAsStream("test_secret_keys.json") - ) { - Files.copy(is, testSecretFile, StandardCopyOption.REPLACE_EXISTING); - } + String test_json = "[\n" + + " {\n" + + " \"id\":\"78864cfb-793b-4157-8ad6-714c9f950a16\",\n" + + " \"creationTime\":\"2007-12-03T10:15:30Z\",\n" + + " \"expiryTime\":\"2007-12-03T11:15:30Z\",\n" + + " \"algorithm\":\"HmacSHA256\",\n" + + " \"encoded\":\"YSeCdJRB4RclxoeE69ENmTe2Cv8ybyKhHP3mq4M1r8o=\"\n" + + " }\n" + + "]"; + Files.write(testSecretFile, Collections.singletonList(test_json), + StandardOpenOption.WRITE); Instant date = Instant.parse("2007-12-03T10:15:30.00Z"); ManagedSecretKey secretKey = new ManagedSecretKey( diff --git a/hadoop-hdds/framework/src/test/resources/test_secret_keys.json b/hadoop-hdds/framework/src/test/resources/test_secret_keys.json deleted file mode 100644 index 87e1307869ab..000000000000 --- a/hadoop-hdds/framework/src/test/resources/test_secret_keys.json +++ /dev/null @@ -1,9 +0,0 @@ -[ - { - "id":"78864cfb-793b-4157-8ad6-714c9f950a16", - "creationTime":"2007-12-03T10:15:30Z", - "expiryTime":"2007-12-03T11:15:30Z", - "algorithm":"HmacSHA256", - "encoded":"YSeCdJRB4RclxoeE69ENmTe2Cv8ybyKhHP3mq4M1r8o=" - } -] \ No newline at end of file From f20b1ef3c7c86b58fe7c4ca0b35b491a6983b671 Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Mon, 30 Jan 2023 16:13:01 -0800 Subject: [PATCH 22/27] Fix findbugs and correct time duration configs. --- .../java/org/apache/hadoop/hdds/HddsConfigKeys.java | 6 +++--- .../common/src/main/resources/ozone-default.xml | 9 +++------ .../security/symmetric/LocalSecretKeyStore.java | 8 +++++--- .../hdds/security/symmetric/SecretKeyConfig.java | 13 +++++++------ .../security/symmetric/LocalSecretKeyStoreTest.java | 6 ++---- .../hdds/scm/security/SecretKeyManagerService.java | 6 +++--- .../src/main/compose/ozonesecure-ha/docker-config | 4 ---- .../dist/src/main/compose/ozonesecure/docker-config | 6 +----- 8 files changed, 24 insertions(+), 34 deletions(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsConfigKeys.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsConfigKeys.java index 850a4fe0c2eb..8a870299fb72 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsConfigKeys.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsConfigKeys.java @@ -222,11 +222,11 @@ public final class HddsConfigKeys { public static final String HDDS_SECRET_KEY_EXPIRY_DURATION = "hdds.secret.key.expiry.duration"; - public static final String HDDS_SECRET_KEY_EXPIRY_DURATION_DEFAULT = "P7D"; + public static final String HDDS_SECRET_KEY_EXPIRY_DURATION_DEFAULT = "7d"; public static final String HDDS_SECRET_KEY_ROTATE_DURATION = "hdds.secret.key.rotate.duration"; - public static final String HDDS_SECRET_KEY_ROTATE_DURATION_DEFAULT = "P1D"; + public static final String HDDS_SECRET_KEY_ROTATE_DURATION_DEFAULT = "1d"; public static final String HDDS_SECRET_KEY_ALGORITHM = "hdds.secret.key.algorithm"; @@ -236,7 +236,7 @@ public final class HddsConfigKeys { public static final String HDDS_SECRET_KEY_ROTATE_CHECK_DURATION = "hdds.secret.key.rotate.check.duration"; public static final String HDDS_SECRET_KEY_ROTATE_CHECK_DURATION_DEFAULT - = "PT10M"; + = "10m"; /** * Do not instantiate. diff --git a/hadoop-hdds/common/src/main/resources/ozone-default.xml b/hadoop-hdds/common/src/main/resources/ozone-default.xml index dde6ce9f3faa..69bd82aa7c15 100644 --- a/hadoop-hdds/common/src/main/resources/ozone-default.xml +++ b/hadoop-hdds/common/src/main/resources/ozone-default.xml @@ -3612,30 +3612,27 @@ hdds.secret.key.expiry.duration - P7D + 7d SCM, SECURITY 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 hdds.secret.key.rotate.duration - P1D + 1d SCM, SECURITY The duration that SCM periodically generate a new symmetric secret keys. - The formats accepted are based on the ISO-8601 duration format PnDTnHnMn.nS hdds.secret.key.rotate.check.duration - PT10M + 10m SCM, SECURITY 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 diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java index 41635e808ab9..aee2d9c71bf7 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java @@ -43,6 +43,7 @@ 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.Objects.requireNonNull; import static java.util.stream.Collectors.toList; /** @@ -50,7 +51,7 @@ * JSON file on local file system. */ public class LocalSecretKeyStore implements SecretKeyStore { - public static final Set SECRET_KEYS_PERMISSIONS = + private static final Set SECRET_KEYS_PERMISSIONS = newHashSet(OWNER_READ, OWNER_WRITE); private static final Logger LOG = LoggerFactory.getLogger(LocalSecretKeyStore.class); @@ -59,7 +60,7 @@ public class LocalSecretKeyStore implements SecretKeyStore { private final ObjectMapper mapper; public LocalSecretKeyStore(Path secretKeysFile) { - this.secretKeysFile = secretKeysFile; + this.secretKeysFile = requireNonNull(secretKeysFile); this.mapper = new ObjectMapper() .registerModule(new JavaTimeModule()) .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false); @@ -108,7 +109,8 @@ public synchronized void save(Collection secretKeys) { private void createSecretKeyFiles() { try { if (!Files.exists(secretKeysFile)) { - if (!Files.exists(secretKeysFile.getParent())) { + if (secretKeysFile.getParent() != null + && !Files.exists(secretKeysFile.getParent())) { Files.createDirectories(secretKeysFile.getParent()); } Files.createFile(secretKeysFile); diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java index cbca9dc2b2a1..be3be0b372ba 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java @@ -23,6 +23,7 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.time.Duration; +import java.util.concurrent.TimeUnit; import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_KEY_DIR_NAME; import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_KEY_DIR_NAME_DEFAULT; @@ -54,13 +55,13 @@ public SecretKeyConfig(ConfigurationSource conf, String component) { HDDS_SECRET_KEY_FILE_DEFAULT); localSecretKeyFile = Paths.get(metadataDir, component, keyDir, fileName); - String rotateDurationStr = conf.get(HDDS_SECRET_KEY_ROTATE_DURATION, - HDDS_SECRET_KEY_ROTATE_DURATION_DEFAULT); - this.rotateDuration = Duration.parse(rotateDurationStr); + long rotateDurationInMs = conf.getTimeDuration(HDDS_SECRET_KEY_ROTATE_DURATION, + HDDS_SECRET_KEY_ROTATE_DURATION_DEFAULT, TimeUnit.MILLISECONDS); + this.rotateDuration = Duration.ofMillis(rotateDurationInMs); - String expiryDurationStr = conf.get(HDDS_SECRET_KEY_EXPIRY_DURATION, - HDDS_SECRET_KEY_EXPIRY_DURATION_DEFAULT); - this.expiryDuration = Duration.parse(expiryDurationStr); + long expiryDurationInMs = conf.getTimeDuration(HDDS_SECRET_KEY_EXPIRY_DURATION, + HDDS_SECRET_KEY_EXPIRY_DURATION_DEFAULT, TimeUnit.MILLISECONDS); + this.expiryDuration = Duration.ofMillis(expiryDurationInMs); this.algorithm = conf.get(HDDS_SECRET_KEY_ALGORITHM, HDDS_SECRET_KEY_ALGORITHM_DEFAULT); diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java index d573c6fe6781..c406ce2b08f6 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStoreTest.java @@ -30,10 +30,8 @@ import javax.crypto.spec.SecretKeySpec; import java.io.File; import java.io.IOException; -import java.io.InputStream; import java.nio.file.Files; import java.nio.file.Path; -import java.nio.file.StandardCopyOption; import java.nio.file.StandardOpenOption; import java.nio.file.attribute.PosixFilePermission; import java.time.Duration; @@ -127,7 +125,7 @@ public void testOverwrite() throws Exception { @Test public void testLoadExistingFile() throws Exception { // copy test file content to the backing file. - String test_json = "[\n" + + String testJson = "[\n" + " {\n" + " \"id\":\"78864cfb-793b-4157-8ad6-714c9f950a16\",\n" + " \"creationTime\":\"2007-12-03T10:15:30Z\",\n" + @@ -136,7 +134,7 @@ public void testLoadExistingFile() throws Exception { " \"encoded\":\"YSeCdJRB4RclxoeE69ENmTe2Cv8ybyKhHP3mq4M1r8o=\"\n" + " }\n" + "]"; - Files.write(testSecretFile, Collections.singletonList(test_json), + Files.write(testSecretFile, Collections.singletonList(testJson), StandardOpenOption.WRITE); Instant date = Instant.parse("2007-12-03T10:15:30.00Z"); diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java index 9f2bcbea5bf1..57e3c4d56963 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java @@ -84,10 +84,10 @@ public SecretKeyManagerService(SCMContext scmContext, .setNameFormat(getServiceName()) .build()); - String rotationCheckDurationStr = conf.get( + long rotationCheckInMs = conf.getTimeDuration( HDDS_SECRET_KEY_ROTATE_CHECK_DURATION, - HDDS_SECRET_KEY_ROTATE_CHECK_DURATION_DEFAULT); - rotationCheckDuration = Duration.parse(rotationCheckDurationStr); + HDDS_SECRET_KEY_ROTATE_CHECK_DURATION_DEFAULT, TimeUnit.MILLISECONDS); + rotationCheckDuration = Duration.ofMillis(rotationCheckInMs); start(); } diff --git a/hadoop-ozone/dist/src/main/compose/ozonesecure-ha/docker-config b/hadoop-ozone/dist/src/main/compose/ozonesecure-ha/docker-config index 5c6f516898d4..c9577874aa4e 100644 --- a/hadoop-ozone/dist/src/main/compose/ozonesecure-ha/docker-config +++ b/hadoop-ozone/dist/src/main/compose/ozonesecure-ha/docker-config @@ -138,7 +138,3 @@ OZONE_CONF_DIR=/etc/hadoop OZONE_LOG_DIR=/var/log/hadoop no_proxy=om,scm,recon,s3g,kdc,localhost,127.0.0.1 - -OZONE-SITE.XML_hdds.secret.key.rotate.duration=PT5M -OZONE-SITE.XML_hdds.secret.key.rotate.check.duration=PT1M -OZONE-SITE.XML_hdds.secret.key.expiry.duration=PT1H \ No newline at end of file diff --git a/hadoop-ozone/dist/src/main/compose/ozonesecure/docker-config b/hadoop-ozone/dist/src/main/compose/ozonesecure/docker-config index c71311915db3..b95571f7a1c5 100644 --- a/hadoop-ozone/dist/src/main/compose/ozonesecure/docker-config +++ b/hadoop-ozone/dist/src/main/compose/ozonesecure/docker-config @@ -168,8 +168,4 @@ OZONE-SITE.XML_ozone.om.multitenancy.ranger.sync.timeout=10s # Potential TODO: We could trigger BG sync automatically during OM leadership # change or let all OMs write to AccessController if this dev flag is set. # -OZONE-SITE.XML_ozone.om.tenant.dev.skip.ranger=true - -OZONE-SITE.XML_hdds.secret.key.rotate.duration=PT5M -OZONE-SITE.XML_hdds.secret.key.rotate.check.duration=PT1M -OZONE-SITE.XML_hdds.secret.key.expiry.duration=PT1H +OZONE-SITE.XML_ozone.om.tenant.dev.skip.ranger=true \ No newline at end of file From d62d4525a1acfe55a65c307c14a76d5614d20264 Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Mon, 30 Jan 2023 17:02:02 -0800 Subject: [PATCH 23/27] Fix findbugs. --- .../security/symmetric/LocalSecretKeyStore.java | 13 ++++++++----- .../dist/src/main/compose/ozonesecure/docker-config | 2 +- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java index aee2d9c71bf7..48cc633b67b9 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/LocalSecretKeyStore.java @@ -41,6 +41,9 @@ import java.util.UUID; import static com.google.common.collect.Sets.newHashSet; +import static java.nio.file.Files.createDirectories; +import static java.nio.file.Files.createFile; +import static java.nio.file.Files.exists; import static java.nio.file.attribute.PosixFilePermission.OWNER_READ; import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE; import static java.util.Objects.requireNonNull; @@ -108,12 +111,12 @@ public synchronized void save(Collection secretKeys) { private void createSecretKeyFiles() { try { - if (!Files.exists(secretKeysFile)) { - if (secretKeysFile.getParent() != null - && !Files.exists(secretKeysFile.getParent())) { - Files.createDirectories(secretKeysFile.getParent()); + if (!exists(secretKeysFile)) { + Path parent = secretKeysFile.getParent(); + if (parent != null && !exists(parent)) { + createDirectories(parent); } - Files.createFile(secretKeysFile); + createFile(secretKeysFile); } Files.setPosixFilePermissions(secretKeysFile, SECRET_KEYS_PERMISSIONS); } catch (IOException e) { diff --git a/hadoop-ozone/dist/src/main/compose/ozonesecure/docker-config b/hadoop-ozone/dist/src/main/compose/ozonesecure/docker-config index b95571f7a1c5..c9df04e92275 100644 --- a/hadoop-ozone/dist/src/main/compose/ozonesecure/docker-config +++ b/hadoop-ozone/dist/src/main/compose/ozonesecure/docker-config @@ -168,4 +168,4 @@ OZONE-SITE.XML_ozone.om.multitenancy.ranger.sync.timeout=10s # Potential TODO: We could trigger BG sync automatically during OM leadership # change or let all OMs write to AccessController if this dev flag is set. # -OZONE-SITE.XML_ozone.om.tenant.dev.skip.ranger=true \ No newline at end of file +OZONE-SITE.XML_ozone.om.tenant.dev.skip.ranger=true From 5248a627bb523e35565c27a4565546f239cfe97c Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Mon, 30 Jan 2023 17:47:33 -0800 Subject: [PATCH 24/27] Fix findbugs. --- .../hadoop/hdds/scm/security/SecretKeyManagerService.java | 2 +- .../dist/src/main/compose/ozonesecure-ha/docker-config | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java index 57e3c4d56963..25d37b5dcc3c 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java @@ -98,7 +98,7 @@ public void notifyStatusChanged() { try { if (scmContext.isLeaderReady()) { // Asynchronously initialize SecretKeys for first time leader. - if (secretKeyManager.isInitialized()) { + if (!secretKeyManager.isInitialized()) { scheduler.schedule(() -> { try { secretKeyManager.checkAndInitialize(); diff --git a/hadoop-ozone/dist/src/main/compose/ozonesecure-ha/docker-config b/hadoop-ozone/dist/src/main/compose/ozonesecure-ha/docker-config index c9577874aa4e..ba264da7b294 100644 --- a/hadoop-ozone/dist/src/main/compose/ozonesecure-ha/docker-config +++ b/hadoop-ozone/dist/src/main/compose/ozonesecure-ha/docker-config @@ -138,3 +138,7 @@ OZONE_CONF_DIR=/etc/hadoop OZONE_LOG_DIR=/var/log/hadoop no_proxy=om,scm,recon,s3g,kdc,localhost,127.0.0.1 + +OZONE-SITE.XML_hdds.secret.key.rotate.duration=5m +OZONE-SITE.XML_hdds.secret.key.rotate.check.duration=1m +OZONE-SITE.XML_hdds.secret.key.expiry.duration=1h From 53c38faaee65466e8d86402d61466fa2edfcc24c Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Mon, 30 Jan 2023 22:40:28 -0800 Subject: [PATCH 25/27] Fix checkstyle. --- .../hadoop/hdds/security/symmetric/SecretKeyConfig.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java index be3be0b372ba..3765f279f433 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java @@ -55,11 +55,13 @@ public SecretKeyConfig(ConfigurationSource conf, String component) { HDDS_SECRET_KEY_FILE_DEFAULT); localSecretKeyFile = Paths.get(metadataDir, component, keyDir, fileName); - long rotateDurationInMs = conf.getTimeDuration(HDDS_SECRET_KEY_ROTATE_DURATION, + long rotateDurationInMs = conf.getTimeDuration( + HDDS_SECRET_KEY_ROTATE_DURATION, HDDS_SECRET_KEY_ROTATE_DURATION_DEFAULT, TimeUnit.MILLISECONDS); this.rotateDuration = Duration.ofMillis(rotateDurationInMs); - long expiryDurationInMs = conf.getTimeDuration(HDDS_SECRET_KEY_EXPIRY_DURATION, + long expiryDurationInMs = conf.getTimeDuration( + HDDS_SECRET_KEY_EXPIRY_DURATION, HDDS_SECRET_KEY_EXPIRY_DURATION_DEFAULT, TimeUnit.MILLISECONDS); this.expiryDuration = Duration.ofMillis(expiryDurationInMs); From 1660ecb9584ec3f946bc0dea20d5633fb57d742e Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Thu, 2 Feb 2023 13:47:29 -0800 Subject: [PATCH 26/27] Add document to explain the config. --- hadoop-hdds/common/src/main/resources/ozone-default.xml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/hadoop-hdds/common/src/main/resources/ozone-default.xml b/hadoop-hdds/common/src/main/resources/ozone-default.xml index 69bd82aa7c15..b2cfb6948119 100644 --- a/hadoop-hdds/common/src/main/resources/ozone-default.xml +++ b/hadoop-hdds/common/src/main/resources/ozone-default.xml @@ -3616,6 +3616,8 @@ SCM, SECURITY 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. @@ -3632,7 +3634,9 @@ SCM, SECURITY 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. + 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. From 3275b8ac303971eb83fe8f3eff246412b318d2c5 Mon Sep 17 00:00:00 2001 From: Duong Nguyen Date: Tue, 21 Feb 2023 19:00:21 -0800 Subject: [PATCH 27/27] pr comment actions. --- .../src/main/resources/ozone-default.xml | 7 +++-- .../security/symmetric/SecretKeyConfig.java | 12 ++++++++ .../hdds/security/symmetric/package-info.java | 3 +- .../scm/security/SecretKeyManagerService.java | 28 ++++++------------- 4 files changed, 26 insertions(+), 24 deletions(-) diff --git a/hadoop-hdds/common/src/main/resources/ozone-default.xml b/hadoop-hdds/common/src/main/resources/ozone-default.xml index b2cfb6948119..6926b3986359 100644 --- a/hadoop-hdds/common/src/main/resources/ozone-default.xml +++ b/hadoop-hdds/common/src/main/resources/ozone-default.xml @@ -3616,8 +3616,8 @@ SCM, SECURITY 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. + This default value, in combination with hdds.secret.key.rotate.duration=1d, results in 7 secret keys (for the + last 7 days) are kept valid at any point of time. @@ -3645,7 +3645,8 @@ SCM, SECURITY The algorithm that SCM uses to generate symmetric secret keys. - The formats accepted are based on the ISO-8601 duration format PnDTnHnMn.nS + A valid algorithm is the one supported by KeyGenerator, as described at + https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#KeyGenerator. diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java index 3765f279f433..f2a9181051ba 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/SecretKeyConfig.java @@ -34,6 +34,8 @@ import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_SECRET_KEY_EXPIRY_DURATION_DEFAULT; import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_SECRET_KEY_FILE; import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_SECRET_KEY_FILE_DEFAULT; +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.hdds.HddsConfigKeys.HDDS_SECRET_KEY_ROTATE_DURATION; import static org.apache.hadoop.hdds.HddsConfigKeys.HDDS_SECRET_KEY_ROTATE_DURATION_DEFAULT; import static org.apache.hadoop.hdds.HddsConfigKeys.OZONE_METADATA_DIRS; @@ -46,6 +48,7 @@ public class SecretKeyConfig { private final Duration rotateDuration; private final Duration expiryDuration; private final String algorithm; + private final Duration rotationCheckDuration; public SecretKeyConfig(ConfigurationSource conf, String component) { String metadataDir = conf.get(HDDS_METADATA_DIR_NAME, @@ -67,6 +70,11 @@ public SecretKeyConfig(ConfigurationSource conf, String component) { this.algorithm = conf.get(HDDS_SECRET_KEY_ALGORITHM, HDDS_SECRET_KEY_ALGORITHM_DEFAULT); + + long rotationCheckInMs = conf.getTimeDuration( + HDDS_SECRET_KEY_ROTATE_CHECK_DURATION, + HDDS_SECRET_KEY_ROTATE_CHECK_DURATION_DEFAULT, TimeUnit.MILLISECONDS); + this.rotationCheckDuration = Duration.ofMillis(rotationCheckInMs); } public Path getLocalSecretKeyFile() { @@ -84,4 +92,8 @@ public Duration getExpiryDuration() { public String getAlgorithm() { return algorithm; } + + public Duration getRotationCheckDuration() { + return rotationCheckDuration; + } } diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/package-info.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/package-info.java index ea7465df7c79..2997fe0a26bb 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/package-info.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/symmetric/package-info.java @@ -57,6 +57,7 @@ * * *

- * The overall design is documented here. + * The original overall design can be found at + * HDDS-7733. */ package org.apache.hadoop.hdds.security.symmetric; diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java index 25d37b5dcc3c..27ce30a8a18b 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/security/SecretKeyManagerService.java @@ -30,7 +30,6 @@ 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; @@ -38,8 +37,6 @@ 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; /** @@ -51,6 +48,7 @@ public class SecretKeyManagerService implements SCMService, Runnable { private final SCMContext scmContext; private final SecretKeyManager secretKeyManager; + private final SecretKeyConfig secretKeyConfig; /** @@ -59,7 +57,6 @@ public class SecretKeyManagerService implements SCMService, Runnable { private final Lock serviceLock = new ReentrantLock(); private ServiceStatus serviceStatus = ServiceStatus.PAUSING; - private final Duration rotationCheckDuration; private final ScheduledExecutorService scheduler; @SuppressWarnings("parameternumber") @@ -68,7 +65,7 @@ public SecretKeyManagerService(SCMContext scmContext, SCMRatisServer ratisServer) { this.scmContext = scmContext; - SecretKeyConfig secretKeyConfig = new SecretKeyConfig(conf, + secretKeyConfig = new SecretKeyConfig(conf, SCM_CA_CERT_STORAGE_DIR); SecretKeyStore secretKeyStore = new LocalSecretKeyStore( secretKeyConfig.getLocalSecretKeyFile()); @@ -84,11 +81,6 @@ public SecretKeyManagerService(SCMContext scmContext, .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(); } @@ -135,14 +127,9 @@ public void run() { } try { - boolean rotated = secretKeyManager.checkAndRotate(); - if (rotated) { - LOG.info("SecretKeys have been updated."); - } else { - LOG.info("SecretKeys have not been updated."); - } + secretKeyManager.checkAndRotate(); } catch (TimeoutException e) { - LOG.error("Error occurred when updating SecretKeys", e); + LOG.error("Error occurred when updating SecretKeys.", e); } } @@ -153,9 +140,10 @@ public String getServiceName() { @Override public void start() { - LOG.info("Scheduling rotation checker with interval {} seconds", - rotationCheckDuration.toMillis() / 1000); - scheduler.scheduleAtFixedRate(this, 0, rotationCheckDuration.toMillis(), + LOG.info("Scheduling rotation checker with interval {}", + secretKeyConfig.getRotationCheckDuration()); + scheduler.scheduleAtFixedRate(this, 0, + secretKeyConfig.getRotationCheckDuration().toMillis(), TimeUnit.MILLISECONDS); }