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

(cognito): Client Secret handler resource update breaks references #23796

Closed
laurelmay opened this issue Jan 23, 2023 · 7 comments · Fixed by #23798, #29620 or rwlxxvii/containers#124 · May be fixed by NOUIY/aws-solutions-constructs#98 or NOUIY/aws-solutions-constructs#99
Labels
@aws-cdk/aws-cognito Related to Amazon Cognito bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.

Comments

@laurelmay
Copy link
Contributor

laurelmay commented Jan 23, 2023

Describe the bug

In #23591 installLatestAwsSdk. This results in a resource update for custom resources. The custom resource that fetches the client secret does not have an onUpdate handler (https://github.com/aws/aws-cdk/blame/0798876e5e6c1a665033c759aed3bc0eab05a892/packages/%40aws-cdk/aws-cognito/lib/user-pool-client.ts#L449).

When the update occurs, the response object does not have a UserPoolClient.ClientSecret field, resulting in failures when .userPoolClientSecret is referenced.

This results in stacks that fail to update

Expected Behavior

Updates should happen gracefully. The same content as the onCreate handler should be run for update events.

Current Behavior

There is no update handler. Updates fail with an error like:

CustomResource attribute error: Vendor response doesn't contain UserPoolClient.ClientSecret key in object arn:aws-us-gov:cloudformation:us-gov-west-1:123456789012:stack/StackName/UUID|ResourceId|UUID in S3 bucket cloudformation-custom-resource-storage-usgovwest1

CloudWatch Logs shows:

{
    "RequestType": "Update",
    "ResourceType": "Custom::DescribeCognitoUserPoolClient",
    "ResourceProperties": {
        "ServiceToken": "...",
        "InstallLatestAwsSdk": "true",
        "Create": "{\"region\":\"us-gov-west-1\",\"service\":\"CognitoIdentityServiceProvider\",\"action\":\"describeUserPoolClient\",\"parameters\":{\"UserPoolId\":\"...\",\"ClientId\":\"...\"},\"physicalResourceId\":{\"id\":\"...\"}}"
    },
    "OldResourceProperties": {
        "ServiceToken": "...",
        "InstallLatestAwsSdk": "false",
        "Create": "{\"region\":\"us-gov-west-1\",\"service\":\"CognitoIdentityServiceProvider\",\"action\":\"describeUserPoolClient\",\"parameters\":{\"UserPoolId\":\"...\",\"ClientId\":\"...\"},\"physicalResourceId\":{\"id\":\"...\"}}"
    }
}

{
    "Status": "SUCCESS",
    "Reason": "OK",
    "PhysicalResourceId": "...",
    "StackId": "...",
    "RequestId": "...",
    "LogicalResourceId": "...",
    "NoEcho": false,
    "Data": {}
}

I can confirm, but do not want to share, the contents of the Create object match between the two events.

Reproduction Steps

import * as cdk from "aws-cdk-lib";
import * as cognito from "aws-cdk-lib/aws-cognito";
import * as secretsmanager from "aws-cdk-lib/aws-secretsmanager";

const app = new cdk.App();
const stack = new cdk.Stack(app, 'TestStack');
const userPool = new cognito.UserPool(stack, 'UserPool');
const client = userPool.addClient('TestClient', { generateSecret: true });
const secret = new secretsmanager.Secret(stack, 'ClientSecret', {
  secretStringValue: client.userPoolClientSecret;
});
  • update the stack using v2.61.1

Possible Solution

Copy the onCreate handler to onUpdate.

Additional Information/Context

Typically, if the User Pool ID or the User Pool Client ID changes, the User Pool Client Secret would too. But other properties passed to the resource can change (like installLatestAwsSdk) and this will result in a resource update

CDK CLI Version

2.61.1

Framework Version

No response

Node.js Version

16

OS

Linux

Language

Typescript

Language Version

No response

Other information

I am preparing a patch that implements the suggested fix.

@laurelmay laurelmay added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 23, 2023
@github-actions github-actions bot added the @aws-cdk/aws-cognito Related to Amazon Cognito label Jan 23, 2023
@Mattias-
Copy link

Thanks for the report, I'm seeing the same issue. From the changelog it seems like this could be related to the Cognito changes introduced in 2.60.0 #23796

@laurelmay
Copy link
Contributor Author

laurelmay commented Jan 24, 2023

@Mattias- It was a changed introduced in 2.60.0; however, it was actually the "aws-custom-resource: switch off installLatestAwsSdk by default" fix instead of the Cognito one. At least for the specific issue above.

The new feature that relates to client secrets is for configuring the Google IdP and specifying a Secrets Manager secret for its clientSecretValue.

But I do worry other custom resources may have similar issues due to #23591. It'd be probably any custom resource that specifies onCreate but not onUpdate, had installLatestAwsSdk changed to false, and has a reference to its return value. So I think that could include:

  • AcceleratorSecurityGroupPeer.fromVpc:
    const lookupAcceleratorSGCustomResource = new AwsCustomResource(scope, id + 'CustomResource', {
    onCreate: {
    service: 'EC2',
    action: 'describeSecurityGroups',
    parameters: {
    Filters: [
    {
    Name: 'group-name',
    Values: [
    globalAcceleratorSGName,
    ],
    },
    {
    Name: 'vpc-id',
    Values: [
    vpc.vpcId,
    ],
    },
    ],
    },
    // We get back a list of responses, but the list should be of length 0 or 1
    // Getting no response means no resources have been linked to the AGA
    physicalResourceId: PhysicalResourceId.fromResponse(ec2ResponseSGIdField),
    },
    policy: AwsCustomResourcePolicy.fromSdkCalls({
    resources: AwsCustomResourcePolicy.ANY_RESOURCE,
    }),
    // APIs are available in 2.1055.0
    installLatestAwsSdk: false,
    });
  • Table.addPartitionIndex:
    const partitionIndexCustomResource = new cr.AwsCustomResource(this, `partition-index-${indexName}`, {
    onCreate: {
    service: 'Glue',
    action: 'createPartitionIndex',
    parameters: {
    DatabaseName: this.database.databaseName,
    TableName: this.tableName,
    PartitionIndex: {
    IndexName: indexName,
    Keys: index.keyNames,
    },
    },
    physicalResourceId: cr.PhysicalResourceId.of(
    indexName,
    ),
    },
    policy: cr.AwsCustomResourcePolicy.fromSdkCalls({
    resources: cr.AwsCustomResourcePolicy.ANY_RESOURCE,
    }),
    // APIs are available in 2.1055.0
    installLatestAwsSdk: false,
    });
    It doesn't seem like the output is used
  • EmrContainerStartJobRun.updateRoleTrustPolicy:
    const eksClusterInfo = new cr.AwsCustomResource(this, 'GetEksClusterInfo', {
    onCreate: {
    service: 'EMRcontainers',
    action: 'describeVirtualCluster',
    parameters: {
    id: this.props.virtualCluster.id,
    },
    outputPaths: ['virtualCluster.containerProvider.info.eksInfo.namespace', 'virtualCluster.containerProvider.id'],
    physicalResourceId: cr.PhysicalResourceId.of('id'),
    },
    policy: cr.AwsCustomResourcePolicy.fromSdkCalls({
    resources: cr.AwsCustomResourcePolicy.ANY_RESOURCE,
    }),
    // APIs are available in 2.1055.0
    installLatestAwsSdk: false,
    });

Also anyone else who uses any AwsCustomResource, directly or indirectly would be impacted if they change the feature flag of their existing app and that definition doesn't specify onUpdate.

@joelgarrett
Copy link

This seems to still be broken in 2.62.x and 2.63.x versions.

@mergify mergify bot closed this as completed in #23798 Feb 16, 2023
mergify bot pushed a commit that referenced this issue Feb 16, 2023
…erence (#23798)

Because there wasn't previously a handler for `onUpdate` events, an
empty object would be returned. When `installLatestAwsSdk` was changed
to `false`, this was an update. Typically, updates aren't an issue
because basically any other property being updated signifies a
replacement. `installLatestAwsSdk` is just a very unique case where it
doesn't (and where a user usually can't update it).

When the empty object is returned, this results in an update failure in
CloudFormation because the specific property isn't available.

Fixes: #23796

----

### All Submissions:

* [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Construct Runtime Dependencies:

* [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@alexb-uk
Copy link

For anyone else that ends up here, I can confirm that this is still an issue for AcceleratorSecurityGroupPeer.fromVpc:

CustomResource attribute error: Vendor response doesn't contain SecurityGroups.0.GroupId key in object arn:aws:cloudformation:eu-west-1:xxxxx:stack/test/aa-bb-cc|AcceleratorListenerAcceleratorGroupGlobalAcceleratorSGCustomResource0000|cc-bb-aa in S3 bucket cloudformation-custom-resource-storage-euwest1

when running the latest CDK 2.75.0

@denyskublytskyi
Copy link

Same error with the latest CDK 2.75.1

CustomResource attribute error: Vendor response doesn't contain UserPoolClient.ClientSecret key in object

@markusz
Copy link
Contributor

markusz commented May 16, 2023

Fixed in 2.79.1

mergify bot pushed a commit that referenced this issue Apr 12, 2024
…y Group reference (#29620)

### Issue # (if applicable)

Closes #23796

### Reason for this change

In #23591 `installLatestAwsSdk`. This results in a resource update for custom resources. The custom resource that fetches the security groups does not have an onUpdate handler (https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-globalaccelerator/lib/_accelerator-security-group.ts#L32).

When the empty object is returned, this results in an update failure in
CloudFormation because the specific property isn't available and so it will fail with error below:

```
CustomResource attribute error: Vendor response doesn't contain SecurityGroups.0.GroupId key in object
```

When the update occurs, the response object does not have a `SecurityGroups.0.GroupId` field, resulting in failures when `SecurityGroups` is referenced.

### Description of changes
Update the onCreate to onUpdate for custom resources to mitigate the CloudFormation update failure. Documentations: https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.custom_resources.AwsCustomResource.html#oncreate. 
Similar fix for Cognito: #23798

### Description of how you validated changes

The integration test is updated with the latest assets.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment