Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(aws_iam.PolicyDocument): Method 'fromJson(obj)' only takes parent 'Statement' JSON element #29975

Open
frfavoreto opened this issue Apr 26, 2024 · 3 comments
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@frfavoreto
Copy link

Describe the bug

Method fromJson() from aws_iam.PolicyDocument ignores Id and Version parent elements of a JSON Policy text, taking only the Statement and its nested elements.

newPolicyDocument.addStatements(...obj.Statement.map((s: any) => PolicyStatement.fromJson(s)));
return newPolicyDocument;

}

https://github.com/aws/aws-cdk/blob/v2.137.0/packages/aws-cdk-lib/aws-iam/lib/policy-document.ts#L61

Expected Behavior

Method fromJson() to accept all the JSON elements passed as an input.

Current Behavior

When using the method, it only considers Statement element (and its sub elements) and auto generate Version one. During this process, Id is ignored and the synth'ed template doesn't have any Id element in the Policy Document.

Reproduction Steps

Create any IAM Policy from a JSON text (with Version and Id elements included) like below:

const myJsonText ={
    "Version": "2012-10-17",
    "Id": "KMS-Key-Policy-Example",
    "Statement": [
        {
          "Sid": "Example SID",
            "Effect": "Allow",
            "Principal": {
                "AWS": "arn:aws:iam::123456789012:root"
            },
            "Action": "kms:*",
            "Resource": "*"
        }
    ]
}

export class testStack extends Stack {
    constructor(scope: Construct, id: string, props?: StackProps) {
      super(scope, id, props);

      const myPolicyTest = iam.PolicyDocument.fromJson(myJsonText);

      const key = new kms.Key(this, "myKMSkey",
        {
            policy: myPolicyTest,
        });
      
    }
  }

The generated policy does not have Id element. "Version": "2012-10-17" is automatically generated.

"myKMSkey6B023671": {
   "Type": "AWS::KMS::Key",
   "Properties": {
    "KeyPolicy": {
     "Statement": [
      {
       "Action": "kms:*",
       "Effect": "Allow",
       "Principal": {
        "AWS": "arn:aws:iam::123456789012:root"
       },
       "Resource": "*",
       "Sid": "Example SID"
      }
     ],
     "Version": "2012-10-17"
    }
   },
   "UpdateReplacePolicy": "Retain",
   "DeletionPolicy": "Retain",
   "Metadata": {
    "aws:cdk:path": "KmsKeyMissingIdTsStack/myKMSkey/Resource"
   }
  },

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

All supported versions, including latest 2.139.0

Framework Version

No response

Node.js Version

20

OS

Mac

Language

TypeScript

Language Version

No response

Other information

No response

@frfavoreto frfavoreto added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 26, 2024
@github-actions github-actions bot added the @aws-cdk/aws-iam Related to AWS Identity and Access Management label Apr 26, 2024
@ashishdhingra ashishdhingra self-assigned this Apr 26, 2024
@ashishdhingra ashishdhingra added needs-reproduction This issue needs reproduction. and removed needs-triage This issue or PR still needs to be triaged. labels Apr 26, 2024
@ashishdhingra
Copy link
Contributor

Reproducible using code below:

const myJsonText ={
      "Version": "testversion",
      "Id": "KMS-Key-Policy-Example",
      "Statement": [
          {
            "Sid": "Example SID",
              "Effect": "Allow",
              "Principal": {
                  "AWS": "arn:aws:iam::123456789012:root"
              },
              "Action": "kms:*",
              "Resource": "*"
          }
      ]
    };

    const myPolicyTest = iam.PolicyDocument.fromJson(myJsonText);
    console.debug(myPolicyTest.toJSON());

    const key = new kms.Key(this, "myKMSkey", {
      policy: myPolicyTest,
    });

It generates the following debug output during cdk synth. Id property appears to be ignored, whereas Version property appears to be auto-generated:

{
  Statement: [
    {
      Action: 'kms:*',
      Effect: 'Allow',
      Principal: [Object],
      Resource: '*',
      Sid: 'Example SID'
    }
  ],
  Version: '2012-10-17'
}
Resources:
  myKMSkey6B023671:
    Type: AWS::KMS::Key
    Properties:
      KeyPolicy:
        Statement:
          - Action: kms:*
            Effect: Allow
            Principal:
              AWS: arn:aws:iam::123456789012:root
            Resource: "*"
            Sid: Example SID
        Version: "2012-10-17"
    UpdateReplacePolicy: Retain
    DeletionPolicy: Retain
    Metadata:
      aws:cdk:path: TypescriptStack/myKMSkey/Resource
  CDKMetadata:
    Type: AWS::CDK::Metadata
    Properties:
      Analytics: v2:deflate64:H4sIAAAAAAAA/yXGsQ5AMBAA0G+xt0dZ7EYbHyDVnuSUa+KKiPh3EdN7JZiqhiKzp2jng15ohLtP1gVlTxnCKnC3eKlm4hav57NDifvm8HsT2VOiyI/i6BFmyY+yAGPAZLMQ6W3nRCtC9/sC8s+Yhm8AAAA=
    Metadata:
      aws:cdk:path: TypescriptStack/CDKMetadata/Default
    Condition: CDKMetadataAvailable
...
...

@ashishdhingra ashishdhingra added p2 effort/small Small work item – less than a day of effort and removed needs-reproduction This issue needs reproduction. labels Apr 26, 2024
@ashishdhingra
Copy link
Contributor

ashishdhingra commented Apr 26, 2024

@frfavoreto I was able to reproduce the issue at my end. According to this, setting Version property here appears to be by design. Can you share your use cases why you need them to be included in the JSON rather than auto generated or assigned?

@ashishdhingra ashishdhingra removed their assignment Apr 26, 2024
@frfavoreto
Copy link
Author

@ashishdhingra Version is not too important here, even though someone might want to use the older 2008-10-17 value for whatever retro-compatibility reasons.

However, Id is something useful, specially in audit compliance scenarios. If we are creating a policy from a custom JSON content using iam.PolicyDocument.fromJson() this is perfectly reasonable to expect the policy to be created entirely from the crafted JSON as an input. Whether the policy is going to be valid or not (depending on the service) this is something to be evaluated at deployment time.

Additionally, some services like SQS and SNS might require this element and have specific requirements for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management bug This issue is a bug. effort/small Small work item – less than a day of effort p2
Projects
None yet
Development

No branches or pull requests

2 participants