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): UserPoolDomain baseUrl is incorrect in us-gov-west-1 #20182

Closed
laurelmay opened this issue May 2, 2022 · 3 comments
Closed

(cognito): UserPoolDomain baseUrl is incorrect in us-gov-west-1 #20182

laurelmay opened this issue May 2, 2022 · 3 comments
Assignees
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 May 2, 2022

Describe the bug

In the us-gov-west-1 region, the Cognito Hosted UI always uses the fips endpoint (<domain-prefix>.auth-fips.<region>.amazoncognito.com). The UserPoolDomain.baseUrl function assumes the endpoint is always <prefix>.auth.<region>.amazoncognito.com. This results in a value that does not work in us-gov-west-1.

Expected Behavior

I expected calling baseUrl to return a URL that works in my current region.

Current Behavior

Calling baseUrl() returns a URL that does not work in us-gov-west-1.

Reproduction Steps

import 'source-map-support/register';
import * as cdk from "aws-cdk-lib";
import * as cognito from "aws-cdk-lib/aws-cognito";

const UNIQUE_PREFIX = "sample";

const app = new cdk.App();
const stack = new cdk.Stack(app, "DomainBroken");
const userPool = new cognito.UserPool(stack, "UserPool");
const domain = userPool.addDomain("domain", {
  cognitoDomain: {
    domainPrefix: UNIQUE_PREFIX,
  },
});
new cdk.CfnOutput(stack, "Domain", { value: domain.baseUrl() });

Change the UNIQUE_PREFIX constant to actually contain a unique value and deploy this stack to us-gov-west-1 and try to navigate to the URL contained in the DomainBroken.Domain output. The name does not resolve.

Because I understand that not everyone has access to GovCloud, I have deployed a sample stack setting UNIQUE_PREFIX to govrepro. The CDK returns https://govrepro.auth.us-gov-west-1.amazoncognito.com; however, that does not resolve. https://govrepro.auth-fips.us-gov-west-1.amazoncognito.com does.

Possible Solution

Adding a parameter to baseUrl

This could be fixed by adding an optional fips?: boolean parameter to baseUrl (that defaults to false). The nice side effect here is that you'd be able to get FIPS URLs for Hosted UIs in US East/West and other regions that support it; the downside is that users may pass true in regions that do not support FIPS endpoints.

Code for adding FIPS parameter
diff --git a/packages/@aws-cdk/aws-cognito/lib/user-pool-domain.ts b/packages/@aws-cdk/aws-cognito/lib/user-pool-domain.ts
index b201b3153..a9859b7c5 100644
--- a/packages/@aws-cdk/aws-cognito/lib/user-pool-domain.ts
+++ b/packages/@aws-cdk/aws-cognito/lib/user-pool-domain.ts
@@ -152,10 +152,13 @@ export class UserPoolDomain extends Resource implements IUserPoolDomain {
 
   /**
    * The URL to the hosted UI associated with this domain
+   *
+   * @param fips whether to return the fips variant of the hosted UI
    */
-  public baseUrl(): string {
+  public baseUrl(fips?: boolean): string {
     if (this.isCognitoDomain) {
-      return `https://${this.domainName}.auth.${Stack.of(this).region}.amazoncognito.com`;
+      const authBase = 'auth' + (fips ? '-fips' : '');
+      return `https://${this.domainName}.${authBase}.${Stack.of(this).region}.amazoncognito.com`;
     }
     return `https://${this.domainName}`;
   }

Using a CfnCondition

Because this only impacts a single region (for now), it's possible baseUrl could return a CloudFormation condition but that scales poorly and adds a condition in the stack.

Code for using a CloudFormation condition
diff --git a/packages/@aws-cdk/aws-cognito/lib/user-pool-domain.ts b/packages/@aws-cdk/aws-cognito/lib/user-pool-domain.ts
index b201b3153..dc958f01f 100644
--- a/packages/@aws-cdk/aws-cognito/lib/user-pool-domain.ts
+++ b/packages/@aws-cdk/aws-cognito/lib/user-pool-domain.ts
@@ -1,5 +1,5 @@
 import { ICertificate } from '@aws-cdk/aws-certificatemanager';
-import { IResource, Resource, Stack, Token } from '@aws-cdk/core';
+import { CfnCondition, Fn, IResource, Resource, Stack, Token } from '@aws-cdk/core';
 import { AwsCustomResource, AwsCustomResourcePolicy, AwsSdkCall, PhysicalResourceId } from '@aws-cdk/custom-resources';
 import { Construct } from 'constructs';
 import { CfnUserPoolDomain } from './cognito.generated';
@@ -154,10 +154,17 @@ export class UserPoolDomain extends Resource implements IUserPoolDomain {
    * The URL to the hosted UI associated with this domain
    */
   public baseUrl(): string {
-    if (this.isCognitoDomain) {
-      return `https://${this.domainName}.auth.${Stack.of(this).region}.amazoncognito.com`;
+    if (!this.isCognitoDomain) {
+      return `https://${this.domainName}`;
     }
-    return `https://${this.domainName}`;
+    const isGovCondition = new CfnCondition(this, "IsGovWest", {
+      expression: Fn.conditionEquals(Stack.of(this).region, "us-gov-west-1")
+    });
+    return Fn.conditionIf(
+      isGovCondition.logicalId,
+      `https://${this.domainName}.auth-fips.${Stack.of(this).region}.amazoncognito.com`,
+      `https://${this.domainName}.auth.${Stack.of(this).region}.amazoncognito.com`
+    ).toString();
   }
 
   /**

Using region-info

This also feels like a good use case for a region-info in some ways, though, us-gov-west-1 is really the only "special" region I am aware of. The service is not yet available in other US government regions nor are User Pools available yet in aws-cn.

Adding Documentation

It would also be possible to just add documentation saying that the function doesn't work in GovCloud and to provide a workaround.

Documentation patch
diff --git a/packages/@aws-cdk/aws-cognito/lib/user-pool-domain.ts b/packages/@aws-cdk/aws-cognito/lib/user-pool-domain.ts
index b201b3153..5f71fdf25 100644
--- a/packages/@aws-cdk/aws-cognito/lib/user-pool-domain.ts
+++ b/packages/@aws-cdk/aws-cognito/lib/user-pool-domain.ts
@@ -152,6 +152,10 @@ export class UserPoolDomain extends Resource implements IUserPoolDomain {
 
   /**
    * The URL to the hosted UI associated with this domain
+   *
+   * The URL returned by this method will not work in us-gov-west-1 as it does not return
+   * a FIPS-compliant endpoint URL. Instead, build the URL manually using a format such as:
+   * `https://${domain.domainName}.auth-fips.${Stack.of(domain).region}.amazoncognito.com`.
    */
   public baseUrl(): string {
     if (this.isCognitoDomain) {

Additional Information/Context

The Amazon Cognito service in AWS GovCloud (US) does not support custom domains, so the only available option is a prefixed Cognito domain.

FIPS endpoints are available but not required in us-east-1, us-east-2, us-west-1, and us-west-2.

CDK CLI Version

2.22.0

Framework Version

No response

Node.js Version

16.14.1

OS

Fedora

Language

Typescript

Language Version

No response

Other information

I am happy to help implement a fix for this issue.

@laurelmay laurelmay added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 2, 2022
@github-actions github-actions bot added the @aws-cdk/aws-cognito Related to Amazon Cognito label May 2, 2022
@corymhall
Copy link
Contributor

@kylelaker I think that I'm fine with the solution of adding a fips flag to baseUrl, I can't
really think of a better one.

We do already have an issue tracking this so I'm going to close this issue in favor of #12500

@github-actions
Copy link

github-actions bot commented May 3, 2022

⚠️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.

@laurelmay
Copy link
Contributor Author

Thanks! Opened #20200 to resolve this.

mergify bot pushed a commit that referenced this issue May 5, 2022
… url for gov cloud regions (#20200)

This ensures that users in GovCloud can retrieve a URL that works in
their region and allows users in us-{east,west}-{1,2} to also use the
FIPs endpoints.

Partially discussed in #20182.

Resolves #12500

----

### All Submissions:

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

### Adding new Unconventional Dependencies:

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

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/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*
wphilipw pushed a commit to wphilipw/aws-cdk that referenced this issue May 23, 2022
… url for gov cloud regions (aws#20200)

This ensures that users in GovCloud can retrieve a URL that works in
their region and allows users in us-{east,west}-{1,2} to also use the
FIPs endpoints.

Partially discussed in aws#20182.

Resolves aws#12500

----

### All Submissions:

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

### Adding new Unconventional Dependencies:

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

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/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*
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 bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Development

No branches or pull requests

2 participants