fix(elasticloadbalancingv2): ALB auth return internal server error #24510

WinterYukky · 2023-03-08T12:20:07Z

Summary

Allow HTTPS outbound traffic for security groups attached to the Application Load Balancer if the Application Load Balancer is configured with an authentication configuration.

Why is this PR needed?

Application Load Balancer authentication requires HTTPS outbound traffic.
However, the security group attached to the ApplicationLoadBalancer does not allow traffic to the outside, so the code as described in the documentation will not work by itself.

This issue is also documented.
https://aws.amazon.com/premiumsupport/knowledge-center/elb-configure-authentication-alb/?nc1=h_ls

Related issues

Following opened issues were fixed by #21939, but related this PR.
Closes #19035 #18944.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

aws-cdk-automation · 2023-04-01T00:09:08Z

This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

…x/alb-auth-return-internal-server-error

vinayak-kukreja

Hey, thank you for your contribution. This change looks good to me. 🚀

Getting a second opinion from @corymhall

corymhall

@WinterYukky thanks for the PR! I think for this change I would like for someone from the core team to run the integration test to make sure that it works. I also think the integration test needs to have assertions that verify that the request will succeed.

The only other general question I have is around allow all traffic. I can't find any documentation around any published IPs for Cognito and obviously we wouldn't be able to know the IPs of other IdPs, but I think we need to think about how the user could configure it with specific IPs.

corymhall · 2023-04-13T10:30:25Z

packages/@aws-cdk/aws-elasticloadbalancingv2/test/integ.alb.oidc.ts

+import * as elbv2 from '../lib';
+
+// This test can only be run as a dry-run at this time due to requiring a certificate
+class AlbOidcStack extends Stack {


@WinterYukky can you set this test up using specific env variables. Use this test as a reference https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk-testing/framework-integ/test/aws-certificatemanager/test/integ.certificate-name.ts

Our test framework recognizes those env variables and will replace them with dummy values.

corymhall · 2023-04-13T10:31:02Z

packages/aws-cdk-lib/aws-elasticloadbalancingv2-actions/lib/cognito-action.ts

@@ -88,5 +89,6 @@ export class AuthenticateCognitoAction extends elbv2.ListenerAction {
        sessionTimeout: options.sessionTimeout?.toSeconds().toString(),
      },
    }, options.next);
+    this._bindHook = (_scope, listener) => listener.connections.allowToAnyIpv4(Port.tcp(443), 'Allow to IdP endpoint');


Rather than doing this _bindHook method I think I would rather just overwrite the bind method on both of these actions.

aws-cdk-automation · 2023-04-21T00:07:40Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-04-22T00:07:54Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

…x/alb-auth-return-internal-server-error

gitpod-io · 2023-04-22T04:41:45Z

Pull request has been modified.

WinterYukky

Thank you for reviewing @corymhall !!
I've fixed it in line with your comments. Can you review it again?

WinterYukky · 2023-04-30T15:34:21Z

...ws-cdk-testing/framework-integ/test/aws-elasticloadbalancingv2-actions/test/integ.cognito.ts

+const testUser = new CognitoUser(testCase, 'User', {
+  userPool: testCase.userPool,
+  username: 'test-user@example.com',
+  password: 'TestUser@123',
+});
+// this function signs in to the website and returns text content of the authenticated page body
+const signinFunction = new lambda.Function(testCase, 'Signin', {
+  functionName: 'cdk-integ-alb-cognito-signin-handler',
+  code: lambda.Code.fromAsset('alb-cognito-signin-handler'),
+  handler: 'index.handler',
+  runtime: lambda.Runtime.NODEJS_18_X,
+  environment: {
+    TEST_USERNAME: testUser.username,
+    TEST_PASSWORD: testUser.password,
+    TEST_URL: `https://${domainName}`,
+  },
+  memorySize: 1024,
+  timeout: Duration.minutes(5),
+});
+const invoke = test.assertions.invokeFunction({
+  functionName: signinFunction.functionName,
+});
+invoke.expect(integ.ExpectedResult.objectLike({
+  Payload: '"Authenticated"',
+}));


I added an assertion to sign in using puppeteer and verify authenticated page content.

WinterYukky · 2023-04-30T15:41:40Z

packages/aws-cdk-lib/aws-elasticloadbalancingv2-actions/lib/cognito-action.ts

+
+  /**
+   * Allow HTTPS outbound traffic to communicate with the IdP.
+   *
+   * Set this property to false if the IP address used for the IdP endpoint is identifiable
+   * and you want to control outbound traffic.
+   * Then allow HTTPS outbound traffic to the IdP's IP address using the listener's `connections` property.
+   *
+   * @default true
+   * @see https://repost.aws/knowledge-center/elb-configure-authentication-alb
+   */
+  readonly allowHttpsOutbound?: boolean;


I added a property that allows customers to control whether to allow HTTPS outbound traffic. This makes it possible to meet the requirements of customers who don't want to allow outbound traffic.

WinterYukky · 2023-04-30T15:43:13Z

packages/aws-cdk-lib/aws-elasticloadbalancingv2-actions/lib/cognito-action.ts

+  public bind(scope: Construct, listener: elbv2.IApplicationListener, associatingConstruct?: IConstruct | undefined): void {
+    super.bind(scope, listener, associatingConstruct);
+
+    if (!this.allowHttpsOutbound) return;
+    listener.connections.allowToAnyIpv4(Port.tcp(443), 'Allow to IdP endpoint');
+  }


I rewrote it to a method override instead of bindHook

…x/alb-auth-return-internal-server-error

corymhall

Great work! Love the integ tests!

corymhall · 2023-05-05T14:00:13Z

...ages/@aws-cdk-testing/framework-integ/test/aws-elasticloadbalancingv2/test/integ.alb.oidc.ts

@@ -0,0 +1,198 @@
+import * as integ from '@aws-cdk/integ-tests-alpha';


This is awesome!

mergify · 2023-05-05T14:03:09Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-05-05T14:45:54Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: e6b5f52
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-05-05T14:46:18Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

fix(elbv2): alb auth return internal server error

2941da6

github-actions bot added the p2 label Mar 8, 2023

aws-cdk-automation requested a review from a team March 8, 2023 12:20

github-actions bot added the valued-contributor [Pilot] contributed between 6-12 PRs to the CDK label Mar 8, 2023

aws-cdk-automation previously requested changes Mar 8, 2023

View reviewed changes

test(elbv2): add oidc integ test

98353ee

WinterYukky changed the title ~~fix(elbv2): ALB auth return internal server error~~ fix(elasticloadbalancingv2): ALB auth return internal server error Mar 8, 2023

WinterYukky added 2 commits April 1, 2023 16:06

Merge branch 'main' of https://github.com/WinterYukky/aws-cdk into fi…

4025500

…x/alb-auth-return-internal-server-error

chore: move to aws-cdk-lib

c39e749

vinayak-kukreja self-assigned this Apr 12, 2023

vinayak-kukreja reviewed Apr 12, 2023

View reviewed changes

corymhall previously requested changes Apr 13, 2023

View reviewed changes

Merge branch 'main' of https://github.com/WinterYukky/aws-cdk into fi…

e8ed756

…x/alb-auth-return-internal-server-error

WinterYukky added 5 commits April 23, 2023 08:43

feat(elasticloadbalancingv2): add allowHttpsOutbound props

3fa1ef7

test(elastiloadbalancingv2): add integ test assertion

9da112f

test(elbv2-actions): add integ test assertions

a491c5b

test(elbv2-actions): Change the domain to a unique

b0c4074

chore(elbv2): remove bindHook property

e5b2899

WinterYukky commented Apr 30, 2023

View reviewed changes

WinterYukky added 5 commits May 1, 2023 00:47

Merge branch 'main' into fix/alb-auth-return-internal-server-error

de816e9

chore: update integ tests

fee4210

Merge branch 'main' of https://github.com/WinterYukky/aws-cdk into fi…

46c1605

…x/alb-auth-return-internal-server-error

Merge branch 'aws:main' into fix/alb-auth-return-internal-server-error

b46a573

test(elbv2): update integ

11da833

Merge branch 'main' into fix/alb-auth-return-internal-server-error

04d7b60

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label May 5, 2023

corymhall approved these changes May 5, 2023

View reviewed changes

Merge branch 'main' into fix/alb-auth-return-internal-server-error

e6b5f52

mergify bot merged commit 75212eb into aws:main May 5, 2023

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(elasticloadbalancingv2): ALB auth return internal server error #24510

fix(elasticloadbalancingv2): ALB auth return internal server error #24510

WinterYukky commented Mar 8, 2023

aws-cdk-automation left a comment

aws-cdk-automation commented Apr 1, 2023

vinayak-kukreja left a comment

corymhall left a comment

corymhall Apr 13, 2023

corymhall Apr 13, 2023

aws-cdk-automation commented Apr 21, 2023

aws-cdk-automation commented Apr 22, 2023

gitpod-io bot commented Apr 22, 2023

WinterYukky left a comment

WinterYukky Apr 30, 2023

WinterYukky Apr 30, 2023

WinterYukky Apr 30, 2023

corymhall left a comment

corymhall May 5, 2023

mergify bot commented May 5, 2023

aws-cdk-automation commented May 5, 2023

mergify bot commented May 5, 2023

		@@ -0,0 +1,198 @@
		import * as integ from '@aws-cdk/integ-tests-alpha';

fix(elasticloadbalancingv2): ALB auth return internal server error #24510

fix(elasticloadbalancingv2): ALB auth return internal server error #24510

Conversation

WinterYukky commented Mar 8, 2023

Summary

Why is this PR needed?

Related issues

aws-cdk-automation left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Apr 1, 2023

vinayak-kukreja left a comment

Choose a reason for hiding this comment

corymhall left a comment

Choose a reason for hiding this comment

corymhall Apr 13, 2023

Choose a reason for hiding this comment

corymhall Apr 13, 2023

Choose a reason for hiding this comment

aws-cdk-automation commented Apr 21, 2023

aws-cdk-automation commented Apr 22, 2023

gitpod-io bot commented Apr 22, 2023

WinterYukky left a comment

Choose a reason for hiding this comment

WinterYukky Apr 30, 2023

Choose a reason for hiding this comment

WinterYukky Apr 30, 2023

Choose a reason for hiding this comment

WinterYukky Apr 30, 2023

Choose a reason for hiding this comment

corymhall left a comment

Choose a reason for hiding this comment

corymhall May 5, 2023

Choose a reason for hiding this comment

mergify bot commented May 5, 2023

aws-cdk-automation commented May 5, 2023

AWS CodeBuild CI Report

mergify bot commented May 5, 2023