Merge ~bryce/ubuntu/+source/ssl-cert:ssl-cert-sru-lp1853021-hirsute into ubuntu/+source/ssl-cert:ubuntu/devel

Proposed by Bryce Harrington
Status: Merged
Approved by: Bryce Harrington
Approved revision: 89e28e390817c31ac0fce1c485648805be5a319c
Merge reported by: Bryce Harrington
Merged at revision: 89e28e390817c31ac0fce1c485648805be5a319c
Proposed branch: ~bryce/ubuntu/+source/ssl-cert:ssl-cert-sru-lp1853021-hirsute
Merge into: ubuntu/+source/ssl-cert:ubuntu/devel
Diff against target: 161 lines (+71/-21)
3 files modified
debian/changelog (+10/-0)
debian/control (+4/-3)
make-ssl-cert (+57/-18)
Reviewer Review Type Date Requested Status
Christian Ehrhardt  (community) Approve
Paride Legovini (community) Needs Information
Canonical Server Pending
git-ubuntu developers Pending
Review via email: mp+393784@code.launchpad.net

Description of the change

Fix for server-next bug regarding a missing capability in make-ssl-cert to specify the expiration length of a self-signed certificate.

Essentially this simply needed a way to pass in a value for the -days option for openssl, however the script lacked provisions for conveniently adding new options. Instead of hacking in the new option, I've implemented a general option handling capability that should make the script more maintainable into the future.

While not mentioned in the original bug report, I expect the desire is to see this command line support also SRU'd to past releases. This may be too invasive a change with too narrow a use case to warrant SRU, so at least for now I'm targeting it to hirsute only. However, I've gone ahead and filled in the SRU template anyway. So, if the fix proves worthy and desirable, it should be straightforward to backport.

PPA: https://launchpad.net/~bryce/+archive/ubuntu/ssl-cert-sru-lp1853021/+packages
Bug: https://bugs.launchpad.net/ubuntu/+source/ssl-cert/+bug/1853021

To post a comment you must log in.
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

The PPA seems not to carry the new version?
root@h:~# vim /usr/sbin/make-ssl-cert
root@h:~# /usr/sbin/make-ssl-cert -h
Usage: /usr/sbin/make-ssl-cert template output [--force-overwrite]
Usage: /usr/sbin/make-ssl-cert generate-default-snakeoil [--force-overwrite]

review: Needs Fixing
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

I like adding proper arg handling, but IMHO this is one of the cases were we will end up with a lot of pain doing it as Ubuntu delta.

How about - after gettign it reviewed and tested here - filing it against https://bugs.debian.org/cgi-bin/pkgreport.cgi?repeatmerged=no&src=ssl-cert and proposing it against https://salsa.debian.org/apache-team/ssl-cert which is the salsa repo.

P.S. might be also worth to mention salsa in d/control

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Some more inline comments added

Also this has whitespace damage in line 40 (the original has that, but you could fix it in your proposal)

   39 done
   40 ····
   41 db_get make-ssl-cert/hostname

There are more - worth to clean up and unify when proposing to Debian IMHO.

In general some shellcheck treatment could raise the quality while we are at it.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

BTW - my reasoning for proposing this to Debian once cleaned - Plenty of other things packages but also guides/howtos on the net use this command. Going together with changed options will ensure they are usable as-is on both sides.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

The parsing right now is very bash'y e.g.

--expiration-days=5 OK

--expiration-days 5 FAILS
  Unrecognized option --expiration-days

--expiration-days= 5
 opt_expiration_days=
 template=5

Package: util-linux
Essential: yes
and has /usr/bin/getopt, so should we just use that to improve on it (up for discussion)

d2000a1... by Bryce Harrington

d/control: Update git links to salsa

Revision history for this message
Bryce Harrington (bryce) wrote :

PPA is updated at https://launchpad.net/~bryce/+archive/ubuntu/ssl-cert-sru-lp1853021-snakeoil now

> I like adding proper arg handling, but IMHO this is one of the cases were we will end up with a lot of pain doing it as Ubuntu delta.

Completely agreed, same thought occurred to me, and yes I do keep a TODO to forward to Debian once the MP is acceptable.

> P.S. might be also worth to mention salsa in d/control

Good idea, the existing links are obsolete. I've updated them.

> The parsing right now is very bash'y e.g.
> --expiration-days=5 OK
> --expiration-days 5 FAILS
> Unrecognized option --expiration-days

I know, and I'm not happy about this as I strongly prefer the latter getopts-style format personally. However, I thought adding getopts might be a bridge too far for this package. I figure this was a less invasive change, but would be more than happy to move to getopts if Debian is ok with it. If you feel strongly that getopts *should* be used I'll update to use that.

Regarding shellcheck, yes I noticed there are numerous issues as well. I opted, however, to only fix shellcheck issues involved with the exact code lines I changed, to keep the patch minimal. I noticed the whitespace was handled irregularly elsewhere in the script so didn't worry about that, but I'll go and make sure all my touched lines are consistently using spaces instead of tabs. Followup patches to address both shellcheck and whitespace may make sense to consider doing when upstreaming the patch to Debian.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

> If you feel strongly that getopts *should* be used I'll update to use that.

I'd personally prefer, but my opinions are not strong enough to
force/convince you.
Since we go through Debian it depends on their preference more than on mine.

> ... upstreaming the patch to Debian.

Thanks, that is the right way. If they are slow or denying even if we
are kind and responsive we can still think about adding it as delta
then.

Revision history for this message
Paride Legovini (paride) wrote :

Should we also lower the default validity of the certificate to something like 800 days as part of this change? This would address the specific issue raised by LP: #1853021, and I think it makes sense to have defaults that produce certificates that are compatible with browsers.

(Should we *only* lower the cert validity if Debian is slow/unresponsive, instead of adding --expiration-days in a delta? The risk I see is Debian implementing the same in a different way, and ending up with two different interfaces that are difficult to reconcile and that could require even more deltas in other packages.)

review: Needs Information
Revision history for this message
Bryce Harrington (bryce) wrote :

@Paride given the new standard is 825 days, I also wondered if that might be a more sensible value to set as the default. One drawback is that doing so would certainly be a behavioral change, and may make the changes non-SRU-able. But going forward that might be a good change; another thing to discuss with Debian I suppose.

Regardless, I do think that adding --expiration-days is worthwhile. From the referenced links, I see there was a lot of debate, with some CA's preferring longer expiration times, and others shorter. So I think from the distro POV having this as a settable option gives our users flexibility that they need.

@Christian, one of the considerations I made with skipping getopts is that getopts only handles single-letter options, but this script's one parameter is a long option. The other usage mode of permitting "<template> <output>" arguments also needs a bit of special handling to use cleanly with getopts. It ends up being a number of lines for argument parsing to handle just a couple options.

I notice this is not marked Approved yet; do I interpret this correctly to mean you prefer this work be done only in Debian rather than Ubuntu, or are you ok with the branch as-is to land as temporary delta for Ubuntu while we wait on Debian?

Revision history for this message
Bryce Harrington (bryce) wrote :

I've gathered the review feedback for suggested changes to forward to debian into this branch:

https://code.launchpad.net/~bryce/ubuntu/+source/ssl-cert/+git/ssl-cert/+ref/ssl-cert-sru-lp1853021-debian

I split up the diff into discrete changes, ordered in (I think) likelihood of their acceptance. This includes fixing whitespace and shellcheck issues (except one) as Christian suggested. I changed the default expiration period to 825 as Paride suggested. I finished with a conversion to using getopts, although as mentioned above I think it gets a bit too much for this script; I left it as the final patch so if Debian doesn't care for it, it can be popped off the stack easy.

Please look this over and +1 or give any other feedback before I send off to Debian.

Also, I'd appreciate clarification if I should proceed with landing the Ubuntu MP as (temporary) delta, or wait until this is taken by Debian.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Thanks for wrapping all that up Bryce.
I'm +1 on the current content and state of the discussion.

For the uploads I'm strongly voting for "try to convince Debian first". I mentioned it and also Paride nicely put it as "ending up with two different interfaces that are difficult to reconcile and that could require even more deltas in other package".

I'd only consider an Ubuntu only upload for this if Debian really fails to participate in the discussion at all.
Both Distributions have feature freezes early on in 2021 so proposing and pushing for this change now is the right time.

In terms of SRUability e.g. of the default time - I'd not bother about that now. Let us make 21.04 great (as proposed) and later decide on the SRUs if we can include those changes or not (I assume we can't).

review: Approve
Revision history for this message
Bryce Harrington (bryce) wrote :

Thanks for the clarification. I've proposed a MP to debian here:

https://salsa.debian.org/apache-team/ssl-cert/-/merge_requests/2

Revision history for this message
Bryce Harrington (bryce) wrote :

Debian accepted the changes discussed above, and we've sync'd the same in via the new 1.1.0. The changes not taken (e.g. altering the default validity time) are technically not strictly necessary for resolving the original bug report so I don't think that needs followed up on.

Per Christian's last comment above will hold on SRUing until there is a specific request for it, and decide at that time.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/debian/changelog b/debian/changelog
index 01fad6a..6d51bd9 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,13 @@
1ssl-cert (1.0.39ubuntu1) hirsute; urgency=medium
2
3 * make-ssl-cert: Add a --expiration-days parameter
4 (LP: #1853021)
5 - This parameter enables users to customize the expiration of self-signed
6 certificates.
7 * d/control: Update git links to salsa
8
9 -- Bryce Harrington <bryce@canonical.com> Fri, 13 Nov 2020 00:39:17 +0000
10
1ssl-cert (1.0.39) unstable; urgency=medium11ssl-cert (1.0.39) unstable; urgency=medium
212
3 * Always put the common name also in the SubjectAltName. This is required13 * Always put the common name also in the SubjectAltName. This is required
diff --git a/debian/control b/debian/control
index 324d965..f958433 100644
--- a/debian/control
+++ b/debian/control
@@ -1,12 +1,13 @@
1Source: ssl-cert1Source: ssl-cert
2Section: utils2Section: utils
3Priority: optional3Priority: optional
4Maintainer: Debian Apache Maintainers <debian-apache@lists.debian.org>4Maintainer: Ubuntu Developers <ubuntu-devel-discuss@lists.ubuntu.com>
5XSBC-Original-Maintainer: Debian Apache Maintainers <debian-apache@lists.debian.org>
5Uploaders: Tollef Fog Heen <tfheen@debian.org>, Stefan Fritsch <sf@debian.org>6Uploaders: Tollef Fog Heen <tfheen@debian.org>, Stefan Fritsch <sf@debian.org>
6Build-Depends: debhelper (>= 7), po-debconf7Build-Depends: debhelper (>= 7), po-debconf
7Standards-Version: 3.9.88Standards-Version: 3.9.8
8Vcs-Browser: http://anonscm.debian.org/gitweb/?p=pkg-apache/ssl-cert.git9Vcs-Git: https://salsa.debian.org/apache-team/ssl-cert.git
9Vcs-Git: git://anonscm.debian.org/pkg-apache/ssl-cert.git10Vcs-Browser: https://salsa.debian.org/apache-team/ssl-cert
1011
11Package: ssl-cert12Package: ssl-cert
12Architecture: all13Architecture: all
diff --git a/make-ssl-cert b/make-ssl-cert
index 152e9f9..ded374a 100755
--- a/make-ssl-cert
+++ b/make-ssl-cert
@@ -6,6 +6,20 @@
6db_version 2.06db_version 2.0
7db_capb backup7db_capb backup
88
9progname=$(basename "${0}")
10
11usage() {
12 cat >&2 <<EOF
13Usage: ${progname} [options] <template> <output>
14 ${progname} [options] generate-default-snakeoil
15
16Options:
17 --force-overwrite
18 --expiration-days=<days>
19EOF
20 exit "${1}"
21}
22
9ask_via_debconf() {23ask_via_debconf() {
10 RET=""24 RET=""
11 if db_settitle make-ssl-cert/title ; then25 if db_settitle make-ssl-cert/title ; then
@@ -56,35 +70,60 @@ create_temporary_cnf() {
56 sed -e s#@HostName@#"$HostName"# -e s#@SubjectAltName@#"$SubjectAltName"# $template > $TMPFILE70 sed -e s#@HostName@#"$HostName"# -e s#@SubjectAltName@#"$SubjectAltName"# $template > $TMPFILE
57}71}
5872
59# Takes two arguments, the base layout and the output cert.73# Process arguments
6074subcommand=
61if [ $# -lt 2 ] && [ "$1" != "generate-default-snakeoil" ]; then75opt_force_overwrite="false"
62 printf "Usage: $0 template output [--force-overwrite]\n";76opt_expiration_days="3650"
63 printf "Usage: $0 generate-default-snakeoil [--force-overwrite]\n";77for arg in "${@}"; do
64 exit 1;78 case "${arg}" in
65fi79 --help | -h | \?)
80 usage 0
81 ;;
82 --force-overwrite)
83 opt_force_overwrite="true"
84 ;;
85 --expiration-days=*)
86 opt_expiration_days="${arg#*=}"
87 ;;
88 generate-default-snakeoil)
89 subcommand="generate-default-snakeoil"
90 ;;
91 --*)
92 printf "Unrecognized option ${arg}\n\n"
93 usage 1
94 ;;
95 *)
96 subcommand="manual"
97 template="${arg}"
98 ;;
99 esac
100 shift
101done
102
103if [ "${subcommand}" = "manual" ]; then
104 output="${1}"
105 [ -n "${output}" ] || usage 1
66106
67if [ "$1" != "generate-default-snakeoil" ]; then
68 template="$1"
69 output="$2"
70 # be anal in manual mode.107 # be anal in manual mode.
71 if [ ! -f $template ]; then108 if [ ! -f $template ]; then
72 printf "Could not open template file: $template!\n";109 printf "Could not open template file: $template!\n";
73 exit 1;110 exit 1;
74 fi111 fi
75 if [ -f $output ] && [ "$3" != "--force-overwrite" ]; then112 if [ -f $output ] && [ "${opt_force_overwrite}" != "true" ]; then
76 printf "$output file already exists!\n";113 printf "$output file already exists!\n";
77 exit 1;114 exit 1;
78 fi115 fi
79 ask_via_debconf116 ask_via_debconf
80else117elif [ "${subcommand}" = "generate-default-snakeoil" ]; then
81 template="/usr/share/ssl-cert/ssleay.cnf"118 template="/usr/share/ssl-cert/ssleay.cnf"
82 if [ -f "/etc/ssl/certs/ssl-cert-snakeoil.pem" ] && [ -f "/etc/ssl/private/ssl-cert-snakeoil.key" ]; then119 if [ -f "/etc/ssl/certs/ssl-cert-snakeoil.pem" ] && [ -f "/etc/ssl/private/ssl-cert-snakeoil.key" ]; then
83 if [ "$2" != "--force-overwrite" ]; then120 if [ "${opt_force_overwrite}" != "true" ]; then
84 exit 0121 exit 0
85 fi122 fi
86 fi123 fi
87 make_snakeoil124 make_snakeoil
125else
126 usage 1
88fi127fi
89128
90# # should be a less common char129# # should be a less common char
@@ -102,8 +141,8 @@ create_temporary_cnf
102141
103umask 077142umask 077
104143
105if [ "$1" != "generate-default-snakeoil" ]; then144if [ "${subcommand}" = "manual" ]; then
106 if ! openssl req -config $TMPFILE -new -x509 -days 3650 -nodes -sha256 \145 if ! openssl req -config $TMPFILE -new -x509 -days ${opt_expiration_days} -nodes -sha256 \
107 -out $output -keyout $output > $TMPOUT 2>&1146 -out $output -keyout $output > $TMPOUT 2>&1
108 then147 then
109 echo Could not create certificate. Openssl output was: >&2148 echo Could not create certificate. Openssl output was: >&2
@@ -114,8 +153,8 @@ if [ "$1" != "generate-default-snakeoil" ]; then
114 # hash symlink153 # hash symlink
115 cd $(dirname $output)154 cd $(dirname $output)
116 ln -sf $(basename $output) $(openssl x509 -hash -noout -in $(basename $output))155 ln -sf $(basename $output) $(openssl x509 -hash -noout -in $(basename $output))
117else156elif [ "${subcommand}" = "generate-default-snakeoil" ]; then
118 if ! openssl req -config $TMPFILE -new -x509 -days 3650 -nodes -sha256 \157 if ! openssl req -config $TMPFILE -new -x509 -days ${opt_expiration_days} -nodes -sha256 \
119 -out /etc/ssl/certs/ssl-cert-snakeoil.pem \158 -out /etc/ssl/certs/ssl-cert-snakeoil.pem \
120 -keyout /etc/ssl/private/ssl-cert-snakeoil.key > $TMPOUT 2>&1159 -keyout /etc/ssl/private/ssl-cert-snakeoil.key > $TMPOUT 2>&1
121 then160 then

Subscribers

People subscribed via source and target branches