HTTP 500 error from GitHub when user is not logged in and 2FA is enabled #24
Comments
TimothyJones
changed the title
TimothyJones
changed the title
|
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. |
|
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
added
bug
|
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.
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. |
|
that would be killer @TimothyJones - i shot over a note with some thoughts as well. |
|
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. |
|
perfect - i'll take care of that. looking forward to contributing to this project on that note and moving fwd. |
|
i've already made changes to the existing repo to better support stages - i'll put up another branch w/ those changes tomorrow |
|
that SAM stage bug was annoying ;) |
|
@TimothyJones can you provide me push right to the repo for a branch? |
|
I can't issue granular permissions without moving the repo to an organisation. Would you be able to fork and make a PR instead? |
|
sure! i'll set that up. |
|
@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. |
|
awesome - thanks @TimothyJones! I'll be going over this tomorrow. Appreciate it. |
|
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? |
|
We've just tested this again and I can't seem to reproduce this error. Perhaps it was resolved by GitHub? We've tried:
Can anyone provide the steps to reproduce this issue? |
|
@TimothyJones I know this is an old issue but could it be that you forgot to add the |
|
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! |
|
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 🙂 |
|
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... |
@kujtimiihoxha Apologies, I missed replying to this for some reason. I believe that should be |
|
@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. |
|
Hi all, I think I've just encountered this problem. Here's what happened for me:
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. |
|
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:
If you find out any extra info, please let us know! I'd love to get this fixed for good. |
|
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 |
|
Customisation of state param has been registered as feature request. It's a no go for now. |
|
So I've got the |
|
Hii Everyone Thanks |
|
@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). |
|
Thanks @TimothyJones for that quick response. Apologies for asking an inappropriate ques, out of the scope of the repo. Thanks for your help! |
|
Did anyone ever get a resolution to this problem? I'm running into the same issue. |
Originally posted by @lolejar in #23 (comment)
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
The text was updated successfully, but these errors were encountered: