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] ALB dns name has upper case that cognito does not accept as a callback url #11171

Open
jolo-dev opened this issue Oct 28, 2020 · 14 comments
Labels
@aws-cdk/aws-cognito Related to Amazon Cognito @aws-cdk/aws-elasticloadbalancingv2 Related to Amazon Elastic Load Balancing V2 bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@jolo-dev
Copy link
Contributor

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

  • CLI Version : 1.70.0
  • Framework Version: 1.70.0
  • Node.js Version: 12.18.3
  • OS : Ubuntu
  • Language (Version): Python 3.8

Other


This is 🐛 Bug Report

@jolo-dev jolo-dev added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 28, 2020
@github-actions github-actions bot added the @aws-cdk/aws-cognito Related to Amazon Cognito label Oct 28, 2020
@jolo-dev jolo-dev changed the title [aws-cognito] loadBalancerDnsName should be all lower case [aws-cognito] callback-url should be all lower case Oct 28, 2020
@jolo-dev
Copy link
Contributor Author

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
Copy link
Contributor

nija-at commented Oct 30, 2020

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 nija-at changed the title [aws-cognito] callback-url should be all lower case [cognito] callback-url from ALB has upper case that cognito does not accept Oct 30, 2020
@nija-at nija-at added the @aws-cdk/aws-elasticloadbalancingv2 Related to Amazon Elastic Load Balancing V2 label Oct 30, 2020
@nija-at
Copy link
Contributor

nija-at commented Oct 30, 2020

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?

@nija-at nija-at assigned njlynch and unassigned rix0rrr Oct 30, 2020
@nija-at nija-at changed the title [cognito] callback-url from ALB has upper case that cognito does not accept [cognito] ALB dns name has upper case that cognito does not accept as a callback url Oct 30, 2020
@jolo-dev
Copy link
Contributor Author

@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 nija-at added effort/small Small work item – less than a day of effort p1 and removed needs-triage This issue or PR still needs to be triaged. labels Nov 2, 2020
@nija-at
Copy link
Contributor

nija-at commented Nov 2, 2020

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

@nija-at nija-at added p2 and removed p1 labels Mar 1, 2021
@namedgraph
Copy link

namedgraph commented Nov 30, 2021

@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
Copy link

Hello, any updates on this issue?

@kdenehy
Copy link

kdenehy commented Feb 8, 2022

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
Copy link

vitran96 commented Mar 2, 2022

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

rahuldeverani added a commit to rahuldeverani/CDK_CustomResource that referenced this issue Aug 13, 2022
@github-actions
Copy link

github-actions bot commented Mar 2, 2023

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.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Mar 2, 2023
@jolo-dev
Copy link
Contributor Author

jolo-dev commented Mar 4, 2023

I would say this should stay open.

@github-actions github-actions bot removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Mar 4, 2023
@csmcallister
Copy link

Keep this open

@tasiogr
Copy link

tasiogr commented Jan 26, 2024

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

@gamaleonardo
Copy link

gamaleonardo commented Mar 14, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-cognito Related to Amazon Cognito @aws-cdk/aws-elasticloadbalancingv2 Related to Amazon Elastic Load Balancing V2 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