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

feat: parallel limited time load of user META info #71

Merged
merged 22 commits into from
Sep 2, 2022

Conversation

RYNEQ
Copy link
Collaborator

@RYNEQ RYNEQ commented Jul 8, 2022

Now Public IP, ASN, Region, ... are loaded using separated helper daemon process
Data are shared using shared memory between two processes
we have a minimum wait time to let helper process complete, if not ,main
process will terminate assuming meta data cannot be found (No Internet or etc)

Now Public IP, ASN, Region, ... are loaded using separated helper daemon process
Data are shared using shared memory between two processes
we have a minimum wait time to let helper process complete, if not ,main
process will terminate assuming meta data cannot be found (No Internet or etc)
@xhdix xhdix requested review from xhdix and bassosimone July 9, 2022 09:30
@xhdix xhdix added bug Something isn't working priority/high labels Jul 9, 2022
@xhdix xhdix added this to In progress in Wiki Censorship via automation Jul 9, 2022
utils/trace.py Outdated
args=(USER_META_INFO_NO_INTERNET, USER_META_INFO_PUBLIC_IP,
USER_META_INFO_NETWORK_ASN, USER_META_INFO_NETWORK_NAME,
USER_META_INFO_COUNTRY_CODE, USER_META_INFO_CITY, USER_META_INFO_DONE, user_iface),
daemon=True).start()
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be cool here if it would be possible to drop privileges for the background process. I am not sure whether this is possible with the Process API. But, if that's the case, I'd really consider doing that. If Process does not support that, a possible way to implement this would be perhaps to drop privileges in get_meta before performing other network-bound operations. I think a good user to drop privileges to is nobody by default and maybe a user should be able to specify the user to drop privileges to (but nobody as the default still looks reasonably good).

Now request is made in serial but lack of timeout is handled using separated process
@RYNEQ
Copy link
Collaborator Author

RYNEQ commented Jul 15, 2022

Now everything in get_meta is done in separated process to overcome lack of timeout in gethostbyname in some cases
call is made serially before trace
Also a privilege drop is done in separated process nobody and nogroup (this needs test in windows)

@xhdix xhdix requested a review from bassosimone July 19, 2022 06:54
Copy link
Contributor

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

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

More comments on how I think we should be dropping privileges plus a suggestion to use a separate process, if possible, since this strikes me as more robust.


def drop_privileges():
if os.name == 'posix':
if os.geteuid() == 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

A long time ago, I have read this paper about dropping privileges: Setuid Demystified.

The most important TL;DR from that paper is to use setresuid and setresgid when available.

In addition to that, I also recommend setting minimal supplementary groups.

More than 10 yeas ago I summarized my understanding of the whole dropping privileges dance in code that I was maintaining at the time. It may be useful to take a look and improve the dropping process.

Copy link
Collaborator Author

@RYNEQ RYNEQ Jul 19, 2022

Choose a reason for hiding this comment

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

Thank you
I agree this is a better approach, I will change the method according to this.

utils/geolocate.py Show resolved Hide resolved
uid = os.geteuid()
gid = os.getegid()
os.setegid(65534)
os.seteuid(65534)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, also: I suggest to use getpwnam to obtain the correct IDs. We cannot assume they're always as such, even though they probably are. We would like to ensure we're using the right IDs.

Copy link
Collaborator Author

@RYNEQ RYNEQ Jul 19, 2022

Choose a reason for hiding this comment

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

We did so until 2d1dee9 as it requires loading libraries (pwd & grp) I checked the current state of BSD & Linux and it seems they are using overflowuid (from /proc/sys/fs/overflowuid ) as the noboy and nogroup id, so we thought we can reduce imported modules by hard-coding this id as it seems in current 64bit systems it is always 65534

@xhdix xhdix requested a review from bassosimone August 11, 2022 21:22
we use independent process in posix system to make privilege dropping
stable and thread based approach in windows to prevent windows system
get suspicious and block requests
we use independent process in posix system to make privilege dropping
stable and thread based approach in windows to prevent windows system
get suspicious and block requests
move unix privilege dropping into process itself
utils/geolocate.py Outdated Show resolved Hide resolved
utils/geolocate.py Outdated Show resolved Hide resolved
utils/trace.py Outdated Show resolved Hide resolved
utils/geolocate.py Outdated Show resolved Hide resolved
@RYNEQ RYNEQ force-pushed the feat/parallel-metadata-query branch from a4fc56b to bf5c82a Compare August 28, 2022 12:03
utils/geolocate.py Outdated Show resolved Hide resolved
utils/geolocate.py Outdated Show resolved Hide resolved
utils/geolocate.py Outdated Show resolved Hide resolved
utils/geolocate.py Outdated Show resolved Hide resolved
xhdix and others added 4 commits September 2, 2022 17:49
Co-authored-by: Simone Basso <bassosimone@gmail.com>
Co-authored-by: Simone Basso <bassosimone@gmail.com>
Copy link
Contributor

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

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

🐳

@xhdix
Copy link
Contributor

xhdix commented Sep 2, 2022

there is a race condition that should be fixed. but it's a UX bug, so we are good for now and will take care of it later.
Thank you @RYNEQ and @bassosimone ❤️ 🌺 🎉

@xhdix xhdix merged commit 73363e2 into main Sep 2, 2022
Wiki Censorship automation moved this from In progress to Done Sep 2, 2022
@xhdix xhdix deleted the feat/parallel-metadata-query branch September 2, 2022 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority/high
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants