Hi Helmut,
On Thu, 30 Jul 2015 at 22:21:21 +0200, Helmut Grohne wrote:
> Thanks for your work on dropbear. Much appreciated.
And thanks for the extensive feedback! I know it's quite a heavy
changelog, so I didn't really expect anyone other than Gerrit perhaps to
check it out that closely.
> On Sat, Jul 11, 2015 at 03:20:52PM +0200, Guilhem Moulin wrote:
>> Note that while the current maintainer (Gerrit, CC'ed) told me to go
>> ahead and proceed with a NMU, they are not able to sponsor me at the
>> moment. Furthermore I'm currently a DM and would be open to
>> co-maintenance once time is ripe.
>
> My reading of Gerrit's mail is a bit more limited. He wrote to d-devel:
> | Unfortunately I cannot support you, but don't hesitate to NMU the
> | dropbear package to be able to proceed.
>
> May be this is hair-splitting, but I'd read this as NMUing specifically
> for the purpose of splitting out the initramfs stuff (and then moving it
> to a different source package). I'd not assume that doing a new upstream
> release is covered by the above. Maybe Gerrit can clarify that or we can
> go via a delayed queue.
Yeah true, I might have taken too much liberty here. I noticed upstream
was already two releases ahead so I wildly pulled in the new tarball :-/
(plus it fixed #775222 ;-) Then I couldn't split the package without
doing major refactoring (which thanks to debhelper automatically solved
#689618, #692932, #777324, #793006, #793917). And later I guess I got
carried away with enthusiasm on the way and decided to try to fix the
remaining bugs (some of them were quite old, and most fixes were rather
easy; at least the one relevant to the initramfs feature, which didn't
seem to be maintained anymore, and which bothered me personally since I
use this feature myself) :-P
> Despite me having lots of comments, your changes are actually of high
> quality. Don't get scared! Some comments are picky/matter of taste. Feel
> free to disagree.
No worries. Constructive criticism never hurts anyway ;-)
> Since you bump the upstream release your NMU version should be -0.1
> instead of -1.1.
Thanks, fixed.
> I note that the new tilde expansion functionality in the upstream
> release is a bit strange. If my own home is "/home/helmut", it will
> expand "~/foo" to "/home/helmut//foo" (note the double slash). Worse, it
> will expand expand "~otheruser/foo" to "/home/helmut/otheruser/foo",
> which does not match the general expectation of tilde expansion.
Well spotted. I guess one ought to file a bug.
> Gerrit asked for the binaries to be located in "dropbear", but you place
> them in "dropbear-bin". Maybe Gerrit can also ack this? (It was stated
> as a preference.)
In fact in a later mail in the d-d thread Gerrit agreed to make
“dropbear” a transitional package and place binaries to “dropbear-bin”,
as suggested to me by Adam Borowski:
https://lists.debian.org/debian-devel/2015/06/msg00290.html
> Since dropbear-initramfs relies on initramfs-tools 0.94 features, the
> dependency should be versioned.
Thanks, fixed. (Didn't think it mattered since even oldoldstable has
said feature.)
> If you disable password logins by default, please mention in a NEWS
> entry!
It was never enabled in the initramfs (disabling it without notice would
have been foolish otherwise, indeed). The change I made (initramfs-only
only) is to no longer make the server *advertise* it, so that clients
won't prompt for a password and try to send it to the server (which is
bound to reject it anyway). Sorry for the confusion.
> Would it make sense to install the NEWS file to the transitional package
> only? (i.e. mv debian/NEWS debian/dropbear.NEWS)
Yes it would! Fixed, thanks for the suggestion.
> It might be safer in the future if dropbear-{initramfs,run} use a (=
> ${binary:Version}) dependency on dropbear-bin. Will you remember to bump
> such a dependency when dropbear-run starts to use a new option? (But
> --link-doc incurs this dependency anyway.)
You mean if upstream starts deprecate options used in
dropbear-{initramfs,run}? Which by the way seems to happen right now
with ‘-d’ with is now an alias for ‘-r’ but has been removed from the
manpage, see #761143. (And now I realize I could have written a note
regarding s/-d/-r/ in the NEWS regarding that change :-/)
Yeah that makes sense, I tightened the dependency by specifying the
${binary:Version}.
> Please consider whether Multi-Arch:foreign markings are appropriate for
> some packages.
Alright, this one is new to me. I'm not sure how blindly I can follow
https://wiki.debian.org/Multiarch/Implementation#dh.281.29_and_autotools
because dropbear-bin ships an executable ‘/usr/lib/dropbear/dropbearconvert’.
So checked the package source for openssh and found that openssh-server
uses Multi-Arch:foreign, but openssh-sftp-server, which ships
‘/usr/lib/openssh/sftp-server’, doesn't. So all in all I'm unsure what
to in my case.
> Why are dropbear-{initramfs,run} marked as Architecture:any? Do they
> contain any architecture specific files? Is that an artifact of
> --link-doc only? Maybe reconsider whether --link-doc is worth
> duplicating these packages for each architecture.
Yes, it's to make dh_installdocs happy, and avoid it spitting
WARNING: --link-doc between architecture all and not all packages breaks binNMUs
What are you suggesting as an alternative?
> I think it is an unfortunate choice to license the initramfs unlock
> stuff under GPL when the rest of the package is MIT. That's your choice
> of course. The mixing of GPL-2+ and GPL-3+ doesn't make things better.
Well I'd agree it doesn't hurt either (beside making the copyright file
longer) :-P. Anyway the original author of the initramfs feature chose
GPL-2+ back in 2008, so I picked the same license when working on the
same file.
> Your GPL license paragraphs should include the actual license grant, not
> just the reference to the GPL. (I think this point would be sufficient
> for a ftp-master rejection. This incurs the moreinfo tag.)
Oops. I believe I started doing that quite a while ago when I learned
packaging by example. I think took that one from git-annex
http://source.git-annex.branchable.com/?p=source.git;a=blob;f=debian/copyright
That said I'm happy to learn more and I have added the header already.
> dropbear-initramfs.postinst exits before running #DEBHELPER# unless $1
> is "configure". #DEBHELPER# should be run unconditionally.
Good catch, thanks.
> In dropbear-initramfs.postinst, when ssh-keygen is unavailable, the
> creation of the $pubkey could be avoided.
This one is really nitpicking IMHO :-P Fixed anyway.
> Some of those comments also apply to dropbear-run.postinst.
Fixed.
> It should not be necessary to pass --host to dh_auto_configure, as it
> does that automatically for cross builds only.
Didn't know that. Fixed, thanks.
> You pass CFLAGS to dh_auto_configure. Why do you not pass CPPFLAGS or
> LDFLAGS to configure? Both typically enable further hardening features.
No I *extend* CFLAGS with -DSFTPSERVER_PATH='"/usr/lib/sftp-server"'.
Aren't CPPFLAGS and LDFLAGS passed as is already? The build log seems
to suggests it at least.
> In general, I'd find sponsoring this NMU much easier if the package
> split and the fixing of those many bugs could happen in separate
> uploads. Each part is complex and the fallout is hard to estimate. I
> understand that such a separation would mean more work for you. Do you
> think that doing this in two steps is worth the added effort?
Actually I spitted the package after merging the new upstream version
(which is easily revertible anyway), but *before* starting fixing bugs
in the BTS, so maybe it wouldn't be that hard to perform the upload in
two steps.
https://gitweb.guilhem.org/dropbear
That said, while I understand the heavy changelog is frown upon by
potential sponsors, I have to say I would find it quite frustrating if
my work on this package, including the fixes to the many bugs regarding
dropbear-initramfs (which I heavily rely on in my own infrastructure),
wasn't eventually uploaded :-P
However any step in the right direction is good to take IMHO :-) So
thanks again for your feedback and interest!
--
Guilhem.
Attachment:
signature.asc
Description: Digital signature