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

*passport-steam* failing #1347

Closed
Martii opened this issue Apr 18, 2018 · 13 comments
Closed

*passport-steam* failing #1347

Martii opened this issue Apr 18, 2018 · 13 comments
Labels
bug You've guessed it... this means a bug is reported. question A question has been encountered by anyone and has remained unanswered until cleared.

Comments

@Martii
Copy link
Member

Martii commented Apr 18, 2018

Both 1.0.8, 1.0.9 and the OUJS forks (commit specified) are producing ?authfail.

@Martii Martii added the tracking upstream Waiting, watching, wanting. label Apr 18, 2018
Martii added a commit to Martii/OpenUserJS.org that referenced this issue Apr 18, 2018
* Don't know all the consequences beside OpenUserJS#1347 but will retest what I can


NOTE(S):
* Leaving README.md alone since we might go back
Martii added a commit that referenced this issue Apr 18, 2018
* Don't know all the consequences beside #1347 but will retest what I can


NOTE(S):
* Leaving README.md alone since we might go back

Auto-merge
@Martii
Copy link
Member Author

Martii commented Apr 18, 2018

Still failing when deployed via git... not sure what to do and I'm outta time for now.


Note: may be related to the extra session found at #1344 ... another one popped up.

@Martii
Copy link
Member Author

Martii commented Apr 19, 2018

Some notes out loud (may add/change to this):

  1. This is directly related to the non-user profiled session found at Existence checks for #1343 #1344 earlier today ... tested on dev and it creates one on auth fail. sessionStore.clear(); has no effect when the "stuck" session data is removed (other than logging out everything by removal of the MongoDB session cookies).
  2. aLoggedIn argument at
    exports.verify = function (aId, aStrategy, aUsername, aLoggedIn, aDone) {
    shows as undefined e.g. blank in console output... which triggers this section at
    } else if (aUser) {
    // user was found matching name but not can't be authenticated
    return aDone(null, false, 'username is taken');
    which in turns triggers
    if (isDev || isDbg) {
    console.error(colors.red('`User` not found'));
    }
    aRes.redirect(doneUri + (doneUri === '/' ? 'login' : '') + '?authfail');
    return;
    }
  3. Confirmed that
    profile: false,
    is set to match docs at /liamcurry/passport-steam/blob/92974ba/README.md#configure-strategy
  4. if (strategy === 'steam') {
    strategyInstance._verify = function (aIgnore, aId, aDone) {
    verifyPassport(aId, strategy, username, aReq.session.user, aDone);
    };
    } else {
    has the right number of parameters according to docs at /liamcurry/passport-steam/blob/92974ba/README.md#configure-strategy
  5. Reverted way back to 9688002 in dev... still failing e.g. probable that it's steam site related and/or the dependency (however since passport-steam@1.0.8 doesn't work... it's possibly site related instead)
  6. Removed passport-aol and passport-yahoo (including in strategy JSON list) temporarily in dev to ensure passport-openid wasn't the issue... removed node_modules and re npm i'd... no difference.

@Martii Martii added the bug You've guessed it... this means a bug is reported. label Apr 19, 2018
@Martii
Copy link
Member Author

Martii commented Apr 19, 2018

@sizzlemctwizzle

Could use some expertise here please.

@Martii
Copy link
Member Author

Martii commented Apr 19, 2018

Lovely... it looks like my auth key changed... dumped dev account Martiii and recreated it... it has a different auth digest (I call it a hash key) then pro now. Logging into dev is solid so far with OUJS logout and then Steam logout. This issue seems similar to what happened with passport-google and them. :S

So two issues here:

  • When an auth key doesn't match it doesn't automatically destroy the cookie/session server side. Work around for usage in Existence checks for #1343 #1344 but still I think it should destroy it then and there for tidiness (e.g. this is currently a session leak that auto-repairs on expiration which can be hours later (from Nudge session maxAge #1252) or never like it was before Set maxAge idle session to four (4) hours #1202... keeps username in there though). (btw the "stuck" session expired so session length and session active data match right now). Here's a diff that should fix that from another section of code. Seems to work when I try with my hosed Steam account on (local) pro e.g. length and active remain the same count (but username is blanked):
diff --git a/controllers/auth.js b/controllers/auth.js
index 7b01d3c..41d08c0 100644
--- a/controllers/auth.js
+++ b/controllers/auth.js
@@ -243,6 +243,12 @@ exports.callback = function (aReq, aRes, aNext) {
         console.error(colors.red('`User` not found'));
       }
 
+      if (aReq.session.destroy) {
+        aReq.session.destroy();
+      } else { // TODO: Remove conditional and this fallback when satisifed
+        delete aReq.session.user;
+      }
+
       aRes.redirect(doneUri + (doneUri === '/' ? 'login' : '') + '?authfail');
       return;
     }

(Note: Still not satisfied to remove the else yet but here the .user isn't added until a bit later in the same function. Also assume it could be used earlier on the "catastrophic" error trap earlier in the same callback... perhaps other places too... need to dig)

  • The session server side "stuck" cookie in Existence checks for #1343 #1344 was before 2018-04-18 16:32:28.227 +00:00 and after 2018-04-17 21:07:38.075 +00:00 ... in which I wasn't on OUJS... so there could be someone else with the same or similar issue (perhaps just a bad, or incomplete, attempt in logging in too) that initially created it.

A good question is... Does Steam (or any auth) expire your openid/oauth after not logging in for a while? ❓ (it's probably been 5 to 6 months since I last used it)... If so that's really bad for OUJS accounts. Alternatively Steam could have changed their digest auth key algorithm server side before/during/after their http to https migration without our knowledge.


Rechecked OUJS forks... they appear to be solid as well with Martiii recreation on dev... so can revert the revert of that. So that leaves the question of why the original Steam digest auth key isn't working.


Ref(s):

@Martii Martii added the question A question has been encountered by anyone and has remained unanswered until cleared. label Apr 19, 2018
@Martii
Copy link
Member Author

Martii commented Apr 20, 2018

@sizzlemctwizzle

I think I've identified the bug... indirectly mentioned in the above comment with:

Alternatively Steam could have changed their digest auth key algorithm server side before/during/after their http to https migration without our knowledge.

Comparing my digest on pro to the digest from the creation of the new account on dev.... the difference is the aId contains https:... now instead of http:...... and running both, full, values through the crypto routine produces the exact hash key that I need for Steam authentication respectively.

What this means is everyone, and I do mean everyone, that has Steam as their auth prior to their https switch... their account access with this auth is currently orphaned... I do see a potential recovery option however we should try to figure that part out to keep things private as possible and preferably as smooth as possible (e.g. right now it is painstakingly lengthy to convert as the values are currently unknown)

Also there were a few comments on their repo that they "might go back" which will compound the issue in reverse.

Ref(s):

@Martii Martii self-assigned this Apr 21, 2018
Martii added a commit to Martii/OpenUserJS.org that referenced this issue Apr 21, 2018
Martii added a commit that referenced this issue Apr 21, 2018
Applies to #1347 

Auto-merge
@Martii Martii assigned Martii and unassigned Martii Apr 21, 2018
Martii added a commit to Martii/OpenUserJS.org that referenced this issue Apr 22, 2018
* This was "our bug" and probably should have been anticipated back in the begginning of OUJS with Steam
* This takes care of the wishy-washy replies I read on a possible reversion from secure to unsecure in their routines. We never utilize the plain text value stored in `aId` so it's not important to match site secure status

NOTE(S):
* There is a manual recovery path discovered for those who have access to the DB directly but working on offloading it to the users. Need sleep first then more testing.
* Still keeping steam auth read-only until the DB can be examined further and this issue recovery

Applies to OpenUserJS#1347
Martii added a commit that referenced this issue Apr 22, 2018
* This was "our bug" and probably should have been anticipated back in the begginning of OUJS with Steam
* This takes care of the wishy-washy replies I read on a possible reversion from secure to unsecure in their routines. We never utilize the plain text value stored in `aId` so it's not important to match site secure status

NOTE(S):
* There is a manual recovery path discovered for those who have access to the DB directly but working on offloading it to the users. Need sleep first then more testing.
* Still keeping steam auth read-only until the DB can be examined further and this issue recovery

Applies to #1347

Auto-merge
@sizzlemctwizzle
Copy link
Member

  1. Blindly hash the value we get back from steam and see if it matches the stored hash. If it matches, then login.
  2. If not, replace /^https:/ with http: on the value we get from seam. Hash it and see if it matches the stored hash.
  3. If it does, hash the the unmodified value we got from steam, and update the hash that we store to this new value. Finally, login.

@Martii
Copy link
Member Author

Martii commented Apr 22, 2018

@sizzlemctwizzle

Thank you!!!

We're mostly on the same page here. Although we need to narrow down the exact date Steam did this (some can be done with User.authed and/or User._since and around April 7th, 2018) and add the test to anything before that and apply the variant of No 3. We may have some accounts in the last two'ish weeks that are already correct. No. 2 is permanent now with aa2eda3 in case Steam "changes their mind" and goes back to http (commented in commit). We don't use the plain text value anywhere other than hashing it to a digest.

I'd like to keep most of the changes extremely simple in the code, and avoid using async which would be needed (very messy still for a recovery/migration that shouldn't happen ever again), it is going to still fail but with a different QSP... e.g. offload it to the user... then they can try again with their newly saved/migrated/recovered hash.

I'm currently anonymously auditing the other strategies in case there is a similar potential issue. (if you want to view it do git diff on clone 1). You're the expert if you can fill out the rest of the below list but I don't expect you to recall back that far,,, hence the audit. :)

  • Steam of course does contain a secure/unsecure reference
  • Google has an array and doesn't contain a secure/unsecure reference
  • Github of course isn't hashed and doesn't contain a secure/unsecure reference
  • Twitter doesn't contain a secure/unsecure reference
  • Facebook doesn't contain a secure/unsecure reference
  • Reddit doesn't contain a secure/unsecure reference
  • Imgur... doesn't contain a secure/unsecure reference
  • AOL ... Dep/site failure... removed in *passport-aol* failing #1369
  • Yahoo... DOES CONTAIN secure reference.

Just need the rest of the auths now but that can be whenever someone signs in

  • GitLab... doesn't contain a secure/unsecure reference

@Martii
Copy link
Member Author

Martii commented Apr 23, 2018

Ref:

Martii added a commit to Martii/OpenUserJS.org that referenced this issue Apr 23, 2018
* Should run this code for at least 6 months
* Fix a bug in implementation with mongoose


Applies to OpenUserJS#1347
Martii added a commit that referenced this issue Apr 23, 2018
* Should run this code for at least 6 months
* Fix a bug in implementation with mongoose


Applies to #1347 

Auto-merge
@Martii
Copy link
Member Author

Martii commented Apr 23, 2018

My steam account migrated/recovered well on pro... So if the users on OUJS are observing this issue and have an existing steam account please login it even if it's just for a moment. This is a two time login migration/recovery. Single would have entailed a rather large rewrite and the recovery won't be in there forever imho. e.g. it has at least 6 months before it should be removed to allow those users to recover during that time.

I would like to have a few days (and see some users doing it in stdout) before turning off readonly for new/attached steam accounts.

@Martii Martii removed the tracking upstream Waiting, watching, wanting. label Apr 23, 2018
Martii added a commit to Martii/OpenUserJS.org that referenced this issue Apr 23, 2018
* Best to be safe on this in case another auth is present and for some way rare case it has the exact same digest. Can handle these manually if need be but should be extremely rare.

Applies to OpenUserJS#1347
Martii added a commit that referenced this issue Apr 23, 2018
* Put in a mult-auth collision check

* Best to be safe on this in case another auth is present and for some way rare case it has the exact same digest. Can handle these manually if need be but should be extremely rare.

Applies to #1347

* Fix conditional positioning

* Add a few more stdout and stderr messages to monitor

Applies to #1354

Auto-merge
@Martii Martii added the needs testing Anyone can add this but it is primarily there for the Assignee indicating that Testers are wanted. label Apr 23, 2018
@Martii
Copy link
Member Author

Martii commented May 4, 2018

11 days and not a single Steam user migrating? I have seen some of the users logged in that have it as an alternate but they haven't bothered yet.

For pro reference:

Tick tock, tick tock... 😸

@Martii Martii added needs mitigation Needs additional followup. and removed needs testing Anyone can add this but it is primarily there for the Assignee indicating that Testers are wanted. labels May 9, 2018
@Martii
Copy link
Member Author

Martii commented May 9, 2018

Finally one user migrated besides me... not a very used passport.

@Martii Martii added this to the Issue-1347removal milestone May 9, 2018
@Martii Martii mentioned this issue May 9, 2018
@Martii
Copy link
Member Author

Martii commented Jun 22, 2018

Still only myself and one other that has recovered their Steam auth... as of today...

@Martii
Copy link
Member Author

Martii commented Jun 22, 2018

Closing but mitigation label still active.

@Martii Martii closed this as completed Jun 22, 2018
@Martii Martii removed their assignment Jun 22, 2018
@Martii Martii self-assigned this Nov 1, 2018
@Martii Martii reopened this Nov 1, 2018
Martii added a commit to Martii/OpenUserJS.org that referenced this issue Nov 1, 2018
* Plenty of notice for this... no excuses.
* Leaving redirect conditionals in as this probably won't be the only instance we need this down the line.

Post OpenUserJS#1347 OpenUserJS#1353
Martii added a commit that referenced this issue Nov 1, 2018
* Plenty of notice for this... no excuses.
* Leaving redirect conditionals in as this probably won't be the only instance we need this down the line.

Post #1347 #1353

Auto-merge
@Martii Martii removed the needs mitigation Needs additional followup. label Nov 1, 2018
@Martii Martii removed their assignment Nov 1, 2018
@Martii Martii closed this as completed Nov 1, 2018
@OpenUserJS OpenUserJS locked as resolved and limited conversation to collaborators Apr 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug You've guessed it... this means a bug is reported. question A question has been encountered by anyone and has remained unanswered until cleared.
Development

No branches or pull requests

2 participants