iann0036 / iam-dataset

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

Rollback SecretsManager changes and add Lambda Layer / CreateJobQueue mappings #23

Closed dfangl closed 1 year ago

dfangl commented 1 year ago

SecretsManager -??????

As written in https://github.com/iann0036/iam-dataset/pull/22#issuecomment-1540460778, it seems wrong to include the -?????? suffix in cases, where the SecretId is already used, as the SecretId is basically the secret ARN, which already contains the suffix (https://docs.aws.amazon.com/secretsmanager/latest/apireference/API_GetSecretValue.html#API_GetSecretValue_RequestSyntax). This is in contrast to the Name in the CreateSecret operation, which needs this suffix, as it does not have an id postfixed.

Batch.CreateJobQueue

There was a mapping missing due to the casing of the parameter jobQueueName -> JobQueueName.

lambda.*Layer*

The Lambda layer APIs contain a field called VersionNumber (https://docs.aws.amazon.com/lambda/latest/dg/API_DeleteLayerVersion.html) but in the arn template it is called LayerVersion.

iann0036 commented 1 year ago

Hey @dfangl,

Thanks heaps for your contributions! I've adjusted the PR as follows:

SecretsManager -??????

Adjusted to re-introduce the trailing single-char wildcards, but regex eliminating any trailing secret random chars. There is an edge case that AWS have identified where a secret name is itself one which has a trailing dash and 6 chars. It looks like this simply fails where a partial ARN is used. New format:

"arn_override": {
    "template": "arn:${Partition}:secretsmanager:${Region}:${Account}:secret:%%regex%${SecretId}%/(?:arn.*:)?([a-zA-Z0-9/_\\+=\\.@-]+)(?:-[a-zA-Z0-9]{6})?/g%%-??????"
}

Batch.CreateJobQueue

Accepted as provided.

lambda.Layer

Accepted, and adjusted to also parse the LayerName field in resource_mappings (with a regex match due to optional ARN format):

"resource_mappings": {
    "LayerName": {
        "template": "%%regex%${LayerName}%/^(?:^arn\\:aws\\:.+layer\\:)?([a-zA-Z0-9-_]+)$/g%%"
    },
    "LayerVersion": {
        "template": "${VersionNumber}"
    }
}

Merging this, however feel free to comment back if you disagree with any of the updates. Thanks again!

dfangl commented 1 year ago

It's rather hard for me to look at the exact changes, as the diff seems to be broken.

I am fine with the lambda changes!

But for secretsmanager, I still have some concern:

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).

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-zohQOF
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?

iann0036 commented 1 year ago

Hey @dfangl,

Good call on the greedy matching, happy to adjust my original to be non-greedy as per:

"arn_override": {
    "template": "arn:${Partition}:secretsmanager:${Region}:${Account}:secret:%%regex%${SecretId}%/(?:arn.*:)?([a-zA-Z0-9/_\\+=\\.@-]+?)(?:-[a-zA-Z0-9]{6})?$/g%%-??????"
}

I don't see a way to conditionally avoid having the trailing wildcards with my current syntax - would need something beyond what I have now.