suggestions from debian packaging
Brought to you by:
soupboy
Menu
▾
▴
1 Attachments
#8 suggestions from debian packaging
as of trying to prepare the pam-ssh-agent-auth for inclusion in debian (see [1] for details), there are some suggestions i'd like to make with for the program:
-
copyright notice: for those files that don't have an explicit copyright notice in themselves, the only statement about their license is in your debian/copyright file so far, and is ambiguous ("BSD" can refer both to 2-clause and 3-clause BSD, and the file you referenced is deprecated in Debian). if you add a section to the README or CONTRIBUTORS file that states "This projects' files are written by the authors mentioned in CONTRIBUTORS, and published under the terms below unless noted differently in the file itself", followed by a license statement as is found in pam_ssh_agent_auth.c or similar.
-
warnings: one of the checks debian packages usually run through are checks for warnings. most of them concern themself with unused results of functions that can fail; just grep for 'warn' in your build output to see the details.
-
man page: i'm no expert myself, but the pod file does not seem to conform to some usual guidelines, and has a typo. see the patch attached.
-
sudoers: i'm using pam-ssh-agent-auth without env_keep+="SSH_AUTH_SOCK" in sudoers. is that really required? my impression is that it's only needed when people want to use their agents from inside sudo (ie. ssh from there or do sudo again), which might be worth mentioning, but should not be something everyone configures when he sets up pam-ssh-agent-auth.
-
debian/ directory: the packaging data you provide is in parts outdated, in parts overly complex. it is generally not expected of software authors to provide their own debian/ directory; feel free to keep it (possibly with updated contents), but be aware that is is discarded in the debian package, unless you do the complete packaging (including making sure everything conforms to the debian policy) yourself.
so in short: if you want to participate in debian packaging, you are very welcome to do so and participate in [1] (helpful resources for starting can be found in [2]); if you can't or don't want to (eg because you are not using debian yourself), i'll see to it that even if [1] does not yield results soon, there is at least something easily installable available.
- autotools: i tried to rebuild the autotools files involved using autoreconf (not debian standard procedure, but it sometimes helps when old autotools versions created the files in the first place, to get rid of some warnings), but it failed. ("missing template: BROKEN_STRNVIS", and many warnings). is there a particular autotools setup you use?
best regards, and thanks for accepting my earlier patches
chrysn
[1] https://bugs.debian.org/595817
[2] https://wiki.debian.org/IntroDebianPackaging
In order:
1) Copyright notices: I knew I missed some, I'll go through and fix that
2) I build without warning with gcc 4.8.1; I'll go back and see if I can enable more warnings and find any that are being emitted.
3) thanks for the patch!
4) It is required in sudo < 1.7.12 iirc (perhaps 1.8); it changed the handling of the environment pre/post authentication, so it is no longer required in recent versions.
5) I threw the debian stuff together at the same time I threw together the rpm and BSD packaging stuff. None of it is especially guidelines compliant (and hasn't been especially well maintained). If you have current packaging stuff for debian, I'll happily incorporate it. I generally assume packagers throw out the stuff in the the archive; I include it primarily for individuals wanting to install via package on their platform after downloading the tarball.
and as for autotools; the autotools stuff is a heavily modified version of the original openssh autotools stuff, and was rather severely wonky to begin with. You cannot use 'autoreconf' as you would normally, but instead you must run aclocal followed by autoconf in the old way. With autoconf 2.63 and 1.11.1 it generates without warning. I'll update for more recent versions of the autotools; I hadn't realized how outdated centos6 was. But running aclocal + autoconf will regenerate without error even on current autotools, but with many warnings (mostly related to the change requiring AC_LANG_SOURCE being the new required way to specify sourcecode in AC_COMPILE_IFELSE in recent versions; which can be ignored for now).
I'm more curious though why you needed to autoreconf? did it fail configure or build on ubuntu? if so, that's definitely a bug, and I'll spin up an ubuntu today after work and get that resolved. (I didn't have a recent ubuntu vm handy to test the latest autoconf changes on over the weekend)
Files: compat.c config.h.in CONTRIBUTORS dists/* get_command_line.c get_command_line.h identity.h Makefile.in pam_ssh_agent_auth.pod pam_ssh_agent_auth.sudoers.conf pam_static_macros.h README
Copyright: Jamie Beverly
License: BSD-3-clause
Comment: These files don't have explicit license statements, but are covered by
the project's license statement. Until this has been clarified better, the
statement is taken from upstream's debian/copyright file. (It is assumed that
2-clause BSD license is meant by it, but it references the Debian common BSD
file, which is a 3-clause license).
(you can check the complete one out if you use git[1].)
gcc -g -O2 -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -fPIC -Wall -Wpointer-arith -Wuninitialized -Wsign-compare -Wno-pointer-sign -fstack-protector-all -I. -I. -D_FORTIFY_SOURCE=2 -DHAVE_CONFIG_H -c iterate_ssh_agent_keys.c
iterate_ssh_agent_keys.c: In function ‘pamsshagentauth_session_id2_gen’:
iterate_ssh_agent_keys.c:140:11: warning: ignoring return value of ‘getcwd’, declared with attribute warn_unused_result [-Wunused-result]
getcwd(pwd, sizeof(pwd) - 1);
environment variables: sudo 1.7.2 came out in 2009; imo this should warrant a historical note at best these days.
autotools: i didn't have a particular reason to need autoreconf, i only made the experience with other packages that in general, things tend to run more smoothly with autoreconf (especially when it comes to building on exotic platforms). if the autotools building is something customized and well understood by you here, i'll just leave it in place as is.
[1] git clone git://prometheus.amsuess.com/pam-ssh-agent-auth -b debian
Apparently several commits ago I fixed most of the issues with autoreconf; so only had to fix a few things to make it happy. it should be silent now. Aside from fixing the debian packaging mess, I think I got everything.
thank you, jamie.
i've updated the debian copyright files accordingly.
unused return values: i'm concerned about your solution, as it changes the packed structure depending on which syscalls happend to work. my attached patch inserts empty strings instead. signing clients can decide for themselves what it means to them, but at least they won't get out of sync with the stream.
concerning sudo versions: i did a few tests of my own; it seems the setting is still required in sudo versions as high as 1.8.5. did you test 1.7.2, or was it just your best guess?
autoreconf: autoreconf works now (and generates practically identical files), but ./configure does not any more ("configure: error: cannot find sources (config.h) in . or ..")
it might be a matter of personal style, but i'll mention it anyway: i would have a much easier time reviewing pam-ssh-agent-auth if you committed changes individually (instead of huge patches fixing bunches of things like rev101), and if you didn't check in files like config.h.in and configure (which should be present in the tarball nevertheless, of course). that might be influenced by my use of git, but i think it might benefit this project too.
valid points all.
1) I expect your patch looks like what I had intended to write, creating empty strings or similar when calls fail, but I was in a bit of a hurry last night.
2) 1.8.5? It was just a guess from memory that it was fixed in 1.7.12, apparently my guess was off. I'll update the docs to reflect the correct sudo-version.
3) I didn't encounter the configure issue myself, let me see how to reproduce it and fix today after work.
4) I'll try to remember to check generated files in separately in the future. They do need to be checked in, however, as persons checking out of svn may not have autoconf/automake/aclocal on their host; plus, they should be treated as versioned artifacts, as they can differ depending upon tool versions, and the version checked in trunk is a version known to work. But for the purposes of making the patches easier to consume, I'll commit the input files, then the generated files and indicate purely generated commits with in their comment.
Hi,
The project is now packaged in Debian, please file packaging related bugs/suggestions to Debian's BTS.
https://tracker.debian.org/pkg/pam-ssh-agent-auth
Since this issue is still open, I guess it's as good a place as any to mention that the project is removed from testing and thus risk getting being dropped by Debian unless action is taken.
Relevant links: Please migrate to openssl1.1 in buster, pam-ssh-agent-auth REMOVED from testing, https://packages.debian.org/libpam-ssh-agent-auth
Actually covering the possible migration to openssl 1.1 is likely a topic for a separate issue. However, a relevant question could be whether targeting the newer openssl version or completely replacing the package with the code from pam_ssh_agent_auth-2.0 is be the better path to take?