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

[Bryce Harrington]

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

[Christian Ehrhardt  (paelzer)]

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

[Christian Ehrhardt  (paelzer)]

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

[Christian Ehrhardt  (paelzer)]

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.

[Christian Ehrhardt  (paelzer)]

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.

[Christian Ehrhardt  (paelzer)]

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 on 2020-11-16

d/control: Update git links to salsa

[Bryce Harrington (bryce)]

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.

[Christian Ehrhardt  (paelzer)]

> 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.

[Paride Legovini (paride)]

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

[Bryce Harrington (bryce)]

@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?

[Bryce Harrington (bryce)]

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.

[Christian Ehrhardt  (paelzer)]

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

[Bryce Harrington (bryce)]

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

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

[Bryce Harrington (bryce)]

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.

diff --git a/debian/changelog b/debian/changelog
index 01fad6a..6d51bd9 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,13 @@
+ssl-cert (1.0.39ubuntu1) hirsute; urgency=medium
+
+  * make-ssl-cert: Add a --expiration-days parameter
+    (LP: #1853021)
+    - This parameter enables users to customize the expiration of self-signed
+      certificates.
+  * d/control: Update git links to salsa
+
+ -- Bryce Harrington <[e-mail]>  Fri, 13 Nov 2020 00:39:17 +0000
+
 ssl-cert (1.0.39) unstable; urgency=medium
 
   * 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 @@
 Source: ssl-cert
 Section: utils
 Priority: optional
-Maintainer: Debian Apache Maintainers <[e-mail]>
+Maintainer: Ubuntu Developers <[e-mail]>
+XSBC-Original-Maintainer: Debian Apache Maintainers <[e-mail]>
 Uploaders: Tollef Fog Heen <[e-mail]>, Stefan Fritsch <[e-mail]>
 Build-Depends: debhelper (>= 7), po-debconf
 Standards-Version: 3.9.8
-Vcs-Browser: http://anonscm.debian.org/gitweb/?p=pkg-apache/ssl-cert.git
-Vcs-Git: git://anonscm.debian.org/pkg-apache/ssl-cert.git
+Vcs-Git: https://salsa.debian.org/apache-team/ssl-cert.git
+Vcs-Browser: https://salsa.debian.org/apache-team/ssl-cert
 
 Package: ssl-cert
 Architecture: 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 @@
 db_version 2.0
 db_capb backup
 
+progname=$(basename "${0}")
+
+usage() {
+    cat >&2 <<EOF
+Usage: ${progname} [options] <template> <output>
+       ${progname} [options] generate-default-snakeoil
+
+Options:
+       --force-overwrite
+       --expiration-days=<days>
+EOF
+    exit "${1}"
+}
+
 ask_via_debconf() {
     RET=""
     if db_settitle make-ssl-cert/title ; then
@@ -56,35 +70,60 @@ create_temporary_cnf() {
     sed -e s#@HostName@#"$HostName"# -e s#@SubjectAltName@#"$SubjectAltName"# $template > $TMPFILE
 }
 
-# Takes two arguments, the base layout and the output cert.
-
-if [ $# -lt 2 ] && [ "$1" != "generate-default-snakeoil" ]; then
-    printf "Usage: $0 template output [--force-overwrite]\n";
-    printf "Usage: $0 generate-default-snakeoil [--force-overwrite]\n";
-    exit 1;
-fi
+# Process arguments
+subcommand=
+opt_force_overwrite="false"
+opt_expiration_days="3650"
+for arg in "${@}"; do
+    case "${arg}" in
+        --help | -h | \?)
+            usage 0
+            ;;
+        --force-overwrite)
+            opt_force_overwrite="true"
+            ;;
+        --expiration-days=*)
+            opt_expiration_days="${arg#*=}"
+            ;;
+        generate-default-snakeoil)
+            subcommand="generate-default-snakeoil"
+            ;;
+        --*)
+            printf "Unrecognized option ${arg}\n\n"
+            usage 1
+            ;;
+        *)
+            subcommand="manual"
+            template="${arg}"
+            ;;
+    esac
+    shift
+done
+
+if [ "${subcommand}" = "manual" ]; then
+    output="${1}"
+    [ -n "${output}" ] || usage 1
 
-if [ "$1" != "generate-default-snakeoil" ]; then
-    template="$1"
-    output="$2"
     # be anal in manual mode.
     if [ ! -f $template ]; then
 	printf "Could not open template file: $template!\n";
 	exit 1;
     fi
-    if [ -f $output ] && [ "$3" != "--force-overwrite" ]; then
+    if [ -f $output ] && [ "${opt_force_overwrite}" != "true" ]; then
         printf "$output file already exists!\n";
         exit 1;
     fi
     ask_via_debconf
-else
+elif [ "${subcommand}" = "generate-default-snakeoil" ]; then
     template="/usr/share/ssl-cert/ssleay.cnf"
     if [ -f "/etc/ssl/certs/ssl-cert-snakeoil.pem" ] && [ -f "/etc/ssl/private/ssl-cert-snakeoil.key" ]; then
-        if [ "$2" != "--force-overwrite" ]; then
-             exit 0
+        if [ "${opt_force_overwrite}" != "true" ]; then
+            exit 0
         fi
     fi
     make_snakeoil
+else
+    usage 1
 fi
 
 # # should be a less common char
@@ -102,8 +141,8 @@ create_temporary_cnf
 
 umask 077
 
-if [ "$1" != "generate-default-snakeoil" ]; then
-    if ! openssl req -config $TMPFILE -new -x509 -days 3650 -nodes -sha256 \
+if [ "${subcommand}" = "manual" ]; then
+    if ! openssl req -config $TMPFILE -new -x509 -days ${opt_expiration_days} -nodes -sha256 \
 	-out $output -keyout $output > $TMPOUT 2>&1
     then
 	echo Could not create certificate. Openssl output was: >&2
@@ -114,8 +153,8 @@ if [ "$1" != "generate-default-snakeoil" ]; then
     # hash symlink
     cd $(dirname $output)
     ln -sf $(basename $output) $(openssl x509 -hash -noout -in $(basename $output))
-else
-    if ! openssl req -config $TMPFILE -new -x509 -days 3650 -nodes -sha256 \
+elif [ "${subcommand}" = "generate-default-snakeoil" ]; then
+    if ! openssl req -config $TMPFILE -new -x509 -days ${opt_expiration_days} -nodes -sha256 \
 	-out /etc/ssl/certs/ssl-cert-snakeoil.pem \
         -keyout /etc/ssl/private/ssl-cert-snakeoil.key > $TMPOUT 2>&1
     then