Added ability to pass in encrypted key/values into the CLI and API #4547

nmaludy · 2019-02-13T01:27:07Z

This PR adds the ability to pass in encrypted key/value pairs on the CLI and via the API. The values remain encrypted and then are decrypted prior to being stored in the database.

To signify to the CLI that these values must be decrypted this adds a -d/--debug flag to st2 key set, example:

st2 key set -e -d myencryptedvalue ABC123

A new metadata parameter is now allowed in st2 key load data files called decrypt: true this signifies that the value for a given key is encrypted and needs to be decrypted before being processed.

- name: myencryptedvalue
  value: ABC123
  decrypt: true
  secret: true

Finally the API endpoint PUT /api/v1/keys/{name} accepts a new parameter decrypt that again signifies that the value is encrypted and must be decrypted:

PUT /api/v1/keys/myencryptedvalue
{
  "name": "myencryptedvalue",
  "value": "ABC123",
  "decrypt": true,
  "secret": true
}

The decrypting operation is handled within the API so that st2client doesn't rely on st2common. This also makes it a consistent experience from CLI and API.

TODO

st2docs PR once this design is approved - Documentation on how to load pre-encrypted secrets into the datastore st2docs#869

…ecrypt them prior to being stored in the database

…pi-decrypt

arm4b · 2019-02-14T20:07:50Z

FYI this comes from the StackStorm/puppet-st2#250 PR where @nmaludy had no choice but to copy entire python cryptography code for st2 decrypt.

This PR solves it by adding new option to CLI/API.

Kami · 2019-03-14T19:19:37Z

Thanks for contributing this change.

Could we avoid the decrypt -> encrypt round-trip and simply tell API the value is encrypted and to store it as-is?

The limitation of course is that the value needs to be encrypted with the same key which is used by the StackStorm installation. But that's also the case with this approach.

I guess the only downside of that approach would be that there is no "integrity check" on write - user would only find out that encrypted value is corrupted or not encrypted with the same key which is used on this particular installation on access / read time.

nmaludy · 2019-03-14T19:20:58Z

@Kami right, i intentionally decrypt the value prior to saving to the datastore, so that the key validity is checked on write rather than read (at some point in the future).

Kami · 2019-03-14T19:24:02Z

Alright, in this case I'm find with this approach 👍

It's almost always better to do integrity checks early on, on write :)

nmaludy · 2019-03-14T20:33:40Z

I'm thinking about renaming the flag from --decrypt to --preencrypted

Does that seem more clear?

Kami · 2019-03-14T20:41:16Z

Yeah, --decrypt is a bit config. Perhaps encrypted which signals that a value is already encrypted?

Kami · 2019-03-18T16:00:50Z

Thanks for updating the PR.

How do you think this should tie into RBAC? It would probably be safer if only admins can pass pre-encrypted values to the PUT API to avoid potential secrets leakage?

In theory, we could allow every user to use this functionality since we only use decrypt on the API side to ensure encrypted value is encrypted with the correct key and not corrupted, but then we need to be careful that we never return plain-text decrypted version back to the user inside PUT response.

preencrypted value.

1. Rename attribute and option name from "decrypt" to "pre_encrypted". This is a more accurate name and signals the API that the value is already encrypted and should be stored as-is. 2. Make sure pre_encrypted always implies secret=True. If we didn't do that, any user could read (decrypt) any encrypted value. Big no-no. 3. For additional safety, only allow RBAC admins to use "pre_encrypted" functionality.

Kami · 2019-03-18T16:54:42Z

Per discussion with @nmaludy on Slack, I made the following changes:

Change option name from decrypt to pre_encrypted

After some more thought, pre_encrypted is much better name and doesn't cause any confusion with decrypt which does something totally different.

It signals the API that the value is already encrypted and should be used and stored as-is.

We do actually perform decryption on the API side, but that's an implementation detail and only done to ensure the integrity of the value (to ensure it has been encrypted with the correct key and that it's not corrupted).

Besides, that, we treat and store value as-is.

Make sure pre_encrypted always implies secret=True

If we didn't do that, any user could read / decrypt any pre-encrypted value which is a big no-no and a security issue.

Only allow RBAC admins to use pre_encrypted=True

This is an additional safe guard.

Primary use case for this feature is migration / restore of datastore values from one StackStorm instance which uses the same crypto key to another using st2 key load command.

I still need to work on the test coverage, especially around the security edge cases to ensure there is no accidental secrets leakage.

which are already encrypted. Now "encrypted: True" and "secret: True" parameter in a value implies value is pre-encrypted and is treated as such (aka used as-is).

accidental data / secrets leakage.

Kami · 2019-03-18T17:44:37Z

Added test cases, and made the changes so st2 key list -n 100 -j > data.json ; st2 key load data.json now also works out of the box for encrypted values without needing to manipulate the format in data.json.

I approved the PR, but since it's touches a security critical code, I also requested review from @m4dcoder.

bigmstone

👍

m4dcoder · 2019-03-18T19:02:02Z

st2client/st2client/commands/keyvalue.py

@@ -168,6 +168,10 @@ def __init__(self, resource, *args, **kwargs):
        self.parser.add_argument('-e', '--encrypt', dest='secret',
                                 action='store_true',
                                 help='Encrypt value before saving.')
+        self.parser.add_argument('--pre-encrypted', dest='pre_encrypted',


Why not just simply name the argument --encrypted?

Yeah, I was thinking about it, but I thought it might be a bit confusing in combination with the existing --encrypt argument.

I'n fine either way though.

Adding mutually exclusive to the args will minimize the confusion.

m4dcoder · 2019-03-18T19:06:32Z

st2client/st2client/commands/keyvalue.py

+        self.parser.add_argument('--pre-encrypted', dest='pre_encrypted',
+                                 action='store_true',
+                                 help=('Value provided is already encrypted with the instance '
+                                 'crypto key and should be stored as-is.'))


Can we make the args --encrypt and --encrypted/--pre-encrypted mutually exclusive? Meaning these args cannot both be present at the same time.

Good point, will add it.

m4dcoder · 2019-03-18T19:12:29Z

st2common/st2common/openapi.yaml.j2

@@ -4885,6 +4885,9 @@ definitions:
      secret:
        description: Encrypt value before saving the value.
        type: boolean
+      decrypt:


This is a surprise. How come we are adding decrypt attribute in this API spec?

You are commenting on an old diff, attribute is now named pre_encrypted.

…pted".

Kami · 2019-03-18T20:13:50Z

Made CLI flags mutually exclusive - 970b3eb and changed the argument and CLI flag name from --pre-encrypted to --encrypted - 88607e3.

m4dcoder

LGTM. Not going to nit pick on ya but I couldn't find a specific unit test case for failure of checking integrity of setting an encrypted value.

m4dcoder · 2019-03-18T20:39:07Z

st2common/st2common/models/api/keyvalue.py

+                msg = ('Failed to verify the integrity of the provided value for key "%s". Ensure '
+                       'that the value is encrypted with the correct key and not corrupted.' %
+                       (name))
+                raise ValueError(msg)


I may have missed it but I don't see a unit test that shows failure in checking integrity.

Good catch - I did plan to add that one, but forgot to.

Will add it.

Added in 17427bf.

nmaludy added 2 commits February 12, 2019 20:20

Added ability to pass in encrypted values into the CLI and API then d…

800fe97

…ecrypt them prior to being stored in the database

Merge remote-tracking branch 'upstream/master' into feature/kvp-cli-a…

559f7ce

…pi-decrypt

Kami added this to the 3.0.0 milestone Feb 13, 2019

arm4b requested a review from Kami February 14, 2019 20:03

Merge branch 'master' into feature/kvp-cli-api-decrypt

80fe135

arm4b added CLI feature API RFR labels Feb 18, 2019

Merge branch 'master' into feature/kvp-cli-api-decrypt

247e51c

nmaludy mentioned this pull request Mar 14, 2019

Documentation on how to load pre-encrypted secrets into the datastore StackStorm/st2docs#869

Merged

Kami added 3 commits March 18, 2019 17:04

Merge branch 'master' into feature/kvp-cli-api-decrypt

d6fd8c8

Throw an error if we fail to decrypt a value when working with a

820b809

preencrypted value.

Kami added 4 commits March 18, 2019 18:11

Add changelog entry.

677c4b4

Update "st2 key load" CLI command so it works out of the box with values

5228fa7

which are already encrypted. Now "encrypted: True" and "secret: True" parameter in a value implies value is pre-encrypted and is treated as such (aka used as-is).

Update changelog.

4f6fa27

Update test cases to test the new behavior, verify there is no

ddd1514

accidental data / secrets leakage.

Kami requested a review from m4dcoder March 18, 2019 17:44

Kami approved these changes Mar 18, 2019

View reviewed changes

bigmstone approved these changes Mar 18, 2019

View reviewed changes

Re-generate openapi file.

8e34d21

m4dcoder requested changes Mar 18, 2019

View reviewed changes

Kami added 2 commits March 18, 2019 20:57

Make --encrypt and --pre-encrypt arguments mutually exclusive.

970b3eb

Change attribute and CLI flag name from "--pre-encrypted" to "--encry…

88607e3

…pted".

Use "parser.add_mutually_exclusive_group".

b77e98a

m4dcoder approved these changes Mar 18, 2019

View reviewed changes

Add a test case for a failed integrity check.

17427bf

Kami merged commit 0e9de9d into StackStorm:master Mar 19, 2019

cognifloyd removed the RFR label Aug 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ability to pass in encrypted key/values into the CLI and API #4547

Added ability to pass in encrypted key/values into the CLI and API #4547

nmaludy commented Feb 13, 2019 •

edited by Kami

Loading

arm4b commented Feb 14, 2019

Kami commented Mar 14, 2019

nmaludy commented Mar 14, 2019

Kami commented Mar 14, 2019

nmaludy commented Mar 14, 2019

Kami commented Mar 14, 2019

Kami commented Mar 18, 2019

Kami commented Mar 18, 2019

Kami commented Mar 18, 2019

bigmstone left a comment

m4dcoder Mar 18, 2019

Kami Mar 18, 2019 •

edited

Loading

m4dcoder Mar 18, 2019

m4dcoder Mar 18, 2019

Kami Mar 18, 2019

m4dcoder Mar 18, 2019

Kami Mar 18, 2019

Kami commented Mar 18, 2019

m4dcoder left a comment

m4dcoder Mar 18, 2019

Kami Mar 18, 2019

Kami Mar 19, 2019

Added ability to pass in encrypted key/values into the CLI and API #4547

Added ability to pass in encrypted key/values into the CLI and API #4547

Conversation

nmaludy commented Feb 13, 2019 • edited by Kami Loading

arm4b commented Feb 14, 2019

Kami commented Mar 14, 2019

nmaludy commented Mar 14, 2019

Kami commented Mar 14, 2019

nmaludy commented Mar 14, 2019

Kami commented Mar 14, 2019

Kami commented Mar 18, 2019

Kami commented Mar 18, 2019

Kami commented Mar 18, 2019

bigmstone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kami Mar 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kami commented Mar 18, 2019

m4dcoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmaludy commented Feb 13, 2019 •

edited by Kami

Loading

Kami Mar 18, 2019 •

edited

Loading