accumulo icon indicating copy to clipboard operation
accumulo copied to clipboard

Per table crypto + other crypto improvements

Open milleruntime opened this issue 4 years ago • 9 comments

  • Create table prefix properties for encrypting tables
  • Replace instance.crypto.service with instance.crypto.opts.factory for configuring a crypto factory per instance that returns services
  • Change scope in CryptoEnvironment to WAL, Table and Recovery
  • Replace CryptoServiceFactory class with a new interface and implement three types of factories: NoCryptoServiceFactory, GenericCryptoServiceFactory, and PerTableCryptoServiceFactory
  • Utilize general.custom prefix for shared crypto configuration
  • Use TableConfiguration to get CryptoService from table properties
  • Create CryptoServiceFactory.none() method to return no crypto
  • Make CryptoEnvironment return Optional for getDecryptionParams
  • Add logger to AESCryptoService
  • Update CryptoTest with new configuration and added tests
  • Add init checks to AESCryptoService
  • Add new method getAllCryptoProperties() to AccumuloConfiguration
  • Fixes for WALEncryptedIT
  • Create PerTableCryptoIT
  • Refactor bcfile class PrintInfo and rename to PrintBCInfo to fix rfile-info command
  • Add test to PerTableCryptoIT and Fix OfflineIterator
  • Closes #2001

milleruntime avatar Jul 09 '21 19:07 milleruntime

Depending on which PR is merged first, the properties changed here should match the design set in #2190.

Manno15 avatar Jul 09 '21 21:07 Manno15

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!)

ctubbsii avatar Jul 09 '21 23:07 ctubbsii

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.

milleruntime avatar Jul 22 '21 12:07 milleruntime

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.

milleruntime avatar Jul 28 '21 13:07 milleruntime

@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 CryptoFactoryLoader PerTableCryptoFactory CryptoFactory interface CryptoEnvironment interface TableConfiguration ServerContext

milleruntime avatar Aug 04 '21 14:08 milleruntime

@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?

ctubbsii avatar Sep 28 '21 20:09 ctubbsii

@ctubbsii I re-base'd my per-table crypto changes again with latest for 2.1. Did you want to take another look at this PR before I rewrite the tests? There are a lot of changes but I think the design is solid. Here is a summary of the big updates for this feature:

  1. New CryptoFactoryLoader class. This class replaces CryptoServiceFactory. It can create a new instance of CryptoFactory and has convenience methods to get the CryptoService directly.
  • public static CryptoFactory newInstance(AccumuloConfiguration conf, ClassloaderType ct)
  • public static CryptoFactory none()
  • public static CryptoService getCryptoJavaTable(AccumuloConfiguration conf) (Used by server code)
  • public static CryptoService getCryptoFromTableProps(InstanceOperations instOps, Map<String,String> tableProps, TableId TableId) (Used by client code)
  1. New CryptoFactory interface. Single method: CryptoService getService(CryptoEnvironment environment, Map<String,String> properties)
  • Create 2 basic factory implementations: PerTableCryptoFactory and NoCryptoFactory
  1. Modifications to the current Crypto.
  • Drop the init method from CryptoService
  • Add TableId to CryptoEnvironment
  1. Disable Crypto in the RFile API. This is a stop gap until we figure out how to support crypto in the RFile API

Classes with major changes: Property CryptoFactoryLoader PerTableCryptoFactory CryptoFactory interface CryptoEnvironment interface TableConfiguration ServerContext

milleruntime avatar Jun 09 '22 14:06 milleruntime

@ctubbsii I re-base'd my per-table crypto changes again with latest for 2.1. Did you want to take another look at this PR before I rewrite the tests?

I do want to take another look, but please don't let me hold you up in making progress.

There are a lot of changes but I think the design is solid. Here is a summary of the big updates for this feature:

  1. New CryptoFactoryLoader class. This class replaces CryptoServiceFactory. It can create a new instance of CryptoFactory and has convenience methods to get the CryptoService directly.

My understanding is that this is just an internal utility class to load the configured SPI CryptoFactory instance that is intended to be a singleton for the process. Is that correct?

  1. New CryptoFactory interface. Single method: CryptoService getService(CryptoEnvironment environment, Map<String,String> properties)

I think I prefer the name CryptoServiceFactory, since it builds CryptoService objects and not Crypto objects. Another name could be something like FileCryptoModule to make it more clear that it's a comprehensive "black box" that is responsible for file encryption/decryption and to avoid the "Factory" terminology.

ctubbsii avatar Jun 09 '22 15:06 ctubbsii

This PR is ready for review. I still have a few tasks that can be follow on work but I want to keep this PR to the minimum changes required. I have done some manual testing using Uno and all the ITs are passing.

Here are the follow on tasks I have in mind:

  1. Modify or drop the AESCryptoService enabled property
  2. Create an option for wal-info and rfile-info to just print headers
  3. Add test for recovery scope
  4. Test utils: CreateEmpty & GenerateSplits
  5. Make CryptoException extend GeneralSecurityException
  6. Check for encryption on Upgrader

Edit: Added another

milleruntime avatar Aug 03 '22 14:08 milleruntime

I am going to merge this PR so we can do follow up tasks and more testing.

milleruntime avatar Sep 15 '22 15:09 milleruntime