Generate datastore_crypto_key on install if not provided

[cognifloyd]

OK. I wasn't going to implement this, but I did it anyway.

Other installation methods can call st2-generate-symmetric-crypto-key on install
to generate the key. But we need to put it in a kubernetes secret, so that is too
late to use for a new installation. Instead, we can generate the crypto key with
helm primitives.

Closes: #225

[cognifloyd]

The last helm-e2e test used the templates in this PR to generate this k8s secret:

  datastore_crypto_key: eyJhZXNLZXlTdHJpbmciOiJTWGxSTElGYklNREtTL0ZrcEdIMG1aM3NmVHo4SVNGTGg3Qk04TkZENDZZIiwiaG1hY0tleSI6eyJobWFjS2V5U3RyaW5nIjoiVHNuYkEzOFI1WHdXdG9XMWRrTC1wcGlWVWY0SklnNmNkRTUtdFY3M1NNcyIsInNpemUiOjI1Nn0sIm1vZGUiOiJDQkMiLCJzaXplIjoyNTZ9

decoding that, we have this datastore_crypto_key:

{"aesKeyString":"SXlRLIFbIMDKS/FkpGH0mZ3sfTz8ISFLh7BM8NFD46Y","hmacKey":{"hmacKeyString":"TsnbA38R5XwWtoW1dkL-ppiVUf4JIg6cdE5-tV73SMs","size":256},"mode":"CBC","size":256}

Which is perfectly valid.

[eric-al]

This looks great. A bit of extra security by default with zero effort required!

[arm4b]

Looks like it's making the impossible possible :)

Overall I'm worried about the implementation and if that's a good way or not to try this in the chart really.
Even the slightest security risk behind the implementation/diff is sufficient to avoid the drill here generating the K/V crypto key.

I'd rely on someone better from the @StackStorm/tsc with security to review this. Maybe @punkrokk ?

[cognifloyd]

If there is a security issue with this formula, then st2-core would need to change, and every installation everywhere would have to update their datastore crypto key. This chart would, of course, also have to be updated to match whatever formula st2-core uses to generate these keys. I think the formula is easier to understand here than in st2-core, so it should be fairly simple to migrate this if ever needed in the future.

That said, I look forward to hearing what @punkrokk or other @StackStorm/TSC members have to say about this.

[arm4b]

No worries. To be clear, I'm good with this new feature itself.

However, I'm looking for feedback if the key generation here is cryptographically secure. I didn't look what st2 does under the hood, but I'd trust those who good at this and if trying to mimic the st2 behavior for st2-krypto-key-generation in the Helm template engine is good enough.

I think the folks would need to dig deeper into the code you provided 👍

[cognifloyd]

Here is the st2 formula for generating the crypto key, trimmed down to make it easier to follow what is going on:

• The primary line of st2-generate-symmetric-crypto-key (note how key_size=256 is hard-coded): st2common/bin/st2-generate-symmetric-crypto-key#L49

  from st2common.util.crypto import AESKey
  ...
  aes_key = AESKey.generate(key_size=256)

• st2common.util.crypto.AESKey.generate: st2common/st2common/util/crypto.py#L133-L145 and #L431-L450

  import base64
  import os
  ...
  DEFAULT_AES_KEY_SIZE = 256
  ...
  class AESKey(object):
      ...
      @classmethod
      def generate(self, key_size=DEFAULT_AES_KEY_SIZE):
          """
          Generate a new AES key with the corresponding HMAC key.
          ...
          """
          ...
          aes_key_bytes = os.urandom(int(key_size / 8))
          aes_key_string = Base64WSEncode(aes_key_bytes)
  
          hmac_key_bytes = os.urandom(int(key_size / 8))
          hmac_key_string = Base64WSEncode(hmac_key_bytes)
  
          return AESKey(
              aes_key_string=aes_key_string,
              hmac_key_string=hmac_key_string,
              hmac_key_size=key_size,
              mode="CBC",  # CBC is the only supported mode
              size=key_size,
          )
  
  
  def Base64WSEncode(s):
      """
      Return Base64 web safe encoding of s. Suppress padding characters (=).
      Uses URL-safe alphabet: - replaces +, _ replaces /. Will convert s of type
      unicode to string type first.
      ...
      NOTE: Taken from keyczar (Apache 2.0 license)
      """
      ...
      return base64.urlsafe_b64encode(s).decode("utf-8").replace("=", "")

• in standard library, base64.urlsafe_b64encode: https://github.com/python/cpython/blob/3.6/Lib/base64.py#L111-L118

  def urlsafe_b64encode(s):
      """Encode bytes using the URL- and filesystem-safe Base64 alphabet.
      Argument s is a bytes-like object to encode.  The result is returned as a
      bytes object.  The alphabet uses '-' instead of '+' and '_' instead of
      '/'.
      """
      return b64encode(s).translate(_urlsafe_encode_translation)

• in standard library, base64.b64encode: https://github.com/python/cpython/blob/3.6/Lib/base64.py#L51-L62

  def b64encode(s, altchars=None):
      """Encode the bytes-like object s using Base64 and return a bytes object.
      Optional altchars should be a byte string of length 2 which specifies an
      alternative alphabet for the '+' and '/' characters.  This allows an
      application to e.g. generate url or filesystem safe Base64 strings.
      """
      encoded = binascii.b2a_base64(s, newline=False)  # this is implemented in C: https://github.com/python/cpython/blob/3.6/Modules/binascii.c#L525
      ...
      return encoded

• and finally: https://github.com/python/cpython/blob/3.6/Lib/base64.py#L108

  _urlsafe_encode_translation = bytes.maketrans(b'+/', b'-_')  # ie replace '+' with '-'; replace '/' with '_'

[cognifloyd]

And here is the implementation of randBytes which is a sprig template function available in helm templates:
https://github.com/Masterminds/sprig/blob/3ac42c7bc5e4be6aa534e036fb19dde4a996da2e/crypto.go#L70-L76

import (
	...
	"crypto/rand"
	...
	"encoding/base64"
	...
)
...
func randBytes(count int) (string, error) {
	buf := make([]byte, count)
	if _, err := rand.Read(buf); err != nil {
		return "", err
	}
	return base64.StdEncoding.EncodeToString(buf), nil
}

This creates a bytes buffer of the desired size, and then fills that buffer with random data, and then base64 enccodes it.

[punkrokk]

My only comment is we could probably use AES512. Nice to have not required (yet).

I think the function to create the key is fine.

[cognifloyd]

If we add support for AES512 in StackStorm itself, then we can update this to use the better encryption by default.

[arm4b]

Thanks for the PR @cognifloyd, and @punkrokk for additional review 👍