getall results in an InvalidCiphertextException

jinnko commented 6 years ago

Following an upgrade from credstash-1.14.0 to credstash-1.15.0 we're no longer able to use the getall command together with encryption contexts.

Versions

Credstash is installed in a docker container based on openjdk:7-jdk-alpine using the following steps:

apk --no-cache add python3 python3-dev libffi-dev openssl-dev build-base 
pip3 install credstash

Working versions from July 12th

Successfully installed

asn1crypto-0.24.0
boto3-1.7.56
botocore-1.10.56
cffi-1.11.5
credstash-1.14.0
cryptography-2.0.3
docutils-0.14
idna-2.7
jmespath-0.9.3
pycparser-2.18
python-dateutil-2.7.3
s3transfer-0.1.13
six-1.11.0

Broken combination from July 13th onwards

Successfully installed

asn1crypto-0.24.0
boto3-1.7.58
botocore-1.10.58
cffi-1.11.5
credstash-1.15.0
cryptography-2.2.2
docutils-0.14
idna-2.7
jmespath-0.9.3
pycparser-2.18
python-dateutil-2.7.3
s3transfer-0.1.13
six-1.11.0

Error message

# credstash getall role=build@docker-openjdk filename=test.json
Traceback (most recent call last):
  File "/usr/bin/credstash.py", line 89, in decrypt
    EncryptionContext=self.encryption_context
  File "/usr/lib/python3.6/site-packages/botocore/client.py", line 314, in _api_call
    return self._make_api_call(operation_name, kwargs)
  File "/usr/lib/python3.6/site-packages/botocore/client.py", line 612, in _make_api_call
    raise error_class(parsed_response, operation_name)
botocore.errorfactory.InvalidCiphertextException: An error occurred (InvalidCiphertextException) when calling the Decrypt operation: 

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/bin/credstash", line 11, in <module>
    sys.exit(main())
  File "/usr/bin/credstash.py", line 925, in main
    getAllAction(args, region, **session_params)
  File "/usr/bin/credstash.py", line 246, in func_wrapper
    return func(*args, **kwargs)
  File "/usr/bin/credstash.py", line 351, in getAllAction
    **session_params)
  File "/usr/bin/credstash.py", line 338, in getAllSecrets
    names)
  File "/usr/lib/python3.6/multiprocessing/pool.py", line 266, in map
    return self._map_async(func, iterable, mapstar, chunksize).get()
  File "/usr/lib/python3.6/multiprocessing/pool.py", line 644, in get
    raise self._value
  File "/usr/lib/python3.6/multiprocessing/pool.py", line 119, in worker
    result = (True, func(*args, **kwds))
  File "/usr/lib/python3.6/multiprocessing/pool.py", line 44, in mapstar
    return list(map(*args))
  File "/usr/bin/credstash.py", line 337, in <lambda>
    lambda credential: getSecret(credential, version, region, table, context, dynamodb, kms, **kwargs),
  File "/usr/bin/credstash.py", line 501, in getSecret
    return open_aes_ctr_legacy(key_service, material)
  File "/usr/bin/credstash.py", line 596, in open_aes_ctr_legacy
    key = key_service.decrypt(b64decode(material['key']))
  File "/usr/bin/credstash.py", line 103, in decrypt
    raise KmsError(msg)
credstash.KmsError: KMS ERROR: Could not decrypt hmac key with KMS. The encryption context provided may not match the one used when the credential was stored.

Steps to reproduce

From a working credstash version

credstash -r eu-west-2 put /build/test/docker-openjdk value role=build@docker-openjdk filename=test.json
From a container with the broken version attempt to get the item

credstash get /build/test/docker-openjdk role=build@docker-openjdk filename=test.json
```
value
```
From a container with the broken version attempt to getall the item

credstash getall role=build@docker-openjdk filename=test.json
```
...see stack trace above...
```

jinnko commented 6 years ago

FWIW, downgrading to credstash-1.14.0, which in turn pulls in cryptography-2.0.3 results in a working getall function, so this suggests the issue is either in credstash-1.15.0 or cryptography-2.2.2.

rdalverny commented 6 years ago

Same here with macOS/homebrew, for the same versions (works fine with 1.14.0, breaks with 1.15.0).

alex-luminal commented 6 years ago

are any of those items using ripemd? it got taken out with 1.15. Theres a migration script you can run if anything uses it.

rdalverny commented 6 years ago

Ah, right indeed, thanks! :+1: That would be worth mentioning it in the README or the release more explicitly, like:

If you upgrade to 1.15 AND were using now removed WHIRLPOOL or RIPEMD digests, you will need to run this migration script to update them to the default SHA256.

for those that do not follow the project that close.

rdalverny commented 6 years ago

Curious, on our side all our digests are SHA256, the migration script reports No digests to update!; and we keep getting this error. So it might be somewhere else, digging deeper.

rdalverny commented 6 years ago

Ok, it seems that, calling credstash getall env=mycontext:

1.14.0 silently ignores failed decryptions and just returns what it can with the provided context (except: pass in getAllSecrets);
1.15.0 does not manage exceptions over a decryption failure (because of mismatching context between the one provided and the one used for one secret).

Now, an option could be to return None from getSecret and filtering those out in the returned dictionary, instead of propagating the exception (and killing the pool), but I'm not sure if that's desired behaviour:

diff --git a/credstash.py b/credstash.py
index 8c685ee..765c732 100755
--- a/credstash.py
+++ b/credstash.py

@@ -338,7 +339,9 @@ def getAllSecrets(version="", region=None, table="credential-store",
         names)
     pool.close()
     pool.join()
-    return dict(zip(names, results))
+    res = dict(zip(names, results))
+
+    return {k: v for k, v in res.items() if v is not None}

@@ -498,7 +501,11 @@ def getSecret(name, version="", region=None,

     key_service = KeyService(kms, None, context)

-    return open_aes_ctr_legacy(key_service, material)
+    try:
+        return open_aes_ctr_legacy(key_service, material)
+    except Exception as e:
+        print(material['name'] + ': ' + str(e))
+        return None

or would storing the context as a DDB column be relevant?

jinnko commented 6 years ago

Storing the context as a DDB column such that it can be used to filter the results before attempting to decrypt everything would be a big win in my opinion. We use multiple contexts to filter creds for different microservices, and with just a few hundred key-value pairs it can take up to 30 seconds to get the results.

chenrui333 commented 4 years ago

I just tried with latest v1.16.2, it seems ok?

$ /usr/local/Cellar/credstash/1.16.2/bin/credstash getall
{
    "a.new.testing.key": "xxx",
    "airflow.mysql.password.airflow_user": "xxx",
    "datadog.key": "xxx",

gokuatkai commented 3 years ago

@chenrui333 the issue is when you have credentials stored with other contexts, and you don't so basically any version will work.

We are having a similar issue with any version >1.14 of credstash. It just hangs for ever with tons of errors (we have a lot of credentials with different context). @rdalverny do you maybe remember what you had in mind when you said:

but I'm not sure if that's desired behaviour

To me the patch you provided seems OK (except the print obviously) Do you think we can work on a PR together?

fugue / credstash