Implement new Encryption interface WIP by milleruntime · Pull Request #499 · apache/accumulo

milleruntime · 2018-05-23T19:27:18Z

Removed all old CryptoModule properties and code
Created EncryptionStrategy interface
Made default encryption NoEncryptionStrategy
Created 3 experimental config properties for crypto, one of which is sensitive
Modified BCFile to only write EncryptionStrategy
Implmented usable example strategy AESCBCEncryptionStrategy
Modified Initialize to use site config

* Removed all old CryptoModule properties and code * Created EncryptionStrategy interface * Made default encryption NoEncryptionStrategy * Created 3 experimental config properties for crypto, one of which is sensitive * Modified BCFile to only write EncryptionStrategy * Implmented usable example strategy AESCBCEncryptionStrategy * Modified Initialize to use site config

milleruntime · 2018-05-23T21:32:36Z

server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java

-          decryptingInput = input;
-        }
-
+        decryptingInput = input;


I kept this because LogSorter uses both input and decryptingInput... although I am not sure why

ctubbsii

First pass. I may have misunderstood some stuff on the first pass (the role of the EncryptionStrategy class name being serialized; I think it's in place of the parameters). Overall, I like all the deleted code. 😺

ctubbsii · 2018-05-23T23:00:43Z

core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java

+          this.cipherOut = encryptionStrategy.encryptStream(fsBufferedOutput);
          this.out = compressionAlgo.createCompressionStream(cipherOut, compressor, 0);
-        } catch (IOException e) {
+        } catch (Exception e) {


Is it possible to avoid overly broad catching of Exception here?

Perhaps... if we make the encrypt/decrypt methods throw IOException, this would force the strategy to handle all the funky encryption exceptions. For instance, the AESCBC strategy I wrote can also throw NoSuchAlgorithmException, NoSuchPaddingException, InvalidKeyException, InvalidAlgorithmParameterException. A catch all for these that logs and returns an unencrypted stream is another approach.

We definitely want to hard-throw the exception. Logging and proceeding without encryption is only marginally better than a completely silent failure.

ctubbsii · 2018-05-23T23:02:45Z

core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java

-      // secretKeyEncryptionStrategy.encryptSecretKey(cryptoParameters);
-
+      this.encryptionStrategy = EncryptionStrategyFactory
+          .setupConfiguredEncryption(accumuloConfiguration, EncryptionStrategy.Scope.RFILE);


For a new pluggable interface that we expect users to implement directly as (eventually) public API, we should avoid passing in AccumuloConfiguration, which is an internal-only object type.

I would recommend passing in a Map<String, String> containing everything that start with CRYPTO_SENSITIVE_PREFIX with the prefix stripped off.

ctubbsii · 2018-05-23T23:06:14Z

core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java

-          }
+          long offsetCryptoParameter = out.position();
+          String es = this.encryptionStrategy.getClass().getName();
+          out.writeUTF(es);


I don't think the overall encryption strategy should be serialized into the files. The pluggable encryption strategy could change, and the current strategy should be required to be able to interpret anything previously written. So, I don't think it's necessary to serialize the name of the strategy here... but perhaps we should permit it some opportunity write some magic values/parameters that it (or its future replacement, if swapped out) could recognize?

The use case of switching encryption strategies should be considered. Files written with previously configured encryption strategies may need different properties to be decrypted. One possible way to handle this is with a prefix per an encryption strategy and this prefix is persisted. This would look something like the following.

crypto.strategy.s201805.class=com.foo.Best crypto.strategy.s201805.props.mykey=XU24GNYXE74VSGJM table.file.crypto.strategy=s201805 tserver.wal.crypto.strategy=s201805

In the above example when files are written, s201805 would be persisted in the file and used for future decryption. Using this scheme only s201805 need be written to the file, the class names does not as it comes from config associated with s201805. This scheme also allows different tables to use different encryption.

For the use case of switching to a new encryption strategy, the config would look like the following.

crypto.strategy.s201805.class=com.foo.Best crypto.strategy.s201805.props.mykey=XU24GNYXE74VSGJM crypto.strategy.s201903.class=com.foo.Better crypto.strategy.s201903.props.thekey=56GU5ET6TQKZZCKP table.file.crypto.strategy=s201903 tserver.wal.crypto.strategy=s201903

The s201805 props need to be kept around as long as there are files encrypted with that config. New files that are written will persist s201903 and use that config.

The table.file.crypto.strategy and tserver.wal.crypto.strategy were something I conjured up on a whim. Not sure if there are pitfalls to this approach. Could have system level config for selecting crypto like the following.

crypto.strategy.s201805.class=com.foo.Best crypto.strategy.s201805.props.mykey=XU24GNYXE74VSGJM crypto.strategy.s201903.class=com.foo.Better crypto.strategy.s201903.props.thekey=56GU5ET6TQKZZCKP crypto.current.strategy=s201903

I like these ideas. I wrote the class name because it was easiest and wasn't sure how to handle changing encryption strategies.

@ctubbsii do @keith-turner 's ideas match up with your thoughts?

Yeah, I think using an identifier that isn't (necessarily) the class name is best for this kind of thing.

An alternative approach is to not have an id in the config. Also do not have the class name in the config. Instead the strategy can provide a versioning string at write time that is persisted in the rfile. At read time this versioning string is provided back to the strategy. This empowers the strategy to handle versioning in whatever what it likes.

If there is no encryption strategy class in the config or the file then how do you know what class to load?

That should be determined by an overall crypto module, which is capable of supplying an appropriate decrypter for a given id.

Oh my bad I meant to write : An alternative approach is to not have an id in the config. Also do not have the class name in the rfile.

ctubbsii · 2018-05-23T23:08:22Z

core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java

+          out.writeUTF(es);

+          out.writeLong(offsetIndexMeta);
+          out.writeLong(offsetCryptoParameter);


These variables could use some docs, or better names, or something. It's not clear from this code what these values are, or why they are being written to the file in this location.

ctubbsii · 2018-05-23T23:08:50Z

core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java

-      WBlockState wbs = new WBlockState(compressAlgo, out, fsOutputBuffer, conf, cryptoModule,
-          cryptoParams);
+      WBlockState wbs = new WBlockState(compressAlgo, out, fsOutputBuffer, conf,
+          encryptionStrategy);


Did you run the formatter? This seems like it will collapse to one line.

ctubbsii · 2018-05-23T23:11:10Z

core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java

+        } catch (Exception e) {
          compressAlgo.returnDecompressor(decompressor);
-          throw e;
+          throw new IOException(e);


This isn't necessary if the type was already IOException.

ctubbsii · 2018-05-23T23:12:37Z

core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java

+      // Do a version check - API_VERSION_2 used experimental crypto parameters, no longer supported
+      if (!version.compatibleWith(BCFile.API_VERSION_3)
+          && !version.compatibleWith(BCFile.API_VERSION_1)) {
        throw new RuntimeException("Incompatible BCFile fileBCFileVersion.");


This should be a more specific exception type.

Would be nice to include the version seen in the message. Also if its ver 2 could give a nice message about it not being supported.

Think throw IOException is appropriate here? I will update the message.

Do you need it to stay a RuntimeException? If so, there's an UncheckedIOException. Either IOException or a more specific RuntimeException would be fine with me.

ctubbsii · 2018-05-23T23:17:10Z

core/src/main/java/org/apache/accumulo/core/security/crypto/EncryptionStrategy.java

+   * @return true if initialization was successful
+   * @since 2.0
+   */
+  boolean init(Scope encryptionScope, AccumuloConfiguration configuration) throws Exception;


These should throw a more narrow exception type.

Did you have something in mind? I wanted to make the interface flexible enough to handle whatever work needs to be done during init... which could be a lot of different things.

🤷‍♂️

ctubbsii · 2018-05-23T23:28:38Z

core/src/main/java/org/apache/accumulo/core/security/crypto/EncryptionStrategyFactory.java

+          EncryptionStrategy.class);
+      return clazz.newInstance();
+    } catch (Exception e) {
+      throw new RuntimeException(e);


Multi-catch would work well here. Also, you should probably use the ConfigurationTypeHelper.getClassInstance() for consistent class loading and code reuse (as well as context support... which may not matter right now... but perhaps eventually).

ctubbsii · 2018-05-23T23:30:22Z

core/src/main/java/org/apache/accumulo/core/security/crypto/NoEncryptionStrategy.java

+
+  @Override
+  public boolean init(Scope encryptionScope, AccumuloConfiguration conf) {
+    return true;


What's this boolean for? Should probably succeed (void) or fail (throw), but not succeed with false return code... unless it's used for something else?

I have code in the factory that returns a NoEncryptionStrategy when init returns false. I guess false is more of a fail quietly and throw exception is fail loudly.

I don't think we should ever fail quietly when it comes to encryption. 😺

Haha fair enough. I will make it return void.

…-prints

…arameter-prints Added a custom exception and crypto info prints

Updated service to make decisions

Add initial gcm to crypto service

Got Findbugs passing - had to refactor AESCryptoService

milleruntime · 2018-07-05T17:41:25Z

I am going to rebase since there have been a lot of commits between @PircDef and I.

milleruntime added 2 commits May 23, 2018 15:25

Fix formatting

cdc19f7

milleruntime commented May 23, 2018

View reviewed changes

ctubbsii reviewed May 23, 2018

View reviewed changes

PircDef and others added 13 commits June 11, 2018 12:22

Added a custom exception and crypto info prints

a9be4a3

Removed AccumuloConfiguration from API

bd4cc9d

Merge branch 'crypto' into feature/add-custom-exception-and-parameter…

08939c6

…-prints

Merge pull request #6 from PircDef/feature/add-custom-exception-and-p…

85bcc41

…arameter-prints Added a custom exception and crypto info prints

PR Updates

f355903

Fix config bug

8f40502

Change to table properties

6b5a150

Ideas for version string

e186bbf

New Crypto interface ideas

ac90e35

WIP

220a98a

Recent updates - code now compiles

fea6df5

Updated service to make decisions

ad5c391

JK now compiles

5fdc0d4

milleruntime changed the title ~~Implement new Encryption interface~~ Implement new Encryption interface WIP Jun 19, 2018

milleruntime and others added 11 commits June 19, 2018 14:36

Merge pull request #7 from PircDef/updated-crypto-service

0e980aa

Updated service to make decisions

Static vars for testing and fix CryptoServiceFactory

1236465

Fix CryptoTest - RFileTest still errors, AESGCMCryptoModule not yet impl

b23c104

Initial AES/GCM

4ff3b4c

Merge pull request #8 from PircDef/add-initial-gcm-to-crypto-service

619af1f

Add initial gcm to crypto service

Move version into impl - breaking change

2ed8292

Changed to use getParameters

4b3bca4

Adding back RFileCipherOutputStream

001e773

Implement parameters parser for crypto service (#9)

dbcb13f

Got some of CryptoTest working

ffb490a

Got Findbugs passing - had to refactor AESCryptoService

Possible fix to GCM crypto

41cec96

More fixes to CryptoTest

968c33d

milleruntime closed this Jul 12, 2018

milleruntime deleted the crypto branch August 13, 2018 20:03

milleruntime added the v2.0.0 label Nov 9, 2018

ctubbsii added this to the 2.0.0 milestone Jul 12, 2024

Conversation

milleruntime commented May 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctubbsii left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milleruntime Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keith-turner Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milleruntime commented Jul 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

milleruntime Jun 11, 2018 •

edited

Loading

keith-turner Jun 13, 2018 •

edited

Loading