iann0036 / iam-dataset

A consolidated cloud IAM dataset
MIT License
231 stars 23 forks source link

SecretsManager Resource ARN rendering #24

Closed dfangl closed 3 weeks ago

dfangl commented 1 year ago

I am recreating my comments on PR #23 in an issue, to give the discussion better visibility and not mingle it with the rest of the PR.

Regex greedy matching

I am not sure if the regex has to have some global modifiers (but according to https://github.com/iann0036/iamlive/blob/c6e3ca4a7b757e08b904d0f69d6a4b0e5c1f9894/iamlivecore/logger.go#L438-L466 it does not seem that way), but then the regex would, as per default matching greedy (I think/guess also in go? Please correct me on this) in its capturing group 1, already capture the six-char suffix. Therefore, the non-capturing group (?:-[a-zA-Z0-9]{6})? won't have any impact, and the rendered result looks like this: arn:aws:secretsmanager:us-east-1:000000000000:secret:test-secret-c21292d6-zohQOF-??????. The name of this secret was test-secret-c21292d6.

I test the rendering of the regex with a python version, but it should behave the same, and I confirmed the regex with regex101: https://regex101.com/r/TNZWew/2

The general implications of using -?????? per default

At least for the iamlive case, it might be discussable if suggesting a policy with -?????? is the best way to go. I can't find the correct link right now, but I think the explanation for the suffix was that new secrets with an identical name cannot be accessed by people with the permission for the old one, basically.

Also, albeit less important for this project (and solvable with a little more engineering on our side), we currently depend on the resource being rendered correctly for LocalStack, as it will retrieve resource based policies based on this arn (again, this can be worked around, if necessary).

As a compromise for all these things above, and I haven't checked the concrete implementation within the dataset model yet, but we could try to append the -?????? only if the previous part does not have a suffix matching already. (it would make problems for the partial arns again, but as AWS states, it likely won't find it anyway in this case).

The main problem I see is, that SecretId could either be the complete ARN with suffix, only the name, or some partial arn (probably without suffix).

Originally posted by @dfangl in https://github.com/iann0036/iam-dataset/issues/23#issuecomment-1541862210

dfangl commented 1 year ago

I am not sure if all the users of the dataset could correctly render it, but what of a regex like this:

^(?:arn.*:)?([a-zA-Z0-9/_\\+=\\.@-]+(?:(?<!-[a-zA-Z0-9]{6})-\?{6})?)(?:-\?{6})?$

(can be analysed here: https://regex101.com/r/TNZWew/3). It uses a negative lookbehind to match or not match the -??????, depending on another candidate in the SecretId.

For this though, we would need to define the regex like this:

%%regex%${SecretId}-??????%/^(?:arn.*:)?([a-zA-Z0-9/_\\+=\\.@-]+(?:(?<!-[a-zA-Z0-9]{6})-\?{6})?)(?:-\?{6})?$/g%%

Not sure if everything works correctly if the parameter part contains a constant as well. Basically, it would have this behavior (input is the SecretId including the question mark suffix):

Input: arn:aws:secretsmanager:us-east-1:000000000000:secret:test-secret-c21292d6-abcdef-??????
Group 1: test-secret-c21292d6-abcdef
Input: arn:aws:secretsmanager:us-east-1:000000000000:secret:test-secret-c21292d6-??????
Group 1: test-secret-c21292d6-??????
Input: arn:aws:secretsmanager:us-east-1:000000000000:secret:test-secret-c21292d6-abcdef-abcdef-??????
Group 1: test-secret-c21292d6-abcdef-abcdef
Input: arn:aws:secretsmanager:us-east-1:000000000000:secret:test-secret-c21292d6-abcdef-abcde-??????
Group 1: test-secret-c21292d6-abcdef-abcde-??????

I might have overengineered this, what do you think?

EDIT: Just found out that golang does not support negative lookbehinds. This prevents this solution. https://github.com/google/re2/wiki/Syntax

This (https://regex101.com/r/TNZWew/4) could be an alternative:

^(?:arn.*:)?((?:[a-zA-Z0-9/_\\+=\\.@]-?)+?(?:[a-zA-Z0-9]{6}|\?{6}))(?:-\?{6})?$

Haven't thought about simplifying it yet, might be possible easier.

iann0036 commented 3 months ago

Updated again in https://github.com/iann0036/iam-dataset/pull/32

dfangl commented 3 weeks ago

Fixed with #32 .