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

Check OCSP as part of determining if the certificate is due for renewal #7829

Merged
merged 7 commits into from Mar 3, 2020

Conversation

bmw
Copy link
Member

@bmw bmw commented Mar 3, 2020

Fixes #1028.

Doing this now because of https://community.letsencrypt.org/t/revoking-certain-certificates-on-march-4/.

The new ocsp_revoked_by_paths function is taken from #7649 with the optional argument removed for now because it is unused.

This function was added in this PR because storage.py uses self.latest_common_version() to determine which certificate should be looked at for determining renewal status at

if self.ocsp_revoked(self.latest_common_version()):
logger.debug("Should renew, certificate is revoked.")
return True
# Renews some period before expiry time
default_interval = constants.RENEWER_DEFAULTS["renew_before_expiry"]
interval = self.configuration.get("renew_before_expiry", default_interval)
expiry = crypto_util.notAfter(self.version(
"cert", self.latest_common_version()))

I think this is unnecessary and you can just look at the currently linked certificate, but I don't think we should be changing the logic that code has always had now.

Copy link
Contributor

@ohemorange ohemorange left a comment

Choose a reason for hiding this comment

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

The factoring out of these lines was just for testing purposes right?

 if self.use_openssl_binary:	        if self.use_openssl_binary:
            return self._check_ocsp_openssl_bin(cert_path, chain_path, host, url)	            return self._check_ocsp_openssl_bin(cert_path, chain_path, host, url)
        return _check_ocsp_cryptography(cert_path, chain_path, url)	        return _check_ocsp_cryptography(cert_path, chain_path, url)

We're just putting this check inline for renewal instead of doing prefetching? That's probably fine, I think, especially with the LE situation. We may want to double check about the server load.

certbot/certbot/ocsp.py Show resolved Hide resolved
@bmw
Copy link
Member Author

bmw commented Mar 3, 2020

The factoring out of these lines was just for testing purposes right?

if self.use_openssl_binary:	        if self.use_openssl_binary:
           return self._check_ocsp_openssl_bin(cert_path, chain_path, host, url)	            return self._check_ocsp_openssl_bin(cert_path, chain_path, host, url)
       return _check_ocsp_cryptography(cert_path, chain_path, url)	        return _check_ocsp_cryptography(cert_path, chain_path, url)

I'm not sure I fully understand your question, but to try and answer it anyway, nothing there was changed in this PR, but the reason this code is written that way in general is indeed for testing as well as the fact we'll use the OpenSSL branch in prod if cryptography doesn't have OCSP support. See

def __init__(self, enforce_openssl_binary_usage=False):
self.broken = False
self.use_openssl_binary = enforce_openssl_binary_usage or not ocsp

We're just putting this check inline for renewal instead of doing prefetching? That's probably fine, I think, especially with the LE situation. We may want to double check about the server load.

Since many browsers will check OCSP, I doubt Certbot doing it twice a day will be a big issue, but that's a good thing to verify.

@ohemorange
Copy link
Contributor

I'm not sure I fully understand your question, but to try and answer it anyway, nothing there was changed in this PR, but the reason this code is written that way in general is indeed for testing as well as the fact we'll use the OpenSSL branch in prod if cryptography doesn't have OCSP support. See

No, I meant that in joona's PR these lines are refactored into their own ocsp_times function. I assume that was just for testing purposes there and doesn't need to be done here as well?

@bmw
Copy link
Member Author

bmw commented Mar 3, 2020

Oh I see. I think the diff shown on Joona's PR is weird.

In both his PR and mine, the functions being called at the bottom of ocsp_revoked_by_paths is

        if self.use_openssl_binary:
            return self._check_ocsp_openssl_bin(cert_path, chain_path, host, url, response_file)
        return _check_ocsp_cryptography(cert_path, chain_path, url, response_file)

except in this PR we don't pass a response_file parameter because we don't have a use for it yet.

The ocsp_times function is new functionality with its own functions to call based on the value of use_openssl_binary which you're right we don't need here because it's being added in Joona's PR for functionality in the Apache plugin which doesn't exist here yet.

Copy link
Collaborator

@adferrand adferrand left a comment

Choose a reason for hiding this comment

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

And also ok for me!

@bmw bmw merged commit 3147026 into master Mar 3, 2020
@bmw bmw deleted the ocsp-renew branch March 3, 2020 19:07
@bmw bmw added this to the 1.3.0 milestone Mar 5, 2020
Tomoyuki-GH added a commit to Tomoyuki-GH/certbot that referenced this pull request Apr 3, 2020
* Use dummy values for ancestor (#7462)

* [Apache v2] AugeasBlockNode find_comments() implementation (#7457)

* find_comments implementation and AugeasCommentNode creation

* Use dummy value for ancestor

* Add NotImplementedError when calling find_comments with exact parameter

* Remove parameter 'exact' from find_comments interface

* Fix comment

* Remove references to TLS-SNI-01 outside of ACME (#7479)

This is a big part of #7214. It removes all references to TLS-SNI-01 outside of acme (and pytest.ini). Those changes will come in a subsequent PR. I thought this one was getting big enough.

* Remove references to TLS-SNI-01 in Apache plugin

* Remove references to TLS-SNI-01 from certbot-nginx

* Remove references to TLS-SNI from Certbot.

* Remove TLS-SNI reference from docs

* add certbot changelog

* Clarify test behavior

* Fix invalid escape sequence \. rebuild_dependencies.py (#7486)

Signed-off-by: Mickaël Schoentgen <contact@tiger-222.fr>

* Don't use dev version of 3.8. (#7485)

Now that Python 3.8 is out, we don't need to use the development version.

* Don't use acme.test_util outside of acme. (#7484)

`certbot-compatibility-test` is using code in `acme` that I proposed making private and not trivially importable in https://github.com/certbot/certbot/issues/5775.

To fix it, I switched to using Certbot's test utilities which I proposed keeping public to help with writing tests for plugins. When doing this I had to change the name of the key because `rsa1024_key.pem` doesn't exist in Certbot.

I also deleted the keys in `certbot-compatibility-test`'s testdata because because they are unused.

* Use distro library for all OS version detection (#7467)

This pull request ensures that we use distro package in all the distribution version detection. It also replaces the custom systemd /etc/os-release parsing and adds a few version fingerprints to Apache override selection.

Fixes: #7405

* Revert "Try to use platform.linux_distribution() before distro equivalent (#7403)"

This reverts commit ca3077d0347aae12163a43bf74a0c8321284367e.

* Use distro for all os detection code

* Address review comments

* Add changelog entry

* Added tests

* Fix tests to return a consistent os name

* Do not crash on non-linux systems

* Minor fixes to distro compatibility checks

* Make the tests OS independent

* Update certbot/util.py

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

* Skip linux specific tests on other platforms

* Test fixes

* Better test state handling

* Lower the coverage target for Windows tests

* Remove changelog entry about unpackaged scripts. (#7490)

We don't package rebuild_dependencies.py so I don't think we need to mention changes to it in our changelog which is primarily read by users and packagers.

* Deprecate more code related to TLS-SNI-01 (#7483)

I tried to finish up #7214 by removing the code in acme but we can't really do that until #7478 is resolved which we cannot do until we release 0.40.0.

Since we have to wait, this PR adds deprecation warnings for code that uses the TLS-SNI-01 code or was only used by the long deprecated TLS-SNI-01 code.

I'd like this PR to land before our next release.

* Deprecate more code related to TLS-SNI-01.

* Assert about warning message.

* Describe distributed Certbot components. (#7493)

* Clarify when the changelog should be modified (#7491)

* Dropped deprecated flags from commands (#7482)

This pull request addresses #7451 by removing the deprecated flags.

* Dropped deprecated flags from commands

* Updated changelog for dropped flags and deleted outdated tests

* removed init-script part of apache test

* Use fresh authorizations in dry runs (#7442)

* acme: re-populate uri in deactivate_authorization

* Use fresh authorizations in dry runs

--dry-run now deactivates 'valid' authorizations if it encounters them
when creating a new order.

Resolves #5116.

* remove unused code

* typo in local-oldest-requirements

* better error handling

* certbot-ci: AUTHREUSE to 100 + unskip dry-run test

* improve test coverage for error cases

* restore newline to local-oldest-requirements.txt

* Build Windows installers with pinned dependencies (#7498)

* Consume constraints file

* Independent pywin32 dependency definition in setup.py and construct.py

* Don't use --agree-dev-preview in tests. (#7501)

* Update changelog for 0.40.0 release

* Release 0.40.0

* Add contents to CHANGELOG.md for next version

* Bump version to 0.41.0

* Add back Python 3.4 support (#7510)

* Revert "Deprecation warnings for Python 3.4 (#7378)"

This reverts commit 6fcdfb0e5006be85500fad67a5a67b47befedb2a.

* Revert "Migrate certbot-auto users on CentOS 6 to Python 3.6 (#7268)"

This reverts commit e19b2e04c75b6df4e3f8a455700aa95fca79bcc3.

* add changelog entry

* keep mona in authors

* Add back Python 3.4 support (#7510) (#7511)

* Revert "Deprecation warnings for Python 3.4 (#7378)"

This reverts commit 6fcdfb0e5006be85500fad67a5a67b47befedb2a.

* Revert "Migrate certbot-auto users on CentOS 6 to Python 3.6 (#7268)"

This reverts commit e19b2e04c75b6df4e3f8a455700aa95fca79bcc3.

* add changelog entry

* keep mona in authors

(cherry picked from commit 9b848b1d65783000a13ef3f94ac5fe0e8c3879e7)

* Update changelog for 0.40.1 release

* Release 0.40.1

* Add contents to CHANGELOG.md for next version

* Bump version to 1.0.0

* Pin all build dependencies for the Windows installer (#7504)

This PR uses pipstrap to bootstrap the venv used to build Windows installers. This effectively pin all build dependencies, since pynsist is already installed through pip_install.py script.

* Use pipstrap

* Pin also NSIS version

* [Apache v2] Implement set_parameters() (#7461)

* find_comments implementation and AugeasCommentNode creation

* set_parameters implementation

* Change parameters to a property

* Remove parameters property setter

* More pythonic iteration handling

* Match our Travis logic in Azure. (#7514)

In Travis, the full test suite doesn't run on PRs for point release branches, just on commits for them. I think this behavior makes sense because what we actually want to test before a point release is the exact commit we want to release after any squashing/merging has been done. This PR modifies Azure to match this behavior.

After this PR lands, I need to update the tests required to pass on GitHub.

* Deprecate config_changes (#7469)

Closes #7454

* Deprecate config_changes

* Error on config_changes

* Fix tests for main.py

* Fix CHANGELOG entry

* Remove remnants of config_changes

* Fix CHANGELOG and add removed functions

* dns-rfc2136: use TCP to query SOA records (#7503)

* Use tcp query on dns-rfc2136 plugin

To improve network robust; fixes #7502.

* Update CHANGELOG.md

* Fix dns-rfc2136 test cases

* Add UDP fallback to dns-rfc2136

* Remove tls sni common (#7527)

* fixes #7478

* add changelog entry

* remove get_systemd_os_info (#7526)

Fixes #7500.

* Make uncomplicated modules private (#7528)

* Create _internal package for Certbot's non-public modules

* Move account.py to _internal

* Move auth_handler.py to _internal

* Move cert_manager.py to _internal

* Move client.py to _internal

* Move error_handler.py to _internal

* Move lock.py to _internal

* Move main.py to _internal

* Move notify.py to _internal

* Move ocsp.py to _internal

* Move renewal.py to _internal

* Move reporter.py to _internal

* Move storage.py to _internal

* Move updater.py to _internal

* update apache and nginx oldest requirements

* Keep the lock file as certbot.lock

* nginx oldest tests still need to rely on newer certbot

* python doesn't have good dependency resolution, so specify the transitive dependency

* update required minimum versions in nginx setup.py

* Move log.py to _internal (#7531)

Part of #5775. Methodology similar to #7528, but slightly more manual.

* Move items in certbot/display to _internal (#7532)

* Move display/completer.py to _internal/

* Move display/dummy_readline.py to _internal/

* Move display/enhancements.py to _internal/

* Create __init__.py in _internal/display

* Move eff.py to _internal (#7530)

* Move eff.py to _internal

* missed a few certbot.effs in tests

* remove sublime autocompletion

* fix messy scripting

* [Apache v2] Adding nodes 1/3 : add_child_block() (#7497)

* Implement add_child_block()

* Add comments and example

* Check augas path inconsistencies in initialization

* Remove TLS-SNI objects in ACME (#7535)

* fixes #7214

* update changelog

* remove unused import

* Move items in certbot/plugins to _internal (#7533)

* Create and initialize _internal/plugins

* Move plugins/manual.py to _internal/

* Move plugins/disco.py to _internal/

* Move plugins/selection.py to _internal/

* Move plugins/webroot.py to _internal/

* Move plugins/null.py to _internal/

* Move plugins/standalone.py to _internal/

* add missed internalization

* shorten line

* Update outdated init comment

* Move constants.py to _internal (#7534)

* Don't call core constants from nginx plugin

* Move constants.py to _internal/

* Move ENHANCEMENTS from now-internal constants to public plugins.enhancements

* Update display.enhancements.ask from its 2015 comment

* fix docstring

* Remove python2 and certbot-auto references in how to set up a Certbot build environment. (#7549)

Fixes #7548.

This PR udpdates installation instructions to get rid of python2 and certbot-auto in the how-to explaining the Certbot development environment setup.

Instead, Python 3 is used, and appropriate instructions for APT and RPM based distributions are provided.

* Implement add_child_directive (#7517)

* [Apache v2] Implement save() and unsaved_files() (#7520)

* Implement save() and unsaved_files()

* Linter fix

* [Apache v2] Implement delete_child() (#7521)

* Implement delete_child

* Fix linter

* [Windows] Fix certbot renew task failure under NT AUTHORITY\SYSTEM account (#7536)

Turned out that the scheduled task that runs `certbot renew` twice a day, is failing. Without any kind of log of course, otherwise it would not be fun.

It can be revealed by opening a powershell under the `NT AUTHORITY\SYSTEM` account, under which the scheduled task is run. Under theses circumstances, the bug is revealed: Certbot breaks when trying to invoke `certbot.compat.filesystem._get_current_user()`. Indeed the logic there implied to call `win32api.GetUserNameEx(win32api.NameSamCompatible)` and this function does not return always a useful value.

For normal account, it will be typically `DOMAIN_OR_MACHINE_NAME\YOUR_USER_NAME` (e.g. `My Machine\Adrien Ferrand`). But for the account `NT AUTHORITY\SYSTEM`, it will return `MACHINE_NAME\DOMAIN$`, which is a nonsense and makes fail the resolution of the actual SID of the account at the end of `_get_current_user()`.

This PR fixes this behavior by using an explicit construction of the account name that works both for normal users and `SYSTEM`.

* Use a different way to resolve current user account, that works both for normal users and SYSTEM.

* Add a comment to run Certbot under NT AUTHORITY\SYSTEM

* [Windows] Avoid letsencrypt.log permissions error during scheduled certbot renew task (#7537)

While coding for #7536, I ran into another issue. It appears that Certbot logs generated during the scheduled task execution have wrong permissions that make them almost unusable: they do not have an owner, and their ACL contains nonsense values (non existant accounts name).

The class `logging.handler.RotatingFileHandler` is responsible for these logs, and become mad when it is in a Python process run under a scheduled task owned by `SYSTEM`. This is precisely our case here.

This PR avoids (but not fix) the issue, by changing the owner of the scheduled task from `SYSTEM` to the `Administrators` group, that appears to work fine.

* Use Administrators group instead of SYSTEM to run the certbot renew task

* Move configuration.py to _internal (#7542)

Part of #5775. Methodology similar to #7528. Also refactors NGINX test util to use certbot.tests.util.ConfigTestCase.

* refactor nginx tests to no longer rely on certbot.configuration internals

* Move configuration.py to _internal

* Deprecate certbot register --update-registration (#7556)

Closes #7452.

* Fix shebang in rebuild_deps (#7557)

When you try to run this script, it crashes with:
```
standard_init_linux.go:211: exec user process caused "exec format error"
```
This is caused by the script being written to have the contents:
```
\
#!/bin/sh
set -e
...
```
This fixes the problem by removing the slash and moving the shebang to the first line of the string.

* Internalize modules called by internal plugins (#7543)

* Move hooks.py to _internal

* Move cli.py to _internal

* Update pinned dependencies (#7558)

Fixes #7184.

I updated #7358 to track the issue of unpinning all of these dependencies.

* pin back configargparse

* Pin back zope packages.

* update deps

* Add changelog entry.

* run build.py

* fixes #7553 (#7560)

* [Apache v2] Initial ApacheParser skeleton (#7559)

* Fix metadata & primary references in Augeas tests.

When performing actions only on one of the trees in DualNodeParser, the two
trees get out-of-sync. Similarly, we can't expect that the metadata between
the two trees will remain the same.

Did a pass over the tests to re-wire metadata and primary usage.

* Add ApacheParser skeleton.

Fix plumbing in configurator & dualparser to initialize ApacheParser
alongside AugeasParser.

* Silence coverage reports for now

* Implement add_child_comment (#7518)

* Remove unused apache docs (#7575)

Part of #5775. We don't use these docs anywhere, so delete them.

Removes:
- `certbot-apache/readthedocs.org.requirements.txt`
- `certbot-apache/docs/` folder
- docs include in `MANIFEST.in`
- docs dependencies in `setup.py`

* Remove unused certbot-compatibility-test docs (#7577)

Part of #5775. We don't use these docs anywhere, so delete them.

Removes:
- `certbot-compatibility-test/readthedocs.org.requirements.txt`
- `certbot-compatibility-test/docs/` folder
- docs include in `MANIFEST.in`
- docs dependencies in `setup.py`

* Remove DNS plugin API docs. (#7578)

Replace DNS plugins' API documentation with a note that plugins adhere to certbot's plugin interface.

* remove unused route53 tools (#7586)

* Remove unused nginx docs (#7576)

Part of #5775. We don't use these docs anywhere, so delete them.

Removes:
- `certbot-nginx/readthedocs.org.requirements.txt`
- `certbot-nginx/docs/` folder
- docs include in `MANIFEST.in`
- docs dependencies in `setup.py`

* Remove unused nginx docs

* Add changelog entry about the removal

* Make the contents of the apache plugin private (#7579)

Part of #5775.

Tree:
```
certbot-apache/certbot_apache
├── __init__.py
├── _internal
│   ├── apache_util.py
│   ├── augeas_lens
│   │   ├── httpd.aug
│   │   └── README
│   ├── centos-options-ssl-apache.conf
│   ├── configurator.py
│   ├── constants.py
│   ├── display_ops.py
│   ├── entrypoint.py
│   ├── http_01.py
│   ├── __init__.py
│   ├── obj.py
│   ├── options-ssl-apache.conf
│   ├── override_arch.py
│   ├── override_centos.py
│   ├── override_darwin.py
│   ├── override_debian.py
│   ├── override_fedora.py
│   ├── override_gentoo.py
│   ├── override_suse.py
│   └── parser.py
└── tests
    ├── ...
```

* Create _internal folder for certbot_apache

* Move apache_util.py to _internal

* Move display_ops.py to _internal

* Move override_centos.py to _internal

* Move override_gentoo.py to _internal

* Move override_darwin.py to _internal

* Move override_suse.py to _internal

* Move override_debian.py to _internal

* Move override_fedora.py to _internal

* Move override_arch.py to _internal

* Move parser.py to _internal

* Move obj.py to _internal

* Move http_01.py to _internal

* Move entrypoint.py to _internal

* Move constants.py to _internal

* Move configurator.py to _internal

* Move augeas_lens to _internal

* Move options-ssl-apache.conf files to _internal

* move augeas_lens in MANIFEST

* Clean up some stray references to certbot_apache that could use _internal

* Correct imports and lint

* Make the contents of the DNS plugins private (#7580)

Part of #5775.

```
modify_item () {
    mkdir certbot-dns-$1/certbot_dns_$1/_internal
    git grep -l "from certbot_dns_$1 import dns_$1" | xargs sed -i "s/from certbot_dns_$1 import dns_$1/from certbot_dns_$1._internal import dns_$1/g"
    git grep -l "certbot_dns_$1\.dns_$1" | xargs sed -i "s/certbot_dns_$1\.dns_$1/certbot_dns_$1._internal.dns_$1/g"
    git checkout -- certbot-dns-$1/certbot_dns_$1/__init__.py
    echo '"""Internal implementation of \`~certbot_dns_$1.dns_$1\` plugin."""' > certbot-dns-$1/certbot_dns_$1/_internal/__init__.py
    mv certbot-dns-$1/certbot_dns_$1/dns_$1.py certbot-dns-$1/certbot_dns_$1/_internal
    git checkout -- CHANGELOG.md
    git status
    git add -A
    git commit -m "Move certbot-dns-$1 to _internal structure"
}
```

Structure now looks like this:
```
certbot-dns-cloudflare/
├── certbot_dns_cloudflare
│   ├── dns_cloudflare_test.py
│   ├── __init__.py
│   └── _internal
│       ├── dns_cloudflare.py
│       └── __init__.py
```

* Move certbot-dns-cloudflare to _internal structure

* Move certbot-dns-cloudxns to _internal structure

* Move certbot-dns-digitalocean to _internal structure

* Move certbot-dns-dnsimple to _internal structure

* Move certbot-dns-dnsmadeeasy to _internal structure

* Move certbot-dns-gehirn to _internal structure

* Move certbot-dns-google to _internal structure

* Move certbot-dns-linode to _internal structure

* Move certbot-dns-luadns to _internal structure

* Move certbot-dns-nsone to _internal structure

* Move certbot-dns-ovh to _internal structure

* Move certbot-dns-rfc2136 to _internal structure

* Move certbot-dns-sakuracloud to _internal structure

* Init file comments need to be comments

* Move certbot-dns-route53 to _internal structure

* Fix comment in route53 init

* Refactor certbot/ and certbot/tests/ to use the same structure as the other packages (#7544)

Summary of changes in this PR:
- Refactor files involved in the `certbot` module to be of a similar structure to every other package; that is, inside a directory inside the main repo root (see below).
- Make repo root README symlink to `certbot` README.
- Pull tests outside of the distributed module.
- Make `certbot/tests` not be a module so that `certbot` isn't added to Python's path for module discovery.
- Remove `--pyargs` from test calls, and make sure to call tests from repo root since without `--pyargs`, `pytest` takes directory names rather than package names as arguments.
- Replace mentions of `.` with `certbot` when referring to packages to install, usually editably.
- Clean up some unused code around executing tests in a different directory.
- Create public shim around main and make that the entry point.

New directory structure summary:
```
repo root ("certbot", probably, but for clarity all files I mention are relative to here)
├── certbot
│   ├── setup.py
│   ├── certbot
│   │   ├── __init__.py
│   │   ├── achallenges.py
│   │   ├── _internal
│   │   │   ├── __init__.py
│   │   │   ├── account.py
│   │   │   ├── ...
│   │   ├── ...
│   ├── tests
│   │   ├── account_test.py
│   │   ├── display
│   │   │   ├── __init__.py
│   │   │   ├── ...
│   │   ├── ... # note no __init__.py at this level
│   ├── ...
├── acme
│   ├── ...
├── certbot-apache
│   ├── ...
├── ...
```

* refactor certbot/ and certbot/tests/ to use the same structure as the other packages

* git grep -lE "\-e(\s+)\." | xargs sed -i -E "s/\-e(\s+)\./-e certbot/g"

* git grep -lE "\.\[dev\]" | xargs sed -i -E "s/\.\[dev\]/certbot[dev]/g"

* git grep -lE "\.\[dev3\]" | xargs sed -i -E "s/\.\[dev3\]/certbot[dev3]/g"

* Remove replacement of certbot into . in install_and_test.py

* copy license back out to main folder

* remove linter_plugin.py and CONTRIBUTING.md from certbot/MANIFEST.in because these files are not under certbot/

* Move README back into main folder, and make the version inside certbot/ a symlink

* symlink certbot READMEs the other way around

* move testdata into the public api certbot zone

* update source_paths in tox.ini to certbot/certbot to find the right subfolder for tests

* certbot version has been bumped down a directory level

* make certbot tests directory not a package and import sibling as module

* Remove unused script cruft

* change . to certbot in test_sdists

* remove outdated comment referencing a command that doesn't work

* Install instructions should reference an existing file

* update file paths in Dockerfile

* some package named in tox.ini were manually specified, change those to certbot

* new directory format doesn't work easily with pyargs according to http://doc.pytest.org/en/latest/goodpractices.html#tests-as-part-of-application-code

* remove other instance of pyargs

* fix up some references in _release.sh by searching for ' . ' and manual check

* another stray . in tox.ini

* fix paths in tools/_release.sh

* Remove final --pyargs call, and now-unnecessary call to modules instead of local files, since that's fixed by certbot's code being one layer deeper

* Create public shim around main and make that the entry point

* without pyargs, tests cannot be run from an empty directory

* Remove cruft for running certbot directly from main

* Have main shim take real arg

* add docs/api file for main, and fix up main comment

* Update certbot/docs/install.rst

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

* Fix comments in readthedocs requirements files to refer to current package

* Update .[docs] reference in contributing.rst

* Move plugins tests to certbot tests directory

* add certbot tests to MANIFEST.in so packagers can run python setup.py test

* move examples directory inside certbot/

* Move CHANGELOG into certbot, and create a top-level symlink

* Remove unused sys and logging from main shim

* nginx http01 test no longer relies on certbot plugins common test

* Make the contents of the nginx plugin private (#7589)

Part of #5775.

* Create _internal folder certbot-nginx

* Move configurator.py to _internal

* Move constants.py to _internal

* Move display_ops.py to _internal

* Move http_01.py to _internal

* Move nginxparser.py to _internal

* Move obj.py to _internal

* Move parser_obj.py to _internal

* Move parser.py to _internal

* Update location and references for tls_configs

* exclude parser_obj from coverage

* Update pull_request_template.md (#7596)

* Update pull_request_template.md

* Remove line breaks

Github seems to be keeping the line breaks rather than ignoring them, making it be formatted weirdly, so remove them.

* Fix refactor (#7597)

Clean up some places missed by #7544.

Found this when running test farm tests. They were working as of 5d90544, and I will truly shocked if subsequent changes (all to the windows installer) made them stop working.

* Release script needs to target new CHANGELOG location

* Clean up various other CHANGELOG path references

* Update windows paths for new certbot location

* Add certbot to packages list for windows installer

* Implement redirect by default (#7595)

* Change redirect default to yes so that it happens automatically in noninteractive mode

* Update changelog

* Refactor tests out of packaged module for dns plugins (#7599)

* Refactor tests out of module for certbot-dns-cloudflare

* Refactor tests out of module for certbot-dns-cloudxns

* Refactor tests out of module for certbot-dns-digitalocean

* Refactor tests out of module for certbot-dns-dnsimple

* Refactor tests out of module for certbot-dns-dnsmadeeasy

* Refactor tests out of module for certbot-dns-gehirn

* Refactor tests out of module for certbot-dns-google

* Refactor tests out of module for certbot-dns-linode

* Refactor tests out of module for certbot-dns-luadns

* Refactor tests out of module for certbot-dns-nsone

* Refactor tests out of module for certbot-dns-ovh

* Refactor tests out of module for certbot-dns-rfc2136

* Refactor tests out of module for certbot-dns-sakuracloud

* Refactor tests out of module for certbot-dns-route53

* Move certbot-dns-google testdata/ under tests/

* Use pytest for dns plugins

* Exclude pycache and .py[cod]

* Refactor tests out of packaged module for acme plugin (#7600)

* Move acme tests to tests/ directory outside of acme module

* Fix call to messages_test in client_test

* Move test_util.py and testdata/ into tests/

* Update manifest to package tests

* Exclude pycache and .py[cod]

* Exclude pycache and .py[cod] from certbot package (#7608)

* Refactor tests out of packaged module for nginx plugin (#7606)

* Refactor tests out of packaged module for nginx plugin

* Exclude pycache and .py[cod]

* Refactor tests out of packaged module for apache plugin (#7607)

Part of #7593.

* Refactor tests out of packaged module for apache plugin

* Exclude pycache and .py[cod]

* Change tests path in tox.ini

* Defines the RenewableCert API (#7603)

This is my proposed fix for #7540. I would ideally like this to be included in our 1.0 release.

I came up with this design by adding all attributes used either in our own plugins, 3rd party plugins listed at https://certbot.eff.org/docs/using.html#third-party-plugins, or our public API code.

Despite me thinking that zope is unneeded nowadays, I initially tried to use it to define this interface since we have it and it gives us a way to define expected attributes, but it doesn't work because zope interface objects also have a method called `names` which conflict with the API.

I talked about this with Adrien out of band and did some of my own research and there are some minor benefits with this new approach of using properties:

1. It's more conventional.
2. If you also change the implementation to inherit from the class, Python will error if all properties aren't defined.
3. The PEP 526 style type annotations with mypy seem to (currently) only be used to validate code using the class, not the class implementation itself. You can add a type annotation saying the class needs to have this attribute, never define it, and mypy won't complain.

With this new approach, I had to fix `names` because pylint was complaining that the arguments differed, however, we never used the optional parameter to `names` outside of tests so I just deleted the code altogether.

* fixes #7540

* move to properties

* [Apache v2] Implement find_ancestors (#7561)

* Implement find_ancestors

* Create the node properly and add assertions

* Update certbot-apache/certbot_apache/augeasparser.py

Co-Authored-By: ohemorange <ebportnoy@gmail.com>

* Remove comment

* Upgrade to pywin32>=227 (#7615)

Current version of pywin32 used in certbot (225) does not have wheels available for Python 3.8. Installing certbot for development in this case requires to build from source. On Windows, this implies a Visual Studio C++ environment up and ready, which is absolutely not fun.

Let's upgrade to pywin32 227, that provides these wheels for all Python versions from 3.5 up to current dev status of 3.9.

* [Apache v2] Move the apachectl parsing to apache_util (#7569)

* Move the Apache CLI parsing to apache_util

* Fix test mocks

* Address review comments

* Fix the parsernode metadata dictionary

* acme/setup.py: comment refers to "PyOpenSSL" not "mock" (#7619)

* Update changelog for 1.0.0 release

* Release 1.0.0

* Add contents to certbot/CHANGELOG.md for next version

* Bump version to 1.1.0

* document main (#7610)

I deleted the exceptions because I think it's not feasible to document the possible exceptions raised by all of Certbot.

* update external plugin (#7604)

The old plugin at https://github.com/marcan/certbot-external says it's obsolete and points people to https://github.com/EnigmaBridge/certbot-external-auth. The new plugin is also an installer.

I also removed the reference to #2782 about us adding similar functionality since that's been done for a long time. We could reference our manual plugin instead, but I think that devalues their plugin a bit which I don't think is necessary or correct as it has different features.

* Add full API documentation (#7614)

A lot of Certbot's files don't have API documentation which is fixed by this PR. To do this, from the top level certbot directory I ran:
```
sphinx-apidoc -Me -o docs/api certbot
```
I then merged the resulting `modules.rst` file with `docs/api.rst`.

* fix bad links in docs (#7623)

This PR fixes the failures at https://travis-ci.com/certbot/website/builds/139193502#L1316.

Once this PR lands, I'll update certbot/website#508 to include this commit.

* Don't list DNS plugins as alpha quality. (#7624)

They should be considered production quality like our other packaged code.

* Don't list adding type annotations as a PR req. (#7627)

* Reorganize imports (#7616)

* Isort execution

* Fix pylint, adapt coverage

* New isort

* Fix magic_typing lint

* Second round

* Fix pylint

* Third round. Store isort configuration

* Fix latest mistakes

* Other fixes

* Add newline

* Fix lint errors

* [Apache v2] Implement parsed_files (#7562)

* Implement parsed_files

* Add parsed_files stub to ApacheParserNodes and fix assertions

* Update certbot-apache/certbot_apache/interfaces.py

Co-Authored-By: ohemorange <ebportnoy@gmail.com>

* Add more descriptive comments

* Update certbot-apache/certbot_apache/augeasparser.py

Co-Authored-By: ohemorange <ebportnoy@gmail.com>

* Update certbot-apache/certbot_apache/dualparser.py

Co-Authored-By: ohemorange <ebportnoy@gmail.com>

* Update certbot-apache/certbot_apache/interfaces.py

Co-Authored-By: ohemorange <ebportnoy@gmail.com>

* Lint certbot code on Python 3, and update Pylint to the latest version (#7551)

Part of #7550

This PR makes appropriate corrections to run pylint on Python 3.

Why not keeping the dependencies unchanged and just run pylint on Python 3?
Because the old version of pylint breaks horribly on Python 3 because of unsupported version of astroid.

Why updating pylint + astroid to the latest version ?
Because this version only fixes some internal errors occuring during the lint of Certbot code, and is also ready to run gracefully on Python 3.8.

Why upgrading mypy ?
Because the old version does not support the new version of astroid required to run pylint correctly.

Why not upgrading mypy to its latest version ?
Because this latest version includes a new typshed version, that adds a lot of new type definitions, and brings dozens of new errors on the Certbot codebase. I would like to fix that in a future PR.

That said so, the work has been to find the correct set of new dependency versions, then configure pylint for sane configuration errors in our situation, disable irrelevant lintings errors, then fixing (or ignoring for good reason) the remaining mypy errors.

I also made PyLint and MyPy checks run correctly on Windows.

* Start configuration

* Reconfigure travis

* Suspend a check specific to python 3. Start fixing code.

* Repair call_args

* Fix return + elif lints

* Reconfigure development to run mainly on python3

* Remove incompatible Python 3.4 jobs

* Suspend pylint in some assertions

* Remove pylint in dev

* Take first mypy that supports typed-ast>=1.4.0 to limit the migration path

* Various return + else lint errors

* Find a set of deps that is working with current mypy version

* Update local oldest requirements

* Remove all current pylint errors

* Rebuild letsencrypt-auto

* Update mypy to fix pylint with new astroid version, and fix mypy issues

* Explain type: ignore

* Reconfigure tox, fix none path

* Simplify pinning

* Remove useless directive

* Remove debugging code

* Remove continue

* Update requirements

* Disable unsubscriptable-object check

* Disable one check, enabling two more

* Plug certbot dev version for oldest requirements

* Remove useless disable directives

* Remove useless no-member disable

* Remove no-else-* checks. Use elif in symetric branches.

* Add back assertion

* Add new line

* Remove unused pylint disable

* Remove other pylint disable

* Execute Windows installer integration tests on several Windows versions (#7641)

This PRs extends the installer tests on Azure Pipeline, in order to run the integration tests on a certbot instance installed with the Windows installer for several Windows versions, corresponding to the scope of supported versions on Certbot:
* Windows Server 2012 R2
* Windows Server 2016
* Windows Server 2019

One can see the result on: https://dev.azure.com/adferrand/certbot/_build/results?buildId=311

* Try specific installer-build step

* Install Python manually

* Add tests on windows 2019

* discourage dns plugins (#7639)

* Remove warning about dev preview (#7640)

* Add docker-compose as a requirement of certbot-ci (#7120)

Fixes #7110 

This PR declares docker-compose as a requirement for certbot-ci. This way, a recent version of docker-compose is installed in the standard virtual environment set up by `tools/venv.py` and `tools/venv3.py`, and so is available to pytest integration tests from `tox` or in the virtual environment enabled.

* Add docker-compose as a dev dependency and declares it in certbot-ci requirements

* Update docker-compose 1.25.0

* Remove other 3.8-dev references. (#7646)

* Implement get_virtual_hosts() for ParserNode interfaces (#7564)

* How to uninstall certbot-auto (#7648)

* Include header files for compilation. (#7650)

* Remove POST-as-GET fallback to GET (#6994)

* Update CHANGELOG.md (#7659)

* Fix gating to ensure that no parsernode functionality is run unless explicitly requested (#7654)

* Modifications needed for merging to master

* [Apache v2] Add apacheconfig as a dependency (#7643)

* Add apacheconfig as a dependency.

* Change apacheconfig to a dev dependency

* Bump apacheconfig dep to 0.3.1

* Implement a sunset mechanism in certbot-auto for systems not supported anymore (#7587)

* Sunset mechanism

* Simplify code

* Update letsencrypt-auto-source/letsencrypt-auto.template

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

* Update template

* Deprecate for all RHEL/CentOS 6 32bits flavors

* Add a wrapper to uname to do tests on fake 32 bits versions

* Replace all occurences

* Add some tests about sunset mechanism

* Update letsencrypt-auto-source/tests/centos6_tests.sh

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

* Update letsencrypt-auto-source/tests/centos6_tests.sh

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

* Various corrections

* Recreate script

* Update comment position

* Test also install only

* Fix docker

* Update letsencrypt-auto-source/tests/centos6_tests.sh

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

* What error command is doing here ?

* Fix permissions

* Rebuild script

* Add changelog

* Update letsencrypt-auto-source/letsencrypt-auto.template

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

* Update changelog

* Trigger CI

* Handle old venv path

* Modify test

* Fix test error detection from subpaths

* Edit echo

* Use set -e

* Update letsencrypt-auto-source/letsencrypt-auto.template

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

* Corrections

Co-authored-by: Brad Warren <bmw@users.noreply.github.com>

* improve help about supply selecting in delete command (#7673)

for #6625

* Do not document private members (#7675)

It looks like we're currently documenting functions that are marked private (prefixed with an underscore) such as https://certbot.eff.org/docs/api/certbot.crypto_util.html#certbot.crypto_util._load_cert_or_req. I do not think we should do this because the functionality is private, should not be used, and including it in our docs just adds visual noise.

This PR stops us from documenting private code and fixes up `tools/sphinx-quickstart.sh` so we don't document it in future modules.

* Do not document private code.

* Don't document private members in the future.

* Fix certbot-auto regarding python 3.4 -> python 3.6 migration for CentOS 6 users (#7519)

* Revert "Add back Python 3.4 support (#7510)"

This reverts commit 9b848b1d65783000a13ef3f94ac5fe0e8c3879e7.

* Fix certbot-auto

* Use a more consistent way to enable rh-python36

* Avoid to call CompareVersions unecessarily

* Control rh-python36 exit code

* Fix travis config

* Remove vscode config

* Ignore vscode

* Fix merge conflicts regarding #7587 (#70)

* Add changelog entry

* Finish sentence

* Update certbot/CHANGELOG.md

Co-Authored-By: Joona Hoikkala <joohoi@users.noreply.github.com>

* Update letsencrypt-auto-source/tests/centos6_tests.sh

Co-Authored-By: Joona Hoikkala <joohoi@users.noreply.github.com>

* Update letsencrypt-auto-source/tests/centos6_tests.sh

Co-Authored-By: Joona Hoikkala <joohoi@users.noreply.github.com>

* Update letsencrypt-auto-source/tests/centos6_tests.sh

Co-Authored-By: Joona Hoikkala <joohoi@users.noreply.github.com>

* Update letsencrypt-auto-source/tests/centos6_tests.sh

Co-Authored-By: Joona Hoikkala <joohoi@users.noreply.github.com>

* Update letsencrypt-auto-source/tests/centos6_tests.sh

Co-Authored-By: Joona Hoikkala <joohoi@users.noreply.github.com>

* Update comments

* Improve warning message

* Update changelog

Co-authored-by: Joona Hoikkala <joohoi@users.noreply.github.com>

* Update changelog for 1.1.0 release

* Release 1.1.0

* Add contents to certbot/CHANGELOG.md for next version

* Bump version to 1.2.0

* Add missing directory field (#7687)

Fixes #7683.

* Add missing directory field to error message

* Added change to CHANGELOG.md

* Do not list the name twice. (#7689)

* Cleanup disabled warnings list in pytest.ini. (#7690)

* Fix minimum certbot version in plugins (#7684)

Fixes the problem found at https://github.com/certbot/certbot/pull/7682#discussion_r367140415.

* Don't run some tests multiple times. (#7685)

* Include added/deleted TXT record name in RFC 2136 debug log (#7696)

* Spelling and grammar fixes (#7695)

* Downgrade NSIS and upgrade Python (#7702)

* Add --allow-downgrade to chocolatey command.

* Upgrade tests to use Python 3.8.1.

* Drop Travis tests for Python 3.4 (#7394)

* Minor release script improvements (#7697)

* Do not use git diff.

* Add a warning on exit.

* Don't run Python 3.5 tests twice. (#7704)

* unpin macos (#7705)

* fixes #1948 -- MD5 on FIPS systems (#7708)

* use MD5 in non-security mode to get around FIPS issue

* update CHANGELOG

* add myself to AUTHORS

* ignore hashlib params

* Update documentation files to remove claiming support for Python 3.4 (#7395)

* Fix collections.abc imports for Python 3.9 (#7707)

* Fix collections.abc imports for Python 3.9

* Update AUTHORS.md

* No longer ignore collections.abc deprecation warning

* Update changelog

* Remove outdated comment

* Disabling no-name-in-module not needed as linting is on Python 3

* Remove ECDHE-RSA-AES128-SHA from NGINX ciphers list (#7719)

As mentioned in https://github.com/certbot/certbot/pull/7712#discussion_r370419867, it's time to remove this ciphersuite now that Windows 2008 R2 and Windows 7 are EOLed.

* Remove ECDHE-RSA-AES128-SHA from NGINX ciphers list to celebrate Windows 2008 R2 deprecation

* Update changelog

* Drop Python 3.4 support (#7721)

Fixes #7393.

* Remove Python 3.4 classifiers

* Remove unneeded typing dependency

* Exclude Python 3.4 in python_requires

* Remove Python 3.4 deprecation warning

* update changelog

* Disable old SSL versions and ciphersuites to follow Mozilla recommendations in Apache (#7712)

Part of #7204.

Makes the smaller changes described at https://github.com/certbot/certbot/issues/7204#issuecomment-571838185 to disable many old ciphersuites and TLS versions < 1.2. Does not add checks for OpenSSL version or modify session tickets.

Since Apache uses TLS protocol blacklisting instead of whitelisting (as in NGINX), we additionally may not need to determine if the server supports TLS1.3 and turn it on or off based on Apache version.

* Update SSL versions and ciphersuites based on Mozilla intermediate recommendations for apache

* Update constants with hashes of new config files

* Update changelog

* Unpin Python 3.4 dependencies (#7709)

* Unpin dependencies pinned back for py3.4 support.

* update pinned packages

* run build.py

* Update boto3 and deps to work with requests

* Update dns-lexicon version. (#7723)

* dns-cloudflare: Implement limited-scope API Tokens (#7583)

A while ago Cloudflare added support for limited-scope API Tokens in place of using a global API key, but support for them in cloudflare/python-cloudflare took a while to get through.

In summary, this PR:
- Implements token functionality through the INI file parameter `dns_cloudflare_api_token` (in addition to the traditional `dns_cloudflare_email` and `dns_cloudflare_api_key`). This needed a more advanced parameter validator than the built in `required_variables` mechanism.
- Updates the docs to reflect the new option, needed token permissions, and version details of the `cloudflare` module

* Update python-cloudflare version

* Add Cloudflare API Token support to certbot-dns-cloudflare

* Add token-specific errors to certbot-dns-cloudflare

* Tidy up certbot-dns-cloudflare

* Implement Cloudflare API Tokens in testing for certbot-dns-cloudflare(needs work)

* Further tidying of certbot-dns-cloudflare

* Update CHANGELOG with Cloudflare API Tokens implementation

* Improve testing of certbot-dns-cloudflare

* Improve certbot-dns-cloudflare test formatting

* Further improve testing for certbot-dns-cloudflare

* Change needed permissions for token

* Add documentation regarding python-cloudflare version

* Fix changelog, references to python-cloudflare and docs

* Fix behaviour when domain does not match cloudflare root domain. Improve error handling.

* Improve testing

* Improve hints and error handling

* Add backwards compatibility docs (#7611)

Fixes #7463.

* Add backwards compatibility docs.

* Exclude certbot-auto

* Remove SSLCompression off line from all config options (#7726)

Based on discussion at https://github.com/certbot/certbot/pull/7712#discussion_r371451761.

* Remove SSLCompression off line from all config options

* Update changelog

* Add space between words.

* Update instructions about how to build docs (#7605)

* Turn off Travis notifications in test branches. (#7733)

When I want to manually run the full test suite to test something, I've been manually deleting our notification setup from `.travis.yml` to avoid spamming IRC with my personal test failures.

This PR sets this behavior up to happen automatically by turning off IRC notifications in test branches. You can see this working by noticing the IRC notification section in the bottom of the config for this PR at https://travis-ci.com/certbot/certbot/builds/146827907/config and the fact that it is absent from a `test-` branch based on this one at https://travis-ci.com/certbot/certbot/jobs/282059094/config.

* Parse `$hostname` in `server_name`

* Add CHANGELOG entry

* Forgot to remove a `breakpoint()` statement

* Use unrestrictive umask for challenge directory

* Update changelog

* Update changelog for 1.2.0 release

* Release 1.2.0

* Add contents to certbot/CHANGELOG.md for next version

* Bump version to 1.3.0

* Wrap makedirs() within exception handelrs

* Missing import

* Windows installer integration tests (#7724)

As discussed in #7539, we need proper tests of the Windows installer itself in order to variety that all the logic contained in a production-grade runtime of Certbot on Windows is correctly setup by each version of the installer, and so for a variety of Windows OSes. 

This PR handles this requirement. The new `windows_installer_integration_tests` module in `certbot-ci` will:
* run the given Windows installer
* check that Certbot is properly installed and working
* check that the scheduled renew task is set up
* check that the scheduled task actually launch the Certbot renew logic

The Windows nightly tests are updated accordingly, in order to have the tests run on Windows Server 2012R2, 2016 and 2019.

These tests will evolve as we add more logic on the installer. 

* Configure an integration test testing the windows installer

* Write the test module

* Configurable installer path, prepare azure pipelines

* Fix option

* Update test_main.py

* Add confirmation for this destructive test

* Use regex to validate certbot --version output

* Explicit dependency on a log output

* Use an exception to ask confirmation

* Use --allow-persistent-changes

* Set recreate = true in tox.ini. (#7746)

Fixes #7745.

* Add triggers for only a single CI system (#7748)

* Configure travis-test to only run on Travis.

* Configure azure-test to only run on Azure.

* Add docs and comments to keep it up-to-date.

* restore CHANGELOG in root directory

* Add test for $hostname parsing

* Remove todo::

* Fixing existing tests

* Don't display todo comments in docs (#7753)

Currently if you go to https://certbot.eff.org/docs/api/certbot.crypto_util.html, there is a todo comment displayed at the top of the page. These todos were written for developers, not users, so I do not think they should be shown from our documentation.

This PR makes the quick and easy fix of configuring Sphinx not to show these todo items. I created #7752 to track removing all of these todos from our docstrings and disabling the Sphinx todo extension.

* Set todo_include_todos=False in sphinx-quickstart

* Remove todos from existing docs.

* Remove text that certbot.tests.utils isn't public (#7754)

* Remove link to letsencrypt readthedocs (#7757)

After a brief discussion in Mattermost, I shut down letsencrypt.readthedocs.io. Turns out we were linking to it in our README here so let's remove the broken link.

I didn't update the link to point to one of the readthedocs projects we still have because are main Certbot docs are self-hosted rather than being on readthedocs.

* Really remove old docs link from README (#7758)

* Move ocsp.py to public api (#7744)

We should move ocsp.py to public API, as an upcoming OCSP prefetching functionality in Apache plugin relies on it, and as the plugins are note released in lockstep with the Certbot core, we need to be careful when changing those APIs.

* Move ocsp.py to public api

* Fix type annotations, move to pointing to an interface and fix linting

* Add certbot.ocsp to documentation table of contents

* Modify tests to reflect the changes in ocsp.py

* Add changelog entry

* Fix notAfter mock for tests

* Print script output in case of a failure. (#7759)

These tests failed at https://travis-ci.com/certbot/certbot/jobs/285202481 but do not include any output from the script about what went wrong because the string created from `subprocess.CalledProcessError` does not include value of output.

This PR fixes that by printing these values which `pytest` will include in the output if the test fails.

* Fix unpinned tests (#7760)

Our nightly tests failed last night due to a new release of `virtualenv` and `pip`'s lack of dependency resolution: https://travis-ci.com/certbot/certbot/jobs/285797857#L280. It looks like we were not the only ones affected by this problem: https://github.com/pypa/virtualenv/issues/1551

This fixes the problem by using `-I` to skip the logic where `pip` decides a dependency is already satisfied and has it reinstall/update the packages passed to `pip` and all of their dependencies.

You can see our nightly tests passing with this change at https://github.com/certbot/certbot/runs/439231061.

* Remove duplicate pyparsing pin

* update pyparsing comment

* Remove _internal from docstring.

* Remove useless pylint error suppression directives (#7657)

As pylint is evolving, it improves its accuracy, and several pylint error suppression (`# pylint: disable=ERROR) added in certbot codebase months or years ago are not needed anymore to make it happy.

There is a (disabled by default) pylint error to detect the useless suppressions (pylint-ception: `useless-suppression`). It is not working perfectly (it has also false-positives ...) but it is a good start to clean the codebase.

This PR removes several of these useless suppressions as detected by the current pylint version we use.

* Remove useless suppress

* Remove useless lines

* more robustly stop patches (#7763)

* Remove letshelp-certbot (#7761)

* remove references to letshelp

* remove letshelp files

* Remove line continuation

Co-authored-by: ohemorange <ebportnoy@gmail.com>

* Fix spurious pylint errors. (#7780)

This fixes (part of) the problem identified in https://github.com/certbot/certbot/pull/7657#issuecomment-586506340.

When I tested our pylint setup on Python 3.5.9, 3.6.9, or 3.6.10, tests failed with:
```
************* Module acme.challenges
acme/acme/challenges.py:57:15: E1101: Instance of 'UnrecognizedChallenge' has no 'jobj' member (no-member)
************* Module acme.jws
acme/acme/jws.py:28:16: E1101: Class 'Signature' has no '_orig_slots' member (no-member)
```
These errors did not occur for me on Python 3.6.7 or Python 3.7+.

You also cannot run our lint setup on Python 2.7 because our pinned version of pylint's dependency `asteroid` does not support Python 2. Because of this, `pylint` is not installed in the virtual environment created by `tools/venv.py` and our [`lint` environment in tox specifies that Python 3 should be used](https://github.com/certbot/certbot/blob/fd64c8c33b2176e6569d64d30776bd5fc9fd3820/tox.ini#L132).

I tried updating pylint and its dependencies to fix the problem, but they still occur so I think adding back these disable checks on these lines again is the best fix for now.

* Correct AutoHSTS docs (#7767)

domains is a list of strings, not a single string.

* Correct AutoHSTS docs.

* Fix Apache enable_autohsts docs.

* add pgp key docs (#7765)

Fixes #7613.

* Move our macOS tests to Azure Pipelines (#7793)

[Our macOS tests are failing](https://travis-ci.com/certbot/certbot/builds/149965318) again this time due to the problem described at https://travis-ci.community/t/macos-build-fails-because-of-homebrew-bundle-unknown-command/7296/14.

I tried adding `update: true` to the Homebrew config as described in that thread, but [it didn't work](https://travis-ci.com/certbot/certbot/builds/150070374). I also tried updating the macOS image we use which [didn't work](https://travis-ci.com/certbot/certbot/builds/150072389).

Since we continue to have problems with macOS on Travis, let try moving the tests to Azure Pipelines.

* test macos

* Remove Travis macOS setup

* add displayName

* Refactor cli.py, splitting in it smaller submodules (#6803)

* Refactor cli.py into a package with submodules

* Added unit tests for helpful module in cli.

* Fixed linter errors

* Fixed pylint issues

* Updated changelog.md

* Fixed test failing and mypy error. Appeared a new pylint error (seems to be in conflict with mypy)

mypy require zope.interface to be imported but when imported it is not used and pylint throws an error.

* Fixed pylint errors

* Apply changes to cli since last merge from master (efc8d49806b14a31d88cfc0f1b6daca1dd373d8d)

* Fix lint

* Remaining lint errors

Co-authored-by: Adrien Ferrand <adferrand@users.noreply.github.com>

* Add test

* Use UTF-8 encoding for nginx plugin

* Use `io` module instead of `codecs`

See https://mail.python.org/pipermail/python-list/2015-March/687124.html

* Added test for valid/invalid unicode characters

* Fix lint problems with long lines

* Relpace deprecated `logger.warn()` with `logger.warning()`

* Remove `unicode_support/` path in test case

* Add test case for `_parse_ssl_options()`

* Add logging test for `_parse_files()`

* Trivial code clean-up

* Add my name to AUTHORS.md

:)

* Add this change to CHANGELOG.md

* Add `TestCase.assertLogs()` backport for Python 2.7

* Add simple comments

* acme: ignore params in content-type check (#7342)

* acme: ignore params in content-type check

Fixes the warning in #7339

* Suppress coverage complaint in test

* Update CHANGELOG

* Repair symlink

Co-authored-by: Adrien Ferrand <adferrand@users.noreply.github.com>

* Fix issue #7165 in _create_challenge_dirs(), attempt to fix pylint errors (#7568)

* fix issue #7165 by checking if directory exists before trying to create it, fix possible pylint issues in webroot.py

* fix get_chall_pref definition

* Update CHANGELOG.md

* Update CHANGELOG.md

Co-authored-by: Adrien Ferrand <adferrand@users.noreply.github.com>

* remove _internal docs (#7801)

* Fixed typo & some trivial documentation change

* Update comment in testdata file

* Update parser test to better assert logging output

* Split advanced pipeline (#7813)

I want to do what I did in https://github.com/certbot/certbot/pull/7733 to our Azure Pipelines setup, but unfortunately this isn't currently possible. The only filters available for service hooks for the "build completed" trigger are the pipeline and build status. See 
![Screen Shot 2020-02-26 at 3 04 56 PM](https://user-images.githubusercontent.com/6504915/75396464-64ad0780-58a9-11ea-97a1-3454a9754675.png)

To accomplish this, I propose splitting the "advanced" pipeline into two cases. One is for builds on protected branches where we want to be notified if they fail while the other is just used to manually run tests on certain branches.

* update letstest reqs (#7809)

I don't fully understand why, but since I updated my macbook to macOS Catalina, the test script currently fails to run for me with the versions of our dependencies we have pinned. Updating the dependencies solves the problem though and you can see Travis also successfully running tests with these new dependencies at https://travis-ci.com/certbot/certbot/builds/150573696.

* Remove unused notify code. (#7805)

This code is unused and hasn't been modified since 2015 except for various times our files have been renamed. Let's remove it.

* Change how _USE_DISTRO is set for mypy (#7804)

If you run `mypy --platform darwin certbot/certbot/util.py` you'll get:
```
certbot/certbot/util.py:303: error: Name 'distro' is not defined
certbot/certbot/util.py:319: error: Name 'distro' is not defined
certbot/certbot/util.py:369: error: Name 'distro' is not defined
```
This is because mypy's logic for handling platform specific code is pretty simple and can't figure out what we're doing with `_USE_DISTRO` here. See https://mypy.readthedocs.io/en/stable/common_issues.html#python-version-and-system-platform-checks for more info.

Setting `_USE_DISTRO` to the result of `sys.platform.startswith('linux')` solves the problem without changing the overall behavior of our code here though.

This fixes part of https://github.com/certbot/certbot/issues/7803, but there's more work to be done on Windows.

* Fix tests on macOS Catalina (#7794)

This PR fixes the failures that can be seen at https://dev.azure.com/certbot/certbot/_build/results?buildId=1184&view=results.

You can see this code running on macOS Catalina at https://dev.azure.com/certbot/certbot/_build/results?buildId=1192&view=results.

* Remove references to deprecated flags in Certbot. (#7509)

Related to https://github.com/certbot/certbot/pull/7482, this removes some references to deprecated options in Certbot.

The only references I didn't remove were:

* In `certbot/tests/testdata/sample-renewal*` which contains a lot of old values and I think there's even some value in keeping them so we know if we make a change that suddenly causes old renewal configuration files to error.
* In the Apache and Nginx plugins and I created https://github.com/certbot/certbot/issues/7508 to resolve that issue.

* Remove codecov (#7811)

After getting a +1 from everyone on the team, this PR removes the use of `codecov` from the Certbot repo because we keep having problems with it.

Two noteworthy things about this PR are:

1. I left the text at https://github.com/certbot/certbot/blob/4ea98d830bcc3d1b980a4055243c6a6a25d8dc54/.azure-pipelines/INSTALL.md#add-a-secret-variable-to-a-pipeline-like-codecov_token because I think it's useful to document how to set up a secret variable in general.
2. I'm not sure what the text "Option -e makes sure we fail fast and don't submit to codecov." in `tox.cover.py` refers to but it seems incorrect since `-e` isn't accepted or used by the script so I just deleted the line.

As part of this, I said I'd open an issue to track setting up coveralls (which seems to be the only real alternative to codecov) which is at https://github.com/certbot/certbot/issues/7810.

With my change, failure output looks something like:
```
$ tox -e py27-cover
...
Name                                                         Stmts   Miss  Cover   Missing
------------------------------------------------------------------------------------------
certbot/certbot/__init__.py                                      1      0   100%
certbot/certbot/_internal/__init__.py                            0      0   100%
certbot/certbot/_internal/account.py                           191      4    98%   62-63, 206, 337
...
certbot/tests/storage_test.py                                  530      0   100%
certbot/tests/util_test.py                                     374     29    92%   211-213, 480-484, 489-499, 504-511, 545-547, 552-554
------------------------------------------------------------------------------------------
TOTAL                                                        14451    647    96%
Command '['/path/to/certbot/dir/.tox/py27-cover/bin/python', '-m', 'coverage', 'report', '--fail-under', '100', '--include', 'certbot/*', '--show-missing']' returned non-zero exit status 2
Test coverage on certbot did not meet threshold of 100%.
ERROR: InvocationError for command /Users/bmw/Development/certbot/certbot/.tox/py27-cover/bin/python tox.cover.py (exited with code 1)
_________________________________________________________________________________________________________________________________________________________ summary _________________________________________________________________________________________________________________________________________________________
ERROR:   py27-cover: commands failed
```
I printed the exception just so we're not throwing away information.

I think it's also possible we fail for a reason other than the threshold not meeting the percentage, but I've personally never seen this, `coverage report` output is not being captured so hopefully that would inform devs if something else is going on, and saying something like "Test coverage probably did not..." seems like overkill to me personally.

* remove codecov

* remove unused variable group

* remove codecov.yml

* Improve tox.cover.py failure output.

* Don't run advanced tests on PRs. (#7820)

When I wrote https://github.com/certbot/certbot/pull/7813, I didn't understand the default behavior for pull requests if you don't specify `pr` in the yaml file. According to https://docs.microsoft.com/en-us/azure/devops/pipelines/build/triggers?view=azure-devops&tabs=yaml#pr-triggers:

> If no pr triggers appear in your YAML file, pull request builds are automatically enabled for all branches...

This is not the behavior we want. This PR fixes the problem by disabling builds on PRs.

You should be able to see this working because the advanced tests should not run on this PR but they did run on https://github.com/certbot/certbot/pull/7811.

* Document safe and simple usage by services without root privileges (#7821)

Certificates are public information by design: they are provided by
web servers without any prior authentication required.  In a public
key cryptographic system, only the private key is secret information.

The private key file is already created as accessible only to the root
user with mode 0600, and these file permissions are set before any key
content is written to the file.  There is no window within which an
attacker with access to the containing directory would be able to read
the private key content.

Older versions of Certbot (prior to 0.29.0) would create private key
files with mode 0644 and rely solely on the containing directory
permissions to restrict access.  We therefore cannot (yet) set the
relevant default directory permissions to 0755, since it is possible
that a user could install Certbot, obtain a certificate, then
downgrade to a pre-0.29.0 version of Certbot, then obtain another
certificate.  This chain of events would leave the second
certificate's private key file exposed.

As a compromise solution, document the fact that it is safe for the
common case of non-downgrading users to change the permissions of
/etc/letsencrypt/{live,archive} to 0755, and explain how to use chgrp
and chmod to make the private key file readable by a non-root service
user.

This provides guidance on the simplest way to solve the common problem
of making keys and certificates usable by services that run without
root privileges, with no requirement to create a custom (and hence
error-prone) executable hook.

Remove the existing custom executable hook example, so that the
documentation contains only the simplest and safest way to solve this
very common problem.

Signed-off-by: Michael Brown <mbrown@fensystems.co.uk>

* Check OCSP as part of determining if the certificate is due for renewal (#7829)

Fixes #1028.

Doing this now because of https://community.letsencrypt.org/t/revoking-certain-certificates-on-march-4/.

The new `ocsp_revoked_by_paths` function  is taken from https://github.com/certbot/certbot/pull/7649 with the optional argument removed for now because it is unused.

This function was added in this PR because `storage.py` uses `self.latest_common_version()` to determine which certificate should be looked at for determining renewal status at https://github.com/certbot/certbot/blob/9f8e4507ad0cb3dbedb726dda4c46affb1eb7ad3/certbot/certbot/_internal/storage.py#L939-L947

I think this is unnecessary and you can just look at the currently linked certificate, but I don't think we should be changing the logic that code has always had now.

* Check OCSP status as part of determining to renew

* add integration tests

* add ocsp_revoked_by_paths

* Update changelog for 1.3.0 release

* Release 1.3.0

* Add contents to certbot/CHANGELOG.md for next version

* Bump version to 1.4.0

* Fix issues with Azure Pipelines (#7838)

This PR fixes two issues.

First, it fixes #7814 by removing our tests on Windows Server 2012. I also added the sentence "Certbot supports Windows Server 2016 and Windows Server 2019." to https://community.letsencrypt.org/t/beta-phase-of-certbot-for-windows/105822.

Second, it fixes the test failures which can be seen at https://dev.azure.com/certbot/certbot/_build/results?buildId=1309&view=results by no longer manually installing our own version of Python and instead using the one provided by Azure.

These small changes are in the same PR because I wanted to fix test failures ASAP and `UsePythonVersion` is not available on Windows 2012. See https://github.com/certbot/certbot/pull/7641#discussion_r358510854.

You can see tests passing with this change at https://dev.azure.com/certbot/certbot/_build/results?buildId=1311&view=results.

* stop testing on win2012

* switch to UsePythonVersion

* Add changes to the correct changelog entry (#7833)

https://github.com/certbot/certbot/p…
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.

Check OCSP as part of determining if the certificate is due for renewal
3 participants