StyraInc / opa-aws-cloudformation-hook

AWS Cloudformation Hook for OPA-powered infrastructure policy enforcement
Apache License 2.0
35 stars 5 forks source link

AWS Secrets Manager Bearer Token Value Parsing #35

Closed jimmyraywv closed 2 years ago

jimmyraywv commented 2 years ago

Does the AWS Secrets Manager bearer token value need parsing? Your code:

https://github.com/StyraInc/opa-aws-cloudformation-hook/blob/main/hooks/src/styra_opa_hook/handlers.py#L28-L44

Returns the SecretString from SecretsManager: {"opa_auth_token": "<VALUE>"}.

Are you expecting that entire string {"opa_auth_token": "<VALUE>"} to be configured in the OPA sever token auth? Or do we need to parse the actual value out with something like:

`if 'SecretString' in resp:
    return list(json.loads(resp['SecretString']).values())[0]`
anderseknert commented 2 years ago

Hi Jimmy! The SecretString is expected to be the token itself, not presented as a key/value JSON object. I tried to express this in the docs:

Note that the token should be provided as a plain string in the secret (i.e. the SecretString) and not wrapped in a JSON object.

But perhaps it could be phrased better? 🤔

jimmyraywv commented 2 years ago

So, maybe I am doing something wrong, but I don't see how you can enter just a value. It requires a key as well. When you retrieve the SecretString, it is a K/V pair, in a string. You can enter just a key with no value, but it still returns a K/V string, just with an empty value position.

Also, your hook config schema refers to the opaAuthTokenSecret key being set to opa_auth_token.

{"opaAuthTokenSecret":{"description":"ARN referencing a secret containing a token to use for authenticating against OPA (secret key must be 'opa_auth_token')","type":"string"},
jimmyraywv commented 2 years ago

So, if you use the plain-text, and not the key/value fields then it should work, for a value with no key. In the AWS CLI, it would be:

aws secretsmanager create-secret --name opa_auth_otken --secret-string "<VALUE>"

I guess the secret key must be 'opa_auth_token') confused me.

Perhaps it should be referenced as "Secret Name", instead.

anderseknert commented 2 years ago

Ah, yes, that's probably from an older iteration! Thanks for pointing that out 👍 Do you want to submit a PR to remove that or should I?

anderseknert commented 2 years ago

Fixed in #42. Thanks @pauly4it 👍