aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.35k stars 3.76k forks source link

[cognito] ALB dns name has upper case that cognito does not accept as a callback url #11171

Open jolo-dev opened 3 years ago

jolo-dev commented 3 years ago

By using the aws-elasticloadbalancingv2-actions, I noticed that the Cognito construct wants to have the callback URL in all lower case. Because it is not the case, the callback to the loadbalancer is not reached.

Reproduction Steps

Basically, I took it from here

    const lb = new elbv2.ApplicationLoadBalancer(this, 'LB', {
      vpc,
      internetFacing: true,
    });

    const userPool = new cognito.UserPool(this, 'UserPool');
    const userPoolClient = new cognito.UserPoolClient(this, 'Client', {
      userPool,

      // Required minimal configuration for use with an ELB
      generateSecret: true,
      authFlows: {
        userPassword: true,
      },
      oAuth: {
        flows: {
          authorizationCodeGrant: true,
        },
        scopes: [cognito.OAuthScope.EMAIL],
        callbackUrls: [
          `https://${lb.loadBalancerDnsName}/oauth2/idpresponse`,
        ],
      },
    });
    const cfnClient = userPoolClient.node.defaultChild as cognito.CfnUserPoolClient;
    cfnClient.addPropertyOverride('RefreshTokenValidity', 1);
    cfnClient.addPropertyOverride('SupportedIdentityProviders', ['COGNITO']);

    const userPoolDomain = new cognito.UserPoolDomain(this, 'Domain', {
      userPool,
      cognitoDomain: {
        domainPrefix: 'test-cdk-prefix',
      },
    });

    lb.addListener('Listener', {
      port: 443,
      certificates: [certificate],
      defaultAction: new actions.AuthenticateCognitoAction({
        userPool,
        userPoolClient,
        userPoolDomain,
        next: elbv2.ListenerAction.fixedResponse(200, {
          contentType: 'text/plain',
          messageBody: 'Authenticated',
        }),
      }),
    });

    new CfnOutput(this, 'DNS', {
      value: lb.loadBalancerDnsName,
    });
  }
}

const app = new App();
new CognitoStack(app, 'integ-cognito');
app.synth();

What did you expect to happen?

A redirect to the loadbalancer.

What actually happened?

The cognito domain appends an error.

Environment

Other


This is :bug: Bug Report

jolo-dev commented 3 years ago

I think a solution is to enforce the callback-url always to lower case. https://stackoverflow.com/questions/55279710/aws-cognito-and-application-load-balancer-error-in-saml-response-processing-red

Currently, it is manual work after a deployment since a Token is generated.

nija-at commented 3 years ago

Unfortunately, there is not an easy way for the CDK to do anything here. Since the DNS name is generated as part of CloudFormation deployment, there is no way for the CDK to know/manipulate the DNS name. This needs to be taken up with the Cognito service team to accept upper case domain names.

The workaround is to add a lambda backed custom resource that does this conversion.

nija-at commented 3 years ago

On the CDK end, we could introduce a loadBalacerDnsNameLower property in the aws-elasticloadbalancerv2 package which automatically creates this custom resource.

@rix0rrr @njlynch - what do you think?

jolo-dev commented 3 years ago

@nija-at Thanks for taking a look into this. I was also wondering if that is not an ELB thing because I don't think a need for having not all lowercase of the loadBalacerDnsName.

nija-at commented 3 years ago

Internal ref w/ Cognito service: t.corp/D18522969

namedgraph commented 2 years ago

@nija-at has loadBalancerDnsNameLower been introduced? This is a real PITA because many tools expect a lower-case hostname but the CDK does not provide any easy way to lower-case a string (a custom resource or a lambda is not "easy").

mohamed-ali commented 2 years ago

Hello, any updates on this issue?

kdenehy commented 2 years ago

Amazing that this issue still exists. Definitely should be fixed by the Cognito team. For non-CDK users, an intrinsic ToLowerCase function for CFN templates would also help. If CDK team isn't going to do anything to help, can you guys please forward issue as appropriate and then close?

vitran96 commented 2 years ago

You can use the custom resource below to get the DNS OnCreate But this is still a token so you cannot lowercase the result directly.

const describeLoadBalancerRole = new Role(
  this,
  "DescribeLoadBalancersRole",
  {
    assumedBy: new CompositePrincipal(
      new ServicePrincipal("ec2.amazonaws.com"),
      new ServicePrincipal("elasticloadbalancing.amazonaws.com"),
      new ServicePrincipal("lambda.amazonaws.com")
    )
  }
);
describeLoadBalancerRole.addToPolicy(
  new PolicyStatement({
    resources: ["*"],
    actions: ["elasticloadbalancing:Describe*"]
  })
);
describeLoadBalancerRole.addToPolicy(
  new PolicyStatement({
    resources: ["*"],
    actions: [
      "ec2:DescribeInstances",
      "ec2:DescribeClassicLinkInstances",
      "ec2:DescribeSecurityGroups"
    ]
  })
);

const describeLoadBalancer = new AwsCustomResource(
  this,
  "DescribeLoadBalancers",
  {
    resourceType: "Custom::DescribeLoadBalancers",
    onCreate: {
      service: "ELBv2",
      action: "describeLoadBalancers",
      parameters: {
        LoadBalancerArns: [loadBalancer.loadBalancerArn]
      },
      physicalResourceId: PhysicalResourceId.of(
        `${id}-AwsSdk-${loadBalancer.loadBalancerFullName}`
      )
    },
    policy: AwsCustomResourcePolicy.fromSdkCalls({
      resources: AwsCustomResourcePolicy.ANY_RESOURCE
    }),
    role: describeLoadBalancerRole,
    logRetention: RetentionDays.FIVE_DAYS
  }
);

const loadBalancerTrueDnsName = describeLoadBalancer.getResponseField(
  "LoadBalancers.0.DNSName"
);
new CfnOutput(this, "loadbalancer-address", {
  value: loadBalancerTrueDnsName
});

If you don't mind using an additional lambda function, you can try this in your cdk:

  1. Create a lambda function that use aws-sdk's ELBv2 to get the dns you want
  2. Create a custom resource to invoke the lambda function
github-actions[bot] commented 1 year ago

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

jolo-dev commented 1 year ago

I would say this should stay open.

csmcallister commented 1 year ago

Keep this open

tasiogr commented 5 months ago

This has been open for almost 3 and a half years, any update?

gamaleonardo commented 3 months ago

I have a similar issue. Impossible to retrieve the load balancer dns name using CfnOutput. It always takes a value like ${token[token.XXX]}. Any update on it? Thanks