Merge ~ahasenack/ubuntu/+source/bind9:eoan-re-enable-eddsa-support into ubuntu/+source/bind9:ubuntu/devel

[Andreas Hasenack]

PPA with testing packages: https://launchpad.net/~ahasenack/+archive/ubuntu/bind9-eddsa-1825712
sudo add-apt-repository ppa:ahasenack/bind9-eddsa-1825712

Re-enable eddsa support, which was disabled in the last merge from Debian. It will pull in openssl 1.1.1 (as opposed to just 1.1.0), and that's why it was disabled in Debian, albeit temporarily. This is a regression in Disco, and Eoan.

There are two tests that can be done: offline and online.

Offline test:
dnssec-keygen -a ED25519 example.com

That will fail with bind9 builds that do not have eddsa support.

Online test:
$ delv +dnssec +multiline @127.0.0.1 ed25519.nl
; fully validated
ed25519.nl. 3600 IN A 77.72.150.82
ed25519.nl. 3600 IN RRSIG A 15 2 3600 (
    20190502000000 20190411000000 27662 ed25519.nl.
    f7HjJcbvekrmuLtXDzjddWJZzZAAFO6fV+NoMCg+UiIl
    nQjUxNcCvDWuR38XAJuHrctvQOlAg1JmIGwYyKM2DQ== )

It will either say "fully validated", as is the case above with a build that has eddsa support, or:
$ delv +dnssec +multiline @127.0.0.1 ed25519.nl
;; validating ed25519.nl/A: no valid signature found
; unsigned answer
ed25519.nl. 3600 IN A 77.72.150.82
ed25519.nl. 3200171710 IN RRSIG A 15 2 3600 (
    20190502000000 20190411000000 27662 ed25519.nl.
    f7HjJcbvekrmuLtXDzjddWJZzZAAFO6fV+NoMCg+UiIl
    nQjUxNcCvDWuR38XAJuHrctvQOlAg1JmIGwYyKM2DQ== )

it will say "unsigned answer" and "no valid signature found".

[Christian Ehrhardt  (paelzer)]

Checked the bug, tests and SRU template - all LGTM

[Christian Ehrhardt  (paelzer)]

Reason it was that way is this in Debian:
+ * Temporarily disable EDDSA to relax OpenSSL version requirement

I wonder if we want/need to modify the symbol?
debian/libdns1104.symbols: (optional)dst__openssleddsa_init@Base 1:9.11.4.P1+dfsg

Maybe make it non optional for us?
Up to you, no show stopper.

[Christian Ehrhardt  (paelzer)]

https://salsa.debian.org/dns-team/bind9/commit/94c7cd6f039b971e9cd9f0cde0a1ec17774aa6f8
also mentions
"there are rather important inline signing bugs in 9.11.4 prior to -P2."

But this is targetted at Disco/Eoan which have 9.11.5.P1 and should have these right?
Maybe before upload double check what came into 11.4.P2 to ensure they are in the version we re-enable this here.

[Christian Ehrhardt  (paelzer)]

Overall LGTM, I have left a few thoughts to consider for you, but none of theses needs re-review unless you want it.

+1 on the MP

review: Approve

[Christian Ehrhardt  (paelzer)]

One final thought - I'd expect this to resolve in Debian post buster when their openssl moves.
But to be sure about it maybe file a Debian bug about it to track back that we can remove this Delta then?

[Andreas Hasenack (ahasenack)]

> https://salsa.debian.org/dns-
> team/bind9/commit/94c7cd6f039b971e9cd9f0cde0a1ec17774aa6f8
> also mentions
> "there are rather important inline signing bugs in 9.11.4 prior to -P2."
>
> But this is targetted at Disco/Eoan which have 9.11.5.P1 and should have these
> right?
> Maybe before upload double check what came into 11.4.P2 to ensure they are in
> the version we re-enable this here.

All but one change in P2 were about signed zones (none specific to the algos we are enabling here, btw):

    --- 9.11.4-P2 released ---

5022. [doc] Update ms-self, ms-subdomain, krb5-self, and
            krb5-subdomain documentation. [GL !708]

5015. [bug] Reloading all zones caused zone maintenance to cease
            for inline-signed zones. [GL #435]

5014. [bug] Signatures loaded from the journal for the signed
            version of an inline-signed zone were not scheduled for
            refresh. [GL #482]

5004. [bug] 'rndc reconfig' could cause inline zones to stop
            re-signing. [GL #439]

[Andreas Hasenack (ahasenack)]

> > https://salsa.debian.org/dns-
> > team/bind9/commit/94c7cd6f039b971e9cd9f0cde0a1ec17774aa6f8
> > also mentions
> > "there are rather important inline signing bugs in 9.11.4 prior to -P2."
> >
> > But this is targetted at Disco/Eoan which have 9.11.5.P1 and should have
> these
> > right?
> > Maybe before upload double check what came into 11.4.P2 to ensure they are
> in
> > the version we re-enable this here.
>
>
> All but one change in P2 were about signed zones (none specific to the algos
> we are enabling here, btw):
>
> --- 9.11.4-P2 released ---
>
> 5022. [doc] Update ms-self, ms-subdomain, krb5-self, and
> krb5-subdomain documentation. [GL !708]
>
> 5015. [bug] Reloading all zones caused zone maintenance to cease
> for inline-signed zones. [GL #435]
>
> 5014. [bug] Signatures loaded from the journal for the signed
> version of an inline-signed zone were not scheduled for
> refresh. [GL #482]
>
> 5004. [bug] 'rndc reconfig' could cause inline zones to stop
> re-signing. [GL #439]

And 9.11.5P1 has these fixes as well (added in 9.11.5rc1 actually)

[Andreas Hasenack (ahasenack)]

> Reason it was that way is this in Debian:
> + * Temporarily disable EDDSA to relax OpenSSL version requirement
>
> I wonder if we want/need to modify the symbol?
> debian/libdns1104.symbols: (optional)dst__openssleddsa_init@Base
> 1:9.11.4.P1+dfsg
>
> Maybe make it non optional for us?
> Up to you, no show stopper.

Good point, thanks for bringing it up.

Removing the (optional) from the symbol will make the build fail if we ever re-introduce --with-eddsa=no, so that's a good trap.

I could also, instead of just removing the --with-eddsa=no line, comment it, and mention the bug and/or the optional symbol. That will remove the delta from the symbols file, and just augment the one we are introducing here anyway in d/rules. I'll work on that.

[Andreas Hasenack (ahasenack)]

Debian bug about re-enabling eddsa support: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=927962

[Andreas Hasenack (ahasenack)]

I need to rebase this, security did an upload to eoan, but the importer is stuck still. Maybe tomorrow.

[Andreas Hasenack (ahasenack)]

Ok, so using an explicit --with-eddsa=yes has a side effect that it also enables these algorithms in the pkcs11 build, and that introduces a new symbol as well. Not passing --with-eddsa is the same as using --with-eddsa=auto, in which case we only get the extra algorithms in the non-pkcs11 build, which is the behavior we had.

I'm not sure where the pkcs11 build is used, or why we have two such builds. This came from Debian and has been there for a long time, predating my involvement. I can't say if having eddsa enabled in the pkcs11 build is a good thing or not. Considering the default (--with-eddsa=auto) disables it in pkcs11, I'm keen on following that behavior.

[Andreas Hasenack (ahasenack)]

I pushed a new update that will fail the build if we are not using openssl 1.1.1 or higher. That being said, both disco and eoan no longer have libssl-dev 1.1.0, so with default distro packages from the archive this check is a noop, and is an extra delta.

Let me know what you think. We can:
a) go back to having the simple delta of just dropping --with-eddsa=no, no further changes
b) keep this last version I pushed, which drops the --with-eddsa=no line, but also drops the "(optional)" flag from the symbols file, making it a mandatory symbol.

[Christian Ehrhardt  (paelzer)]

- ack on following auto which disables in pkcs11 (instead of forcing yes)
- the simple delta will be easier to maintain (=a)

So after our trip through the options (a) seems best now.
Maybe put the explanations in the git commit, at least to us that would be visible.

What happened to "add a comment" to d/rules, wasn't that the easiest to maintain option we reached in IRC?

[Andreas Hasenack (ahasenack)]

Debian just reverted the disabling of eddsa: https://salsa.debian.org/dns-team/bind9/commit/58067b212950efd05793df402fc675ded3ca145c

About the comment, I couldn't find a place for the comment that didn't look odd, because were are dropping something from d/rules, and not changing or adding something, so there is nothing to comment on. And adding "--with-eddsa=auto" just to have a place to add a comment, and not enforce anything ("auto" isn't enforcing) doesn't look good.

Debian also added a patch to disable ed448, claiming it's broken, but with no pointers. For disco, I think we should stick to just enabling eddsa as cosmic had it, and watch what happens in bind about ed448.

[Andreas Hasenack (ahasenack)]

The disable-ed448 patch was later removed when refreshing patches for 9.11.6. We can get that when we merge

[Andreas Hasenack (ahasenack)]

I added a comment to d/rules, explaining how --with-eddsa works and its implications.

[Christian Ehrhardt  (paelzer)]

That looks good now (good explanation) It will be good to do it this way for Disco then.
Since Debian also accepted your change it will not be in Eoan for too long.

+1 to this MP

review: Approve

[Andreas Hasenack (ahasenack)]

Thanks, tagged and uploaded:
$ git push pkg upload/1%9.11.5.P1+dfsg-1ubuntu4
Enumerating objects: 13, done.
Counting objects: 100% (13/13), done.
Delta compression using up to 4 threads
Compressing objects: 100% (9/9), done.
Writing objects: 100% (9/9), 1.19 KiB | 52.00 KiB/s, done.
Total 9 (delta 6), reused 0 (delta 0)
To ssh://git.launchpad.net/~usd-import-team/ubuntu/+source/bind9
 * [new tag] upload/1%9.11.5.P1+dfsg-1ubuntu4 -> upload/1%9.11.5.P1+dfsg-1ubuntu4

$ dput ubuntu ../bind9_9.11.5.P1+dfsg-1ubuntu4_source.changes
Checking signature on .changes
gpg: ../bind9_9.11.5.P1+dfsg-1ubuntu4_source.changes: Valid signature from AC983EB5BF6BCBA9
Checking signature on .dsc
gpg: ../bind9_9.11.5.P1+dfsg-1ubuntu4.dsc: Valid signature from AC983EB5BF6BCBA9
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading bind9_9.11.5.P1+dfsg-1ubuntu4.dsc: done.
  Uploading bind9_9.11.5.P1+dfsg-1ubuntu4.debian.tar.xz: done.
  Uploading bind9_9.11.5.P1+dfsg-1ubuntu4_source.buildinfo: done.
  Uploading bind9_9.11.5.P1+dfsg-1ubuntu4_source.changes: done.
Successfully uploaded packages.

diff --git a/debian/changelog b/debian/changelog
index 4f4897d..c96cb06 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,9 @@
+bind9 (1:9.11.5.P1+dfsg-1ubuntu4) eoan; urgency=medium
+
+  * d/rules: add back EdDSA support (LP: #1825712)
+
+ -- Andreas Hasenack <[e-mail]>  Fri, 26 Apr 2019 14:04:37 +0000
+
 bind9 (1:9.11.5.P1+dfsg-1ubuntu3) eoan; urgency=medium
 
   * SECURITY UPDATE: limiting simultaneous TCP clients is ineffective
diff --git a/debian/rules b/debian/rules
index 1a22081..905a1da 100755
--- a/debian/rules
+++ b/debian/rules
@@ -76,6 +76,13 @@ override_dh_autoreconf: prepare_native_pkcs11 prepare_version
 
 override_dh_auto_configure:
 	debian/checkapi
+	# Behavior of --with-eddsa:
+	#  yes: enables it for openssl and pkcs11
+	#  no: disables it for openssl and pkcs11
+	#  auto, or absent: enables it for openssl if supported, disables
+	#  it for pkcs11
+	# EDDSA requires openssl 1.1.1 or later.
+	# If EDDSA is enabled, extra symbols will appear in libdns110x.
 	dh_auto_configure -B build -- \
 		--libdir=/usr/lib/$(DEB_HOST_MULTIARCH) \
 		--sysconfdir=/etc/bind \
@@ -101,7 +108,6 @@ override_dh_auto_configure:
 		--enable-native-pkcs11 \
 		--with-pkcs11=\$${prefix}/lib/softhsm/libsofthsm2.so \
 		--with-randomdev=/dev/urandom \
-		--with-eddsa=no \
 		$(EXTRA_FEATURES)
 	dh_auto_configure -B build-udeb -- \
 		--sysconfdir=/etc/bind \
@@ -120,7 +126,6 @@ override_dh_auto_configure:
 		--enable-shared \
 		--with-libtool \
 		--with-gssapi=no \
-		--with-eddsa=no \
 		--libdir=/lib/$(DEB_HOST_MULTIARCH) \
 		--includedir=/usr/include/bind-export
 	sh debian/apply-export-patch