[SRU] Backport letsencrypt from bionic

Bug #1640978 reported by Peter Eckersley on 2016-11-11
98
This bug affects 13 people
Affects Status Importance Assigned to Milestone
python-acme (Ubuntu)
High
Unassigned
Xenial
High
Michael Casadevall
python-certbot (Ubuntu)
High
Unassigned
Xenial
High
Michael Casadevall
python-certbot-apache (Ubuntu)
High
Unassigned
Xenial
High
Michael Casadevall
python-certbot-nginx (Ubuntu)
High
Unassigned
Xenial
High
Michael Casadevall
python-letsencrypt (Ubuntu)
Xenial
High
Michael Casadevall
python-letsencrypt-apache (Ubuntu)
Xenial
High
Michael Casadevall

Bug Description

This bug contains a list of known major and other issues fixed between upstream letsencrypt 0.4.1 and the latest version, certbot 0.9.3 (the project has also been renamed to avoid confusion between the python client software and the Let's Encrypt CA service).

[Impact]

MAJOR BUGS FIXED

https://github.com/certbot/certbot/issues/2750
letsencrypt < 0.5.0 was not compatible with future configuration files, so users who run certbot-auto then downgrade to the Xenial packages will encounter errors.

https://github.com/certbot/certbot/issues/2709
Failure to remember choices of authenticator plugins for renewal operation. This would essentially make "letsencrypt renew" useless on Xenial. Numerous less severe automated renewal-related bugs fixed in subsequent releases:
https://github.com/certbot/certbot/issues?utf8=%E2%9C%93&q=is%3Aissue%20milestone%3A0.5.0%20is%3Aclosed%20label%3Arenewal%20
https://github.com/certbot/certbot/issues?q=is%3Aissue+milestone%3A0.7.0+is%3Aclosed+label%3Arenewal
https://github.com/certbot/certbot/issues?utf8=%E2%9C%93&q=is%3Aissue%20milestone%3A0.6.0%20is%3Aclosed%20label%3Arenewal%20
https://github.com/certbot/certbot/issues?utf8=%E2%9C%93&q=is%3Aissue%20milestone%3A0.8.1%20is%3Aclosed%20label%3Arenewal%20
https://github.com/certbot/certbot/issues?utf8=%E2%9C%93&q=is%3Aissue%20milestone%3A0.9.0%20is%3Aclosed%20label%3Arenewal%20

https://github.com/certbot/certbot/issues/2613
Failure to handle IPv6 Virtual hosts in Apache configurations

https://github.com/certbot/certbot/issues/2320
Erroneous behaviour with Apache configs that have multiple vhosts in a single file (these are still not supported for cert installation in 0.9.3, but at least produce clear error messages)

https://github.com/certbot/certbot/issues/2768
Incompatibility with the specified version of the ACME protocol, preventing the Let's Encrypt serverside code from following it correctly

https://github.com/certbot/certbot/issues/2731
Failure to parse Plesk's apache config files

https://github.com/certbot/certbot/issues/1243
Apache plugin errors out when transformations to a configuration turn out to be a no-op.

https://github.com/certbot/certbot/issues/3210
Incorrect handling of RewriteCond directives when trying to avoid Apache inifinite redirect loops

https://github.com/certbot/certbot/issues/1833
Problems running Apache renewal in cron due to cron's default PATH

UX: fail to re-ask for email address if the first one seems invalid:
https://github.com/certbot/certbot/issues/2675

UX: when re-running is a NOOP (due to renewal not being needed yet), print an explanation:
https://github.com/certbot/certbot/issues/1918

OTHER BUGS FIXED

Reduce the risk of incorrect or corrupt state in case of control-C interrupts:
https://github.com/certbot/certbot/issues/3219

Failure to correctly parse certain rewrite directives in Apache configs:
https://github.com/certbot/certbot/issues/2735

Failure to correctly enable HTTP -> HTTPS redirects in some Apache configs:
https://github.com/certbot/certbot/issues/3003

Failure to provide a sensible error if the user requests a Unicode domain:
(support for those is being added in 0.10.0)
https://github.com/certbot/certbot/issues/2661

Directory deletion permission errors are fatal when using the webroot plugin for non-root users (but shouldn't be):
https://github.com/certbot/certbot/issues/2678

UX: provide helpful guidance for people who want to run Certbot as a non-root user:
https://github.com/certbot/certbot/issues/2306

SIGNIFICANT NEW FEATURES WARRANTING AN SRU:

Support --quiet / -q

https://github.com/certbot/certbot/issues/2512

User interface for requesting certificates for multiple domain names with the
webroot plugin:
https://github.com/certbot/certbot/issues/1393

Support for DNS based authentication:
https://github.com/certbot/certbot/issues/1826

[Test Case]

All or almost all of the pull requests for the bugs above include unit test coverage.
Some also include integration or compatibility test coverage.

[Test Plan]

See https://wiki.ubuntu.com/StableReleaseUpdates/Certbot#SRU_Verification_Process

[Regression Potential]

The Certbot team has viewed breakage of existing workflows (especially ones that may be automated) as a serious issue, has strived to avoid them, and has treated workflow changes as regressions where it has occurred.

We have the following test suites in place for Certbot:

* Nosetest unit tests with coverage for each module between 97% and 100%; *test.py in the relevant tree.
* Integration tests that run Certbot against the current copy of Let's Encrypt's serverside boulder codebase. These require docker and are a little more involved to run. See tests/boulder_integration.sh for instructions.
* "Compatibility tests" that run the Apache and Nginx plugins against corpora of configuration files for those webservers; these live in certbot-compatibility-test/
* Test farm tests, which we use to check that our releases run correctly on a wide range of platforms. These spin up Amazon EC2 instances for numerous OSes and run various tests on them. They live in tests/letstest

We recommend that Ubuntu run the first of these test suites during build (but we believe the Debian packages already do that).

All of these tests mitigate the risk of regressions in our releases; nonetheless, some regressions do slip past. Because many of our users auto-update, these tend to be reported and fixed quickly in point releases. For instance, regressions in 0.9.0 were fixed in 0.9.1, 0.9.2 and 0.9.3. Certbot 0.9.3 has been used to issue hundreds of thousands of Certs in the field, so we are fairly confident that no further significant regressions exist in it, and that release is likely to be safe as a Xenial SRU.

At least two changes in functionality between 0.4.1 and 0.9.3 do bear specific consideration for Xenial though:

Debian has added a "certbot renew" twice-daily cron job to their packages between 0.4.1 and 0.9.3; we believe this is low regression risk (having secondary renewal mechanisms in place is a NOOP) but Xenial packages may want to increase the debconf verbosity to get consent for this from Xenial users who are upgrading?

We had a custom log rotation scheme (rotate logs after every run), we now act like a more typical daemon, so packages need to be rotating our logs:
https://github.com/certbot/certbot/issues/3382

[Other Info]

RAOF has offered to sponsor 0.9.3 into Xenial.

Peter Eckersley (pde-lists) wrote :

Also fixes: Launchpad bug #1608214

Peter Eckersley (pde-lists) wrote :

On IRC, rbasak asked:

<rbasak> From an SRU review perspective, what I most want spelled out the exact list of any behaviour changes you expect Xenial users to receive, and your confirmation that you don't believe that there are any other changes.
<rbasak> Your word as upstream carries a great deal of weight on this.
<rbasak> What you've written is great and sound like it covers most of this already - it's just a clear summary of what the final upload to Xenial will do is what I feel is missing - though of course I know you haven't finalised that yet.

Debian's certbot 0.9.3 packages are a close approximation to what we expect users to get. They Provide: and Replace: letsencrypt, and include aliases for all previous "letsencrypt" commands.

With the exception of the two (an auto-renewal cron job and slightly different log rotation) points noted above, we believe that the functionality and workflows supported by certbot 0.9.3 should be an but less buggy, super-set of the functionality and workflows supported by letsencrypt 0.4.1.

Peter Eckersley (pde-lists) wrote :

* "a compatible but less buggy"

I should also note that most of our users have been running the certbot-auto script which automatically upgrades them to the latest release of Certbot as soon as it's public, so the odds of us getting bug reports about workflow changes is fairly high.

Launchpad Janitor (janitor) wrote :

Status changed to 'Confirmed' because the bug affects multiple users.

Changed in python-letsencrypt (Ubuntu):
status: New → Confirmed

Hello everyone -- I'm (the primary, but not only) Debian maintainer of the certbot suite of packages. pde asked me to weigh in here to help answer some of rbasak's questions in a definitive manner.

Since the 0.4.1 release, there have been a handful of user-facing changes, but only a few are workflow changing ones.

The largest is the change in package name from letsencrypt to certbot. This was done for trademark reasons, however, we continue to ship both a transitional package and a symlink for the binary that should preserve users' workflows unmodified.

The second largest change is the addition of an automatic renewal of any certificates, through both a cronjob installed into cron.d and a systemd timer. The certbot binary will only act to renew the certificate, however, when it is "near" to expiring (<15 days). Users who have manually setup a cron already shouldn't experience any failures.

Other than that, there is new functionality available in upstream, but pde has already mentioned all of those. I'm happy to answer any questions or concerns the stable release managers have.

Peter Eckersley (pde-lists) wrote :

@hlieberman My understanding is that the package name change shouldn't affect any workflows, because "sudo apt-get install letsencrypt" will still work and the "letsencrypt" command will still work. Anything I might have missed?

Hi Harlan,

Thank you for taking the time to help us out!

On Sun, Nov 20, 2016 at 02:47:14AM -0000, Harlan Lieberman-Berg wrote:
> The second largest change is the addition of an automatic renewal of any
> certificates, through both a cronjob installed into cron.d and a systemd
> timer.

Does this mean that users who currently won't get auto-renewed will
start getting auto-renewed after this proposed update? If so, will that
also include users who currently have expired (languishing) old
certificates?

All,

Is there any progress on the preparation of the update itself? I hope
you're not waiting on my approval. To be clear, my opinion is that
we should update letsencrypt/certbot in 16.04, and I appreciate all the
interest in helping us do this; it's just a matter of the details
(documenting, understanding and mitigating any behavioural changes as
appropriate, and agreeing on the QA plan).

Robie

> Does this mean that users who currently won't get auto-renewed will
> start getting auto-renewed after this proposed update? If so, will that
> also include users who currently have expired (languishing) old
> certificates?

Yes. Each certificate (or more precisely, each lineage of certificates, where a lineage is a series of certificates that replace each other with new validity dates and possibly new domains added) gets a renewal configuration file in /etc/letsencrypt/renewal/ ; the "certbot renew" command walks through those and tries to renew any that are within 30 days of expiry. The Debian packages run that task twice a day out of the box.

I think we've concluded that we'll add a note-upon-installation telling the sys admin that that's going to start happening, and point to where it can be turned off or tweaked.

On Mon, Nov 28, 2016 at 09:22:28PM -0000, Peter Eckersley wrote:
> I think we've concluded that we'll add a note-upon-installation telling
> the sys admin that that's going to start happening, and point to where
> it can be turned off or tweaked.

I'm afraid that users who automatically update (most of them in 16.04
since it's default) will never see such a message.

While you do this for v16.04, could you also publish certbot v0.9.x for v16.10 at the same time? Or need a new ticket for that?

Hello everyone.

I'm not sure exactly what form you wanted the SRU in, but... I've included a tarball of the source changes files and dsc's, signed, along with the files included in them.

This upload has two changes from the version in sid. First, I tweaked the versions in python-certbot to reduce the dependency on pyopenssl. The version requirement was to work around an undeclared Breaks relationship between a version of pyopenssl and python-cryptography in a later version (16.0.0) than is in Xenial.

Second, I added a new dummy package to assist the upgrade of python-letsencrypt-apache => python-certbot-apache. The dummy package was never used in Debian, as letsencrypt didn't make it to stable before the name-change occurred.

Please let me know if you have any questions, or if you would like the SRU in some other manner.

Robie Basak (racb) wrote :

The format sounds fine, thanks. We need one uploader and SRU team member to review. I suggest we figure out who is doing what and both people review before uploading to avoid confusion.

Since you mentioned that RAOF could sponsor, I subscribed him to the bug. Since both of us are on the SRU team, we should figure out who will wear which hat. RAOF, any comments?

>>> import random
>>> x = ["RAOF", "rbasak"]
>>> random.shuffle(x)
>>> print "{0} should upload and {1} should review".format(x)
RAOF should upload and rbasak should review

Peter Eckersley (pde-lists) wrote :

.format(*x), sorry :)

Will it be reasonable to wait for certbot 0.10.0 (due on Jan 4, according to https://github.com/certbot/certbot/milestones), and go ahead then?

Peter Eckersley (pde-lists) wrote :

I don't think so. We would want 0.10.0 to have at least several weeks of field testing before stable distributions include it as an update, and any regressions in that release would mean we do 0.10.x bugfix releases that would stretch the timeline further.

Those are good arguments, thanks for explaining!

Changed in python-letsencrypt (Ubuntu):
status: Confirmed → Fix Released
Changed in python-acme (Ubuntu):
status: New → Fix Released
Changed in python-letsencrypt-apache (Ubuntu):
status: New → Fix Released
Chris Halse Rogers (raof) wrote :

Hm. It was unclear to me that this requires updating python-acme, python library expected to be used by 3rd parties.

Have any python-acme APIs changed? What are the chances that the update will break downstream users of python-acme?

Changed in python-certbot-nginx (Ubuntu):
status: New → Fix Released
Peter Eckersley (pde-lists) wrote :

The changes in python-acme were comparatively minor. A few bugs were fixed; one new feature was added to the API (support for DNS-01 challenges); some protocol messages were removed (because they were believed to have security problems, were never used by Let's Encrypt, and were removed from the IETF ACME draft).

None of those changes should negatively impact non-Certbot ACME clients that use python-acme. It also does not appear that any of those ACME clients are packaged in Debian unstable:

apt-cache rdepends python-acme
python-acme
Reverse Depends:
  python-certbot
  python-certbot-nginx
  python-certbot-apache

It's the clients that are *unpackaged* that are the most concerning. Packaged clients we can update if necessary; random ad-hoc clients not so much 😀

* nod. We haven't changed any of the preexisting API calls or semantics, so even unpackaged clients should be fine.

Packages uploaded to the unapproved queue, awaiting review.

I changed the version numbers (so that the version in Xenial < Yakkety < Zesty), and added bug links.

I'm confused on a couple of points. Here's my understanding:

AFAICT, there are three source packages in play here: src:python-acme, src:python-letsencrypt, and src:python-certbot.

src:python-acme appears relatively straightforward. A version bump is needed for new features used by this proposed update, but there is no other package renaming going on.

src:python-certbot supersedes src:python-letsencrypt, though of course apt doesn't see that.

src:python-letsencrypt created binaries letsencrypt, python-letsencrypt and python-letsencrypt-doc in Xenial. It doesn't exist in Yakkety or Zesty.

src:python-certbot creates binaries letsencrypt (transitional to certbot), certbot, python-certbot and python-certbot-doc from Yakkety onwards, and doesn't currently exist in Xenial.

The proposed backport of src:python-certbot to Xenial makes some backport-related dependency changes (so it seems) but I don't see anything that changes the structure of package names or transitional packages from what is in sid (and Yakkety, etc).

So to my questions:

1) What happens to a Xenial user who is using the library provided by python-letsencrypt after this update? Is it intentional that src:python-letsencrypt in Xenial will continue to provide it?

2) This is more of a question for an archive admin. Is it acceptable in Xenial for src:python-certbot to take over the letsencrypt binary name, but for us to retain src:python-letsencrypt that also produces it?

3) What happens if a future update (say a security update) is needed against src:python-letsencrypt for users still using the python-letsencrypt binary package in Xenial (from question 1)? This update would still produce the letsencrypt binary, but we'd have a higher version from src:python-certbot in the archive. So would the subsequent binary upload fail?

Does this mean that we also need to SRU src:python-letsencrypt at the same time to stop producing the letsencrypt binary package at the same time?

This is for Xenial only. I haven't considered Yakkety yet, nor have I completed my review since I want to understand and fix this point before considering any other further interactions (which may change if we change how we're doing this here).

Robie Basak (racb) wrote :

Based on https://irclogs.ubuntu.com/2017/01/12/%23ubuntu-release.html#t13:47, can you upload a python-letsencrypt for Xenial that drops the letsencrypt package please? We'll need to accept these all together.

Please remember to run the update-maintainer field when preparing these source packages. See https://wiki.ubuntu.com/DebianMaintainerField for the background. I think this needs doing on some of the existing uploads in the queue also, but let's wait to see if anything else needs fixing.

Peter Eckersley (pde-lists) wrote :

I think it's my job to gently nudge whoever it is that's making the slightly tweaked packages here, but I'm not sure whether it should be Chris or Harlan... if all else is equal perhaps it should be Chris, since we also have a new 0.10.1 release that needs to be packaged for sid and that would be in Harlan's wheelhouse.

Robie Basak (racb) wrote :

Any news? I feel that changing python-letsencrypt as well exceeds the bounds of the SRU hat I'm wearing - it should have review from a different uploader as well.

Sorry; LCA and then email backlog. I'll upload new packages tomorrow.

That's great Chris!

We had some discussions yesterday about whether this has taken long enough that 0.10.x should be considered for the SRU instead. 0.10.1 has been in the field for 13 days; people found a couple of bugs in it, but they only affected new 0.10.x functionality rather than being regressions.

0.10.2 was released yesterday to fix those things and Harlan is working to get it into Debian in time to make the stretch freeze 11 days from now.

Our conclusion was the best thing to do is probably make the 0.9.3 SRU now, and start work on a 0.10.2+ SRU in around 3 weeks, once the 0.10.2 debian packages have received some testing and we've had time to sweep for any final bugs.

Robie Basak (racb) wrote :

Can I suggest that you maintain these packages in a git tree somewhere (if you aren't doing so already), create branches for the Ubuntu stable releases you're targeting, and then get reviews straight from there? I'll be happy to do an SRU review straight from a git branch. Then review changes could get committed there presumably by more people, so we could move faster. Chris will just need to review new commits that land since his last review, and upload when everyone is happy. And I can allow through any upload after just verifying that it is identical to the thing I already reviewed and accepted in git.

It'll save confusion with multiple versions in the queue this way, too.

Peter Eckersley (pde-lists) wrote :

Harlan and the other Debian developers have git trees here:

https://alioth.debian.org/plugins/scmgit/cgi-bin/gitweb.cgi?a=project_list;pf=letsencrypt

Maybe Chris should also get access to those repos?

Chris Halse Rogers (raof) wrote :

python-letsencrypt dropping the letsencrypt binary uploaded.

If you'd like me to have access to the git repos you'll need to add raof-guest to the letsencrypt group (or, alternatively, make the git repository accessible to collab-maint).

Robie Basak (racb) wrote :

python-letsencrypt and python-letsencrypt-apache don't exist in Yakkety or Zesty, so let's mark those Invalid as there is no action to take for these packages neither in Yakkety nor Zesty (only Xenial).

Changed in python-letsencrypt-apache (Ubuntu):
status: Fix Released → Invalid
Changed in python-letsencrypt-apache (Ubuntu Yakkety):
status: New → Invalid
Changed in python-letsencrypt (Ubuntu Yakkety):
status: New → Invalid
Changed in python-letsencrypt (Ubuntu):
status: Fix Released → Invalid
Robie Basak (racb) wrote :

Setting all tasks for which I see an upload in the queue as "In Progress" so I can more easily see what's what. Here's my current view of the queues:

python-acme
queue/xenial/unapproved/cb94bae
queue/yakkety/unapproved/15d7282

python-certbot
queue/xenial/new/e67c69a
queue/yakkety/unapproved/c6e7c0b

python-certbot-apache
queue/xenial/new/a42f913
queue/yakkety/unapproved/b496663

python-certbot-nginx
queue/xenial/new/b15997a
queue/yakkety/new/480f5d3

python-letsencrypt
queue/xenial/unapproved/21d1a2f

python-letsencrypt-apache
[empty]

Changed in python-acme (Ubuntu Xenial):
status: New → In Progress
Changed in python-acme (Ubuntu Yakkety):
status: New → In Progress
Changed in python-certbot (Ubuntu Xenial):
status: New → In Progress
Changed in python-certbot-apache (Ubuntu Xenial):
status: New → In Progress
Changed in python-certbot-apache (Ubuntu Yakkety):
status: New → In Progress
Changed in python-certbot-nginx (Ubuntu Xenial):
status: New → In Progress
Changed in python-certbot-nginx (Ubuntu Yakkety):
status: New → In Progress
Changed in python-letsencrypt (Ubuntu Xenial):
status: New → In Progress
Robie Basak (racb) wrote :

No changes needed to python-letsencrypt-apache: this will be left as-is in Xenial, will use python-letsencrypt which is only being changed to drop the letsencrypt package for the transition (libraries stay all the same).

Changed in python-certbot (Ubuntu Yakkety):
status: New → In Progress
Changed in python-certbot (Ubuntu):
status: New → Fix Released
Changed in python-certbot-apache (Ubuntu):
status: New → Fix Released
Changed in python-letsencrypt-apache (Ubuntu Xenial):
status: New → Invalid
Robie Basak (racb) wrote :

I've verified all the versions of the queue assuming we are backporting in SRUs <= 0.10.2-1 of python-{acme,certbot{,-apache,-nginx}} only and leaving behind python-letsencrypt and python-letsencrypt-apache. So let's retitle for exactly this.

summary: - letsencrypt 0.4.1 contains numerous bugs fixed upstream
+ [SRU] Backport letsencrypt 0.9.3

I spent some time on reviewing this on Friday, but I got interrupted. There was also some discussion on IRC (https://irclogs.ubuntu.com/2017/01/27/%23ubuntu-release.html#t12:31)

My notes:

Can we put a note that there will be a new cronjob active now in debian/NEWS and debian/changelog as suggested on IRC?

What happens if user has installed letsencrypt outside of packaging (for example, to get a newer version while leaving the system one installed)? Will adding a cronjob in the SRU cause any issues?

Can we nominate a person or a team to handle bugs as a consequence of this SRU, and subscribe to package bugs to make sure?

update-maintainer needs running (but I can do this from the queue if no other changes are needed).

Changed in python-certbot-apache (Ubuntu Yakkety):
status: In Progress → Incomplete
Changed in python-certbot (Ubuntu Yakkety):
status: In Progress → Incomplete
Changed in python-acme (Ubuntu Yakkety):
status: In Progress → Incomplete
Peter Eckersley (pde-lists) wrote :

This has been stuck for a while, I suspect because it hasn't been clearly on anyone's plate :(.

Let's fix that:

* Brad Warren on the Certbot team is going to construct a retrospective changelog.txt, and post a link here.
* RAOF should probably revise the packages to include that and the news file.
* Anything else?

Separately, this has taken so long that it might be advisable to switch to an 0.10.2 SRU, since that version is now well-tested, and is also the one that's frozen into Debian Stretch. Thoughts for and against? What extra steps would we need for an 0.10.2 SRU?

Robie Basak (racb) wrote :

I'd like to suggest again that everyone involved does all packaging changes in git, and gets all approvals (from a suitable Ubuntu uploader and the Ubuntu SRU team) in git, before uploading. I really think this will speed things up.

I'm not sure I see everything in https://alioth.debian.org/plugins/scmgit/cgi-bin/gitweb.cgi?pf=letsencrypt, though that may be because repositories have not been renamed following source package renames.

I'm not too bothered about who can push where. Since git is nicely distributed, as long as we can all get to the "current" branch, it shouldn't matter as long as we all agree on where that "current" branch is (as it changes).

RAOF, if you're struggling for time, let me know and I'll see if a colleague on the server team is available to help (since I can only wear the one hat).

summary: - [SRU] Backport letsencrypt 0.9.3
+ [SRU] Backport letsencrypt 0.14.1
Changed in python-acme (Ubuntu Zesty):
status: New → Fix Committed
tags: added: verification-needed verification-needed-zesty
Changed in python-acme (Ubuntu Yakkety):
status: Incomplete → Won't Fix
Changed in python-certbot (Ubuntu Yakkety):
status: Incomplete → Won't Fix
Changed in python-certbot (Ubuntu Zesty):
status: New → Fix Committed
Changed in python-certbot-nginx (Ubuntu Yakkety):
status: In Progress → Won't Fix
Changed in python-certbot-nginx (Ubuntu Zesty):
status: New → Fix Committed
Changed in python-certbot-apache (Ubuntu Zesty):
status: New → Fix Committed
Changed in python-certbot-apache (Ubuntu Yakkety):
status: Incomplete → Won't Fix
summary: - [SRU] Backport letsencrypt 0.14.1
+ [SRU] Backport letsencrypt 0.14.2
Changed in python-acme (Ubuntu Xenial):
status: In Progress → Fix Committed
tags: added: verification-needed-xenial
33 comments hidden view all 113 comments

Hello Peter, or anyone else affected,

Accepted python-certbot into xenial-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/python-certbot/0.14.2-0ubuntu0.16.04.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed.Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-xenial to verification-done-xenial. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-xenial. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Changed in python-certbot-apache (Ubuntu Xenial):
status: In Progress → Fix Committed
Changed in python-certbot (Ubuntu Xenial):
status: In Progress → Fix Committed
Changed in python-certbot-nginx (Ubuntu Xenial):
status: In Progress → Fix Committed
Steve Langasek (vorlon) wrote :

Hello Peter, or anyone else affected,

Accepted python-certbot-nginx into xenial-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/python-certbot-nginx/0.14.2-0ubuntu0.16.04.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed.Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-xenial to verification-done-xenial. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-xenial. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Robie Basak (racb) wrote :

Hello Peter, or anyone else affected,

Accepted python-letsencrypt into xenial-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/python-letsencrypt/0.4.1-1ubuntu0.16.04.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed.Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-xenial to verification-done-xenial. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-xenial. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Changed in python-letsencrypt (Ubuntu Xenial):
status: In Progress → Fix Committed
Changed in python-letsencrypt-apache (Ubuntu Xenial):
status: Invalid → Fix Committed
no longer affects: python-letsencrypt (Ubuntu Yakkety)
no longer affects: python-letsencrypt (Ubuntu)
no longer affects: python-letsencrypt-apache (Ubuntu)
no longer affects: python-letsencrypt-apache (Ubuntu Yakkety)

Unfortunately the test script at https://wiki.ubuntu.com/StableReleaseUpdates/Certbot/TestScript cannot be used to test the Xenial packages yet because the python-certbot-nginx binary still needs to be accepted. See https://launchpad.net/ubuntu/xenial/+queue.

Is someone able to approve this binary?

Brad Warren (bradmwarren) wrote :

Thanks for approving the binary.

Tests passed for the proposed Xenial packages using https://wiki.ubuntu.com/StableReleaseUpdates/Certbot/TestScript.

Brad Warren (bradmwarren) wrote :

While I didn't look any more into the build failure for the letsencrypt package (let me know if you'd like me to), I added a check to https://wiki.ubuntu.com/StableReleaseUpdates/Certbot/TestScript testing that a symlink is properly created in the user's PATH. The test passes on the letsencrypt package in xenial-proposed.

Also after talking to racb in IRC last week, I think we should include a python-letsencrypt package in Xenial similar to the one we have at https://pypi.python.org/pypi/letsencrypt. This allows other Python code to continue to import Certbot through the name letsencrypt.

While the majority of letsencrypt/certbot's internals do not have stable interfaces for other packages to use, there is a plugin interface, base classes, and utilities that are used by Certbot's third party plugins listed at https://certbot.eff.org/docs/using.html#third-party-plugins.

Doing this for python-letsencrypt-apache might also be a good idea so users can continue to install Certbot and our Apache plugin through this package name. This could also allow Python code to import the plugin through the name "letsencrypt_apache", however, the plugin is not expected to be used this way by third parties.

If we do this for the Apache plugin as well, our Python wrapper is available at https://pypi.python.org/pypi/letsencrypt-apache.

Jeremy Bicha (jbicha) wrote :

Should this update just be let into Xenial now?

Someone needs to start yet another backport of certbot for LP: #1745227.

Jeremy Bicha (jbicha) wrote :

Oh, I see the Xenial update was blocked by build failures.

Łukasz Zemczak (sil2100) wrote :

Please someone look into the build failures in the nearest days. If there will be no movement on these packages I will be removing them from xenial-proposed and marking the bug as Won't Fix (as per SRU policy).

Brad Warren (bradmwarren) wrote :

As a developer of letsencrypt/certbot, it'll be pretty disappointing to us if we can't some kind of update here. In addition to the many bugs listed in the original description that would be fixed by this SRU, the python-letsencrypt-apache package has been unable to obtain new certificates since January due to https://bugs.launchpad.net/ubuntu/+source/python-certbot/+bug/1745227.

Łukasz Zemczak (sil2100) wrote :

I agree it would be a shame, but we need someone to sort out the build failures and make sure the packages are in proper shape. Robie, since you were the original backporter, would you be able to drive this forward? The point release is nearing and I'd prefer not to have stale packages in -proposed that I don't intend to release for the point-release.

Kraut.Hosting (kraut.hosting) wrote :

This is still a valid issue for everyone want to do a proper QA chain in software delivery. Both the recent Snap package from Robie as the certbot PPA by our Debian heroes have the problem on different install times you have different versions installed. On a big bunch of boxes this gives us the pain ensuring QA of new versions before roll out to production. Doing cheap cherry picking from the certbot PPA in the past i just gave the packages from proposed a rebuilt try.

Could at first not reproduce the build failure in my pbuilder chain of crap with the rebuilt order of python-letsencrypt, python-acme, python-certbot. But when rebuilding python-letsencrypt then with python-acme_0.14.2-0ubuntu0.16.04.1 as an Build-Depends (without an updated version require) i hit it as well:

   dh_auto_test -O--buildsystem=pybuild
I: pybuild base:184: python2.7 setup.py test
running test
Searching for acme==0.4.1

Note: Bypassing https://pypi.python.org/simple/acme/ (disallowed host; see http://bit.ly/1dg9ijs for details).

Couldn't find index page for 'acme' (maybe misspelled?)
Scanning index of all packages (this may take a while)

Note: Bypassing https://pypi.python.org/simple/ (disallowed host; see http://bit.ly/1dg9ijs for details).

No local packages or download links found for acme==0.4.1
error: Could not find suitable distribution for Requirement.parse('acme==0.4.1')
E: pybuild pybuild:274: test: plugin distutils failed with: exit code=1: python2.7 setup.py test
dh_auto_test: pybuild --test -i python{version} -p 2.7 --dir . returned exit code 13
debian/rules:8: recipe for target 'build' failed
make: *** [build] Error 25

Just like in: https://launchpadlibrarian.net/335455860/buildlog_ubuntu-xenial-amd64.python-letsencrypt_0.4.1-1ubuntu0.16.04.1_BUILDING.txt.gz

Unfortunately i am not too deep into Python here for fixing the fix easily. But as this build failure is the blocker for an SRU i hope someone of the involved rock stars here might figured out easy how to fix it.

Kraut.Hosting (kraut.hosting) wrote :

FYI pushed some backporting from bionic to xenial without touching existing Python dependencies:

https://launchpad.net/~kraut.hosting/+archive/ubuntu/certbot/+packages

Would recommend to rebase this SRU effort on the packages we have in bionic now anyway.

So far my push does only cover Certbot it self but not all the plugins and integrations.

Feedback on this effort is welcome and i am open for some more polishing ;)

Brad Warren (bradmwarren) wrote :

Unless we want this package to suddenly break for approximately 10,000 users in February, I think we probably want to do an SRU to Certbot version 0.21.1 or higher. I wrote more about the problem in the relevant issue at https://bugs.launchpad.net/ubuntu/+source/python-letsencrypt/+bug/1745126.

While we may not want to do regular SRUs for Certbot due to the amount of work involved, I think it probably makes sense to do one now due to breaking changes server side.

Please let me know if there's anything I can do on my end to help get this resolved.

tags: added: upgrade-software-version
no longer affects: python-acme (Ubuntu Yakkety)
no longer affects: python-acme (Ubuntu Zesty)
Changed in python-acme (Ubuntu Xenial):
assignee: nobody → Michael Casadevall (mcasadevall)
status: Fix Committed → In Progress
no longer affects: python-certbot (Ubuntu Yakkety)
no longer affects: python-certbot (Ubuntu Zesty)
Changed in python-certbot (Ubuntu Xenial):
assignee: nobody → Michael Casadevall (mcasadevall)
status: Fix Committed → In Progress
no longer affects: python-certbot-apache (Ubuntu Yakkety)
no longer affects: python-certbot-apache (Ubuntu Zesty)
Changed in python-certbot-apache (Ubuntu Xenial):
assignee: nobody → Michael Casadevall (mcasadevall)
status: Fix Committed → In Progress
no longer affects: python-certbot-nginx (Ubuntu Yakkety)
no longer affects: python-certbot-nginx (Ubuntu Zesty)
Changed in python-certbot-nginx (Ubuntu Xenial):
assignee: nobody → Michael Casadevall (mcasadevall)
importance: Undecided → High
milestone: none → xenial-updates
status: Fix Committed → In Progress
Changed in python-letsencrypt (Ubuntu Xenial):
assignee: nobody → Michael Casadevall (mcasadevall)
importance: Undecided → High
milestone: none → xenial-updates
status: Fix Committed → In Progress
Changed in python-letsencrypt-apache (Ubuntu Xenial):
importance: Undecided → High
milestone: none → xenial-updates
status: Fix Committed → In Progress
Changed in python-certbot-apache (Ubuntu Xenial):
importance: Undecided → High
Changed in python-certbot (Ubuntu Xenial):
importance: Undecided → High
Changed in python-acme (Ubuntu Xenial):
importance: Undecided → High
summary: - [SRU] Backport letsencrypt 0.14.2
+ [SRU] Backport letsencrypt from bionic

Did some major cleanup of this bug. Debdiffs attached for review.

In general, the primary change for the backport was to modify the package to build against Python 2 instead of Python 3; one code change was needed in certbot to disable API documentation as the necessary sphinx module is not available. We're still sorting out the letsencrypt compat shims but I wanted to get these debdiffs reviewed as they are final or very close to.

tags: removed: verification-needed-zesty
Changed in python-letsencrypt-apache (Ubuntu Xenial):
assignee: nobody → Michael Casadevall (mcasadevall)
Changed in python-certbot-apache (Ubuntu Xenial):
milestone: none → xenial-updates
Changed in python-certbot (Ubuntu Xenial):
milestone: none → xenial-updates
Changed in python-acme (Ubuntu Xenial):
milestone: none → xenial-updates

These patches handle straight backports; and are debdiffs from the bionic versions currently being shipped. For the letsencrypt shims, these should be handled as NEW packages and not as a debdiff as they won't exist in bionic and only exist for compat reasons. We're resolving some final issues but this should let Robbie to start looking at it.

Breakdown of josepy for SRU:

josepy was split from python-acme, and is a NEW package going into Trusty. This is a straight backport of the bionic packaging.

--- python-josepy-1.1.0/debian/compat 2018-04-15 20:45:24.000000000 +0000
+++ python-josepy-1.1.0/debian/compat 2018-11-23 19:55:13.000000000 +0000
@@ -1 +1 @@
-11
+9
diff -Nru python-josepy-1.1.0/debian/control python-josepy-1.1.0/debian/control
--- python-josepy-1.1.0/debian/control 2018-04-15 20:45:24.000000000 +0000
+++ python-josepy-1.1.0/debian/control 2018-11-23 19:55:13.000000000 +0000
@@ -1,9 +1,10 @@
 Source: python-josepy
 Section: python
 Priority: optional
-Maintainer: Debian Let's Encrypt <email address hidden>
+Maintainer: Ubuntu Developers <email address hidden>
+XSBC-Original-Maintainer: Debian Let's Encrypt <email address hidden>
 Uploaders: Harlan Lieberman-Berg <email address hidden>
-Build-Depends: debhelper (>= 11~),
+Build-Depends: debhelper (>= 9~),
         dh-python,
         python (>= 2.7),
         python-cryptography (>= 0.8),
diff -Nru python-josepy-1.1.0/debian/rules python-josepy-1.1.0/debian/rules

Explanation: Debhelper 9 was the shipped version in trusty so changes to dependencies to work with it were required.

--- python-josepy-1.1.0/debian/rules 2018-04-15 20:45:24.000000000 +0000
+++ python-josepy-1.1.0/debian/rules 2018-11-23 19:55:13.000000000 +0000
@@ -17,6 +17,6 @@
  find $(CURDIR)/debian/ -type d -name testdata -print0 | xargs -0 rm -rf '{}' \;
  rm -f $(CURDIR)/debian/python*-josepy/usr/bin/jws

-override_dh_installdocs:
- dh_installdocs --doc-main-package=python-josepy-doc -p python-josepy-doc
- dh_installdocs -p python-josepy -p python3-josepy
+#override_dh_installdocs:
+# dh_installdocs --doc-main-package=python-josepy-doc -p python-josepy-doc
+# dh_installdocs -p python-josepy -p python3-josepy

Explanation: installdocs doesn't support -p on Trusty which is used to create symlinks. This behavior mimics the way the docs packages were installed on Xenial for the existing packages; documentation still ends up properly in the -doc package (checked manually).

Breakdown of changes for python ACME; I caught a mistake in the previous debdiff so reuploading a revised one with this comment:

diff -Nru python-acme-0.22.2/debian/compat python-acme-0.22.2/debian/compat
--- python-acme-0.22.2/debian/compat 2018-03-17 15:24:35.000000000 +0000
+++ python-acme-0.22.2/debian/compat 2018-11-25 15:09:41.000000000 +0000
@@ -1 +1 @@
-11
+9

Explaination: debhelper version change

diff -Nru python-acme-0.22.2/debian/control python-acme-0.22.2/debian/control
--- python-acme-0.22.2/debian/control 2018-03-17 15:24:35.000000000 +0000
+++ python-acme-0.22.2/debian/control 2018-11-25 15:09:41.000000000 +0000
@@ -1,13 +1,14 @@
 Source: python-acme
 Section: python
 Priority: optional
-Maintainer: Debian Let's Encrypt <email address hidden>
+Maintainer: Ubuntu Developers <email address hidden>
+XSBC-Original-Maintainer: Debian Let's Encrypt <email address hidden>
 Uploaders: Harlan Lieberman-Berg <email address hidden>,
            Francois Marier <email address hidden>
-Build-Depends: debhelper (>= 11~),
+Build-Depends: debhelper (>= 9~),
                dh-python,
                python-all (>= 2.7),
- python-cryptography (>= 1.3.4),
+ python-cryptography,
                python-docutils,
                python-josepy,
                python-mock,

Explanation: This was an incorrect dependency in bionic; acme works proeprly against cryotpgraphy 1.2 which was shipped in Xenial.

@@ -19,7 +20,7 @@
                python-six (>= 1.9),
                python-tz,
                python3 (>= 3.4),
- python3-cryptography (>= 1.3.4),
+ python3-cryptography,
                python3-docutils,
                python3-josepy,
                python3-mock,

Explanation: See above.

diff -Nru python-acme-0.22.2/debian/rules python-acme-0.22.2/debian/rules
--- python-acme-0.22.2/debian/rules 2018-03-17 15:24:35.000000000 +0000
+++ python-acme-0.22.2/debian/rules 2018-11-25 15:09:30.000000000 +0000
@@ -7,11 +7,10 @@

 override_dh_auto_build:
  dh_auto_build
- PYTHONPATH=. \
- http_proxy='127.0.0.1:9' \
- https_proxy='127.0.0.1:9' \
- sphinx-build -N -bhtml docs/ build/html
+ PYTHONPATH=. http_proxy='127.0.0.1:9' sphinx-build -N -bhtml docs/ build/html

 override_dh_auto_install:
  dh_auto_install
  find $(CURDIR)/debian/ -type d -name testdata -print0 | xargs -0 rm -rf '{}' \;

Explaination: I reused the rules from the existing python-acme version in Trusty, dropping https_proxy. This may not be necessary TBH.

Download full text (10.8 KiB)

Breakdown of python-certbot changes; this is a NEW package going into trusty backported from bionic following the letsencrypt->certbot name change. A revised debdiff is attached to this bug.

diff -Nru python-certbot-0.23.0/debian/compat python-certbot-0.23.0/debian/compat
--- python-certbot-0.23.0/debian/compat 2018-03-23 23:00:13.000000000 +0000
+++ python-certbot-0.23.0/debian/compat 2018-11-25 16:20:44.000000000 +0000
@@ -1 +1 @@
-11
+9

Explanation: Debhelper compat change

diff -Nru python-certbot-0.23.0/debian/control python-certbot-0.23.0/debian/control
--- python-certbot-0.23.0/debian/control 2018-04-07 03:21:05.000000000 +0000
+++ python-certbot-0.23.0/debian/control 2018-11-25 16:20:44.000000000 +0000
@@ -1,47 +1,41 @@
 Source: python-certbot
 Section: python
 Priority: optional
-Maintainer: Debian Let's Encrypt <email address hidden>
+Maintainer: Ubuntu Developers <email address hidden>
+XSBC-Original-Maintainer: Debian Let's Encrypt <email address hidden>
 Uploaders: Harlan Lieberman-Berg <email address hidden>,
            Francois Marier <email address hidden>
-Build-Depends: debhelper (>= 11~),
+Build-Depends: debhelper (>= 9~),
                dh-python,
- python3,
- python3-acme (>= 0.22.0~),
- python3-configargparse (>= 0.10.0),
- python3-configobj,
- python3-cryptography (>= 1.2),
- python3-distutils,
- python3-josepy,
- python3-mock,
- python3-parsedatetime (>= 1.3),
- python3-repoze.sphinx.autointerface,
- python3-requests (>= 2.4.3),
- python3-rfc3339,
- python3-setuptools (>= 1.0),
- python3-sphinx,
- python3-sphinx-rtd-theme,
- python3-tz,
- python3-zope.component,
- python3-zope.interface
+ python,
+ python-acme (>= 0.22.0~),
+ python-configargparse (>= 0.10.0),
+ python-configobj,
+ python-cryptography (>= 1.2),
+ python-josepy,
+ python-mock,
+ python-parsedatetime (>= 1.3),
+ python-requests (>= 2.4.3),
+ python-rfc3339,
+ python-setuptools (>= 1.0),
+ python-sphinx,
+ python-sphinx-rtd-theme,
+ python-tz,
+ python-zope.component,
+ python-zope.interface
 Standards-Version: 4.1.4
 Homepage: https://certbot.eff.org/
 Vcs-Git: https://salsa.debian.org/letsencrypt-team/certbot/certbot.git
 Vcs-Browser: https://salsa.debian.org/letsencrypt-team/certbot/certbot
 Testsuite: autopkgtest-pkg-python

Explanation: Some of the dependencies for certbot do not have Python3 versions in Xenial; this was resolved by having certbot use Python2 and becoming a python2 package; it could also have been resolved by SRUing python-parsedtimedate as it supports Python 3 but doesn't have the necessary deb. An SRU for the bionic package may be needed to remove this with the Python 3 version.

-Package: python3-certbot
+Package: python-...

After testing and further work, a revised python-acme debdiff is attached. This includes one additional backport that allows josepy to be imported through the acme package; this change has already been integrated upstream, and is required to allow packages that assume acme.jose still existed as part of the python-acme module.

Download full text (4.0 KiB)

This is the backported patch to python-acme, as stated before, this is a direct backport from upstream to handle compat issues, and extends the unittests to cover the new backworks compatibility area.

mcasadevall@lighthouse:~/src/le/sru/python-acme-0.22.2/debian/patches$ cat fix-jose-import
Description: Allow josepy to be imported via acme.jose
 This is a backwards compatibility fixed taken from upstream
 from the following commits:
 https://github.com/certbot/certbot/commit/e3cb782e5992ba306de59ba96dfb6f125720cd06.patch
 https://github.com/certbot/certbot/commit/ec297ccf72e95961586ec2382c3e3225ce578aa4.patch
 .
 python-acme (0.22.2-1ubuntu0.16.04.1~ppa4) xenial; urgency=medium
 .
   * Backport to Xenial for LE change (LP: #1640978)
Author: Michael Casadevall <email address hidden>
Bug-Ubuntu: https://bugs.launchpad.net/bugs/1640978

---
The information above should follow the Patch Tagging Guidelines, please
checkout http://dep.debian.net/deps/dep3/ to learn about the format. Here
are templates for supplementary fields that you might want to add:

Origin: upstream
Bug: https://github.com/certbot/certbot/pull/6592
Forwarded: not-needed
Reviewed-By: Michael Casadevall <email address hidden>
Last-Update: 2019-01-11

--- python-acme-0.22.2.orig/acme/__init__.py
+++ python-acme-0.22.2/acme/__init__.py
@@ -10,3 +10,18 @@ supported version: `draft-ietf-acme-01`_
   https://github.com/ietf-wg-acme/acme/tree/draft-ietf-acme-acme-01

 """
+import sys
+
+# This code exists to keep backwards compatibility with people using acme.jose
+# before it became the standalone josepy package.
+#
+# It is based on
+# https://github.com/requests/requests/blob/1278ecdf71a312dc2268f3bfc0aabfab3c006dcf/requests/packages.py
+
+import josepy as jose
+
+for mod in list(sys.modules):
+ # This traversal is apparently necessary such that the identities are
+ # preserved (acme.jose.* is josepy.*)
+ if mod == 'josepy' or mod.startswith('josepy.'):
+ sys.modules['acme.' + mod.replace('josepy', 'jose', 1)] = sys.modules[mod]
--- /dev/null
+++ python-acme-0.22.2/acme/jose_test.py
@@ -0,0 +1,53 @@
+"""Tests for acme.jose shim."""
+import importlib
+import unittest
+
+class JoseTest(unittest.TestCase):
+ """Tests for acme.jose shim."""
+
+ def _test_it(self, submodule, attribute):
+ if submodule:
+ acme_jose_path = 'acme.jose.' + submodule
+ josepy_path = 'josepy.' + submodule
+ else:
+ acme_jose_path = 'acme.jose'
+ josepy_path = 'josepy'
+ acme_jose_mod = importlib.import_module(acme_jose_path)
+ josepy_mod = importlib.import_module(josepy_path)
+
+ self.assertIs(acme_jose_mod, josepy_mod)
+ self.assertIs(getattr(acme_jose_mod, attribute), getattr(josepy_mod, attribute))
+
+ # We use the imports below with eval, but pylint doesn't
+ # understand that.
+ # pylint: disable=eval-used,unused-variable
+ import acme
+ import josepy
+ acme_jose_mod = eval(acme_jose_path)
+ josepy_mod = eval(josepy_path)
+ self.assertIs(acme_jose_mod, josepy_mod)
+ self.assertIs(getattr(acme_jose_mod, attribute), getattr(josepy_...

Read more...

Fabien COMBERNOUS (fc.) wrote :

when will new package arrive on the repositories ?

This update is important since the TLS-SNI-01 validation option is soon turned off by LE.

TLS-SNI-01 validation is reaching end-of-life. It will stop working
temporarily on February 13th, 2019, and permanently on March 13th, 2019.

Changed in python-acme (Ubuntu):
importance: Undecided → High
Changed in python-certbot (Ubuntu):
importance: Undecided → High
Changed in python-certbot-apache (Ubuntu):
importance: Undecided → High
Changed in python-certbot-nginx (Ubuntu):
importance: Undecided → High
tags: added: patch
Robie Basak (racb) wrote :

I'm reviewing this now.

Michael, did you miss the fix from Bionic uploaded in 0.22.2-1ubuntu0.1?

  * Add ready status type to be compatible with the new Let's Encrypt ACMEv2
    endpoint (LP: #1777205).

From the perspective of current Bionic it looks like this is getting dropped in the backport, which I think is by accident?

Though I'd ask now, before I've finished my review (this is taking a while!), to speed things up if we find a change is needed.

Robie Basak (racb) wrote :

This is on python-acme, sorry.

Robie Basak (racb) on 2019-02-07
description: updated
Robie Basak (racb) wrote :

python-certbot backport review:

The changes you've described look good in general, but I have some specific questions for pieces for which I couldn't find any explanation:

python-certbot no longer Recommends certbot, but python3-certbot did. Is this intentional?

In debian/rules, what's the reason for including pkg-info.mk into the backport as a delta? I don't see the variables it defines being used anywhere.

What's the reason for the new clean target in debian/rules?

Why is certbot.timer's installation being removed? Has auto renewal been tested? If so, what's the mechanism it uses now?

Why remove the installation of the cli.ini that disables certbot-internal log rotation?

Some notes to self explaining further bits of the delta I did manage to understand:

python3-distutils build-dep drop not explained but distutils is integrated in Python 2 in Xenial and not in its own packaging so dropping the build dependency is presumably intentional.

The dropping of the python3-repoze.sphinx.autointerface build dependency is explained later with the disable-autointerface-docs patch.

Robie Basak (racb) wrote :

python-letsencrypt backport review:

I think debian/README.source is now redundant and incorrect and should be removed?

I see that pkg-info.mk was included from Debian previously. Is this still necessary?

debian/rules arranges debian/letsencrypt/usr/bin/* but no longer ships a binary package called letsencrypt.

python-letsencrypt-apache backport review:

I think debian/README.source is now redundant and incorrect and should be removed?

Robie Basak (racb) wrote :

Test plan:

Could we agree a specific test plan for this SRU please, in addition to the usual testing agreed in https://wiki.ubuntu.com/StableReleaseUpdates/Certbot? Specifically there are a few things that are special about this one. I'd like to make sure that somebody makes sure that all the gotchas of which we've already become aware and fixed are still fixed in the SRU verification step, before the update is pushed out for automatic update by users.

This includes:

1. letsencrypt rename compatibility, including the CLI, letsencrypt and letsencrypt-apache Python modules.

2. Log rotation behaviour

3. Automatic renewal behaviour

4. Bug 1777205 and its patch

Once agreed we can update the Test Plan section of the bug description.

Brad Warren (bradmwarren) wrote :

I updated and added additional checks to the test script at https://wiki.ubuntu.com/StableReleaseUpdates/Certbot/TestScript including tests for all four of the areas Robie flagged in his most recent post.

Robie Basak (racb) wrote :

Thanks Brad!

As Michael seems to be unavailable at the moment I'll try to fix my review comments up myself. I've asked my colleague Christian to peer-review my work from this point to fulfil Ubuntu SRU review requirements.

What I don't know the answer to is some of my questions from my reviews above. These are in comments 99, 101 and 102.

I assume that the patch from 0.22.2-1ubuntu0.1 needs to be included.

I'm going to drop the bits I think are packaging code skeletons.

Two questions I'm really uncertain about right now:

Why is certbot.timer's installation being removed? Has auto renewal been tested? If so, what's the mechanism it uses now?

Why remove the installation of the cli.ini that disables certbot-internal log rotation?

I'd appreciate any insight into these please. What were the mechanisms for auto renewal and log rotation in 0.4, what are they in 0.22, and what should packaging be doing in 0.22?

I've asked Harlan for upload access to the ~certbot PPA on IRC. I hope to have updated packages uploaded there soon for further testing.

Brad Warren (bradmwarren) wrote :

> I assume that the patch from 0.22.2-1ubuntu0.1 needs to be included.

Yes, I think this patch should be included. The packages in 18.04 included support Let's Encrypt's newer endpoint, however, this feature is broken without this patch.

Thanks for catching this. The updated tests now test against the new API as well.

The other questions here were largely answered in IRC, but to keep this thread here up-to-date and maybe add a little more context...

> Why is certbot.timer's installation being removed?

I think removing this was a mistake. The current Ubuntu 16.04 package does not configure automatic renewal, but I think it should be added as it benefits users and keeps the package closer to the one found in Bionic.

There's actually a lot of discussion about the addition of automatic renewal earlier in this thread from a couple years ago.

> Has auto renewal been tested?

I suspect not. At the very least, I hadn't done so.

Previously my test script was almost exclusively running our upstream tests on the installed packages and did not do much to test things specifically found in the .deb packages.

For testing automatic renewal, however, the updates to the script I made last week check:

1. certbot.timer exists and is enabled.
2. /etc/cron.d/certbot exists.

> Why remove the installation of the cli.ini that disables certbot-internal log rotation?

I think we probably should keep this file unless there is a good reason not to do so. Why make unnecessary modifications to the packages?

In the current Xenial packages, Certbot is using its own log rotation provided by the Python standard library. Each run of Certbot creates a new log file in /var/log/letsencrypt and up to 10 log files are created after which old log files are deleted.

The cli.ini file in the Bionic package turns off this log rotation causing Certbot to always append to the same file at /var/log/letsencrypt/letsencrypt.log with the thinking that this will better play with programs like logrotate.

Robie Basak (racb) wrote :

I've pulled Michael's current work into git trees available at: https://code.launchpad.net/~certbot/+git (each has a branch called lp1640978). Please maintain these branches as fast-forwarding only.

I have updated python-acme, python-certbot, python-letsencrypt and python-letsencrypt-apache based on my review comments above, bumped the ppa suffix versions, tweaked the version strings as needed, pushed to the git branches, and uploaded to the xenial-sru PPA.

Next, I need to check the PPA versions work as expected. Then I'll get a colleague's review. If all is good, we will be able to upload and accept into xenial-proposed.

Brad Warren (bradmwarren) wrote :

I tried manually installing packages from the PPA and running https://wiki.ubuntu.com/StableReleaseUpdates/Certbot/TestScript but the script failed because the Certbot systemd timer wasn't found.

On Wed, Feb 13, 2019 at 05:26:38PM -0000, Brad Warren wrote:
> I tried manually installing packages from the PPA and running
> https://wiki.ubuntu.com/StableReleaseUpdates/Certbot/TestScript but the
> script failed because the Certbot systemd timer wasn't found.

Thanks. I'll follow up.

I also found that I missed review of python-certbot-apache, so I'll get
on to that also.

Robie Basak (racb) wrote :

I've fixed that, updated the git repositories under ~certbot, and re-uploaded to the PPA. I've included some other fixes as well. Full changes are available to view in the git repositories.

Christian, please could you review all changes in the range review/2019-02-08..lp1640978 for the packages python-josepy, python-acme, python-letsencrypt, python-letsencrypt-apache, python-certbot and all commits but the root commit in python-certbot-apache? That should give us full review coverage then.

Brad Warren (bradmwarren) wrote :

Test script passes on the new packages in the PPA.

Download full text (4.5 KiB)

TL;DR
- LGTM a few questions for clarification, but no nacks

----

I took logs for everything non trivial that I did (wonder about) on review.
+ = LGTM
? = ok, but I'd have a question
x = nack, this should be different

python-acme:
? rebase for 0.22.2-1ubuntu0.1
  - The bug #1777205 should get a xenial task before upload
  - and then the bug be verified on the SRU
  - please remember to buildpkg with -v0.4.1-1
+ changelog tweaks
+ backport version string
  Double ubuntu in the version seems odd, but it seems correct.
  It is not part of [2] and an even more special case of [1]
x Add SRU explanation to changelog
  The version is 0.22.2 the entry calls it "version 0.23 in 16.04"
  Seems wrong, either fix or explain why it is that way.
  I see that all others e.g. python-certbot is at 0.23 for real
  will that mismatch be a problem?

python-certbot:
+ recommends certbot
? dh_clean ok but why, would it hurt?
? pkg-info.mk ok but why, would it hurt?
? cli.ini
  That is the one to keep the logrotations as is (ok)
  I thought from our talk Xenial did that as well, but it didn't
  There is nothing about it in pkg/ubuntu/xenial-devel:debian/rules
  nor in pkg/import/0.14.2-1:debian/rules
  I guess the old default was different, which is ok.
  But do we now want/need any conffile cleanup in >=Bionic?
  This would be independent to this SRU, but I wanted to ask.
+ build rules
+ install init no-op
? Restore original timer service installation
  The "original" used the older dh_systemd_enable/dh_systemd_start
  I think your's work only with newer debhelper, it is not "the original"
  I just found you clean that up later - would have been nice to squash those two :-/
+ backport version string (that one I like more)
? use dh_systemd
  The old package used install -D --mode=644 for the timer
  any reason you have chosen mkdir + cp instead ?
  That commit also adds whitespace damage above "# Install certbot.timer"
+ doc paths
+ version/changelog changes
? Add SRU explanation to changelog
  you might mention changes to d/rules and d/compat to make the backport working here

python-letsencrypt:
+ Remove redundant incorrect d/README.source
? further changes to changelog ( I have a general statement about those below)
? It is not clear to me how that "shimming" takes place, extending the changelog on that would be useful as well.

python-josepy:
? changelog/version bumps in general ok
  You mentioned the new certbot.timer and such in python-letsencrypt (which doesn't hold the timers)
  Please choose:
  - Why did you mention it there?
  - Why didn't you mention it here?
  I assume it is because of the package takeover letsencrypt->certbot.
  But really, that could use some words (more about that below already)

python-letsencrypt-apache
+ cleanup
+ changelog
+ version bumps

python-certbot-apache:
+ empty dir
+ VCS
+ cleanup (again ok but why)
+ doc building
+ version string changes
+ doc-base paths
+ SRU explanation (nothing more than what was said already)

I just realized that the summary changelogs all say: "Log rotation is switched to logrotate via /etc/logrotate.d/certbot"
This whole section could need more why it was done and where the old/new tunable...

Read more...

Robie Basak (racb) wrote :

Thank you for the review! I'll look in detail later, but in case it speeds up an iteration, note that I've started from the perspective that this is a backport from Bionic - so the cleanups are on the principle of reducing the diff against Bionic. Where I saw a diff hunk that didn't seem necessary, that's what I removed in my cleanup commits.

Displaying first 40 and last 40 comments. View all 113 comments or add a comment.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers