Update platforms.py "superuser privileges" check by namloc2001 · Pull Request #6600 · celery/celery

namloc2001 · 2021-01-17T16:09:45Z

Description

To fix issue #6579. Making it so that in platforms.py the check for "superuser privileges" is only looking for uid/euid=0, not including the root group (gid/egid=0) which does not have the superuser privileges associated with uid/euid=0.

namloc2001 · 2021-01-17T16:23:14Z

@auvipy I don't suppose you know why the tests would be failing, given this is such a minor change?

    c:\python37-x64\include\pyport.h(6) : fatal error C1083: Cannot open include file: 'inttypes.h': No such file or directory
    
        =============================DEBUG ASSISTANCE=============================
        If you are seeing a compilation error please try the following steps to
        successfully install cryptography:
        1) Upgrade to the latest pip and try again. This will fix errors for most
           users. See: https://pip.pypa.io/en/stable/installing/#upgrading-pip
        2) Read https://cryptography.io/en/latest/installation.html for specific
           instructions for your platform.
        3) Check our frequently asked questions for more information:
           https://cryptography.io/en/latest/faq.html
        =============================DEBUG ASSISTANCE=============================
    
    error: command 'c:\\Program Files (x86)\\Microsoft Visual Studio 10.0\\VC\\Bin\\amd64\\cl.exe' failed with exit status 2
    
    ----------------------------------------
Command "C:\projects\celery\.tox\3.7-unit\Scripts\python.EXE -u -c "import setuptools, tokenize;__file__='C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\pip-install-tcc2n6j9\\cryptography\\setup.py';f=getattr(tokenize, 'open', open)(__file__);code=f.read().replace('\r\n', '\n');f.close();exec(compile(code, __file__, 'exec'))" install --record C:\Users\appveyor\AppData\Local\Temp\1\pip-record-umzf7tol\install-record.txt --single-version-externally-managed --compile --install-headers C:\projects\celery\.tox\3.7-unit\include\site\python3.7\cryptography" failed with error code 1 in C:\Users\appveyor\AppData\Local\Temp\1\pip-install-tcc2n6j9\cryptography\
You are using pip version 10.0.1, however version 20.3.3 is available.
You should consider upgrading via the 'python -m pip install --upgrade pip' command.
=================================== log end ===================================
ERROR: could not install deps [-rC:\projects\celery/requirements/default.txt, -rC:\projects\celery/requirements/test.txt, -rC:\projects\celery/requirements/pkgutils.txt, -rC:\projects\celery/requirements/test-ci-default.txt, -rC:\projects\celery/requirements/docs.txt, -rC:\projects\celery/requirements/docs.txt]; v = InvocationError("'C:\\projects\\celery\\.tox\\3.7-unit\\Scripts\\python.EXE' -m pip install '-rC:\\projects\\celery/requirements/default.txt' '-rC:\\projects\\celery/requirements/test.txt' '-rC:\\projects\\celery/requirements/pkgutils.txt' '-rC:\\projects\\celery/requirements/test-ci-default.txt' '-rC:\\projects\\celery/requirements/docs.txt' '-rC:\\projects\\celery/requirements/docs.txt'", 1)
___________________________________ summary ___________________________________
ERROR:   3.7-unit: could not install deps [-rC:\projects\celery/requirements/default.txt, -rC:\projects\celery/requirements/test.txt, -rC:\projects\celery/requirements/pkgutils.txt, -rC:\projects\celery/requirements/test-ci-default.txt, -rC:\projects\celery/requirements/docs.txt, -rC:\projects\celery/requirements/docs.txt]; v = InvocationError("'C:\\projects\\celery\\.tox\\3.7-unit\\Scripts\\python.EXE' -m pip install '-rC:\\projects\\celery/requirements/default.txt' '-rC:\\projects\\celery/requirements/test.txt' '-rC:\\projects\\celery/requirements/pkgutils.txt' '-rC:\\projects\\celery/requirements/test-ci-default.txt' '-rC:\\projects\\celery/requirements/docs.txt' '-rC:\\projects\\celery/requirements/docs.txt'", 1)

lgtm-com · 2021-01-17T16:27:48Z

This pull request fixes 2 alerts when merging 06d3968 into 855d551 - view on LGTM.com

fixed alerts:

1 for Non-exception in 'except' clause
1 for Module is imported with 'import' and 'import from'

auvipy · 2021-01-17T16:39:12Z

fatal error C1083: Cannot open include file: 'inttypes.h': No such file or directory

fatal error C1083: Cannot open include file: 'inttypes.h': No such file or directory

our windows CI is not perfect & we have to adjust some settings in our github actions migration from travis. it now hangs while running the integration tests

thedrow

The rest of my comments are mostly nitpicks.
After we address those, this is good to go.

lgtm-com · 2021-01-19T11:52:29Z

This pull request introduces 1 alert when merging 56710f0 into 17eda8d - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

thedrow · 2021-01-19T14:58:00Z

Before merging this PR we also need to fully unit test the code.
Please ensure the tests are up to date and are passing.
We're still working on our new CI.

lgtm-com · 2021-01-19T20:10:25Z

This pull request fixes 1 alert when merging 0244382 into 2551162 - view on LGTM.com

fixed alerts:

1 for Non-exception in 'except' clause

thedrow · 2021-02-14T12:40:36Z

Looks OK, what about the tests?

namloc2001 · 2021-02-14T15:28:27Z

@thedrow what about the logger piece? I couldn't work out what you needed here?

lgtm-com · 2021-02-14T16:18:02Z

This pull request fixes 2 alerts when merging cb91ae2 into a5357ca - view on LGTM.com

fixed alerts:

1 for Non-exception in 'except' clause
1 for Module is imported with 'import' and 'import from'

thedrow · 2021-02-21T15:54:01Z

@namloc2001 I can't push to your branch since you didn't allow edits from a maintainer.
I'm working on this and I have begun writing tests. Can you please check the box so we can collaborate?

namloc2001 · 2021-02-21T16:05:13Z

@namloc2001 I can't push to your branch since you didn't allow edits from a maintainer.
I'm working on this and I have begun writing tests. Can you please check the box so we can collaborate?

Hi, allow edits by maintainers is ticked on this PR already. I've also invited you as a collaborator to the branch in case that is needed

thedrow · 2021-02-22T17:59:07Z

@auvipy Is codecov set up in CI?

thedrow · 2021-02-22T17:59:43Z

I'm marking this as a draft until we cover all code paths.

lgtm-com · 2021-02-22T18:43:47Z

This pull request introduces 1 alert and fixes 1 when merging 08d7eb2 into 3384937 - view on LGTM.com

new alerts:

1 for Unused import

fixed alerts:

1 for Module is imported with 'import' and 'import from'

codecov · 2021-02-23T11:51:03Z

Codecov Report

Merging #6600 (1a50cbc) into master (0154422) will increase coverage by 0.00%.
The diff coverage is 33.96%.

@@           Coverage Diff           @@
##           master    #6600   +/-   ##
=======================================
  Coverage   70.42%   70.42%           
=======================================
  Files         138      138           
  Lines       16437    16457   +20     
  Branches     2061     2060    -1     
=======================================
+ Hits        11575    11590   +15     
- Misses       4660     4666    +6     
+ Partials      202      201    -1

Flag	Coverage Δ
unittests	`70.42% <33.96%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
celery/bin/worker.py	`0.00% <0.00%> (ø)`
celery/exceptions.py	`32.25% <0.00%> (-0.71%)`	⬇️
celery/platforms.py	`67.60% <62.06%> (+1.65%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0154422...ea649a4. Read the comment docs.

auvipy · 2021-02-24T04:26:06Z

@auvipy Is codecov set up in CI?

it wasn't. I had internet issues for the last 2 days. so couldn't reply in time, sorry, but you set it up in the meantime. thanks

…used.

This fixes a regression introduced in #6600 which caused an import error on non-unix platforms. Fixes #6797.

* grp is no longer imported unconditionally. This fixes a regression introduced in #6600 which caused an import error on non-unix platforms. Fixes #6797. * Adjust tests to cover the new code paths.

* Update platforms.py Making it so that the check for "superuser privileges" is only looking for uid/euid=0, not including the root group (gid/egid=0) which does not have the superuser privileges associated with uid/euid=0. * Update platforms.py * Update platforms.py * Update platforms.py * Update platforms.py * Update celery/platforms.py Co-authored-by: Omer Katz <[email protected]> * Update celery/platforms.py Co-authored-by: Omer Katz <[email protected]> * Update celery/platforms.py Co-authored-by: Omer Katz <[email protected]> * Update celery/platforms.py Co-authored-by: Omer Katz <[email protected]> * Update platforms.py * Update celery/platforms.py Co-authored-by: Omer Katz <[email protected]> * Update platforms.py * Update platforms.py * Update celery/platforms.py Co-authored-by: Omer Katz <[email protected]> * Update platforms.py * Refactor contribution. * Basic tests. * Catch the SecurityError and exit the program. * More tests. * Ensure no warnings are present. * Cover the case where the platform doesn't have fchown available. * Test that a security error is raised when suspicious group names are used. * Remove unused import. Co-authored-by: Omer Katz <[email protected]>

* grp is no longer imported unconditionally. This fixes a regression introduced in celery#6600 which caused an import error on non-unix platforms. Fixes celery#6797. * Adjust tests to cover the new code paths.

namloc2001 mentioned this pull request Jan 17, 2021

Why does Celery Worker think uid=1000640000 euid=1000640000 is superuser privilege? #6579

Closed

18 tasks

namloc2001 changed the title ~~Update platforms.py~~ Update platforms.py "superuser privileges" check Jan 17, 2021

thedrow suggested changes Jan 17, 2021

View reviewed changes

Comment thread celery/platforms.py Outdated

thedrow added Category: Deployment PR Type: Bugfix labels Jan 17, 2021

thedrow added this to the 5.0.6 milestone Jan 17, 2021

thedrow linked an issue Jan 17, 2021 that may be closed by this pull request

Why does Celery Worker think uid=1000640000 euid=1000640000 is superuser privilege? #6579

Closed

18 tasks

namloc2001 requested a review from thedrow January 19, 2021 09:25

thedrow suggested changes Jan 19, 2021

View reviewed changes

namloc2001 requested a review from thedrow January 19, 2021 11:46

thedrow suggested changes Jan 19, 2021

View reviewed changes

Comment thread celery/platforms.py Outdated

Comment thread celery/platforms.py Outdated

thedrow added the Status: Needs Test Coverage ✘ label Jan 19, 2021

namloc2001 requested a review from thedrow January 21, 2021 17:35

thedrow self-assigned this Feb 21, 2021

thedrow marked this pull request as draft February 22, 2021 17:59

thedrow force-pushed the patch-1 branch from a531430 to 08d7eb2 Compare February 22, 2021 18:07

thedrow added 5 commits February 23, 2021 13:47

Refactor contribution.

6fca676

Basic tests.

ce73c96

Catch the SecurityError and exit the program.

2d45fb4

More tests.

ad9a070

Ensure no warnings are present.

c2b5e9e

thedrow force-pushed the patch-1 branch from df62b88 to c2b5e9e Compare February 23, 2021 11:48

auvipy modified the milestones: 5.0.6, 5.1.0 Feb 24, 2021

Cover the case where the platform doesn't have fchown available.

fe4b21f

thedrow mentioned this pull request Feb 24, 2021

or clauses in conditions should be considered branches coveragepy/coveragepy#1120

Closed

Test that a security error is raised when suspicious group names are …

10246c2

…used.

thedrow marked this pull request as ready for review February 24, 2021 10:00

Remove unused import.

ea649a4

thedrow approved these changes Feb 24, 2021

View reviewed changes

thedrow added Status: Has Test Coverage ✔ and removed Status: Needs Test Coverage ✘ labels Feb 24, 2021

thedrow requested a review from a team February 27, 2021 21:10

auvipy closed this Feb 28, 2021

auvipy reopened this Feb 28, 2021

thedrow merged commit 8a40560 into celery:master Mar 4, 2021

thedrow deleted the patch-1 branch March 4, 2021 15:20

Homopatrol mentioned this pull request May 27, 2021

Upgrade celery to 5.1.0 DefectDojo/django-DefectDojo#4566

Merged

thedrow added a commit that referenced this pull request Jun 6, 2021

grp is no longer imported unconditionally.

03c58fe

This fixes a regression introduced in #6600 which caused an import error on non-unix platforms. Fixes #6797.

thedrow mentioned this pull request Jun 6, 2021

grp is no longer imported unconditionally #6804

Merged

Uh oh!

Uh oh!

Conversation

namloc2001 commented Jan 17, 2021

Description

Uh oh!

namloc2001 commented Jan 17, 2021

Uh oh!

lgtm-com Bot commented Jan 17, 2021

Uh oh!

auvipy commented Jan 17, 2021

Uh oh!

Uh oh!

thedrow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgtm-com Bot commented Jan 19, 2021

Uh oh!

Uh oh!

Uh oh!

thedrow commented Jan 19, 2021

Uh oh!

lgtm-com Bot commented Jan 19, 2021

Uh oh!

thedrow commented Feb 14, 2021

Uh oh!

namloc2001 commented Feb 14, 2021

Uh oh!

lgtm-com Bot commented Feb 14, 2021

Uh oh!

thedrow commented Feb 21, 2021

Uh oh!

namloc2001 commented Feb 21, 2021

Uh oh!

thedrow commented Feb 22, 2021

Uh oh!

thedrow commented Feb 22, 2021

Uh oh!

lgtm-com Bot commented Feb 22, 2021

Uh oh!

codecov Bot commented Feb 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

auvipy commented Feb 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Feb 23, 2021 •

edited

Loading