search

TimothyJones / github-cognito-openid-wrapper

Small shim that allows AWS Cognito to talk to github (by providing an OpenID wrapper around the Github API)
BSD 3-Clause "New" or "Revised" License
305 stars 92 forks source link

HTTP 500 error from GitHub when user is not logged in and 2FA is enabled #24

Open TimothyJones opened 4 years ago

TimothyJones commented 4 years ago

When I am not logged in github in another window (session) I get 500 (Looks like something went wrong!) page instead of Authorization / Login page.

Originally posted by @lolejar in https://github.com/TimothyJones/github-cognito-openid-wrapper/issues/23#issuecomment-562158185

There's an issue with GitHub where it rejects the state sent by Cognito when 2FA is enabled and the user is not logged in. We did a fair bit of testing and couldn't figure out exactly what the issue was - but we think it is to do with the length of the state parameter provided by Cognito.

You can work around this by generating your own (shorter) state token and sending them on to GitHub - but if you do this, the shim needs to maintain state, and there's an extra step where GitHub redirects back to the shim, which redirects back to Cognito.

A colleague has a private fork of this repo which maintains a mapping of the Cognito states, using a dynamo DB table to do this. I'd like to look at merging it in, but I'm in two minds, because it's adding a fair bit of extra resources (and therefore impacts cost and scalability) in order to work around a GitHub bug.

Discussion welcome

lolejar commented 4 years ago

well the best solution if possible would be to make it a feature switch. either create dynamo db and map states or allow the old authentication. anyway me myself don't mind setting up extra resources as I have 2fa enabled and it makes life a bit harder not to be able to login straight but many people don't and for them it would not make sense to set up extra resources.

DannyDouglass commented 4 years ago

hi folks - moving my input here after realizing i missed this issue in #27. imo, this idp option is incomplete without supporting 2fa, regardless of who is at fault. it's a tough pill to swallow to tell users you have to login to github externally first to allow for 2fa scenarios just to access a product leveraging this well-designed approach.

in my research, this is one of the only real resources that shares a path to enabling github sso via a cogntio openid connector. we've hit a few issues with the latest cognito features including tenant claims necessary to support appsync and graphql. my company is based in sf and has some access to aws resources. we'd be delighted to help introduce the support for github 2fa.

@TimothyJones any chance i can get access to that private fork in the meantime?

TimothyJones commented 4 years ago

I no longer have access myself (I've moved positions since then), but I've sent the team a message. I know it includes other proprietary changes, so I'm not sure they're able to share it directly.

I agree that it would be great if this wrapper "just works" with 2FA- although it's a lot of extra infrastructure to add for what is essentially a workaround.

we'd be delighted to help introduce the support for github 2fa.

This would be extremely welcome!

If it would be helpful, I can spend some time this weekend creating a diagram of the flow for the workaround.

DannyDouglass commented 4 years ago

that would be killer @TimothyJones - i shot over a note with some thoughts as well.

TimothyJones commented 4 years ago

Update: We can get the relevant parts up on a branch here for you to experiment with. I'll try to make this happen this weekend.

I don't have the time to test it and make sure it's ready to merge in, but if you can do that work, I'd be very happy to merge whatever you come up with.

DannyDouglass commented 4 years ago

perfect - i'll take care of that. looking forward to contributing to this project on that note and moving fwd.

DannyDouglass commented 4 years ago

i've already made changes to the existing repo to better support stages - i'll put up another branch w/ those changes tomorrow

DannyDouglass commented 4 years ago

that SAM stage bug was annoying ;)

DannyDouglass commented 4 years ago

@TimothyJones can you provide me push right to the repo for a branch?

TimothyJones commented 4 years ago

I can't issue granular permissions without moving the repo to an organisation. Would you be able to fork and make a PR instead?

DannyDouglass commented 4 years ago

sure! i'll set that up.

TimothyJones commented 4 years ago

@DannyDouglass I've pushed a new branch with the code from the private fork. Unfortunately, I didn't have the git metadata from it, so I wasn't able to do a proper merge.

You'll have to read through the diff and bring back in the changes like logging and your stage updates. At the moment, it doesn't build.

https://github.com/TimothyJones/github-cognito-openid-wrapper/tree/workaround-2fa-bug

I'm pushing this without doing a better merge, because it seemed like giving you partially merged code was better than waiting for me to have enough time to give you a nicer merge.

DannyDouglass commented 4 years ago

awesome - thanks @TimothyJones! I'll be going over this tomorrow. Appreciate it.

adam-nygate commented 4 years ago

Hi all, I don't suppose there's been any more work done to find the root cause of this issue? I suppose it's still a problem with the state parameter. Would it not be possible to use some kind of derived state to reduce the length of this parameter rather than making this wrapper stateful?

adam-nygate commented 3 years ago

We've just tested this again and I can't seem to reproduce this error. Perhaps it was resolved by GitHub?

We've tried:

  1. User is not logged in, app is not authorised, has MFA enabled.
  2. User is logged in, app is not authorised, has MFA enabled.
  3. User is not logged in, app is authorsed, has MFA enabled.
  4. User is logged in, app is authorised, has MFA enabled.

Can anyone provide the steps to reproduce this issue? @TimothyJones @DannyDouglass

kujtimiihoxha commented 3 years ago

@TimothyJones I know this is an old issue but could it be that you forgot to add the deploy-shim.sh in your commit?

https://github.com/TimothyJones/github-cognito-openid-wrapper/blob/workaround-2fa-bug/package.json#L22

ButterBridge commented 3 years ago

Hi @adam-nygate I came across this issue after experiencing this problem - user not logged in (incognito mode), app already authorised, with MFA enabled. Any update on progress? Would tie this together nicely - it's been such a nice repo to use!

adam-nygate commented 3 years ago

Hey @ButterBridge, I've tried to replicate your conditions and not experiencing the 500 error. I'm not sure if it would make any difference, but we're using a custom (perhaps shorter) URL for the auth, and perhaps the standard cognito generated one is causing the erring state.

Let me know if I can help troubleshoot 🙂

ButterBridge commented 3 years ago

Hey @adam-nygate thanks for the suggestion - at some point I will put it on a domain and let you know if there are any issues! In the meantime it's a prettily easily avoidable circumstance...

TimothyJones commented 3 years ago

@TimothyJones I know this is an old issue but could it be that you forgot to add the deploy-shim.sh in your commit?

https://github.com/TimothyJones/github-cognito-openid-wrapper/blob/workaround-2fa-bug/package.json#L22

@kujtimiihoxha Apologies, I missed replying to this for some reason. I believe that should be scripts/deploy.sh. The branch was created from a propriety modification of this repo- I would guess that some things were renamed when more deployables were added.

TimothyJones commented 3 years ago

@adam-nygate That's a good observation. It's possible this is related to overall URL length, as the shorter state tokens helped.

I don't think there's anything in the shim that would cause this to be a problem, so a good next step for anyone who wants to get to the bottom of this might be to try to trigger the issue without including Cognito or the shim- at least then we'd be able to file a bug report with github (I think they're most likely at fault here, although it is hard to be sure).

I remember that we found it difficult to reliably reproduce with specific lengths of state token, so I think there might be some caching or other interfering factor, too.

bensullivan commented 3 years ago

Hi all,

I think I've just encountered this problem. Here's what happened for me:

  1. GitHub account with MFA activated
  2. Logged out of GitHub
  3. App is not authorised
  4. Login to app - redirected to github authorize endpoint
  5. Enter github creds for GitHub account from step 1
  6. Boom! (ie expected dialog for GitHub MFA token entry but 500 instead)

I tried shortening the user pool domain with a custom domain this morning but no joy unfortunately. We're about to try the shortest domain we have.

Trying to get some answers out of GitHub via a paid plan.

Can I ask if anyone has had success using the workaround-2fa-bug branch?

Many thanks.

TimothyJones commented 3 years ago

I do know that code based on that branch works in a couple of production environments. I haven't included it in the main repository because it's extra resources and expenses to work around a bug that may be fixed at some indeterminate time in the future.

If I had more time, I'd love to:

1) Figure out how to consistently reproduce the problem and file a bug report in the relevant place 2) Merge in the workaround behind a deployment flag so that users who need the workaround can just enable the flag

Trying to get some answers out of GitHub via a paid plan.

If you find out any extra info, please let us know! I'd love to get this fixed for good.

bensullivan commented 3 years ago

OK - thanks @TimothyJones - I will keep you posted.

Also on chat with AWS Support atm to see if Cognito somehow supports customising the state value passed to a federated OIDC provider.

Cheers

bensullivan commented 3 years ago

Customisation of state param has been registered as feature request. It's a no go for now.

bensullivan commented 3 years ago

So I've got the workaround-2fa-bug branch up and running. It's replacing the state with a uuid on the outbound leg which gets past the bug - yay!. However, I can't see how the loginCallback is being exercised on the way back though - which means the Cognito state isn't being sent back to Cognito which causes a barf. The LoginCallback doesn't seem to be a part of the sam packaging unless I have missed something?

sambhavjain9138 commented 3 years ago

Hii Everyone I signed out of Github and enabled MFA for Cognito. Now when i signin with Github, I am asked to add password, then asked to enter verification code by Github and then successfully logged in to the callback URL. If i am already logged in, It doesn't redirect me to any page for MFA from cognito and directly takes me to callback URL. Is this a normal workflow? Because I was expecting MFA workflow from Cognito?

Thanks

TimothyJones commented 3 years ago

@sambhavjain9138 I don’t know how cognito MFA works with federation, but I don’t think the question is in scope for this repo, I’m afraid.

As far as I’m aware, federated OpenID signin can’t affect cogntio’s MFA (or any other cognito feature beyond federation).

sambhavjain9138 commented 3 years ago

Thanks @TimothyJones for that quick response. Apologies for asking an inappropriate ques, out of the scope of the repo. Thanks for your help!

csumpter commented 2 years ago

Did anyone ever get a resolution to this problem? I'm running into the same issue.