Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

acme-client: 0.1.16 -> 0.2.4 #71853

Merged
merged 4 commits into from Nov 3, 2019
Merged

acme-client: 0.1.16 -> 0.2.4 #71853

merged 4 commits into from Nov 3, 2019

Conversation

ruuda
Copy link
Contributor

@ruuda ruuda commented Oct 23, 2019

Motivation for this change

A lot has happened since 0.1.16 was packaged. The old upstream website now says:

Attention: this version of acme-client has been archived, as it now lives in OpenBSD base. If you're using the upstream version of this code, you're using old code! The live code, /usr/sbin/acme-client in OpenBSD, is well-maintained and current. See the CVS repository for current code.

If you can't use OpenBSD, the current code-base can fairly easily be ported, but this is not an effort I have a need for. Please see the portable version's GitHub README.md for hints. Thank you!

The linked repository is archived though, and its readme says:

Attention acme-client-portable is no longer maintained. Since acme-client made its way into OpenBSD, I've only been using the base version.

So if you're using this client, you're using old code! If you want to use the current version of acme-client, you'll need to use OpenBSD. Which I recommend doing anyway, of course.

If you can't use OpenBSD, it's not a difficult challenge to port the OpenBSD acme-client to your operating system. However, be aware that it won't share the security mechanisms available to the downstream code. Regardless, if anybody would like to take a stab at this, I'd be happy to review your work!

Fortunately, letsencrypt.org links to https://github.com/graywolf/acme-client-portable, which does appear to be a maintained portable version of acme-client, so I took that repository as the new upstream.

It looks like that new acme-client-portable has been modified to build against OpenSSL, but that actually broke LibreSSL compatibility. I included a small patch to restore compatibility. I also submitted that patch upstream.

The new package produces a binary that runs, but I have not tested it extensively. Also, due to the changed upstream, and the portable version being a modified version of the OpenBSD version, it is hard to tell what has changed because of the version bump. I’ll try and see if I can diff the code from the tarballs.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @pmahoney

@ruuda
acme-client: 0.1.16 -> 0.2.3
001f7d0 
The upstream acme-client that used to be at [1] has now been integrated
into OpenBSD, and the portable version that it links to at [2] is marked
as unmaintained. However, letsencrypt.org links to [3] for a portable
version, and indeed, that repository contains a version that has recent
activity, so I switched over to that.

It is hard to tell what the difference is between the OpenBSD version
and what is on Github, and even if that would be easy, there are a lot
of Linux-specific changes. This program is dealing with certificates, so
I feel it is important to at least check that thare are no obviously
unintended differences between the previous version and the new, but I
don't know of a good way of doing that at this point. I will continue
to investigate before I open a pull request.

[1]: https://kristaps.bsd.lv/acme-client/
[2]: https://github.com/kristapsdz/acme-client-portable
[3]: https://github.com/graywolf/acme-client-portable
@ruuda
acme-client: fix Linux build of new upstream
9191da3 
The new source does not include a configure script in the repository,
but we can generate it with automake. Also, the new acme-client-portable
has an OpenSSL compatibility layer, but that actually breaks building
against LibreSSL. Avoid this issue by patching the compatibility layer
to be less eager to define things when linking against LibreSSL. I will
also submit a pull request for that upstream.

I don't expect this to work on Darwin, and the current package suggests
it does, but if the upstream (portable) version is no longer maintained,
for Darwin, perhaps we should just drop support for it. But maybe it
will just work, CI or somebody with a Darwin system will have to try.
pmahoney
pmahoney approved these changes Oct 25, 2019
View reviewed changes
Copy link
Contributor

@pmahoney pmahoney left a comment

Choose a reason for hiding this comment

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

Nice work @ruuda . I was not aware of acme-client-portable. I had been considering proposing deleting this from nixpkgs..

@ruuda
acme-client: 0.2.3 -> 0.2.4
2786604 
My LibreSSL compatibility patch has been merged upstream into
acme-client-portable, and version 0.2.4 that includes it has been
released, so we can remove the patch here.
@ruuda ruuda changed the title acme-client: 0.1.16 -> 0.2.3 acme-client: 0.1.16 -> 0.2.4 Oct 27, 2019
@ruuda
Copy link
Contributor Author

ruuda commented Oct 27, 2019

My compatibility patch has been merged upstream, and version 0.2.4 has been released that includes it, so I updated this PR to drop the patch and use 0.2.4 instead.

@ruuda
Copy link
Contributor Author

ruuda commented Oct 27, 2019

Here is a diffstat of the changes from 0.1.16 to 0.2.4:

Diffstat 
 /dev/null => acme-client-portable-0.2.4/.gitignore |    8 +
 .../null => acme-client-portable-0.2.4/CVS/Entries |   26 +
 .../CVS/Repository                                 |    1 +
 /dev/null => acme-client-portable-0.2.4/CVS/Root   |    1 +
 acme-client-portable-0.1.16/ChangeLog => /dev/null |  854 ---------------
 .../GNUmakefile => /dev/null                       |  190 ----
 .../null => acme-client-portable-0.2.4/Makefile.am |   58 ++
 /dev/null => acme-client-portable-0.2.4/NEWS       |   20 +
 /dev/null => acme-client-portable-0.2.4/README     |   88 ++
 .../acctproc.c                                     |  385 +++++--
 .../acme-client.1                                  |  463 ++------
 .../acme-client.conf.5                             |  177 ++++
 .../base64.c                                       |  147 ++-
 .../certproc.c                                     |  180 +---
 .../chngproc.c                                     |  185 ++--
 .../compat-setresgid.c => /dev/null                |   50 -
 .../compat-setresuid.c => /dev/null                |   50 -
 /dev/null => acme-client-portable-0.2.4/compat.c   |   31 +
 /dev/null => acme-client-portable-0.2.4/compat.h   |   26 +
 acme-client-portable-0.1.16/config.h => /dev/null  |   74 --
 .../configure.ac                                   |   54 +
 .../confs/example.conf                             |   12 +
 .../confs/test.conf                                |   12 +
 .../dbg.c                                          |    8 +-
 .../dnsproc.c                                      |   64 +-
 .../extern.h                                       |  112 +-
 .../fileproc.c                                     |  213 ++--
 .../http.c                                         |  690 +++++-------
 .../http.h                                         |   23 +-
 .../json.c                                         |  632 +++++++----
 .../rsa.c => acme-client-portable-0.2.4/key.c      |  103 +-
 .../rsa.h => acme-client-portable-0.2.4/key.h      |   11 +-
 .../keyproc.c                                      |  170 ++-
 .../m4/ax_check_bsd.m4                             |  126 +++
 .../m4/ax_check_openssl.m4                         |  124 +++
 .../main.c                                         |  694 ++++--------
 .../netproc.c                                      |  801 +++++++-------
 /dev/null => acme-client-portable-0.2.4/parse.h    |   87 ++
 /dev/null => acme-client-portable-0.2.4/parse.y    | 1100 ++++++++++++++++++++
 .../revokeproc.c                                   |  182 ++--
 .../sandbox-darwin.c => /dev/null                  |   64 --
 .../sandbox-null.c => /dev/null                    |   43 -
 .../sandbox-pledge.c => /dev/null                  |  110 --
 .../sandbox-seccomp.c => /dev/null                 |  278 -----
 .../scripts/prepare_builds                         |   33 +
 .../scripts/prepare_readme                         |    8 +
 .../scripts/update.sh                              |   29 +
 .../util-pledge.c => /dev/null                     |   65 --
 .../util-portable.c => /dev/null                   |   95 --
 .../util.c                                         |  127 +--
 50 files changed, 4363 insertions(+), 4721 deletions(-)

The diff itself is rather large, so I only skimmed through it superficially. A few notable things:

  • 0.1.16 used Yoda conditions but 0.2.4 does not, and some formatting changes.
  • 0.1.16 used what looks like a more elaborate BSD compatibility/sandbox layer, 0.2.4 uses HAVE_PLEDGE and HAVE_UNVEIL ifdef guards instead.
  • Changes to the build setup.

@ruuda
Copy link
Contributor Author

ruuda commented Nov 2, 2019

Is there anything I can do to help move this forward?

c0bw3b
c0bw3b reviewed Nov 2, 2019
View reviewed changes
Copy link
Contributor

@c0bw3b c0bw3b left a comment

Choose a reason for hiding this comment

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

Thanks for refreshing this package @ruuda
I'm glad to learn there is a fork maintaining the portable variation for Linux systems

pkgs/tools/networking/acme-client/default.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/acme-client/default.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/acme-client/default.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/acme-client/default.nix Outdated Show resolved Hide resolved
@ruuda
acme-client: address review feedback
e276c24 
 * Replace the manual autoreconf invocation with autoreconfHook.
 * Remove DEFAULT_CA_FILE, which no longer affects the build.
@ruuda
Copy link
Contributor Author

ruuda commented Nov 3, 2019

Thanks for the review @c0bw3b, that was very educational. I addressed your comments, please take another look.

c0bw3b
c0bw3b approved these changes Nov 3, 2019
View reviewed changes
Copy link
Contributor

@c0bw3b c0bw3b left a comment

Choose a reason for hiding this comment

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

Result of nix-review pr 71853 1

1 package were build:
  • acme-client

It builds and runs

@c0bw3b c0bw3b merged commit a7b49ef into NixOS:master Nov 3, 2019
dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Nov 6, 2019
@ruuda @dtzWill
acme-client: 0.1.16 -> 0.2.4
a273e2f 
* acme-client: 0.1.16 -> 0.2.3 (NixOS#71853)

The upstream acme-client that used to be at [1] has now been integrated
into OpenBSD, and the portable version that it links to at [2] is marked
as unmaintained. However, letsencrypt.org links to [3] for a portable
version, and indeed, that repository contains a version that has recent
activity, so I switched over to that.

It is hard to tell what the difference is between the OpenBSD version
and what is on Github, and even if that would be easy, there are a lot
of Linux-specific changes. This program is dealing with certificates, so
I feel it is important to at least check that thare are no obviously
unintended differences between the previous version and the new, but I
don't know of a good way of doing that at this point. I will continue
to investigate before I open a pull request.

[1]: https://kristaps.bsd.lv/acme-client/
[2]: https://github.com/kristapsdz/acme-client-portable
[3]: https://github.com/graywolf/acme-client-portable

* acme-client: fix Linux build of new upstream

The new source does not include a configure script in the repository,
but we can generate it with automake. Also, the new acme-client-portable
has an OpenSSL compatibility layer, but that actually breaks building
against LibreSSL. Avoid this issue by patching the compatibility layer
to be less eager to define things when linking against LibreSSL. I will
also submit a pull request for that upstream.

I don't expect this to work on Darwin, and the current package suggests
it does, but if the upstream (portable) version is no longer maintained,
for Darwin, perhaps we should just drop support for it. But maybe it
will just work, CI or somebody with a Darwin system will have to try.

* acme-client: 0.2.3 -> 0.2.4

My LibreSSL compatibility patch has been merged upstream into
acme-client-portable, and version 0.2.4 that includes it has been
released, so we can remove the patch here.

* acme-client: address review feedback

 * Replace the manual autoreconf invocation with autoreconfHook.
 * Remove DEFAULT_CA_FILE, which no longer affects the build.

(cherry picked from commit a7b49ef)
@ruuda ruuda deleted the acme-client branch October 6, 2022 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmahoney pmahoney pmahoney approved these changes

@c0bw3b c0bw3b c0bw3b approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ruuda @pmahoney @c0bw3b