AES-128-GCM support in Encrypt() and Decrypt()

[hryx]

Hi,

I needed to add support for a stronger content encryption algorithm, so I've added AES-128-GCM support to the Encrypt() and Decrypt() functions.

In order to not destructively alter the API for current users, the approach I took was to create a new package-level variable called ContentEncryptionAlgorithm. The user can set it to any of the values in a new enum:

• EncryptionAlgorithmDESCBC
• EncryptionAlgorithmAES128GCM

The default is the current method, DES-CBC. The user just needs to change the value of ContentEncryptionAlgorithm before calling Encrypt() to change the behavior.

I also updated TestEncrypt() to run using both algorithms sequentially.

I'd love to work together to get this merged in, just let me know what I can do. Many thanks!

[kenkouot]

+1, would love AES128GCM support added.

[hilljgo]

+1, yeah this would actually be awesome!

[fullsailor]

Hey @hryx

Thanks for stepping in to add AES-128 support. Your patch looks great from a code perspective, I'm prepared to accept the additions to Decrypt, but I have some thoughts about adding to the package's API.

ContentEncryptionAlgorithm - I wouldn't normally use a package global, but I don't see an obvious better alternative. This is my fault. You might of saw from my source comments, other algorithms was something I had hoped to add at some point. I only wish I would have thought to include a parameter in Encrypt for specifying the algorithm. Any ideas that don't break backwards compatibility I'd welcome to discuss, otherwise accept it as is.

Also not a huge deal, but I'm already not liking having to choose and build in these encryption algorithms. Seems like a user would just as likely want to use AES-256-CBC to encrypt since its something supported for decrypting. Implementing each possible symmetric cipher & block combination doesn't feel like it belongs in this package.

Thanks again for your PR. I look forward to your thoughts and thoughts from others on these concerns.

[kenkouot]

Unfortunately, I think a package level variable is the only option without adding another function to the library, at which point you might as well break the API to support better flexibility overall.

I think it'd be great if a future iteration accepted ContentEncryption as an interface. The library could provide sensible default implementations, but also expose the interfaces for people with unique use cases to implement. But for now, adding a package level var seems reasonable.

[hryx]

Thanks for the review and feedback, @fullsailor. Totally agreed, this is a cheap workaround to support multiple algorithms, but just like you said, I didn't see any better option without breaking the existing API (which would warrant a thorough discussion with you regardless).

(I'd seen this tacked-on-global-variable approach used before, like in dat.)

So I'd say this is it for the short term, but see below for ideas on a future API.

Implementing each possible symmetric cipher & block combination doesn't feel like it belongs in this package.

I like @kenkouot's solution of taking an interface argument in Encrypt(). The package could support a few built-in types that satisfy the interface, and allow for the user to supply their own.

Perhaps a ContentEncryption interface would define methods that return the info that Encrypt() needs, something like...

type ContentEncryption interface {
    Key() []byte
    AlgorithmID() asn1.ObjectIdentifier
    Parameters() asn1.RawValue
}

One issue is that some state needs to be persisted for those methods to return correct values, such as the nonce in AES. So a constructor could return a struct satisfying ContentEncryption while keeping state.

(That alone doesn't tackle the decryption side, but I'm sure this approach could be extended to aid decryption too.)

If you wanted to roll this change out, I suppose you could just add to the API by creating an EncryptV2() function and leave the old one around. Maybe kinda cheesy, but would work.

Them's my thoughts! Anything else I can address before proceeding?

[fullsailor]

Okay, thank you for sharing your thoughts. I think I've settled that we need some interface type, but I'll open a separate issue for that.

The last thing I think we should add is a note in the Encrypt() doc comment that it picks the content encryption algorithm based on the global variable. As I said in my last, the code is good enough for me and I'm ready to merge.