mirror of
https://github.com/certbot/certbot.git
synced 2026-06-06 07:12:54 -04:00
8 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Adrien Ferrand
|
56f609d4f5 |
Fix unit tests on Windows (#7270)
Fixes #6850 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. * Fix account_tests * Fix hook executable test * Remove the temporary decorator @broken_on_windows * Fix util_test * No broken unit test on Windows anymore * More elegant mock * Fix context manager * Adapt coverage * Corrections * Adapt coverage * Forbid os.access |
||
|
Adrien Ferrand
|
e6bf3fe7f8
|
[Windows] Security model for files permissions - STEP 3f (#7233)
* Correct file permissions on TempHandler
* Forbid os.chown and os.geteuid, as theses functions can be harmful to the security model on Windows.
* Implement copy_ownership
* Apply copy_ownership
* Correct webroot tests (and activate another broken test !)
* Correct lint and mypy
* Ensure to apply mode in makedirs
* Apply strict permissions on directories created with tempfile.mkdtemp(), like on Unix.
* Ensure streamHandler has 0600 on Windows
* Reactivate a test on windows
* Pin oldest requirements to current internal libraries (acme and certbot)
* Add dynamically pywin32 in dependencies: always except for certbot-oldest to avoid to break the relevant tests.
* Administrative privileges are always required.
* Correct security implementation (not the logic yet)
* First correction. Allow to manipulate finely file permissions during their generation
* Align to master + fix lint + resolve correctly symbolic links
* Add a test for windows about default paths
* Strenghthen the detection of Linux/Windows to check the standard files layout.
* Fix lint and mypy
* Reflect non usage of cache discovery from dns google plugin to its tests, solving Windows tests on the way
* Apply suggestions from code review
Co-Authored-By: adferrand <adferrand@users.noreply.github.com>
* Add more details in a comment
* Retrigger build.
* Add documentation.
* Fix a test
* Correct RW clear down
* Update util.py
* Remove unused code
* Fix code style
* Adapt certbot coverage threshold on Linux due to Windows specific LOC addition.
* Various optimizations around file owner and file mode
* Fix last error
* Fix copy_ownership_and_apply_mode
* Fix lint
* Correct mypy
* Extract out first part from windows-file-permissions
* Ignore new_compat in coverage for now
* Create test package for compat
* Add unit tests for security module.
* Add pywin32
* Adapt linux coverages to the windows-specific LOCs added
* Clean imports
* Correct import
* Trigger CI
* Reactivate a test
* Create the certbot.compat package. Move logic in certbot.compat.misc
* Clean comment
* Add doc
* Fix lint
* Correct mypy
* Add executable permissions
* Add the delegate certbot.compat.os module, add check coding style to enforce usage of certbot.compat.os instead of standard os
* Load certbot.compat.os instead of os
* Move existing compat test
* Update local oldest requirements
* Import sys
* Fix some mocks
* Update account_test.py
* Update os.py
* Update os.py
* Update local oldest requirements
* Implement the new linter_plugin
* Fix remaining linting errors
* Fix local oldest for nginx
* Remove custom check in favor of pylint plugin
* Remove check coding style
* Update linter_plugin.py
Co-Authored-By: adferrand <adferrand@users.noreply.github.com>
* Add several comments
* Update the setup.py
* Add documentation
* Update acme dependencies
* Update certbot/compat/os.py
Co-Authored-By: adferrand <adferrand@users.noreply.github.com>
* Update certbot/compat/os.py
Co-Authored-By: adferrand <adferrand@users.noreply.github.com>
* Update certbot/compat/os.py
Co-Authored-By: adferrand <adferrand@users.noreply.github.com>
* Update docs/contributing.rst
Co-Authored-By: adferrand <adferrand@users.noreply.github.com>
* Update linter_plugin.py
Co-Authored-By: adferrand <adferrand@users.noreply.github.com>
* Update linter_plugin.py
Co-Authored-By: adferrand <adferrand@users.noreply.github.com>
* Update docs/contributing.rst
Co-Authored-By: adferrand <adferrand@users.noreply.github.com>
* Update docs/contributing.rst
Co-Authored-By: adferrand <adferrand@users.noreply.github.com>
* Corrections
* Handle os.path. Simplify checker.
* Add a comment to a reference implementation
* Update changelog
* Fix module registering
* Update docs/contributing.rst
Co-Authored-By: adferrand <adferrand@users.noreply.github.com>
* Update docs/contributing.rst
Co-Authored-By: adferrand <adferrand@users.noreply.github.com>
* Update docs/contributing.rst
Co-Authored-By: adferrand <adferrand@users.noreply.github.com>
* Update config and changelog
* Correction
* Correct os
* Fix merge
* Disable pylint checks
* Normalize imports
* Simplify security
* Corrections
* Reorganize module
* Clean code
* Clean code
* Remove coverage
* No cover
* Implement security.chmod
* Disable a test for now
* Disable hard error for now
* Add a first test. Remove unused import
* Recalibrate coverage
* Modifications for misc
* Correct function call
* Add some types
* Remove newline
* Use os_rename
* Implement security.open
* Revert to windows-files-permissions approach
* Fix lint
* Implement security.mkdir and security.makedirs
* Fix lint
* Clean lint
* Clean lint
* Revert "Clean lint"
This reverts commit
|
||
|
Adrien Ferrand
|
74292a10f5 |
[Windows] Security model for files permissions - STEP 3e (#7182)
This PR implements the filesystem.copy_ownership_and_apply_mode method from #6497. This method is used in two places in Certbot, replacing os.chown, to copy the owner and group owner from a file to another one, and apply to the latter the given POSIX mode. * Implement copy_ownership_and_apply_mode * Update certbot/compat/os.py Co-Authored-By: Brad Warren <bmw@users.noreply.github.com> * Remove default values * Rewrite a comment. * Relaunch CI * Pass as keyword arguments * Update certbot/compat/filesystem.py Co-Authored-By: Brad Warren <bmw@users.noreply.github.com> * Update certbot/compat/filesystem.py Co-Authored-By: Brad Warren <bmw@users.noreply.github.com> * Update certbot/compat/filesystem.py Co-Authored-By: Brad Warren <bmw@users.noreply.github.com> * Make the private key permissions transfer platform specific * Update certbot/compat/filesystem.py Co-Authored-By: Brad Warren <bmw@users.noreply.github.com> * Rename variable * Fix comment0 * Add unit test for copy_ownership_and_apply_mode * Adapt coverage * Execute unconditionally chmod with copy_ownership_and_apply_mode. Improve doc. |
||
|
Adrien Ferrand
|
7d61e9ea56 |
[Windows] Security model for files permissions - STEP 3d (#6968)
* Implement security.mkdir and security.makedirs * Fix lint * Correct mock * Rename security into filesystem * Update apache and nginx plugins requirements * Update certbot/plugins/webroot.py Co-Authored-By: ohemorange <ebportnoy@gmail.com> * Reenable pylint here * Move code * Reimplement mkdir * Control errors on eexist, remove superfluous chmod for makedirs * Add proper skip for windows only tests * Fix lint * Fix mypy * Clean code * Adapt coverage threshold on Linux with addition of LOC specific to Windows * Add forbiden functions to tests * Update certbot/compat/os.py Co-Authored-By: ohemorange <ebportnoy@gmail.com> * Simplify code * Sync _get_current_user with part3c * Use the simpliest implementation * Remove exist_ok, simplify code. * Simplify inline comment * Update filesystem_test.py * Update certbot/compat/os.py Co-Authored-By: ohemorange <ebportnoy@gmail.com> * Update certbot/plugins/webroot.py Co-Authored-By: ohemorange <ebportnoy@gmail.com> * Update certbot/plugins/webroot.py Co-Authored-By: ohemorange <ebportnoy@gmail.com> * Add a test to check we set back os.mkdir correctly after filesystem.makedirs is called. * Fix lint, adapt coverage |
||
|
Adrien Ferrand
|
88876b9901 |
[Windows] Security model for files permissions - STEP 3c (#6967)
* Implement security.open
* Clean lint
* Rename security into filesystem
* Update certbot/compat/filesystem.py
Co-Authored-By: ohemorange <ebportnoy@gmail.com>
* Update certbot/util.py
Co-Authored-By: ohemorange <ebportnoy@gmail.com>
* Update certbot/lock.py
Co-Authored-By: ohemorange <ebportnoy@gmail.com>
* Update certbot/compat/os.py
Co-Authored-By: ohemorange <ebportnoy@gmail.com>
* Update certbot/lock.py
Co-Authored-By: ohemorange <ebportnoy@gmail.com>
* Update certbot/compat/os.py
Co-Authored-By: ohemorange <ebportnoy@gmail.com>
* Simplify and make more clear comment on os.open.
* Secure implementation preventing race conditions
* Revert "Secure implementation preventing race conditions"
This reverts commit
|
||
|
Adrien Ferrand
|
9a60f6df78 |
Fix codecov quality gate since flags have been removed (#7173)
Because some users were complaining about staled workflow when flags (https://docs.codecov.io/docs/flags) are enabled, Codecov decided to remove them when calculating the coverage on branches until they improved this functionality. See: https://docs.codecov.io/docs/flags#section-flags-in-the-codecov-ui The flags are still taken into account on PR builds, but not on based branch. This is a problem for us, because we use the flags to compare specifically the coverage of a PR against its base branch for Windows on one side, and Linux on the other side. Without flags taken into account on the base branch, the CI fails because the coverage on Windows is too low. As a temporary fix until the situation is clarified on Codecov side, this PR replaces the validation condition, that was a comparison against the base branch, to a fixed coverage registered in the local .codecov.yml file in Certbot repository. This way, the coverage on PR builds, that takes into account the flags, is validated against an appropriate value. This is a temporary solution, that will require an explicit update of .codecov.yml in the mean time if the coverage significantly increases, or decreases on some developments. But until the situation is fixed, this will allow to have a functional quality gate. |
||
|
Adrien Ferrand
|
278cc8feef |
Disable default aggregated report. Reactivate auto-validation of reports against base branch. (#6939)
Following #6934, this PR finalize two things, as explained in #6934: disable the default aggregated report validate linux and windows reports against the PR base branch |
||
|
Adrien Ferrand
|
fb5974b8c3 |
Improve codecov report integration to CI in Certbot (#6934)
So, we observed lately several inconsistencies in how Codecov behave toward the CI pipeline for PRs in Certbot. One example is #6888. The most annoying thing is that the build of PR is **temporary** marked as failed, until all coverage are run. The correction on the latter is done in two PRs. This is the first part. TL;DR This PR separates the Codecov report in two: one for coverage executed on Windows, one for Linux. This is the correct way to do regarding our current CI pipeline. Actions are required by a GitHub administrator of Certbot once this PR is merged. Complete explanation So the failure stated in the introduction is essentially due to several things interacting together: * AppVeyor generates a coverage report for Windows, that have a coverage value a little lower than on Linux (96%) * Travis generates a coverage report for Linux. Its coverage is higher, and slowly decrease as more specific Windows code is added to Certbot, that cannot be tested on Travis * Since AppVeyor saw its capacity increasing, it finishes its coverage job before the one from Travis * Certbot GitHub repo is configured to require the coverage pipeline to succeed (in whatever that means) to success the overall PR build So here the suite of events: 1) PR is issued. GitHub expect three pipeline to succeed: AppVeyor CI, Travis CI and Codecov (displayed in the PR page) 2) Codecov receive first the report of AppVeyor coverage. It is 96%. It is a failure for now, because coverage in master (AppVeyor+Travis) is 98.6%. 3) GitHub is reported of the failure on Codecov, so fail the PR build 4) Codecov receive then the report of Travis coverage. It is 98%. It merges it with the report from AppVeyor, leading to the 98.6%. The failure becomes a success. 5) GitHub is reported of the success on Codecov, so, nevermind, the PR build is a success finally! So we have a CI flow that change its mind. Great. This is because of 2) and 4), and we could expect that Codecov should handle that. This is not the case: it is somewhat misleading, because Codecov adverts a lot about its capability to merge reports, including from different CI. But it is about the final state, not about the transient state, while reports are progressively received. Two things to things that a transient state is existing, with a result that can change: * first, from Codecov doc itself, explaining that reports should not be trusted during the CI pipeline execution: https://docs.codecov.io/docs/ci-service-relationship#section-checking-ci-status * second, is an example of transient state of `cryptography` project, this is advert by Codecov to be a reference of the implementation:  As you can see above, build state of `cryptography` is failing after the first report is received, and until all coverage reports from Travis are received. So, what can we do about it? Thing is, we are aggregating coverage from very two unrelated sources (two different OS systems), and Codecov has something for that. This is flags: https://docs.codecov.io/docs/flags Flags allow to flag coverage material depending on any logic you apply to the command that uploaded the coverage report (eg. `codecov -F a_flag`). Then, several logics can be applied on it, for instance having in Codecov UI the capability to filter the coverage other a flag, having status of build for each flag and ... having a report for a specific flag. So: 1) I modified Travis and AppVeyor to send their report under a specific flag: `linux` or `windows` 2) I created a project specific `.codecov.yml` configuration in Certbot repository, to instruct Codecov to push two separate reports on GitHub build: one for Linux, one for Windows. Each report can be validated against its specific coverage from the `master` branch (more on this just after) With all of this, now the GitHub is succeeding, because each coverage is validated independently. I think it is the good approach, because it solves the specific issue here, and because it reflects the logic behind: merging coverage from different OS architectures does not make much sense. It would be a long-term problem, because as I said at the beginning, coverages will slowly decrease as more platform specific code is added in Certbot. Now, it is not finished. Two things need to be done: an administrator action, and a second PR Administrator action Certbot GitHub as a a branch protection rule (Settings > Branches > Branch protection rules). It needs to be changed. Indeed this rule is expecting the full coverage report (named `codecov/project`) to be valid on a PR. It needs to be changed to expect two coverage reports: `codecov/project/linux` and `codecov/project/windows`. The `codecov/project` needs to be removed. This can be done once this PR is merged, and the specific coverage reports have been generated on master. Second PR Once this PR is merged and administrative actions have been done. I will make a new PR modifying `.codecov.yml` with two things: * disable the faulty full coverage report, that is not required anymore by GitHub branch protection rules * modify the `linux` and `windows` reports to validate against the relevant coverage calculated from `master` (indeed, in this PR it is a fixed ratio rule, since the coverage to compare on master is the full coverage one, significantly higher) * Tag reports * Set per-project codecov configuration |
