Fix certbot-auto regarding python 3.4 -> python 3.6 migration for CentOS 6 users by adferrand · Pull Request #7519 · certbot/certbot

adferrand · 2019-11-06T10:14:43Z

Fixes #7513
Fixes #7007
Fixes #7367
Fixes #7524

This PR adds back the logic that was removed with #7510 and that was migrating CentOS 6 certbot-auto users to Python 3.6. It adds on top of this the necessary fix that required a fallback. The fix logic is what I described in #7513 (comment)

I launched a full tests suite, that you can see here: https://travis-ci.com/certbot/certbot/builds/135220086

However this tests suite is not sufficient to validate this PR, since the faulty logic that is fixed is not invoked in a development version of certbot-auto. It needs to be validated against a released version.

This is kind of tricky, since you need to avoid certbot-auto to upgrade to 0.40.1, that obviously does not contain the fix, and does not even migrate to Python 3.6 because of the fallback.

To test, I simply changed the LE_AUTO_VERSION value in the letsencrypt-auto-source\letsencrypt-auto in this PR from 0.41.0.dev0 to 0.40.2. Consequently, the script will effectively fetch for new versions, state that the local one is already the latest one, so do not upgrade itself, and so invoke the logic with the fix.

I run tox -e le_auto_centos6 and tox -e le_auto_oraclelinux6 with this approach, and the tests passed.

I double checked by retrieving the output of letsencrypt-auto during these tests, and effectively get what I expected during a non-interactive run of the script:

PASSED: Did not upgrade to Python3 when Python2.7 is present.
PASSED: Successfully upgraded to Python3.4 using letsencrypt-auto < 0.37.0 when only Python2.6 is present.
Skipping upgrade because new OS dependencies may need to be installed.

To upgrade to a newer version, please run this script again manually so you can
approve changes or with --non-interactive on the command line to automatically
install any required packages.
certbot 0.38.0
PASSED: certbot-auto did not upgrade certbot in a non-interactive shell with --non-interactive flag not set.
PASSED: certbot-auto did not install Python3.6 in a non-interactive shell with --non-interactive flag not set.
PASSED: Successfully upgraded to Python3.6 using current letsencrypt-auto when only Python2.6/Python3.4 are present.

This PR is ready for review.

adferrand · 2019-11-06T10:16:06Z

This PR is composed of two commits only, to help the review. The first one revert the code removal without any further changes. The second contains all further changes to fix the script.

bmw

Revert commit looks good to me.

I don't think I like the proposed fix though. I wrote more in response to your comment in the issue at #7513 (comment).

letsencrypt-auto-source/letsencrypt-auto.template

This reverts commit 9b848b1.

adferrand · 2019-11-06T21:34:11Z

New fix, based on discussions done in #7513. This times, we simply ensure that REMOTE_VERSION will not be empty.

I used the same test pattern to check what is going on for a released version. The le_auto tests are passing, with the same expected output.

adferrand · 2019-11-06T21:35:08Z

Still two commits: first one reverts the code removal, second one fixes certbot-auto.

adferrand · 2019-11-07T01:26:34Z

I added the modification mentioned in #7524, to not rely on scl_source to enable rh-python36, since it may not exist.

Full tests suite: https://travis-ci.com/certbot/certbot/builds/135371750

bmw

Small comments but otherwise LGTM.

bmw · 2019-11-07T21:24:42Z

letsencrypt-auto-source/letsencrypt-auto.template

+      REMOTE_VERSION="$LE_AUTO_VERSION"
+    fi
+
    LE_VERSION_STATE=`CompareVersions "$LE_PYTHON" "$LE_AUTO_VERSION" "$REMOTE_VERSION"`


CompareVersions is still run if Python is too old which I don't like for the some of the reasons I put in #7513 (comment).

Instead, can we set LE_VERSION_STATE to "UP_TO_DATE" if REMOTE_VERSION is empty and otherwise call CompareVersions to set its value?

I did that.

In fact I rewrote your idea into this: if REMOTE_VERSION is empty, skip the entire self-upgrading process. It is functionally equivalent, since the self-upgrading process is invoked only for REMOTE_VERSION values UNOFFICIAL or OUTDATED. But it is less error-prone because here now is explicit that the process is skipped, since the code is in a if/else dedicated branch.

bmw · 2019-11-07T21:49:28Z

letsencrypt-auto-source/pieces/bootstrappers/rpm_python3_legacy.sh

+  if [ ! -f /opt/rh/rh-python36/enable ]; then
+      return 0
+  fi
+  . /opt/rh/rh-python36/enable


I think it's very unlikely to matter, but I'd like to keep the set +e and set -e around this. set -e isn't set in scl_source and it relies on this behavior. /opt/rh/rh-python36/enable currently does not and probably never will rely on this behavior, but I'd rather not have to scramble to put out a point release if a new version of rh-python36 suddenly does.

I am strongly against that. The reason is that in general, continuing a script when a command returned a non 0 exit code is a bad idea, because something wrong happened and most likely the system is not in the state expected. Here we would ignore all errors on bad behaviors that can happen (broken enabler script, broken shell ...) and continue the logic without a functional Python distribution, only to catch an hypothetical non 0 exit code in the future.

We called set +e previously because scl_source was relying on this, but this behavior from scl_source was already a questionable design decision. But since it was required, we had to adapt ourselves.

However here, nothing is saying that /opt/rh/rh-python36/enable could ever return a non 0 exit code on an acceptable behavior for a good reason, and I cannot see how changing this behavior could be a good idea. Indeed if maintainers decided to do so, all scripts in the wild that use rh-python36 and enable it could become unstable, or even break if they use also set -e.

Keep in mind that there's a big difference between:

Using set to modify the shell's default behavior and sourcing a script.

Ignoring that script's exit code.

The former causes the script to execute in your modified environment changing the default behavior of the shell interpreting the script while the latter treats the script like a black box and only worries about the exit code the script returns to the caller.

By using /opt/rh/rh-python36/enable directly, I think we're reaching into the internals of the package therefore I don't think we should rely on having consistent behavior. Instead, I think Red Hat wants you to use scl (or in very rare cases scl_source) to interface with these scripts.

As a test, I added the following to the top of /opt/rh/rh-python36/enable and both scl and scl_source still work:

try_something if [ "$?" -ne 0 ]; then do_fallback fi

If they did the equivalent of set -e; . /opt/rh/rh-python36/enable it would not.

Since the official scripts do not do this, I do not think it is worth doing it ourselves as I think it makes certbot-auto too fragile to changes I suspect the SCL maintainers view as internal.

With that said, I think checking the exit code of . /opt/rh/rh-python36/enable is a good idea. Both scl and scl_source does do this so I'd prefer this code looks something like:

set +e if ! . /opt/rh/rh-python36/enable; then error 'Unable to enable rh-python36!' exit 1 fi set -e

OK, I see your point. Indeed I forgot the difference between sourcing a script and calling it. Your proposition matches my concern about controling the sourcing result. I implement that.

adferrand · 2019-11-08T19:13:13Z

Full tests suite here: https://travis-ci.com/certbot/certbot/builds/135695756

adferrand · 2019-11-08T19:19:09Z

le_auto_* test are behaving correctly on a release version.

bmw · 2019-11-08T22:59:15Z

LGTM!

@joohoi, are you interested in continuing with us on the saga of removing Python 3.4 support and giving this PR a second review? To flag this again, the first commit here just reverts #7510. The changes in this PR are otherwise pretty minimal.

joohoi · 2019-11-11T14:15:42Z

Yeah, I'll take a look.

bmw · 2019-11-19T20:22:35Z

Oh no. It seems CentOS 6 does not offer a 32 bit version of SCL. See https://community.letsencrypt.org/t/certbot-failing-to-find-python-version-on-centos-6-10/105634/11 and https://wiki.centos.org/AdditionalResources/Repositories/SCL#head-9c6aea9c13b921d5258446c4c5e5886571bdb741.

I'm not sure if it's feasible and reliable to use SCL packages from another source.

joohoi · 2019-11-20T15:09:17Z

The code looks good to me. I'll be doing some final tests tomorrow in the actual RHEL 6 instance I have running.

There's the issue Brad just pointed out that we need to find a way to navigate through.

Oh no. It seems CentOS 6 does not offer a 32 bit version of SCL.

Uhh, that's really unfortunate. Nice catch btw, I wasn't testing on 32bit, and would have not caught this ever.

I'm not sure if it's feasible and reliable to use SCL packages from another source.

I think we'd be breaking the trust there, and even just asking the user to manually do this would in a way signal that we've validated the repository. And unfortunately CentOS 6 will be around (non-EOL) for another year.

To mitigate this, I think our best chance is to add the check if the system is running 32bit and just keep the version that's currently installed around. This however doesn't help new users of Certbot on 32bit systems. Maybe we would be best off to point out the ended support for them and refuse to install?

adferrand · 2019-11-20T15:59:01Z

I discussed with @bmw about this issue yesterday. We agreed on basically the same than you: it is not possible to support CentOS6 on 32 bits version, and we need to add a proper "end of the road" logic in that situation.

This logic could be reused for Python 2.7 in general, so it should be reliable (so more complex), and so done out of this PR.

As you said, it should take the form of "let's thing as it is, and inform the user). More precisely, we should avoid the rebootstraping to fail, because it would destroy the virtual environment and break the current Certbot installation. So:

For new installations, certbot-auto refuses to run and inform the user, as for any current unsupported system
For existing installations, certbot-auto refuses to upgrade certbot, informs the user, then delegates to the existing certbot installation.

In parallel, we should certainly update our documentation about this.

One last question (@bmw), in that situation, should we still let certbot-auto upgrade itself? In the short term it gives the possibility to change the approach if necessary on a new version, but in the long term, it is harder to maintain, and we could insert at some point a code that is not supported by the old systems and so break totally certbot-auto. So I would be more in favor of just stopping the upgrade also of certbot-auto.

bmw · 2019-11-20T18:36:57Z

One last question (@bmw), in that situation, should we still let certbot-auto upgrade itself?

I think so at least for now on systems that has a version of Python we're testing against available. In addition to potentially allowing us to fix things if we break them, my main concern is I think stopping users from updating past a certain version is a fair bit of work.

The problem is that certbot-auto always updates to the latest version available and not everyone follows of our advice of running certbot-auto without --no-self-upgrade twice per day. The way this update system works is it first determines the latest version number of the certbot package on PyPI and then tries to download certbot-auto from the corresponding tag on GitHub.

The result of this is that you can have an older version of the script suddenly try to update to the latest version of the script. To work around this I think we'd need to essentially keep around an old version of the script as the "latest" version that works which people could upgrade to and write into it a new upgrade path that would only be used on certain systems.

We could do that now, but then we'd have to maintain two versions of certbot-auto and continue making changes to both as we deprecate more systems. I'd be more interested in doing that when we're not planning on making any more changes to the current version of certbot-auto, although at that point I'd be tempted to just avoid making changes altogether until certbot-auto's final EOL.

bmw · 2019-12-20T00:09:11Z

@joohoi, if you're working between now and new years, please prioritize a review on this PR. cryptography dropped Python 3.4 support so we should as well ASAP. I think you can assume the non-x86_64 issue is handled in #7587 correctly.

joohoi

LGTM!

adferrand · 2020-01-08T15:48:04Z

This PR is now synced with master, that ships with the certbot-auto sunset mechanism, applied here for CentOS/RHEL 6 32bits.

bmw · 2020-01-08T21:01:27Z

You can see the full test suite passing on this PR at https://travis-ci.com/certbot/certbot/builds/143268108. (The tested code is identical to the code here except Travis notifications are disabled and .vscode/ was removed and added to .gitignore.)

bmw

We should add a changelog entry about this. How about:

We deprecated support for Python 3.4 in Certbot and its ACME library. Support for Python 3.4 will be removed in the next release of Certbot. certbot-auto users on RHEL 6 based x86_64 systems will be asked to enable Software Collections (SCL) repository so Python 3.6 can be installed. certbot-auto can enable the SCL repo for you on CentOS 6 while users on other RHEL 6 based systems will be asked to do this manually.

Other than that, this LGTM!

certbot/CHANGELOG.md

PR LGTM!

joohoi

LGTM!

My change suggestions below are super small nitpicks, so I decided to approve the PR in order to not to block if they are not going to be changed. I'm fine with the current version as well.

joohoi · 2020-01-10T10:58:08Z

certbot/CHANGELOG.md

  continue to work, but they will no longer receive updates. To install a
  newer version of Certbot on these systems, you should update your OS.
+* Support for Python 3.4 in Certbot and its ACME library is deprecated and will be
+  removed in the next release of Certbot. certbot-auto users on RHEL 6 based x86_64


Suggested change

removed in the next release of Certbot. certbot-auto users on RHEL 6 based x86_64

removed in the next release of Certbot. certbot-auto users on x86_64 based RHEL 6

I personally don't like this new wording. It's not just RHEL 6 that's affected, it's all x86_64 systems based on/derived from RHEL 6 such as CentOS/Oracle Linux/Scientific Linux 6.

I'm not sure the motivation behind the suggestion, but what if we just made the sentence even more explicit by saying something like:

certbot-auto users on x86_64 systems running RHEL 6 or derivatives will be asked to enable...

Your initial emphase was on the category of OS systems, @joohoi ones was on the bitness. Both are important. Your last suggestion gives a balance of that ^^

joohoi · 2020-01-10T10:59:40Z

certbot/certbot/_internal/main.py


+    if sys.version_info[:2] == (3, 4):
+        logger.warning("Python 3.4 support will be dropped in the next release "
+                    "of Certbot - please upgrade your Python version.")


Should we define the minimum Python version to use here?

letsencrypt-auto-source/tests/centos6_tests.sh

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

adferrand · 2020-01-10T15:23:35Z

Ok @joohoi I integrated your comments, the PR is available for a new review.

joohoi

LGTM!

bmw · 2020-01-10T19:14:56Z

LGTM!

adferrand · 2020-01-10T19:29:36Z

PR is not approved yet, @joohoi? @bmw?

bmw · 2020-01-10T19:52:27Z

I think this PR is basically approved by both of us.

I was just going to give Joona a chance to comment on it if he wanted since we deviated from his suggestion, but since we only modified the changelog, I'll approve and merge this on Monday if he doesn't get to it before then.

joohoi

Yeah, LGTM!

This PR corrects two things in our CentOS instructions: 1. On CentOS/RHEL 8 there apparently is no executable named "python" by default and at the very least we don't install one because certbot-auto uses "python3" on the platform. As a result our cron instructions to use "python" to sleep do not work. See https://community.letsencrypt.org/t/crontab-setup-on-centos-8-with-apache/111043. 2. With certbot/certbot#7519, we no longer use EPEL on CentOS/RHEL 6 anymore so we shouldn't be telling RHEL 6 users to enable EPEL manually before running certbot-auto.

* 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…

bmw requested changes Nov 6, 2019

View reviewed changes

letsencrypt-auto-source/letsencrypt-auto.template Show resolved Hide resolved

adferrand added 2 commits November 6, 2019 22:30

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

c7b9288

This reverts commit 9b848b1.

Fix certbot-auto

a775ce3

adferrand force-pushed the fix-python34-deprecation branch from dafbc65 to a775ce3 Compare November 6, 2019 21:31

bmw mentioned this pull request Nov 7, 2019

scl_source may not exist #7524

Closed

Use a more consistent way to enable rh-python36

6dfbb71

adferrand force-pushed the fix-python34-deprecation branch from 9344c43 to 6dfbb71 Compare November 7, 2019 01:07

bmw requested changes Nov 7, 2019

View reviewed changes

Avoid to call CompareVersions unecessarily

b1f2d89

adferrand force-pushed the fix-python34-deprecation branch from 188679d to b1f2d89 Compare November 8, 2019 09:40

Control rh-python36 exit code

4c914ee

bmw added this to the 1.0.0 milestone Nov 8, 2019

adferrand mentioned this pull request Nov 22, 2019

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

Merged

bmw modified the milestones: 1.0.0, 1.1.0 Dec 5, 2019

Merge branch 'master' into fix-python34-deprecation

1bafaf0

joohoi previously approved these changes Jan 3, 2020

View reviewed changes

bmw previously requested changes Jan 8, 2020

View reviewed changes

Add changelog entry

3872208

bmw reviewed Jan 9, 2020

View reviewed changes

certbot/CHANGELOG.md Outdated Show resolved Hide resolved

Finish sentence

31da73f

joohoi previously approved these changes Jan 10, 2020

View reviewed changes

Update certbot/CHANGELOG.md

3c7bd21

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

adferrand dismissed joohoi’s stale review via 3c7bd21 January 10, 2020 15:14

adferrand and others added 7 commits January 10, 2020 16:14

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

4e593a1

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

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

7a32be5

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

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

a8daedd

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

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

0ccd887

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

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

7cafe03

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

Update comments

9fcd82d

Improve warning message

22f7247

joohoi previously approved these changes Jan 10, 2020

View reviewed changes

Update changelog

96ff6e3

adferrand dismissed joohoi’s stale review via 96ff6e3 January 10, 2020 19:10

joohoi approved these changes Jan 12, 2020

View reviewed changes

adferrand merged commit e84ed49 into certbot:master Jan 13, 2020

bmw mentioned this pull request Jan 31, 2020

Correct CentOS instructions certbot/website#528

Merged

davidjb mentioned this pull request May 20, 2020

RHEL/CentOS 6 instructions should mention enabling SCL repository for new Python certbot/website#585

Open

	removed in the next release of Certbot. certbot-auto users on RHEL 6 based x86_64
	removed in the next release of Certbot. certbot-auto users on x86_64 based RHEL 6

Uh oh!

Conversation

adferrand commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adferrand commented Nov 6, 2019

Uh oh!

bmw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adferrand commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adferrand commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adferrand commented Nov 7, 2019

Uh oh!

bmw left a comment

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!

adferrand Nov 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

adferrand commented Nov 8, 2019

Uh oh!

adferrand commented Nov 8, 2019

Uh oh!

bmw commented Nov 8, 2019

Uh oh!

joohoi commented Nov 11, 2019

Uh oh!

bmw commented Nov 19, 2019

Uh oh!

joohoi commented Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adferrand commented Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmw commented Nov 20, 2019

Uh oh!

bmw commented Dec 20, 2019

Uh oh!

joohoi left a comment

Choose a reason for hiding this comment

Uh oh!

adferrand commented Jan 8, 2020

Uh oh!

bmw commented Jan 8, 2020

Uh oh!

bmw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joohoi left a comment

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!

adferrand commented Nov 6, 2019 •

edited

Loading

adferrand commented Nov 6, 2019 •

edited

Loading

adferrand commented Nov 6, 2019 •

edited

Loading

adferrand Nov 8, 2019 •

edited

Loading

joohoi commented Nov 20, 2019 •

edited

Loading

adferrand commented Nov 20, 2019 •

edited

Loading