I had a mini panic attack when I refreshed a waiting PR and was suddenly logged out. I take all the suggested precautions with my GitHub account, so while my mind did wander to the worst case, I figured it was just a weird cookie interaction on my machine.
If anyone at GitHub is reading this, it would have been nice to see an alert with this info on GitHub's status page, which is the first place I checked to see if it was a known issue.
It's possible (but admittedly it costs developer time) to trigger a logout with reason, which is displayed on e.g. the login screen when users are redirected to it.
I’m very interested in the post mortem about the specific race condition.
Since the article mentions session ids returned to the wrong user my first guesses would be
1) something with thread-local storage saving the the auth info in the request handler, and then coroutines or async code that is overwriting that thread-local with info from a new request before the old request has finished.
2) messagequeue based micro service communication that was not properly checking if a received response belongs to the original request and thus returning mismatched data
>1) something with thread-local storage saving the the auth info in the request handler, and then coroutines or async code that is overwriting that thread-local with info from a new request before the old request has finished.
AFAIK github is written in ruby. Does it even have thread local storage?
Typically you’d only have one request at a time on each thread though. So your examples feel unlikely. Looking forward to finding out what happened here.
> 1) something with thread-local storage saving the the auth info in the request handler, and then coroutines or async code that is overwriting that thread-local with info from a new request before the old request has finished.
This would be my guess. This sounds like Ruby. But I've seen very similar bugs in Node, where user data is leaked across requests. The issue is typically the developer is using a global variable when they should be using the request context/storage.
In Node there is an insidious brother of this bug as well. It's where the developer is initializing some bit of code (often a 3rd party library, or loading a file) and they are doing it per request when they should be doing it once globally at process startup. It's just about the opposite of the first bug I mentioned. The result is Node leaks memory on every request until it runs out of memory and the process dies. This bug can be an incredible pain in the ass to hunt down.
I read a tweet saying: if you want to test whether your service has real engagement, log out everybody and check how many of them log back in. This can be seen as a practical test of it.
For low-friction-to-change services that can be a death-sentence, though. If I have to log in when opening my electric scooter rental app, I close it and open another one and probably won't use the other one until strictly necessary.
My company has the same kind of "problem". A logged out customer might be a lost one. So we try extremely hard to never have to log out people. So when rewriting the app, the store couldn't be touched. When changing authentication provider the old tokens still had to work and be exchanged for a looong time etc.
you just have to keep thundering herd of logins in mind though.
While the application components can handle the load, sometimes the authentication system may not do well when everyone tries to login at the same time
One assume, since this was days in the making, that the GitHub security team had operations spool up extra auth tier capacity before booting all sessions.
aws does this every 24 hours for a normal setup and that's pita. They just don't care what their users are doing. Yes I was on middle of something and suddenly saw that I had to log into their web console again. w???
Was very suspicious when I had to log back in in the middle of the day. Definitely would have appreciated an alert banner or notification somewhere. I don't often check the GitHub blog.
While there's not much for GitHub to gain from disclosing the faulty code, I still would've liked to see the actual (pseudo)code that caused the race condition
> The underlying bug existed on GitHub.com for a cumulative period of less than two weeks at various times between February 8, 2021 and March 5, 2021.
> There is no indication that other GitHub.com properties or products were affected by this issue, including GitHub Enterprise Server
Github enterprise does not include commit history, and the above two things together make it sound like this bug was in a change that was being slowly rolled out or a/b tested, and that never made it into a github enterprise release.
It's unlikely anyone outside of github can shed light on this one.
While GitHub's codebase may be a bit more complex than most of the other projects out there, i agree that a writeup of the bug would definitely be useful!
If Wikipedia is to be believed, GitHub had 56 million users in September 2020. If we adjust that to 60 million for the time that's passed, that gives us a ballpark of 600 users who were affected.
So no it's not high. But I'm impressed if they were able to accurately quantify that less than 600 users were impacted by this race condition.
Sessions != Users. I have two or three active sessions at any point, Multiple browsers + mobile. Since the issue seems to be with cookies, we can probably discard mobile.
Also each time you login/ token refreshes is probably a new session. So they easily have million's of sessions daily.
So in the one month period between 6th Feb and 5 March, you have
**(DAU) * 35* (average sessions / user)** .
So in the ballpark of 10M * 35 * 5 sessions ~ so anywhere between 700-1500 M sessions conservatively . That is in the ballpark of 15,000 users.
Also 0.001% is their belief of the impact. I don't see any evidence or statistical model or details supporting the statement.
I am deeply skeptical of this number without knowing the methodology , as you say it would quite impressive to narrow down a race condition to that level.
It's a race condition, so it had to do with two operations happening in a certain incorrect order. I bet they are able to see the exact situations where it occurred with pretty great certainty by reviewing the logs of the two operations that raced and see where the incorrect operation won. They even say they are invalidating all sessions or of an abundance of caution, indicative that they are pretty sure about it.
If anyone at GitHub is reading this, it would have been nice to see an alert with this info on GitHub's status page, which is the first place I checked to see if it was a known issue.