aws-samples / cloudfront-authorization-at-edge

Protect downloads of your content hosted on CloudFront with Cognito authentication using cookies and Lambda@Edge
https://aws.amazon.com/blogs/networking-and-content-delivery/authorizationedge-using-cookies-protect-your-amazon-cloudfront-content-from-being-downloaded-by-unauthenticated-users/
MIT No Attribution
461 stars 157 forks source link

Custom IDP with Amplify and Auth at Edge #236

Closed AzaxSyndrom closed 11 months ago

AzaxSyndrom commented 1 year ago

Hello there

after searching for hours the different issues (closed & open) on both Amplify project and here i couldn't find a similar solution.

Question

Did someone actually manage to make auth-at-Edge stack work with amplify integration and an custom IDP through Cognito ?

Quick problem overview

Amplify modify the state during a custom IDP signIn hence blocking the parse-auth process.

Context

We are using this stack for our applications with the default Cognito UI and it work great (both Cognito user-pool & Azure IDP connections). We would want to use a custom Login page with amplify to replace the hosted UI (same as shared here issue49) We created a spa with amplify & the auth module accessible on /login and modified the check-auth to redirect on our custom login page. Setup the stack for cookie compatibility amplify.

The solution works with amplify when login through Cognito user-pool. Even though the parse-auth function is completely skip during the authentification process from amplify (Auth.signIn) (maybe i'm already doing something wrong here).

But when using the Auth.federatedSignIn({ customProvider: 'my-providerName-setup-inCognito'}); the state generated from the check-auth is not send to oauth2/authorize request. Amplify generate it's own state.
It does have the option to pass a customState attribute to the federatedSignIn methods. But even with that, it still modify it.

So when the authentification flow from IDP finish and that we end up on the parse-auth, the lambda refuse the connexion because the state cannot be parse as intended here

parsedState = JSON.parse( Buffer.from(common.urlSafe.parse(state), "base64").toString() ); Did someone managed a similar solution ?

Possible solutions

1 - Not using amplify and do all the IDP flow manually in the custom login page (still weird for a compliant amplify stack) 2 - Override amplify class to get and set the state correctly -> cumbersome & not easy to maintain later on 3 - Pass the state in a header during check-auth et set it again in parse-auth -> Isn't that posing a security risk ? (like losing the whole interest of the state)

It's a solution i take by heart and would really wanna be able to achieve !

Thank you for the reading ❤️

ottokruse commented 1 year ago

Great question! Very interesting to read what you're up to.

So, matching AmplifyAuth.federatedSignIn with the parseAuth of this solution here does not work because they do state differently.

I think it's easiest to also handle the redirect from Cognito with Amplify (not parseAuth).

So make sure to use as redirectUri the unprotected piece of your site where you have Amplify running. Don't use /parseauth as redirect URI as it would trigger parseAuth.

Let Amplify automatically handle the redirect as it will, get JWTs and set them in cookies, that should work with Auth@Edge too from that moment on. That's the compatibility we intent: the cookies with the JWTs are recognized by Amplify as well as Auth@Edge, irregardless of whether they were set by Amplify or Auth@Edge.

Haven't tried this though so it's unproven. Let me know what you run into.

AzaxSyndrom commented 1 year ago

Well mate... I was so focus to make it work along this stack i did not though of that.
So in that case both Cognito login & IDP login skip the parse auth lambda (and won't use the Auth-At-edge state generated by the check-auth).

Thanks for your quick reply. It worked (for the few test i just did).

Well i still need to do lot more test to see how it behave with the rest of the stack (refresh token and all). On top of that i would want to reduce the number of cookie amplify is returning, will try to modify a bit the behavior on that sens.

And need to try if i can redirect directly to my home instead of my login to avoid useless load + redirect after auth.

Will get back to you on Monday with my differents findings.

Thanks again ! you already made my weekend ❤️

AzaxSyndrom commented 1 year ago

nb : I was already checking how to inject the auth-at-edge state in the amplify call. And then in the Edge get it back and transform it back in the correct format -_- So i'm avoiding quite a lot with this

ottokruse commented 1 year ago

Okay let's see! You're welcome and I hope this is a good path forward for you 👍🏻👍🏻

AzaxSyndrom commented 1 year ago

Hello @ottokruse ! Sorry to come back this late i had to prioritize other subjects.
During my test about the refresh token flow i notice something. By default amplify generate cookie with a dot before the domain ".mycookiedomain" Reading about it, it's suppose to be a standard. But when the refresh flow happens instead of replacing existing Amplify cookie. I got new ones without the dot for the domain. And the Expire max age as well goes to session (as mention in the blog).

The thing is that duplicating the already numberous cookies is problematic (too heavy request, waf rules etc.)

My solution is to force the domain directly in the cookieSetting "cookieSettings": { "idToken": "Secure; HttpOnly; SameSite=Lax; Path=/; Domain=.mywebsite.com", "refreshToken": "Secure; HttpOnly; SameSite=Lax; Path=/; Domain=.mywebsite.com", "nonce": "Secure; HttpOnly; Max-Age=300; SameSite=Lax; Path=/; Domain=.mywebsite.com" },

That works. But again weird. Did i missunderstood some configuration or those process aren't compatible with amplify (that force the dot for the cookie domain).

Another thing i'm struggling with is the current signOut process. It seems it only cleans the ID Token. So in case you signOut with a user then reconnect with another one etc. You duplicate all the others amplify cookies because the cookie got the username in his name.

image

So now trying to update the signOut process to clean everything. Would love to be able to rename amplify cookie but not possible i would say...

ottokruse commented 1 year ago

Hi @AzaxSyndrom

By default amplify generate cookie with a dot before the domain ".mycookiedomain" Reading about it, it's suppose to be a standard.

Actually it is better to not use the leading dot, because that makes cookies readable by subdomains too.

To get Amplify to work with that and prevent the double cookies that you see, use a single space " " as the Amplify cookieStorage domain setting:

Amplify.configure({
  Auth: {
    ...,
    cookieStorage: {
      domain: " ", // Use a single space " " for host-only cookies
      ...
    },
  },
});

Read the comment here for more details: https://github.com/aws-samples/cloudfront-authorization-at-edge/blob/996a07f6bee12b4b62503ac763d5465ee0a06a1c/src/cfn-custom-resources/react-app/index.ts#L64C23-L64C23

ottokruse commented 1 year ago

About the signOut only clearing ID token, it should clear all cookies actually so we need to dig in.

The reason this probably happens is because you cannot explicitly delete cookies, only overwrite them with an empty value and expiry set (then browser understands it must discard it). However, overwrite like that only works if you use the exact same cookie settings as you did when you initially set the cookies. So e.g. exact same path, domain, etc. Have a look if that is the case for you? Note that this PR did fix an issue like this: https://github.com/aws-samples/cloudfront-authorization-at-edge/pull/207 So make sure you are on a version of Auth@Edge that includes that fix (v2.1.2 onwards)

AzaxSyndrom commented 11 months ago

Hi getting back to you about those signOut cookies and the cookieDomain. It all works With the latest version of amplify the clearing cookie problem is gone. We have a working solution. Thanks again !

ottokruse commented 11 months ago

Nice! 🎉