You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2021/07/09 19:10:12 UTC
[GitHub] [accumulo] milleruntime opened a new pull request #2197: Per table crypto + other crypto improvements
milleruntime opened a new pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197
* Closes #2001
* Split crypto properties up into WAL and Table
* Use TableConfiguration to get CryptoService from table properties
* Change scope in CryptoEnvironment to WAL and Table
* Create CryptoServiceFactory.none() method to return no crypto
* Make CryptoEnvironment return Optional for getDecryptionParams
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#issuecomment-877504554
> Depending on which PR is merged first, the properties changed here should match the design set in #2190.
I'll finish reviewing #2190 first, since that's much more narrowly scoped (and fewer than 70 files to review!)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r678556703
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -176,22 +176,6 @@
"One-line configuration property controlling the network locations "
+ "(hostnames) that are allowed to impersonate other users",
"1.7.1"),
- // Crypto-related properties
- @Experimental
- INSTANCE_CRYPTO_PREFIX("instance.crypto.opts.", null, PropertyType.PREFIX,
- "Properties related to on-disk file encryption.", "2.0.0"),
- @Experimental
- @Sensitive
- INSTANCE_CRYPTO_SENSITIVE_PREFIX("instance.crypto.opts.sensitive.", null, PropertyType.PREFIX,
- "Sensitive properties related to on-disk file encryption.", "2.0.0"),
- @Experimental
- INSTANCE_CRYPTO_SERVICE("instance.crypto.service",
- "org.apache.accumulo.core.spi.crypto.NoCryptoService", PropertyType.CLASSNAME,
- "The class which executes on-disk file encryption. The default does nothing. To enable "
- + "encryption, replace this classname with an implementation of the"
- + "org.apache.accumulo.core.spi.crypto.CryptoService interface.",
- "2.0.0"),
-
Review comment:
Yeah, I can see it both ways: "wal" is a tserver feature, but "crypto" support is an instance feature. I see this as somewhat more closely aligned with the way volume chooser does things, where it's specified at the system scope (using `general.` prefix) but a specific implementation could delegate to per-table config to act in a per-table way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r674860371
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
##########
@@ -19,8 +19,10 @@
package org.apache.accumulo.server;
import static com.google.common.base.Preconditions.checkArgument;
+import static org.apache.accumulo.core.spi.crypto.CryptoEnvironment.Scope.WAL;
import java.io.IOException;
+import java.util.Map;
Review comment:
```suggestion
```
If tableCryptoMap is removed I think this can be as well.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r674858942
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
##########
@@ -65,7 +67,8 @@
private DefaultConfiguration defaultConfig = null;
private AccumuloConfiguration systemConfig = null;
private AuthenticationTokenSecretManager secretManager;
- private CryptoService cryptoService = null;
+ private CryptoService walCryptoService = null;
+ private Map<TableId,CryptoService> tableCryptoMap;
Review comment:
```suggestion
```
It doesn't look like this is used anywhere.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r675108757
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
##########
@@ -65,7 +67,8 @@
private DefaultConfiguration defaultConfig = null;
private AccumuloConfiguration systemConfig = null;
private AuthenticationTokenSecretManager secretManager;
- private CryptoService cryptoService = null;
+ private CryptoService walCryptoService = null;
+ private Map<TableId,CryptoService> tableCryptoMap;
Review comment:
Also, we had previously done work in ServerContext to make sure only one Crypto Service was created per tserver. If we want to continue this behavior on the per table level, we will need to do something like a cache to make sure each table only creates one Crypto Service. I am not sure how this could be done though as tablets could be spread across different tablet servers. We may have to do something in the Manager where the tservers ask for the CryptoService per table.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#issuecomment-884889331
> > Depending on which PR is merged first, the properties changed here should match the design set in #2190.
>
> I'll finish reviewing #2190 so we can get that in first, since that's much more narrowly scoped (and fewer than 70 files to review!)
Sounds good. I don't think the complexity of this change is that bad though. And don't let the number intimidate you, more than half of those are Tests.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r675104899
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/InMemoryMap.java
##########
@@ -754,9 +758,11 @@ public void delete(long waitTime) {
aconf = createSampleConfig(aconf);
}
+ // TODO can we drop the crateSampleConfig and just use tableConf?
Review comment:
I added this comment to look at this again and I don't think we can. Removed comment in 515e963.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r679458629
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -894,6 +893,21 @@
TABLE_COMPACTION_STRATEGY_PREFIX("table.majc.compaction.strategy.opts.", null,
PropertyType.PREFIX,
"Properties in this category are used to configure the compaction strategy.", "1.6.0"),
+ // Crypto-related properties
+ @Experimental
+ TABLE_CRYPTO_PREFIX("table.crypto.opts.", null, PropertyType.PREFIX,
+ "Properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ @Sensitive
+ TABLE_CRYPTO_SENSITIVE_PREFIX("table.crypto.opts.sensitive.", null, PropertyType.PREFIX,
+ "Sensitive properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ TABLE_CRYPTO_SERVICE("table.crypto.service",
+ "org.apache.accumulo.core.spi.crypto.NoCryptoService", PropertyType.CLASSNAME,
+ "The class which executes on-disk table encryption/decryption. The default does "
+ + "nothing. This property must be a classname with an implementation of the "
+ + "org.apache.accumulo.core.spi.crypto.CryptoService interface.",
+ "2.1.0"),
Review comment:
OK. Yeah I like `table.crypto.` better as well.
If we are not specifying the crypto service in the table config, do we use the same crypto service for all tables? If not, what in the table config will determine which crypto service to use. Based on what you are saying, I am imagining a config like this:
<pre>
instance.crypto.factory=MyCryptoServiceFactoryThatSupportsPerTableConfig
instance.wal.enabled=true
instance.crypto.wal.opts.key.uri=/home/mike/workspace/uno/CryptoTest-testkeyfile
secureTable:
table.crypto.enabled=true
table.crypto.type=strong
table.crypto.opts.key=/something/more/secure/than/a/file
lessSecureTable:
table.crypto.enabled=true
table.crypto.type=weak
table.crypto.opts.key.uri=/home/mike/workspace/uno/CryptoTest-testkeyfile2
</pre>
With the impl of MyCryptoServiceFactoryThatSupportsPerTableConfig looking like this:
<pre>
if scope == wal && enabled, return AESCryptoService
if scope == table && enabled && type=strong, return StrongerCryptoService
if scope == table && enabled && type=weak, return WeakerCryptoService
</pre>
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#issuecomment-888322764
I tested this branch on Uno locally using continuous ingest and was able to ingest 160 million entries in 20 minutes. The data verified correctly in about 15 minutes.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r678459279
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -176,22 +176,6 @@
"One-line configuration property controlling the network locations "
+ "(hostnames) that are allowed to impersonate other users",
"1.7.1"),
- // Crypto-related properties
- @Experimental
- INSTANCE_CRYPTO_PREFIX("instance.crypto.opts.", null, PropertyType.PREFIX,
- "Properties related to on-disk file encryption.", "2.0.0"),
- @Experimental
- @Sensitive
- INSTANCE_CRYPTO_SENSITIVE_PREFIX("instance.crypto.opts.sensitive.", null, PropertyType.PREFIX,
- "Sensitive properties related to on-disk file encryption.", "2.0.0"),
- @Experimental
- INSTANCE_CRYPTO_SERVICE("instance.crypto.service",
- "org.apache.accumulo.core.spi.crypto.NoCryptoService", PropertyType.CLASSNAME,
- "The class which executes on-disk file encryption. The default does nothing. To enable "
- + "encryption, replace this classname with an implementation of the"
- + "org.apache.accumulo.core.spi.crypto.CryptoService interface.",
- "2.0.0"),
-
Review comment:
Part of the benefit of having crypto properties as `instance.` properties ensures that all tservers are using the same config. The potential for different tservers to have different crypto services means that data written by one tserver might not be able to be read by another, as they could have wildly different behaviors. This is one reason why I think we should keep an `instance.` property to refer to the crypto service factory, which is the overall module that supports all the different crypto that will be needed, and that per-tserver and per-table properties should only modify the behavior of the crypto services produced by this factory with some options.
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -894,6 +893,21 @@
TABLE_COMPACTION_STRATEGY_PREFIX("table.majc.compaction.strategy.opts.", null,
PropertyType.PREFIX,
"Properties in this category are used to configure the compaction strategy.", "1.6.0"),
+ // Crypto-related properties
+ @Experimental
+ TABLE_CRYPTO_PREFIX("table.crypto.opts.", null, PropertyType.PREFIX,
+ "Properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ @Sensitive
+ TABLE_CRYPTO_SENSITIVE_PREFIX("table.crypto.opts.sensitive.", null, PropertyType.PREFIX,
+ "Sensitive properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ TABLE_CRYPTO_SERVICE("table.crypto.service",
+ "org.apache.accumulo.core.spi.crypto.NoCryptoService", PropertyType.CLASSNAME,
+ "The class which executes on-disk table encryption/decryption. The default does "
+ + "nothing. This property must be a classname with an implementation of the "
+ + "org.apache.accumulo.core.spi.crypto.CryptoService interface.",
+ "2.1.0"),
Review comment:
If there were a crypto service factory specified as an `instance.` option, then there's no need to specify a specific crypto service here, as that would be determined by the factory, given the TABLE scope and the options (either the options in the config, in the case of writing a new file, or the options read from the file in the case of reading a previously written file).
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -474,6 +458,21 @@
PropertyType.TIMEDURATION,
"The maximum amount of time to wait after a failure to create or write a write-ahead log.",
"1.7.1"),
+ // Crypto-related properties
+ @Experimental
+ TSERV_WAL_CRYPTO_PREFIX("tserver.wal.crypto.opts.", null, PropertyType.PREFIX,
+ "Properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ @Sensitive
+ TSERV_WAL_CRYPTO_SENSITIVE_PREFIX("tserver.wal.crypto.opts.sensitive.", null, PropertyType.PREFIX,
+ "Sensitive properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ TSERV_WAL_CRYPTO_SERVICE("tserver.wal.crypto.service",
+ "org.apache.accumulo.core.spi.crypto.NoCryptoService", PropertyType.CLASSNAME,
+ "The class which executes on-disk write ahead log encryption/decryption. The default does "
+ + "nothing. This property must be a classname with an implementation of the "
+ + "org.apache.accumulo.core.spi.crypto.CryptoService interface.",
+ "2.1.0"),
Review comment:
I would expect all tservers to use the same WAL options, so that files written by one tserver will be able to be read by another. This implies the use of `instance.` properties for WAL crypto options, not `tserver.` Also, if a crypto service factory is specified in an `instance.` property, then there's no need to specify a specific crypto service here, as the factory would make that decision based on the options and scope.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r674804415
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/InMemoryMap.java
##########
@@ -754,9 +758,11 @@ public void delete(long waitTime) {
aconf = createSampleConfig(aconf);
}
+ // TODO can we drop the crateSampleConfig and just use tableConf?
Review comment:
```suggestion
// TODO can we drop the createSampleConfig and just use tableConf?
```
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r678509814
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -176,22 +176,6 @@
"One-line configuration property controlling the network locations "
+ "(hostnames) that are allowed to impersonate other users",
"1.7.1"),
- // Crypto-related properties
- @Experimental
- INSTANCE_CRYPTO_PREFIX("instance.crypto.opts.", null, PropertyType.PREFIX,
- "Properties related to on-disk file encryption.", "2.0.0"),
- @Experimental
- @Sensitive
- INSTANCE_CRYPTO_SENSITIVE_PREFIX("instance.crypto.opts.sensitive.", null, PropertyType.PREFIX,
- "Sensitive properties related to on-disk file encryption.", "2.0.0"),
- @Experimental
- INSTANCE_CRYPTO_SERVICE("instance.crypto.service",
- "org.apache.accumulo.core.spi.crypto.NoCryptoService", PropertyType.CLASSNAME,
- "The class which executes on-disk file encryption. The default does nothing. To enable "
- + "encryption, replace this classname with an implementation of the"
- + "org.apache.accumulo.core.spi.crypto.CryptoService interface.",
- "2.0.0"),
-
Review comment:
That makes sense. I had only gone with "tserver" for "wal" properties since that is where most of the write ahead log configuration lives.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r674970102
##########
File path: server/base/src/test/java/org/apache/accumulo/server/client/BulkImporterTest.java
##########
@@ -112,11 +113,15 @@ public void testFindOverlappingTablets() throws Exception {
MockTabletLocator locator = new MockTabletLocator();
FileSystem fs = FileSystem.getLocal(new Configuration());
ServerContext context = MockServerContext.get();
+ CryptoService cs = CryptoServiceFactory.none();
+ // TableConfiguration tableConf = EasyMock.createMock(TableConfiguration.class);
+ // expect(context.getTableConfiguration(tableId)).andReturn(tableConf).anyTimes();
+ // expect(tableConf.getCryptoService()).andReturn(CryptoServiceFactory.none()).anyTimes();
Review comment:
Yes I had left these lines in the test while it was failing but I recently fixed it. It is passing now so I'll remove them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r674858942
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
##########
@@ -65,7 +67,8 @@
private DefaultConfiguration defaultConfig = null;
private AccumuloConfiguration systemConfig = null;
private AuthenticationTokenSecretManager secretManager;
- private CryptoService cryptoService = null;
+ private CryptoService walCryptoService = null;
+ private Map<TableId,CryptoService> tableCryptoMap;
Review comment:
```suggestion
private Map<TableId,CryptoService> tableCryptoMap;
```
It doesn't look like this is used anywhere.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r680961733
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -894,6 +893,21 @@
TABLE_COMPACTION_STRATEGY_PREFIX("table.majc.compaction.strategy.opts.", null,
PropertyType.PREFIX,
"Properties in this category are used to configure the compaction strategy.", "1.6.0"),
+ // Crypto-related properties
+ @Experimental
+ TABLE_CRYPTO_PREFIX("table.crypto.opts.", null, PropertyType.PREFIX,
+ "Properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ @Sensitive
+ TABLE_CRYPTO_SENSITIVE_PREFIX("table.crypto.opts.sensitive.", null, PropertyType.PREFIX,
+ "Sensitive properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ TABLE_CRYPTO_SERVICE("table.crypto.service",
+ "org.apache.accumulo.core.spi.crypto.NoCryptoService", PropertyType.CLASSNAME,
+ "The class which executes on-disk table encryption/decryption. The default does "
+ + "nothing. This property must be a classname with an implementation of the "
+ + "org.apache.accumulo.core.spi.crypto.CryptoService interface.",
+ "2.1.0"),
Review comment:
OK cool, I like where this is going. So it looks like these are the only crypto properties we need to bake into Accumulo's Property.java:
<pre>
instance.crypto.factory (don't think we need service in the name)
instance.crypto.wal.opts.
instance.crypto.wal.opts.sensitive.
table.crypto.opts.
table.crypto.opts.sensitive
</pre>
Do you think we need to keep the two we have now `instance.crypto.opts.` and `instance.crypto.opts.sensitive.`? The only thing I can think of is if a user needs generic properties for the factory itself.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r678511778
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -474,6 +458,21 @@
PropertyType.TIMEDURATION,
"The maximum amount of time to wait after a failure to create or write a write-ahead log.",
"1.7.1"),
+ // Crypto-related properties
+ @Experimental
+ TSERV_WAL_CRYPTO_PREFIX("tserver.wal.crypto.opts.", null, PropertyType.PREFIX,
+ "Properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ @Sensitive
+ TSERV_WAL_CRYPTO_SENSITIVE_PREFIX("tserver.wal.crypto.opts.sensitive.", null, PropertyType.PREFIX,
+ "Sensitive properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ TSERV_WAL_CRYPTO_SERVICE("tserver.wal.crypto.service",
+ "org.apache.accumulo.core.spi.crypto.NoCryptoService", PropertyType.CLASSNAME,
+ "The class which executes on-disk write ahead log encryption/decryption. The default does "
+ + "nothing. This property must be a classname with an implementation of the "
+ + "org.apache.accumulo.core.spi.crypto.CryptoService interface.",
+ "2.1.0"),
Review comment:
I will try out both types of configuration using `instance` properties.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r680157387
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -894,6 +893,21 @@
TABLE_COMPACTION_STRATEGY_PREFIX("table.majc.compaction.strategy.opts.", null,
PropertyType.PREFIX,
"Properties in this category are used to configure the compaction strategy.", "1.6.0"),
+ // Crypto-related properties
+ @Experimental
+ TABLE_CRYPTO_PREFIX("table.crypto.opts.", null, PropertyType.PREFIX,
+ "Properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ @Sensitive
+ TABLE_CRYPTO_SENSITIVE_PREFIX("table.crypto.opts.sensitive.", null, PropertyType.PREFIX,
+ "Sensitive properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ TABLE_CRYPTO_SERVICE("table.crypto.service",
+ "org.apache.accumulo.core.spi.crypto.NoCryptoService", PropertyType.CLASSNAME,
+ "The class which executes on-disk table encryption/decryption. The default does "
+ + "nothing. This property must be a classname with an implementation of the "
+ + "org.apache.accumulo.core.spi.crypto.CryptoService interface.",
+ "2.1.0"),
Review comment:
> If we are not specifying the crypto service in the table config, do we use the same crypto service for all tables? If not, what in the table config will determine which crypto service to use.
I think that would be implementation-dependent.
> Based on what you are saying, I am imagining a config like this:
>
> instance.crypto.factory=MyCryptoServiceFactoryThatSupportsPerTableConfig
> instance.wal.enabled=true
> instance.crypto.wal.opts.key.uri=/home/mike/workspace/uno/CryptoTest-testkeyfile
> secureTable:
> table.crypto.enabled=true
> table.crypto.type=strong
> table.crypto.opts.key=/something/more/secure/than/a/file
> lessSecureTable:
> table.crypto.enabled=true
> table.crypto.type=weak
> table.crypto.opts.key.uri=/home/mike/workspace/uno/CryptoTest-testkeyfile2
> With the impl of MyCryptoServiceFactoryThatSupportsPerTableConfig looking like this:
>
> if scope == wal && enabled, return AESCryptoService
> if scope == table && enabled && type=strong, return StrongerCryptoService
> if scope == table && enabled && type=weak, return WeakerCryptoService
Something like that. I would probably just put everything under `table.crypto.opts.`, though. So, something closer to:
```ini
instance.crypto.service.factory=PerTableAESCryptoFactory
instance.crypto.wal.opts.algorithm=AES256+CBC
instance.crypto.wal.opts.key.uri=/path/to/keyfile
; no crypto table (still provide the key, in case it needs to read old data):
table.crypto.opts.algorithm=NONE
table.crypto.opts.key.uri=/path/to/keyfile
; table A:
table.crypto.opts.algorithm=AES128+ECB
table.crypto.opts.key.uri=/path/to/keyfile
; table B:
table.crypto.opts.algorithm=AES512+ECB
table.crypto.opts.key.uri=/path/to/keyfile
```
inside PerTableAESCryptoFactory
```java
if (algorithm == NONE) return new CryptoService(new NoEncrypter(), new AESDecrypter(key));
else return new CryptoService(new AESEncrypter(algorithm, key), new AESDecrypter(key));
```
However, exactly how/where the algorithm/key/other opts get passed depends on what the interfaces look like exactly, and at what point are they called.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#issuecomment-892687796
@ctubbsii mind taking a quick high level look at this branch before I redo the tests? I think I implemented the changes we discussed and got everything compiling (ignore tests for now). Changes include:
- I dropped the `init` method from CryptoService
- Added TableId to CryptoEnvironment
- Created NoCryptoFactory for no crypto
- Used NoCryptoFactory in RFile API, until we figure out how to support crypto in the RFile API
- Implemented CryptoFactoryLoader for loading the factory and services
- Created a basic factory called PerTableCryptoFactory
Classes to take a quick look at:
[Property](https://github.com/apache/accumulo/pull/2197/files#diff-6c2d215eebb1a6157a2b7d95f49496834bd91a150c8aedd97cca6b01aedc05cb)
[CryptoFactoryLoader](https://github.com/apache/accumulo/pull/2197/files#diff-6bc04f7fde1143ced2a2ef3097baf6429467690b93557e44c0c9c2542b6d32e9)
[PerTableCryptoFactory](https://github.com/apache/accumulo/pull/2197/files#diff-8a243847ef39a9dedc8daceaa1629aca85a4a550b2ff20ec3ec249c0bb917625)
[CryptoFactory interface](https://github.com/apache/accumulo/pull/2197/files#diff-3ab2b08082155f62ed88ac1bb83d9456ece437b562425e255bc76170f3004e7f)
[CryptoEnvironment interface](https://github.com/apache/accumulo/pull/2197/files#diff-e9b7d2e72ebd5675e60efb4646ae6cbe868199c557225ec18db0392d2368b300)
[TableConfiguration](https://github.com/apache/accumulo/pull/2197/files#diff-c8804c4993551d0fc6f293963d664d4e35151f7639955f4c7e2b3a7bf2a694b1)
[ServerContext](https://github.com/apache/accumulo/pull/2197/files#diff-44b9172cc16cdb9b671926c7aed1b0d57372715bc181dd79ef0be76828b708f3)
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r681386459
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -894,6 +893,21 @@
TABLE_COMPACTION_STRATEGY_PREFIX("table.majc.compaction.strategy.opts.", null,
PropertyType.PREFIX,
"Properties in this category are used to configure the compaction strategy.", "1.6.0"),
+ // Crypto-related properties
+ @Experimental
+ TABLE_CRYPTO_PREFIX("table.crypto.opts.", null, PropertyType.PREFIX,
+ "Properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ @Sensitive
+ TABLE_CRYPTO_SENSITIVE_PREFIX("table.crypto.opts.sensitive.", null, PropertyType.PREFIX,
+ "Sensitive properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ TABLE_CRYPTO_SERVICE("table.crypto.service",
+ "org.apache.accumulo.core.spi.crypto.NoCryptoService", PropertyType.CLASSNAME,
+ "The class which executes on-disk table encryption/decryption. The default does "
+ + "nothing. This property must be a classname with an implementation of the "
+ + "org.apache.accumulo.core.spi.crypto.CryptoService interface.",
+ "2.1.0"),
Review comment:
> So it looks like these are the only crypto properties we need to bake into Accumulo's Property.java:
Looks mostly right to me. But, I'm not sure about the "sensitive" stuff. I think maybe we could just assume all crypto parameters are "sensitive". This really only has to do with returning the config via the public API for printing in the shell. We could require admin permission to retrieve/view any of those without creating two maps for all the opts.
> instance.crypto.factory (don't think we need service in the name)
Agreed on the parenthetical.
> Do you think we need to keep the two we have now `instance.crypto.opts.` and `instance.crypto.opts.sensitive.`? The only thing I can think of is if a user needs generic properties for the factory itself.
I'm not sure. If you use them, then the interface would probably have to require an `init(Map<String,String> opts, Map<String,String> sensitiveOpts)` method, in addition to a no-arg constructor to instantiate the class. (or just a single map parameter, if we drop the separate set of sensitive props).
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#issuecomment-892687796
@ctubbsii mind taking a quick high level look at this branch before I redo the tests? I think I implemented the changes we discussed and got everything compiling (ignore tests for now). Changes include:
- I dropped the `init` method from CryptoService
- Added TableId to CryptoEnvironment
- Created NoCryptoFactory for no crypto
- Used NoCryptoFactory in RFile API, until we figure out how to support crypto in the RFile API
- Implemented CryptoFactoryLoader for loading the factory and services
- Created a basic factory called PerTableCryptoFactory
Classes to take a quick look at:
[Property](https://github.com/apache/accumulo/pull/2197/files#diff-6c2d215eebb1a6157a2b7d95f49496834bd91a150c8aedd97cca6b01aedc05cb)
[CryptoFactoryLoader](https://github.com/apache/accumulo/pull/2197/files#diff-6bc04f7fde1143ced2a2ef3097baf6429467690b93557e44c0c9c2542b6d32e9)
[PerTableCryptoFactory](https://github.com/apache/accumulo/pull/2197/files#diff-8a243847ef39a9dedc8daceaa1629aca85a4a550b2ff20ec3ec249c0bb917625)
[CryptoFactory interface](https://github.com/apache/accumulo/pull/2197/files#diff-3ab2b08082155f62ed88ac1bb83d9456ece437b562425e255bc76170f3004e7f)
[CryptoEnvironment interface](https://github.com/apache/accumulo/pull/2197/files#diff-e9b7d2e72ebd5675e60efb4646ae6cbe868199c557225ec18db0392d2368b300)
[TableConfiguration](https://github.com/apache/accumulo/pull/2197/files#diff-c8804c4993551d0fc6f293963d664d4e35151f7639955f4c7e2b3a7bf2a694b1)
[ServerContext](https://github.com/apache/accumulo/pull/2197/files#diff-44b9172cc16cdb9b671926c7aed1b0d57372715bc181dd79ef0be76828b708f3)
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#issuecomment-877473856
Depending on which PR is merged first, the properties changed here should match the design set in #2190.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r674816719
##########
File path: server/base/src/test/java/org/apache/accumulo/server/client/BulkImporterTest.java
##########
@@ -112,11 +113,15 @@ public void testFindOverlappingTablets() throws Exception {
MockTabletLocator locator = new MockTabletLocator();
FileSystem fs = FileSystem.getLocal(new Configuration());
ServerContext context = MockServerContext.get();
+ CryptoService cs = CryptoServiceFactory.none();
+ // TableConfiguration tableConf = EasyMock.createMock(TableConfiguration.class);
+ // expect(context.getTableConfiguration(tableId)).andReturn(tableConf).anyTimes();
+ // expect(tableConf.getCryptoService()).andReturn(CryptoServiceFactory.none()).anyTimes();
Review comment:
Are these 3 comments intentionally present?
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii edited a comment on pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
ctubbsii edited a comment on pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#issuecomment-877504554
> Depending on which PR is merged first, the properties changed here should match the design set in #2190.
I'll finish reviewing #2190 so we can get that in first, since that's much more narrowly scoped (and fewer than 70 files to review!)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r679414846
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -894,6 +893,21 @@
TABLE_COMPACTION_STRATEGY_PREFIX("table.majc.compaction.strategy.opts.", null,
PropertyType.PREFIX,
"Properties in this category are used to configure the compaction strategy.", "1.6.0"),
+ // Crypto-related properties
+ @Experimental
+ TABLE_CRYPTO_PREFIX("table.crypto.opts.", null, PropertyType.PREFIX,
+ "Properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ @Sensitive
+ TABLE_CRYPTO_SENSITIVE_PREFIX("table.crypto.opts.sensitive.", null, PropertyType.PREFIX,
+ "Sensitive properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ TABLE_CRYPTO_SERVICE("table.crypto.service",
+ "org.apache.accumulo.core.spi.crypto.NoCryptoService", PropertyType.CLASSNAME,
+ "The class which executes on-disk table encryption/decryption. The default does "
+ + "nothing. This property must be a classname with an implementation of the "
+ + "org.apache.accumulo.core.spi.crypto.CryptoService interface.",
+ "2.1.0"),
Review comment:
I'm not a fan of the `.custom` property prefix naming convention. It implies that we have a bag of properties segregated between "official" properties in Accumulo property namespaces, and "unofficial" properties in a designated namespace for user-provided plugins. But, our reference implementations use the `.custom` property namespace in some cases but not in others. We're not consistent. I think we should just treat it as one big bag of well-named and well-scoped properties and relax the constraint that requires users to use the `.custom` namespace for their properties.
So, that said, I'd be fine leaving the `table.crypto.`, rather than require they be in `table.custom.crytpo.` or something similar. I'm not suggesting copying the property naming convention from VolumeChooser, just the pattern of letting the single factory instance decide whether and how to use these properties, based on its implementation and the provided scope.
So, imagine a `MyCryptoServiceFactoryThatSupportsPerTableConfig` being set as `instance.crypto.service.factory`. The factory has an API that is called, something like `factory.getCryptoService(scopeConfig)`. So, depending on scope, you'd provide different configs:
```java
var walCryptoService = factory.getCryptoService(systemConf.getAllPropertiesWithPrefixStripped("instance.crypto.wal."));
var tableCryptoServiceForTableA = factory.getCryptoService(tableAConf.getAllPropertiesWithPrefixStripped("table.crypto."));
```
Now, it's up to the implementation of the factory to decide what CryptoService to return, based on the scope and scope config. (My example only shows passing in config properties, but you could pass in the scope as a type as well, or some other object performing a similar function of communicating context to the factory.)
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r678581043
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -894,6 +893,21 @@
TABLE_COMPACTION_STRATEGY_PREFIX("table.majc.compaction.strategy.opts.", null,
PropertyType.PREFIX,
"Properties in this category are used to configure the compaction strategy.", "1.6.0"),
+ // Crypto-related properties
+ @Experimental
+ TABLE_CRYPTO_PREFIX("table.crypto.opts.", null, PropertyType.PREFIX,
+ "Properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ @Sensitive
+ TABLE_CRYPTO_SENSITIVE_PREFIX("table.crypto.opts.sensitive.", null, PropertyType.PREFIX,
+ "Sensitive properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ TABLE_CRYPTO_SERVICE("table.crypto.service",
+ "org.apache.accumulo.core.spi.crypto.NoCryptoService", PropertyType.CLASSNAME,
+ "The class which executes on-disk table encryption/decryption. The default does "
+ + "nothing. This property must be a classname with an implementation of the "
+ + "org.apache.accumulo.core.spi.crypto.CryptoService interface.",
+ "2.1.0"),
Review comment:
Just so we are on the same page, you are saying we should just have one `instance.crypto.service` for the whole instance? Then if you want to specify what tables to use it, say just have a list the tables to encrypt?
`instance.crypto.tables=table1,table2,table3`
`instance.crypto.wal.enabled=true`
I think that would be easier then having something like:
`instance.crypto.service.table1=true`
`instance.crypto.service.table2=true`
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r678639078
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -894,6 +893,21 @@
TABLE_COMPACTION_STRATEGY_PREFIX("table.majc.compaction.strategy.opts.", null,
PropertyType.PREFIX,
"Properties in this category are used to configure the compaction strategy.", "1.6.0"),
+ // Crypto-related properties
+ @Experimental
+ TABLE_CRYPTO_PREFIX("table.crypto.opts.", null, PropertyType.PREFIX,
+ "Properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ @Sensitive
+ TABLE_CRYPTO_SENSITIVE_PREFIX("table.crypto.opts.sensitive.", null, PropertyType.PREFIX,
+ "Sensitive properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ TABLE_CRYPTO_SERVICE("table.crypto.service",
+ "org.apache.accumulo.core.spi.crypto.NoCryptoService", PropertyType.CLASSNAME,
+ "The class which executes on-disk table encryption/decryption. The default does "
+ + "nothing. This property must be a classname with an implementation of the "
+ + "org.apache.accumulo.core.spi.crypto.CryptoService interface.",
+ "2.1.0"),
Review comment:
No. I'm saying we should have one `instance.crypto.service.factory`, and that should spit out crypto services based on the per-table config. This is essentially how the VolumeManager per-table feature works.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r679157857
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -894,6 +893,21 @@
TABLE_COMPACTION_STRATEGY_PREFIX("table.majc.compaction.strategy.opts.", null,
PropertyType.PREFIX,
"Properties in this category are used to configure the compaction strategy.", "1.6.0"),
+ // Crypto-related properties
+ @Experimental
+ TABLE_CRYPTO_PREFIX("table.crypto.opts.", null, PropertyType.PREFIX,
+ "Properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ @Sensitive
+ TABLE_CRYPTO_SENSITIVE_PREFIX("table.crypto.opts.sensitive.", null, PropertyType.PREFIX,
+ "Sensitive properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ TABLE_CRYPTO_SERVICE("table.crypto.service",
+ "org.apache.accumulo.core.spi.crypto.NoCryptoService", PropertyType.CLASSNAME,
+ "The class which executes on-disk table encryption/decryption. The default does "
+ + "nothing. This property must be a classname with an implementation of the "
+ + "org.apache.accumulo.core.spi.crypto.CryptoService interface.",
+ "2.1.0"),
Review comment:
We would use the `table.custom` the same way? The volume chooser uses the same class type for `general` properties and for `tables.custom` properties. And what about the wals? Would they have their own factory? Maybe if you give an example of your idea of a config that would help.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r675628522
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
##########
@@ -65,7 +67,8 @@
private DefaultConfiguration defaultConfig = null;
private AccumuloConfiguration systemConfig = null;
private AuthenticationTokenSecretManager secretManager;
- private CryptoService cryptoService = null;
+ private CryptoService walCryptoService = null;
+ private Map<TableId,CryptoService> tableCryptoMap;
Review comment:
> This was an unfinished idea I had to cache the crypto Services in ServerContext so they don't need to be created every time.
@milleruntime I am looking at this comment in isolation, so my response may not be applicable. Its possible you could use a configuration Deriver to create this instead of the Map. Some examples
https://github.com/apache/accumulo/blob/548b037c5917dc1e693733dc3d89599369d79103/server/base/src/main/java/org/apache/accumulo/server/conf/TableConfiguration.java#L269-L275
https://github.com/apache/accumulo/blob/548b037c5917dc1e693733dc3d89599369d79103/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java#L341-L349
https://github.com/apache/accumulo/blob/548b037c5917dc1e693733dc3d89599369d79103/core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java#L525
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r674972395
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
##########
@@ -65,7 +67,8 @@
private DefaultConfiguration defaultConfig = null;
private AccumuloConfiguration systemConfig = null;
private AuthenticationTokenSecretManager secretManager;
- private CryptoService cryptoService = null;
+ private CryptoService walCryptoService = null;
+ private Map<TableId,CryptoService> tableCryptoMap;
Review comment:
This was an unfinished idea I had to cache the crypto Services in ServerContext so they don't need to be created every time. Unless someone thinks this needs to be done for this change to work properly, I can remove this and create a follow on task.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r674895304
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -788,6 +788,20 @@
TABLE_COMPACTION_STRATEGY_PREFIX("table.majc.compaction.strategy.opts.", null,
PropertyType.PREFIX,
"Properties in this category are used to configure the compaction strategy."),
+ // Crypto-related properties
+ @Experimental
+ TABLE_CRYPTO_PREFIX("table.crypto.opts.", null, PropertyType.PREFIX,
+ "Properties related to on-disk file encryption."),
+ @Experimental
+ @Sensitive
+ TABLE_CRYPTO_SENSITIVE_PREFIX("table.crypto.opts.sensitive.", null, PropertyType.PREFIX,
+ "Sensitive properties related to on-disk file encryption."),
+ @Experimental
+ TABLE_CRYPTO_SERVICE("table.crypto.service",
+ "org.apache.accumulo.core.spi.crypto.NoCryptoService", PropertyType.CLASSNAME,
+ "The class which executes on-disk table encryption/decryption. The default does "
+ + "nothing. This property must be a classname with an implementation of the "
+ + "org.apache.accumulo.core.spi.crypto.CryptoService interface."),
Review comment:
All added properties will have to have the since added after changes from #2190 are merged in
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r674968781
##########
File path: server/tserver/src/test/java/org/apache/accumulo/tserver/log/SortedLogRecoveryTest.java
##########
@@ -79,6 +80,7 @@
static final Text cq = new Text("cq");
static final Value value = new Value("value");
static ServerContext context;
+ static CryptoService cs = CryptoServiceFactory.none();
Review comment:
Yup good catch. This was leftover from a previous change.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r679157857
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -894,6 +893,21 @@
TABLE_COMPACTION_STRATEGY_PREFIX("table.majc.compaction.strategy.opts.", null,
PropertyType.PREFIX,
"Properties in this category are used to configure the compaction strategy.", "1.6.0"),
+ // Crypto-related properties
+ @Experimental
+ TABLE_CRYPTO_PREFIX("table.crypto.opts.", null, PropertyType.PREFIX,
+ "Properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ @Sensitive
+ TABLE_CRYPTO_SENSITIVE_PREFIX("table.crypto.opts.sensitive.", null, PropertyType.PREFIX,
+ "Sensitive properties related to on-disk file encryption.", "2.1.0"),
+ @Experimental
+ TABLE_CRYPTO_SERVICE("table.crypto.service",
+ "org.apache.accumulo.core.spi.crypto.NoCryptoService", PropertyType.CLASSNAME,
+ "The class which executes on-disk table encryption/decryption. The default does "
+ + "nothing. This property must be a classname with an implementation of the "
+ + "org.apache.accumulo.core.spi.crypto.CryptoService interface.",
+ "2.1.0"),
Review comment:
Would we use `table.custom` the same way? The volume chooser uses the same class type for `general` properties and for `tables.custom` properties. And what about the wals? Would they have their own factory? Maybe if you give an example of your idea of a config that would help.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r675657522
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
##########
@@ -65,7 +67,8 @@
private DefaultConfiguration defaultConfig = null;
private AccumuloConfiguration systemConfig = null;
private AuthenticationTokenSecretManager secretManager;
- private CryptoService cryptoService = null;
+ private CryptoService walCryptoService = null;
+ private Map<TableId,CryptoService> tableCryptoMap;
Review comment:
I already did on this PR! Check out the change to make sure I did it correctly:
https://github.com/apache/accumulo/pull/2197/files#diff-c8804c4993551d0fc6f293963d664d4e35151f7639955f4c7e2b3a7bf2a694b1
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#discussion_r674797529
##########
File path: server/tserver/src/test/java/org/apache/accumulo/tserver/log/SortedLogRecoveryTest.java
##########
@@ -79,6 +80,7 @@
static final Text cq = new Text("cq");
static final Value value = new Value("value");
static ServerContext context;
+ static CryptoService cs = CryptoServiceFactory.none();
Review comment:
It does not appear this is used anywhere. Can this be removed or am I overlooking something?
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2197: Per table crypto + other crypto improvements
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2197:
URL: https://github.com/apache/accumulo/pull/2197#issuecomment-929595830
@milleruntime I'd like to take another look at this soon, but it seems to be a little out of date right now. Want to rebase it?
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org