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

Delay UID change after loop start #369

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nim-odoo
Copy link

@nim-odoo nim-odoo commented Feb 9, 2023

What do these changes do?

Listening to a port < 1024 without --nosetuid leads to a permission error.

The UID change is done too early: we should first open the port, then change the UID.

Are there changes in behavior for the user?

Users can now drop privileges while listening to port < 1024.

Related issue number

Fixes #304

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • tox testenvs have been executed in the following environments:
    • Linux (Ubuntu 18.04, Ubuntu 20.04, Arch): {py36,py37,py38,py39}-{nocov,cov,diffcov}, qa, docs
    • Windows (7, 10): {py36,py37,py38,py39}-{nocov,cov,diffcov}
    • WSL 1.0 (Ubuntu 18.04): {py36,py37,py38,py39}-{nocov,cov,diffcov}, pypy3-{nocov,cov}, qa, docs
    • FreeBSD (12.2, 12.1, 11.4): {py36,pypy3}-{nocov,cov,diffcov}, qa
    • Cygwin: py36-{nocov,cov,diffcov}, qa, docs
  • Documentation reflects the changes
  • Add a news fragment into the NEWS.rst file
    • Add under the "aiosmtpd-next" section, creating one if necessary
      • You may create subsections to group the changes, if you like
    • Use full sentences with correct case and punctuation
    • Refer to relevant Issue if applicable

Copy link
Collaborator

@waynew waynew left a comment

Choose a reason for hiding this comment

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

I'm not particularly sure how to test for this aside from an integration test (at least using the definition I used on the Salt Project).

I went ahead and implemented my own little sample python asyncio server on my Test Clinic tonight, and verified that setuid to nobody before server creation like we were doing will throw the permissions error.

I'm not 100% positive about any security implications, I'm assuming that the correct thing happens here once we hit the setuid line, but... not positive.

It would be nice if we could have some automated tests for this, but... I am OK with it. FWIW #370 would have a conflict here if we end out merging that one first.

Or vice versa 🙃

@waynew
Copy link
Collaborator

waynew commented Mar 2, 2023

Note that there is a QA issue with comment line being a little too long.

Listening to a port < 1024 without `--nosetuid` leads to a permission
error.

The UID change is done too early: we should first open the port, then
change the UID.

Fixes aio-libs#304
@nim-odoo
Copy link
Author

Hello @pepoluan , is there a chance this can be looked at? Thanks

@pepoluan
Copy link
Collaborator

Hello @pepoluan , is there a chance this can be looked at? Thanks

I'll see what I can do this weekend.

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.

setuid called to early
3 participants