[SRU] postfix tls deploy-server-cert fails with "can't shift that many"

Bug #1881196 reported by robert owings on 2020-05-28
14
This bug affects 1 person
Affects Status Importance Assigned to Milestone
postfix (Ubuntu)
Undecided
Unassigned
Xenial
Undecided
Unassigned
Bionic
Undecided
Unassigned
Eoan
Undecided
Unassigned
Focal
Undecided
Lucas Kanashiro

Bug Description

[Impact]

"postfix tls deploy-server-cert" did not handle a missing optional argument which makes users get a "can't shift that many..." error.

In this SRU we are proposing a microrelease update in Focal from version 3.4.10 to 3.4.13 since the changes are self contained. Moreover, there is a Postfix SRU exception which allows microreleases if the bug is fixed in the current development series:

https://wiki.ubuntu.com/StableReleaseUpdates#Postfix

And according to the described process there is no need to define a Test Case and a Regression Potential sections. Upstream has been doing a good work regarding those stable version bug fixes.

Here is the upstream changelog change between 3.4.10 and 3.4.13:

20200416

 Workaround for broken builds after an incompatible change
 in GCC 10. Files: makedefs, Makefile.in.

 Workaround for broken DANE support after an incompatible
 change in GLIBC 2.31. This avoids the need for new options
 in /etc/resolv.conf. Files: dns/dns.h, dns/dns_lookup.c.

20200419

 Bugfix: segfault in the tlsproxy client role when the server
 role was disabled. This typically happens on systems that
 do not receive mail, after configuring connection reuse for
 outbound TLS. Found during program maintenance. File:
 tlsproxy/tlsproxy.c.

20200420

 Noise suppression: shut up a compiler that special-cases
 string literals. Viktor Dukhovni. File milter/milter.c.

20200422

 Security: disable DANE support on Alpine Linux because
 libc-musl provides no indication whether DNS responses are
 authentic. This broke DANE support without a clear explanation.
 File: makedefs.

20200505

 Noise suppression: shut up a compiler that special-cases
 string literals. Viktor Dukhovni. File smtpd/smtpd_check.c.

20200509

 Bugfix (introduced: Postfix 3.5): maillog_file_rotate_suffix
 default value used the minute instead of the month. Reported
 by Larry Stone. Files: conf/postfix-tls-script,
 proto/MAILLOG_README.html, proto/postconf.proto.
 global/mail_params.h, postfix/postfix.c.

20200510

 Bitrot: avoid U_FILE_ACCESS_ERROR after chroot(), by
 initializing the ICU library before making the chroot()
 call. Files: util/midna_domain.[hc], global/mail_params.c.

20200511

 Noise suppression: avoid "SSL_Shutdown:shutdown while in
 init" warnings. File: tls/tls_session.c.

20200515

 Bugfix (introduced: Postfix 2.2): a TLS error for a PostgreSQL
 client caused a false 'lost connection' error for an SMTP
 over TLS session in the same Postfix process. Reported by
 Alexander Vasarab, diagnosed by Viktor Dukhovni. File:
 tls/tls_bio_ops.c.

 Bugfix (introduced: Postfix 2.8): a TLS error for one TLS
 session may cause a false 'lost connection' error for a
 concurrent TLS session in the same tlsproxy process. File:
 tlsproxy/tlsproxy.c.

20200530

 Bugfix (introduced: Postfix 3.1): "postfix tls deploy-server-cert"
 did not handle a missing optional argument. File:
 conf/postfix-tls-script.

20200610

 Bugfix (introduced: Postfix 3.4): in the Postfix SMTP server,
 the SNI callback reported an error when it was called a
 second time. This happened after the server-side TLS engine
 sent a TLSv1.3 HelloRetryRequest (HRR) to a remote SMTP
 client. Reported by Ján Máté, fixed by Viktor Dukhovni.
 File: tls/tls_misc.c.

This new microrelease fixes the dane issue and the build against GCC 10 which makes us drop a patch applied in version 3.4.7-1 (80_glibc2.30-ftbfs.diff).

[Original Description]

lsb_release -rd
Description: Ubuntu 18.04.4 LTS
Release: 18.04

postfix:
  Installed: 3.3.0-1ubuntu0.2
  Candidate: 3.3.0-1ubuntu0.2
  Version table:
 *** 3.3.0-1ubuntu0.2 500
        500 http://us-west-2.ec2.archive.ubuntu.com/ubuntu bionic-updates/main amd64 Packages
        100 /var/lib/dpkg/status
     3.3.0-1 500
        500 http://us-west-2.ec2.archive.ubuntu.com/ubuntu bionic/main amd64 Packages

Attempting to deploy server certificates with
     postfix tls deploy-server-cert certificate.crt keyfile.key

Expected to deploy new certificates

What happened - command fails with
     /usr/lib/postfix/sbin/postfix-tls-script: 780: shift: can't shift that many

The issue appears to be that the function "deploy-server-cert" in /usr/lib/postfix/sbin/postfix-tls-script expects that there will be three arguments:

/usr/lib/postfix/sbin/postfix-tls-script line 777
     deploy_server_cert() {
     certfile=$1; shift
     keyfile=$1; shift
     deploy=$1; shift
            ...

This works when the function is called by the function new_server_cert, which calls the function with the arguments:
     deploy_server_cert "${certfile}" "${keyfile}" "${deploy}" || return 1

But when this function is invoked directly in line 1154, it is called with only 2 arguments
     deploy_server_cert "${certfile}" "${keyfile}" || exit 1

Related branches

I agree that it seems to miss an argument in the call from deploy-server-cert.
I have compared the versions up to the much more recent 3.5.2-1 but the situation is the same.

Reproducing this doesn't need a lot of pre-setup:
$ apt install postfix ssl-cert
$ postfix tls deploy-server-cert /etc/ssl/certs/ssl-cert-snakeoil.pem /etc/ssl/private/ssl-cert-snakeoil.key

Changed in postfix (Ubuntu):
status: New → Confirmed

The man page doe snot hint to any wrong use in this case as far as I can see:
deploy-server-cert certfile keyfile
  This subcommand deploys the certificates in certfile and private key in keyfile (which are typically generated by the commands above, which will also log and display the
  full command needed to deploy the generated key and certificate). After the new certificate and key are deployed any obsolete keys and certificates may be removed by
  hand. The keyfile and certfile filenames may be relative to the Postfix configuration directory.

The fix could be as easy as
--- a/conf/postfix-tls-script
+++ b/conf/postfix-tls-script
@@ -1039,7 +1039,7 @@ deploy-server-cert)
         *) keyfile="${config_directory}/${2}" ;;
        esac

- deploy_server_cert "${certfile}" "${keyfile}" || exit 1
+ deploy_server_cert "${certfile}" "${keyfile}" "enable" || exit 1
        info_server_deployed "${certfile}" "${keyfile}" "deploy" | $INFO
        ;;

But I'm not expert enough to consider all the side effects. Maybe at this point one would better pass "" instead of "enable".

It seems it has been introduced in this form a long time ago at 3.1.0 and was broken since then.
I wonder why nobody ever hit that, maybe it isn't supposed to be used at all?

Since it seems to be broken in the last version as well I wanted to ask you if you'd mind to bug-report that upstream and we could take the fix from there and backport to Ubuntu releases.

But I must admit that I wasn't able to find a proper bug tracker for postfix?!
I guess that means you should report to one of the mailing lists http://www.postfix.org/lists.html and it would be great if you could then get back here and update the bug with the link to the mail archive entry.

That way we can recheck what the outcome there will be and consider backporting it.

Scott Kitterman (kitterman) wrote :

Postfix doesn't have a bug tracker. Their policy is to fix bugs when they are identified. Emailing postfix-users is the correct path forward.

robert owings (f-info-l) wrote :

Ah. I'll report it to postfix-users as recommended.

robert owings (f-info-l) wrote :

Below is a reply from postfix-users. Works perfectly for me.

--

Below is a patch. I find that the handling of this differs a lot
among shell implementations, from terminating to ignoring.

    Wietse

diff -ur /var/tmp/postfix-3.6-20200523/conf/postfix-tls-script conf/postfix-tls-script
--- /var/tmp/postfix-3.6-20200523/conf/postfix-tls-script 2017-02-18 20:58:20.000000000 -0500
+++ conf/postfix-tls-script 2020-05-30 10:37:04.000000000 -0400
@@ -777,7 +777,7 @@
 deploy_server_cert() {
     certfile=$1; shift
     keyfile=$1; shift
- deploy=$1; shift
+ case $# in 0) deploy=;; *) deploy=$1; shift;; esac

     # Sets key_algo, key_param and cert_param
     check_key "$keyfile" || return 1

tags: added: server-next
Changed in postfix (Ubuntu Xenial):
status: New → Triaged
Changed in postfix (Ubuntu Bionic):
status: New → Triaged
Changed in postfix (Ubuntu Eoan):
status: New → Triaged
Changed in postfix (Ubuntu Focal):
status: New → Triaged
Changed in postfix (Ubuntu):
assignee: nobody → Lucas Kanashiro (lucaskanashiro)
status: Confirmed → In Progress

After proposing the mentioned change to Debian [1] we decided to wait for the next snapshot release since upstream implementation might differ from what was presented in the mailing list. Moreover, this is not an urgent issue.

[1] https://salsa.debian.org/postfix-team/postfix-dev/-/merge_requests/7

Changed in postfix (Ubuntu):
assignee: Lucas Kanashiro (lucaskanashiro) → nobody
status: In Progress → Triaged

https://github.com/vdukhovni/postfix/commit/
119d6abed969b1b30c62722ae31c854b5682beae#diff-1843f98f5710e97bd063d5807334442a

There will be a new 3.4 release in a few days, I'd just update your SRU to
encompass that when it comes out. It's all bug fixes, reported by users or
not.

This bug was fixed in the package postfix - 3.5.3-1

---------------
postfix (3.5.3-1) unstable; urgency=medium

  [Wietse Venema]

  * 3.5.3 LP: #1881196

  [Debian Janitor]

  * Trim trailing whitespace.
  * Fix day-of-week for changelog entries 0.0.20001030.SNAPSHOT-4,
    0.0.20001030.SNAPSHOT-3, 0.0.19991231pl02-1, 0.0.19990122-1.

 -- Scott Kitterman <email address hidden> Mon, 15 Jun 2020 16:23:34 -0400

Changed in postfix (Ubuntu):
status: Triaged → Fix Released

Thanks for the pointer Scott, appreciated. And also for fixing it in Debian and consequently in Groovy which is a sync.

I see this fix in version 3.4.13. Hopefully this postfix SRU [1] will be accepted and we will have version 3.4.11 in Focal, so we have two patch level releases to consider if we want to update to 3.4.13. Checking the changelog between 3.4.11 and 3.4.13 there are 6 "Bugfix", 3 "Noise suppression", and 1 "Bitrot". After a discussion with my team they told me we would need to define self-contained test cases for each of those bug fixes and analyze the impact of the non "Bugfix" changes to convince the SRU team to accept this new micro release.

I did some digging in the postfix-users mailing list and most of those bugs are discussed there but some are hard to reproduce (build a test case), there is a case where the OP offered access to their testing server to one of the postfix maintainers to help debugging the issue because it was not easily reproducible. With that in mind I think I will not be able to define good test cases to justify all of them. If someone could help me defining those test cases I'd be glad to do the rest of the work.

For now, I am considering to just backport this patch to fix the bug reported here:

20200530

 Bugfix (introduced: Postfix 3.1): "postfix tls deploy-server-cert"
 did not handle a missing optional argument. File:
 conf/postfix-tls-script.

[1] https://bugs.launchpad.net/ubuntu/+source/postfix/+bug/1868955

On Wednesday, June 17, 2020 5:51:07 PM EDT Lucas Kanashiro wrote:
> I see this fix in version 3.4.13. Hopefully this postfix SRU [1] will be
> accepted and we will have version 3.4.11 in Focal, so we have two patch
> level releases to consider if we want to update to 3.4.13. Checking the
> changelog between 3.4.11 and 3.4.13 there are 6 "Bugfix", 3 "Noise
> suppression", and 1 "Bitrot". After a discussion with my team they told
> me we would need to define self-contained test cases for each of those
> bug fixes and analyze the impact of the non "Bugfix" changes to convince
> the SRU team to accept this new micro release.

This did not used to be the case for micro-releases where the TB approved the
micro-release exception (as is the case here). Back when I was doing this for
Ubuntu it was enough that the postfix passed it's test suite and it had been
smoke tested on a real system.

We do these updates in Debian routinely without anything more than that and
have never had a problem. I'd find it surprising that Ubuntu was being more
restrictive about post-release updates than Debian.

Scott K

Scott, I asked around about this subject and those past SRUs with micro-release updates were accepted probably based on this:

https://wiki.ubuntu.com/StableReleaseUpdates#New_upstream_microreleases

And since Debian is also including those micro-releases in stable point releases I also think we should that too. However, I did not find the upstream QA process documented and this is mandatory according to the SRU policy for a micro-release update:

"The upstream QA process must be documented/demonstrated and linked from the SRU tracking bug."

If it is documented somewhere could you please provide a link to it? In case it is not available, we could not take advantage of this micro-release SRU process and a Postfix exception would be needed, like we already have for other packages as you can see here:

https://wiki.ubuntu.com/StableReleaseUpdates#Documentation_for_Special_Cases

To do that a process needs to be defined for future Postfix SRUs containing micro-release updates. I see we already have some good DEP-8 tests, so we'd need to write a wiki page describing this process and ask the SRU team to acknowledge it. I am offering myself to drive this if that is the case.

On Thursday, June 18, 2020 9:46:34 AM EDT you wrote:
> To do that a process needs to be defined for future Postfix SRUs
> containing micro-release updates. I see we already have some good DEP-8
> tests, so we'd need to write a wiki page describing this process and ask
> the SRU team to acknowledge it. I am offering myself to drive this if
> that is the case.

The Ubuntu Technical Board already approved this. See:

https://lists.ubuntu.com/archives/ubuntu-devel-announce/2011-October/
000902.html
https://lists.ubuntu.com/archives/technical-board/2012-May/001266.html

Scott K

Awesome, thank you for that. Since it was already approved there is no reason to go through the whole process again, but to keep it documented I'll create a wiki page for this Postfix exception and link it here:

https://wiki.ubuntu.com/StableReleaseUpdates#Documentation_for_Special_Cases

After that I will prepare a SRU with version 3.4.13.

FYI the SRU exception for Postfix is documented here:

https://wiki.ubuntu.com/StableReleaseUpdates#Postfix

Changed in postfix (Ubuntu Focal):
assignee: nobody → Lucas Kanashiro (lucaskanashiro)
status: Triaged → In Progress
description: updated
summary: - postfix tls deploy-server-cert fails with "can't shift that many"
+ [SRU] postfix tls deploy-server-cert fails with "can't shift that many"

Hello robert, or anyone else affected,

Accepted postfix into focal-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/postfix/3.4.13-0ubuntu1 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, what testing has been performed on the package and change the tag from verification-needed-focal to verification-done-focal. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-focal. In either case, without details of your testing we will not be able to proceed.

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

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Changed in postfix (Ubuntu Focal):
status: In Progress → Fix Committed
tags: added: verification-needed verification-needed-focal
robert owings (f-info-l) wrote :

Fix verified for focal.

$ lsb_release -rd
Description: Ubuntu 20.04 LTS
Release: 20.04

Did a fresh, vanilla install of focal.
installed postfix, ssl-cert - accepted all defaults
  # sudo apt install postfix ssl-cert
Tested to ensure that fault existed
  # postfix tls deploy-server-cert /etc/ssl/certs/ssl-cert-snakeoil.pem /etc/ssl/private/ssl-cert-snakeoil.key

applied patch from http://launchpadlibrarian.net/485381331/postfix_3.4.10-1ubuntu1_3.4.13-0ubuntu1.diff.gz
  # patch -d/ --ignore-whitespace -p0 /usr/lib/postfix/sbin/postfix-tls-script <<'EOF'
diff -Nru postfix-3.4.10/conf/postfix-tls-script postfix-3.4.13/conf/postfix-tls-script
--- postfix-3.4.10/conf/postfix-tls-script 2017-02-19 01:58:20.000000000 +0000
+++ postfix-3.4.13/conf/postfix-tls-script 2020-05-30 14:37:04.000000000 +0000
@@ -777,7 +777,7 @@
 deploy_server_cert() {
     certfile=$1; shift
     keyfile=$1; shift
- deploy=$1; shift
+ case $# in 0) deploy=;; *) deploy=$1; shift;; esac

     # Sets key_algo, key_param and cert_param
     check_key "$keyfile" || return 1
EOF

Verified fix. - focal.

--

Did a fresh, vanilla install of focal.
installed ssl-cert
  $ sudo apt install ssl-cert
Installed postfix/focal-proposed from postfix_3.4.13-0ubuntu1_amd64.deb
  $ sudo dpkg -i postfix_3.4.13-0ubuntu1_amd64.deb
Tested script
  $ postfix tls deploy-server-cert /etc/ssl/certs/ssl-cert-snakeoil.pem /etc/ssl/private/ssl-cert-snakeoil.key

Verified fix - focal

--

Did a fresh , vanilla install of bionic
installed postfix, ssl-cert - accepted all defaults
  # sudo apt install postfix ssl-cert
Tested to ensure that fault existed
  # postfix tls deploy-server-cert /etc/ssl/certs/ssl-cert-snakeoil.pem /etc/ssl/private/ssl-cert-snakeoil.key

applied patch from http://launchpadlibrarian.net/485381331/postfix_3.4.10-1ubuntu1_3.4.13-0ubuntu1.diff.gz
  # patch -d/ --ignore-whitespace -p0 /usr/lib/postfix/sbin/postfix-tls-script <<'EOF'
diff -Nru postfix-3.4.10/conf/postfix-tls-script postfix-3.4.13/conf/postfix-tls-script
--- postfix-3.4.10/conf/postfix-tls-script 2017-02-19 01:58:20.000000000 +0000
+++ postfix-3.4.13/conf/postfix-tls-script 2020-05-30 14:37:04.000000000 +0000
@@ -777,7 +777,7 @@
 deploy_server_cert() {
     certfile=$1; shift
     keyfile=$1; shift
- deploy=$1; shift
+ case $# in 0) deploy=;; *) deploy=$1; shift;; esac

     # Sets key_algo, key_param and cert_param
     check_key "$keyfile" || return 1
EOF

Verified fix - Bionic

tags: added: verification-done-focal
removed: verification-needed-focal
tags: added: verification-done
removed: verification-needed
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers