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

ignore UIDs created as a result of CTF mishap #239

Merged
merged 2 commits into from
Dec 4, 2021
Merged

Conversation

ethanwu10
Copy link
Member

During the CTF, a user was created with UID 13371337 for a challenge, which resulted in new users being created with UIDs > 13370000. Ignore all such users when calculating the next UID to generate (the current users in that UID range will keep their UIDs).

During the CTF, a user was created with UID 13371337 for a challenge,
which resulted in new users being created with UIDs > 13370000. Ignore
all such users when calculating the next UID to generate (the current
users in that UID range will keep their UIDs).
@kpengboy
Copy link
Member

kpengboy commented Dec 3, 2021

  • Are we doing anything to prevent this from happening again in the future?
  • Do these high UID accounts have proper quotas set up?
  • Have you considered the implications if we ever want to set up subuid for per-user Docker or the like?

If we can migrate these users to lower UIDs, maybe that would be more robust in the longer run...

@fawaf
Copy link
Member

fawaf commented Dec 3, 2021

ocf had a ctf? as in capture the flag?

@ethanwu10
Copy link
Member Author

ethanwu10 commented Dec 4, 2021

  • Are we doing anything to prevent this from happening again in the future?

To be entirely honest this PR is just to stop new accounts from continuing to be registered with high UIDs, and the only way I really see to fix the problem is to either make the UID selection logic more robust (which will take a while), carve out and document a block of UIDs for future "system" accounts, or both.

  • Do these high UID accounts have proper quotas set up?

As far as I can tell yes; we already have many UIDs >16bits, so there shouldn't:tm: be any problems here

  • Have you considered the implications if we ever want to set up subuid for per-user Docker or the like?

I'm not sure how that's relevant here since this just prevents the UID selection logic from picking these UIDs (unless you're referring to the high UIDs themselves, which is a valid concern)

If we can migrate these users to lower UIDs, maybe that would be more robust in the longer run...

I definitely agree, however that carries some risk while merging this ASAP would at least decrease the number of users we'd need to migrate (though we'd probably need to also go carve out another block in a lower range to migrate to)

@kpengboy
Copy link
Member

kpengboy commented Dec 4, 2021

Personally, I would make a permanent change to ignore all UIDs above perhaps 2 million, so this particular problem can't happen again.

But if that's not immediately desired, I would LGTM this change.

In either case, I would advocate for trying to migrate the created accounts down to lower numbers if possible, after we've stopped the bleeding.

@ethanwu10 ethanwu10 requested a review from emmatyping December 4, 2021 10:06
Copy link
Member

@singingtelegram singingtelegram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ethanwu10 ethanwu10 merged commit 44bb2c7 into master Dec 4, 2021
@ethanwu10 ethanwu10 deleted the ignore-ctf-uids branch December 16, 2021 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants