You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2021/08/12 11:28:11 UTC
[GitHub] [ignite] timoninmaxim commented on a change in pull request #9269: IGNITE-15159 : Manual encrypted snapshots.
timoninmaxim commented on a change in pull request #9269:
URL: https://github.com/apache/ignite/pull/9269#discussion_r687485834
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/file/EncryptedFileIO.java
##########
@@ -218,11 +210,26 @@
* @throws IOException If failed.
*/
private void encrypt(ByteBuffer srcBuf, ByteBuffer res) throws IOException {
Review comment:
This method without `res.rewind()` looks strange. Also this method is used only inside `#encrypt(ByteBuffer)` and never more. Let's merge those 2 methods.
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java
##########
@@ -1505,6 +1508,16 @@ SnapshotFutureTask registerSnapshotTask(
if (prev != null)
return new SnapshotFutureTask(new IgniteCheckedException("Snapshot with requested name is already scheduled: " + snpName));
+ for (Integer grpId : parts.keySet()) {
+ if (!withMetaStorage && cctx.cache().isEncrypted(grpId)) {
Review comment:
Let's move check `!withMetaStorage` before loop. If the metastore is included in snapshot then it's useless to iterate over all `parts`.
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/EncryptedSnapshotTest.java
##########
@@ -0,0 +1,255 @@
+/*
+ * 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.ignite.internal.processors.cache.persistence.snapshot;
+
+import java.util.Collections;
+import java.util.function.Function;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.IgniteDataStreamer;
+import org.apache.ignite.IgniteException;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.encryption.AbstractEncryptionTest;
+import org.apache.ignite.internal.util.distributed.FullMessage;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.util.typedef.internal.CU;
+import org.apache.ignite.lang.IgniteFuture;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+import org.junit.runners.Parameterized;
+
+/**
+ * Snapshot test for encrypted snapshots.
+ */
+public class EncryptedSnapshotTest extends AbstractSnapshotSelfTest {
+ /** Parameters. */
+ @Parameterized.Parameters(name = "Encryption is enabled.")
+ public static Iterable<Boolean> enableEncryption() {
+ return Collections.singletonList(true);
+ }
+
+ /** Name of additional encrypted cache. */
+ private static final String NOT_ENCRYPTED_CACHE_NAME = "notEncryptedCache";
+
+ /** {@inheritDoc} */
+ @Override protected Function<Integer, Object> valueBuilder() {
+ return (i -> new Account(i, i));
+ }
+
+ /** Checks re-encryption fails during snapshot restoration. */
+ @Test
+ public void testReencryptDuringRestore() throws Exception {
+ checkActionFailsDuringSnapshotOperation(true, this::chageCacheGroupKey, "Cache group key change was " +
+ "rejected.", IgniteException.class);
+ }
+
+ /** Checks master key changing failes during snapshot restoration. */
+ @Test
+ public void testMasterKeyChangeDuringRestore() throws Exception {
+ checkActionFailsDuringSnapshotOperation(true, this::chageMasterKey, "Master key change was rejected.",
+ IgniteException.class);
+ }
+
+ /** Checks re-encryption fails during snapshot creation. */
+ @Test
+ public void testReencryptDuringSnapshot() throws Exception {
+ checkActionFailsDuringSnapshotOperation(false, this::chageCacheGroupKey, "Cache group key change was " +
+ "rejected.", IgniteException.class);
+ }
+
+ /** Checks master key changing fails during snapshot creation. */
+ @Test
+ public void testMasterKeyChangeDuringSnapshot() throws Exception {
+ checkActionFailsDuringSnapshotOperation(false, this::chageMasterKey, "Master key change was rejected.",
+ IgniteException.class);
+ }
+
+ /** Checks snapshot action fail during cache group key change. */
+ @Test
+ public void testSnapshotFailsDuringCacheKeyChange() throws Exception {
+ checkSnapshotActionFailsDuringReencryption(this::chageCacheGroupKey);
+ }
+
+ /** Checks snapshot action fail during master key cnahge. */
+ @Test
+ public void testSnapshotFailsDuringMasterKeyChange() throws Exception {
+ checkSnapshotActionFailsDuringReencryption(this::chageMasterKey);
+ }
+
+ /** Checks snapshot restoration fails if different master key is contained in the snapshot. */
+ @Test
+ public void testStartFromSnapshotFailedWithOtherMasterKey() throws Exception {
+ IgniteEx ig = startGridsWithCache(1, CACHE_KEYS_RANGE, valueBuilder(), dfltCacheCfg);
+
+ ig.snapshot().createSnapshot(SNAPSHOT_NAME).get();
+
+ ig.destroyCache(dfltCacheCfg.getName());
+
+ ensureCacheAbsent(dfltCacheCfg);
+
+ stopAllGrids(false);
+
+ masterKeyName = AbstractEncryptionTest.MASTER_KEY_NAME_2;
+
+ GridTestUtils.assertThrowsAnyCause(
+ log,
+ () -> startGridsFromSnapshot(1, SNAPSHOT_NAME),
+ IgniteCheckedException.class,
+ "bad key is used during decryption"
+ );
+ }
+
+ /** Checks it is unavailable to register snapshot task for encrypted caches witout metastore. */
+ @Test
+ public void testSnapshotTaskIsBlockedWithoutMetastore() throws Exception {
+ // Start grid node with data before each test.
+ IgniteEx ig = startGridsWithCache(1, CACHE_KEYS_RANGE, valueBuilder(), dfltCacheCfg);
+
+ GridTestUtils.assertThrowsAnyCause(log,
+ () -> snp(ig).registerSnapshotTask(SNAPSHOT_NAME, ig.localNode().id(), F.asMap(CU.cacheId(dfltCacheCfg.getName()), null),
+ false, snp(ig).localSnapshotSenderFactory().apply(SNAPSHOT_NAME)).get(TIMEOUT),
+ IgniteCheckedException.class,
+ "Metastore is requird because it contains encryption keys");
Review comment:
s/requird/required/
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotCheckTest.java
##########
@@ -554,6 +566,77 @@ public void testClusterSnapshotCheckMultipleTimes() throws Exception {
assertTrue("Threads created: " + createdThreads, createdThreads < iterations);
}
+ /** Checks bytes, signature of partion files. Compares before and after snapshot. Assumes the files are equal. */
+ @Test
+ public void testSnapshotLocalPartitions() throws Exception {
+ // Note: this test is valid only for not-encrypted snapshots. Writting snapshots deltas causes several page writes into file.
Review comment:
there and below: s/Writting/Writing/
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/EncryptedSnapshotTest.java
##########
@@ -0,0 +1,255 @@
+/*
+ * 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.ignite.internal.processors.cache.persistence.snapshot;
+
+import java.util.Collections;
+import java.util.function.Function;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.IgniteDataStreamer;
+import org.apache.ignite.IgniteException;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.encryption.AbstractEncryptionTest;
+import org.apache.ignite.internal.util.distributed.FullMessage;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.util.typedef.internal.CU;
+import org.apache.ignite.lang.IgniteFuture;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+import org.junit.runners.Parameterized;
+
+/**
+ * Snapshot test for encrypted snapshots.
+ */
+public class EncryptedSnapshotTest extends AbstractSnapshotSelfTest {
Review comment:
There are only failure-pass tests. What about add some happy-pass cases? For example:
1. Currently there is no check that data in snapshot is encrypted indeed. And it doesn't affect data for non-encrypted cache.
2. Is it a valuable test to check pipeline: Snapshot -> optional Restore -> Reencryption -> Snapshot -> Restore?
3. Check that doesn't fail pipeline: Create non-encrypted cache -> Snapshot -> Destroy -> Create an encrypted-cache with the same name, and the same in reverse order.
Maybe there are some more happy cases to test? WDYT?
##########
File path: modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotCheckWithIndexesTest.java
##########
@@ -36,6 +38,12 @@
* Cluster-wide snapshot test check command with indexes.
*/
public class IgniteClusterSnapshotCheckWithIndexesTest extends AbstractSnapshotSelfTest {
+ /** Parameters. Encryption is not supported by snapshot validation. */
Review comment:
What is result if we start validate snapshot with both encrypted and non-encrypted caches? Can we check only non-encrypted caches then?
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java
##########
@@ -1505,6 +1508,16 @@ SnapshotFutureTask registerSnapshotTask(
if (prev != null)
return new SnapshotFutureTask(new IgniteCheckedException("Snapshot with requested name is already scheduled: " + snpName));
+ for (Integer grpId : parts.keySet()) {
+ if (!withMetaStorage && cctx.cache().isEncrypted(grpId)) {
+ snpFutTask.onDone(new IgniteCheckedException("Snapshot contains encrypted cache group " + grpId + " but doesn't " +
+ "include metastore. Metastore is requird because it contains encryption keys required to start with encrypted " +
Review comment:
s/requird/required/
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotCheckTest.java
##########
@@ -554,6 +566,77 @@ public void testClusterSnapshotCheckMultipleTimes() throws Exception {
assertTrue("Threads created: " + createdThreads, createdThreads < iterations);
}
+ /** Checks bytes, signature of partion files. Compares before and after snapshot. Assumes the files are equal. */
+ @Test
+ public void testSnapshotLocalPartitions() throws Exception {
+ // Note: this test is valid only for not-encrypted snapshots. Writting snapshots deltas causes several page writes into file.
+ // Every page write calls encrypt(). Every repatable encrypt() produces different record (different bytes) even for same original
Review comment:
s/repatable/repeatable/ ?
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotCheckTest.java
##########
@@ -554,6 +566,77 @@ public void testClusterSnapshotCheckMultipleTimes() throws Exception {
assertTrue("Threads created: " + createdThreads, createdThreads < iterations);
}
+ /** Checks bytes, signature of partion files. Compares before and after snapshot. Assumes the files are equal. */
+ @Test
+ public void testSnapshotLocalPartitions() throws Exception {
+ // Note: this test is valid only for not-encrypted snapshots. Writting snapshots deltas causes several page writes into file.
+ // Every page write calls encrypt(). Every repatable encrypt() produces different record (different bytes) even for same original
+ // data. Re-writting pages from delta to partition file in the shanpshot leads to additional encryption before writting to the
+ // snapshot partition file. Thus, page in original partition and in snapshot partiton has different encrypted CRC and same
+ // de-crypted CRC. Different encrypted CRC looks like different data in point of view of third-party observer.
Review comment:
> partiton has different encrypted CRC and same de-crypted CRC.
Why we can't deencrypt them before this check?
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java
##########
@@ -1438,9 +1439,11 @@ public StandaloneGridKernalContext createStandaloneKernalContext(String snpName,
File snpPart = getPartitionFile(new File(snapshotLocalDir(snpName), databaseRelativePath(folderName)),
grps.get(0).getName(), partId);
- FilePageStore pageStore = (FilePageStore)storeFactory
- .apply(CU.cacheId(grpName), false)
- .createPageStore(getTypeByPartId(partId),
+ int grpId = CU.cacheId(grpName);
+
+ FilePageStore pageStore = (FilePageStore)storeMgr.getPageStoreFactory(grpId,
+ cctx.kernalContext().encryption().getActiveKey(grpId) != null).
Review comment:
In this file on 2021 line you have different logic to find whether a CacheGroup is encrypted or not. What is a difference? Trying to leverage on internal collection to get info about configuration looks strange for me.
`boolean encrypted = cctx.cache().isEncrypted(pair.getGroupId());`
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotFutureTask.java
##########
@@ -620,9 +617,9 @@ private void addPartitionWriters(int grpId, Set<Integer> parts, String dirName)
PageStore store = pageStore.getStore(grpId, partId);
- partDeltaWriters.put(pair,
- new PageStoreSerialWriter(store,
- partDeltaFile(cacheWorkDir(tmpConsIdDir, dirName), partId)));
+ partDeltaWriters.put(pair, new PageStoreSerialWriter(store,
+ partDeltaFile(cacheWorkDir(tmpConsIdDir, dirName), partId),
+ cctx.cache().isEncrypted(grpId) ? grpId : null));
Review comment:
Can move getting `encryptionGrpId` before loop.
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotSelfTest.java
##########
@@ -330,6 +330,10 @@ public void testClusterSnapshotConsistencyUnderLoad() throws Exception {
startGridsWithCache(grids, clientsCnt, key -> new Account(key, balance), eastCcfg, westCcfg);
+ // To prevent encrypted cache creation from client, we create it on the server side.
Review comment:
Don't found such restriction in docs. What is a reason?
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/file/EncryptedFileIO.java
##########
@@ -179,10 +179,14 @@
/** {@inheritDoc} */
@Override public int write(ByteBuffer srcBuf) throws IOException {
- assert position() == 0;
- assert headerSize == srcBuf.capacity();
+ if (headerSize > 0) {
Review comment:
I think we need more javadocs in this class about a structure of a page file, or provide a link to such doc.
It's worth to mention about header (is not encrypted by design, can be set to 0 for some specific cases). Also it looks like implementation of methods depends on headerSize. For example we can invoke `read(ByteBuffer)` only if `headerSize > 0`. But we can `write(ByteBuffer)` if `headerSize == 0`. Also `read(ByteBuffer, position)` and `readFully(ByteBuffer, position)` has different checks but those methods in parent class FileIO do have the same javadocs. Looks very unclear without specific docs.
Also unclear that the `write` methods make rewind() while the `read` methods don't. Should we mention it in docs too?
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/file/EncryptedFileIO.java
##########
@@ -109,7 +109,7 @@
/** {@inheritDoc} */
@Override public int read(ByteBuffer destBuf) throws IOException {
- assert position() == 0;
+ assert position() == 0 && headerSize > 0;
return plainFileIO.read(destBuf);
Review comment:
Why we don't decrypt there? The other method `read(ByteBuffer, long)` decrypts underlying data.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org