Fix unit tests on Windows #7270

adferrand · 2019-07-26T09:10:24Z

This PR makes the last corrections needed to run all unit tests on Windows:

add a function to check if a hook is executable in a cross-platform compatible way
handle correctly the PATH surgery for Windows during hook execution
handle correctly an account compatibility over both ACMEv1 and ACMEv2
remove (finally!) the @broken_on_windows decorator.

bmw

I still need to spin up a Windows VM to do a real test on is_executable here, but otherwise, this LGTM other than a few nits.

bmw · 2019-07-29T23:46:39Z

certbot/compat/misc.py

@@ -30,6 +30,10 @@
    MASK_FOR_PRIVATE_KEY_PERMISSIONS = 0


+# For Linux: define OS specific standard binaries directories
+STD_BINARIES_DIRS = ["/usr/sbin", "/usr/local/bin", "/usr/local/sbin"] if POSIX_MODE else []


nit: I'd change this to "BINARY" in both the name of the constant and comments around it. The singular version feels more idiomatic and have more results on Google.

I also personally would prefer to spell out "standard" rather than "std", but that's a personal preference you can ignore if you want.

bmw · 2019-07-29T23:54:07Z

certbot/plugins/util_test.py

@@ -30,12 +30,14 @@ def test_path_surgery(self, mock_debug):
                self.assertEqual(mock_debug.call_count, 0)
                self.assertEqual(os.environ["PATH"], all_path["PATH"])
        no_path = {"PATH": "/tmp/"}


nit: Can we move this variable into the body if the if statement too?

bmw · 2019-07-30T00:00:33Z

certbot/tests/hook_test.py

+        # certbot.compat.filesystem to give rights only to the current user. This implies removing
+        # all ACEs except the first one from the DACL created by original _generate_dacl function.
+
+        from certbot.compat.filesystem import _generate_dacl


Since this test relies on mocking the internals of filesystem.is_executable, I propose moving the majority of the code in this test to a new class in certbot/tests/compat/filesystem_test.py and updating the code here to just mock out filesystem.is_executable entirely with a comment about it being hard to test without a mock on Windows.

What do you think?

I agree. Done.

bmw · 2019-07-30T00:05:21Z

certbot/tests/util_test.py

@@ -52,26 +53,42 @@ def _call(cls, exe):
        from certbot.util import exe_exists
        return exe_exists(exe)

-    @mock.patch("certbot.util.os.path.isfile")


I have the same high level comments about testing here as I do above. I personally think these tests should be moved to filesystem_test.py and we can keep simple tests here that just mock out filesystem.is_executable if we want.

bmw

Other than two more tiny nits, when manually testing this, I was surprised by the behavior I saw. In a command prompt not running as administrator, I checked out this code, activated the venv, and ran:

import os
import tempfile

import win32security

from certbot.compat import filesystem
from certbot.util import safe_open

with tempfile.TemporaryDirectory() as dir_path:
    file_name = 'foo'
    file_path = os.path.join(dir_path, file_name)
    safe_open(file_path, chmod=0o666).close()
    print(win32security.LookupAccountSid(None, filesystem._get_current_user()))
    assert not filesystem.is_executable(file_path)

The output I got was:

('IEUser', 'MSEDGEWIN10', 1)
Traceback (most recent call last):
  File "test.py", line 15, in <module>
    assert not filesystem.is_executable(file_path)
AssertionError

Can you explain what's going on here?

certbot/tests/compat/filesystem_test.py

certbot/tests/util_test.py

Co-Authored-By: Brad Warren <bmw@users.noreply.github.com>

adferrand · 2019-07-31T07:40:06Z

Other than two more tiny nits, when manually testing this, I was surprised by the behavior I saw. In a command prompt not running as administrator, I checked out this code, activated the venv, and ran:
import os
import tempfile

import win32security

from certbot.compat import filesystem
from certbot.util import safe_open

with tempfile.TemporaryDirectory() as dir_path:
    file_name = 'foo'
    file_path = os.path.join(dir_path, file_name)
    safe_open(file_path, chmod=0o666).close()
    print(win32security.LookupAccountSid(None, filesystem._get_current_user()))
    assert not filesystem.is_executable(file_path)
The output I got was:
('IEUser', 'MSEDGEWIN10', 1)
Traceback (most recent call last):
  File "test.py", line 15, in <module>
    assert not filesystem.is_executable(file_path)
AssertionError
Can you explain what's going on here?

This is because you are member of the Administrators group, and this group has always all the permissions. So effectively, the file is executable for your user.

To give more details here, is_executable does not inspect the DACL itself, but call dacl.GetEffectiveRightsFromAcl instead. This is an API call that gives you the effective permissions for a user on the current DACL, regarding all ACEs applied in it.

I use this high-level approach since filesystem.is_executable is primarily designed to check hooks validity, and these hooks, created outside certbot, are most likely not following our security model.

bmw · 2019-07-31T17:56:29Z

That makes sense for the behavior from GetEffectiveRightsFromAcl, but to change my example slightly, it doesn't seem like Windows is using that same behavior.

Is there a way we can prevent the behavior shown here?

import os
import tempfile

import win32security


from certbot.compat import filesystem
from certbot.hooks import execute
from certbot.util import safe_open

with tempfile.TemporaryDirectory() as dir_path:
    file_name = 'foo.py'
    file_path = os.path.join(dir_path, file_name)
    with safe_open(file_path, chmod=0o666) as f:
        f.write('print("Hello world!")')
    print(win32security.LookupAccountSid(None, filesystem._get_current_user()))
    if filesystem.is_executable(file_path):
        print(execute('test-hook', file_path))

Output was:

('IEUser', 'MSEDGEWIN10', 1)
test-hook command "C:\Users\IEUser\AppData\Local\Temp\tmpehg661r9\foo.py" returned error code 1
Error output from test-hook command foo.py:
Access is denied.

('Access is denied.\n', '')

If you call safe_open with 0o777, it runs fine.

adferrand · 2019-07-31T18:57:08Z

Your are encountering this error because you run this code from a non privileged shell, and the effective rights here are accessible only when you are in a privileged shell. However, Windows documentation about GetEffectiveRightsFromAcl says that the privileges hold by the user at call time are not taken into account to get the result, so you do not see it.

However, the situation you described will not happen: certbot refuses to run on a non-privileged shell. So you will always have the maximum permissions you can get from the file, and is_executable result will be consistent with the effective execution of the hook script.

bmw · 2019-07-31T21:32:25Z

However, Windows documentation about GetEffectiveRightsFromAcl says that the privileges hold by the user at call time are not taken into account to get the result, so you do not see it.

Windows why? Are you familiar with an easy way to take the current privileges into account? I think that'd be ideal, but if not, I think we should add a simple comment in the code describing this behavior.

Also, I just noticed our avoidance of os.access. I assume that's yet another function we can't use on Windows? Grepping through the code, we don't seem to use it anywhere else, but I personally think it's worth preventing it by adding a simple function in compat.os that errors out saying it doesn't work on Windows.

adferrand · 2019-07-31T22:08:47Z

For your first remark, no, I think there is no easy way do to that, and found no clue how it could be properly handled on internet. In fact, I think it is an approach from Posix (having a program designed for an arbitrary user) that is not applicable on Windows. On Windows, you can mark an executable or a script as requiring privileged rights. It appears as so in the interface, and if you try to run it, it will trigger automatically the privileges escalation, and fail if your account cannot acquire them.

So I suppose that you design and write your program with the assumption it will be run under elevated privileges, and mark the executable as so. This is basically what we did for certbot internally, this is also what I will do for the executable created by the installers if possible.

For os.access, yes the official documentation https://docs.python.org/3/library/os.html#os.access talk only about Unix things, and warn about inaccuracy of the outcome for os that goes beyond the POSIX approach. So I expect it to be useless for Windows.

Yes, I think it can be forbidden. I can give a reference to is_executable in the exception about execution rights specifically.

adferrand · 2019-07-31T23:30:38Z

OK @bmw, I forbid os.access and added a comment on is_executable.

bmw

LGTM!

bmw · 2019-07-31T23:59:21Z

Other than the test failures I just noticed which are stopping me from merging the PR 😛

bmw · 2019-08-01T00:46:03Z

Looks like lint is still angry

adferrand · 2019-08-01T16:57:13Z

Pylint is happy again

adferrand added 10 commits July 26, 2019 11:01

Fix account_tests

de11ec7

Fix hook executable test

a664bea

Remove the temporary decorator @broken_on_windows

16310a0

Fix util_test

a6a8c93

No broken unit test on Windows anymore

1dc4f53

More elegant mock

55af673

Fix context manager

8398219

Fix lint

dbb3712

Fix mypy

69fb019

Adapt coverage

e2a9adf

adferrand mentioned this pull request Jul 26, 2019

Fix integration tests on Windows #7271

Merged

bmw self-assigned this Jul 29, 2019

bmw requested changes Jul 30, 2019

View reviewed changes

Adrien Ferrand and others added 3 commits July 30, 2019 21:07

Corrections

af70149

Fix lint

9885c30

Adapt coverage

65f464e

bmw requested changes Jul 31, 2019

View reviewed changes

certbot/tests/compat/filesystem_test.py Outdated Show resolved Hide resolved

certbot/tests/util_test.py Outdated Show resolved Hide resolved

adferrand and others added 2 commits July 31, 2019 06:52

Update certbot/tests/compat/filesystem_test.py

9cd359e

Co-Authored-By: Brad Warren <bmw@users.noreply.github.com>

Update util_test.py

25a080d

Fix pylint

2f6c9e4

adferrand added 5 commits August 1, 2019 00:39

Forbid os.access

11e5f72

Update os_test.py

5e6119d

Update os.py

7b9e811

Fix lint

132f081

Update filesystem.py

811203e

Update filesystem.py

96ffb3c

bmw previously approved these changes Jul 31, 2019

View reviewed changes

Update filesystem.py

9357f0d

adferrand dismissed bmw’s stale review via 9357f0d August 1, 2019 00:29

bmw previously approved these changes Aug 1, 2019

View reviewed changes

Update os.py

f56bcc7

adferrand dismissed bmw’s stale review via f56bcc7 August 1, 2019 08:25

bmw approved these changes Aug 1, 2019

View reviewed changes

bmw merged commit 56f609d into certbot:master Aug 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fix unit tests on Windows #7270

Fix unit tests on Windows #7270

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fix unit tests on Windows #7270

Fix unit tests on Windows #7270

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!