[MIR][SRU] new package - joyent-mdata-client

I've reviewed lp:~utlemming/+junk/mdata_tools/ for an upload to universe.

In general the packaging is of a high quality. Thank you Ben! A few minor issues that I've patched (attached):

* Fix LP bug reference number in debian/changelog.
* Convert debian/copyright to machine-readable dep5 format (current best practice).
* Rename get-packaged-orig-source debian/rules target to get-orig-source (Debian policy section 4.9).
* Fix get-orig-source target to produce orig tarball with directory prefixed.
* Fix get-orig-source target to clean up after itself.

Some items not fixed; I don't think these are major enough to block upload. I suggest we create bugs for these after the package is accepted into NEW:

* get-orig-source target should work from any directory according to Debian policy; but it's an optional target and the spirit of the policy is to make it possible for anybody to see and reconstruct the orig tarball, which is fulfilled here.

* /lib/smartdc/mdata-get is non-standard, and I think deserves some documentation in the packaging. I understand from Ben that it is a legacy which some people might expect, and that it is used for boot when cloud-init isn't working. Do we need to explain this in a README.Debian, perhaps?

Recommendations to upstream:

Please drop "All rights reserved" from the copyright notice; this is not required any more and contradicts the licence statement. Please also explicitly add the licence to each source file. This would make the licensing situation completely unambiguous.

Ideally each source file should have copyright notices, a licence statement, and no "all rights reserved" statement. Right now, they all have a copyright and "all rights reserved" but no licence statement. reqid.c has nothing.

Debian appear to accept the contradiction in general because the intent to licence MIT is clear, so I think this is OK for Ubuntu too. However, it's common practice to annotate source files with a copyright notice and licence; the upstream source files in this project do state copyright and "All rights reserved" but do not state licences in each file. I think this is still acceptable, but more contradictory than what Debian have accepted previously. See http://lists.debian.org/debian-mentors/2013/07/msg00149.html for discussion.

One issue blocking upload (at least for me):

crc32.c contains a copyright statement from Gary S. Brown but does not include licensing information. Thus it is not clear if this file is re-distributable. I found http://lists.debian.org/debian-legal/2003/02/msg00100.html which suggests that Gary S. Brown has never explicitly licensed this code for redistribution. Do we know if it is permitted for this file to be released under the MIT licence? Given the triviality of the code, I guess it is probably easiest to replace crc32.c with a DFSG-compliant implementation? These appear to be available, but it might need some API adaptation.

PS. I noticed that there was no debian/watch file, but upstream don't have releases (yet, at least), so I don't think it makes sense to have one right now.

Thanks Robie for the feedback.

I've fixed in the packaging branch.
1. LP Bug reference number in debian/changelog
2. Implemented dep5 format debian/copyright
3. Used get-orig-source instead of get-packaged-orig-sourc
4. Added directory prefix for get-orig-source (based on Robie's patch)
5. Implemented cleanup.

I've also asked for Joyent to address the Copyright and CRC32 issues. Once I have a new branch to work with from Joyent, I'll rebase and get a new review.

I reviewed mdata-tools as checked into lp:~utlemming/+junk/mdata_tools on
Tuesday 5 November 2013, a temporary staging area while being packaged for
trusty.

- No CVE history visible at the time of the audit
- The package provides several command-line utilities and a corresponding
  serial-line protocol to manage the client-side of metadata in
  SmartOS-managed KVM (and perhaps Zones?) instances.
- No cryptography
- Some Unix-domain sockets
- No 'networking' but relies upon serial communication with a hypervisor
- No daemons
- I didn't see pre/post inst/rm scripts
- I didn't see init scripts
- I didn't see dbus services
- I didn't see setuid executables
- I didn't see sudo fragments
- I didn't see udev rules
- Provides mdata-get, mdata-put, mdata-delete, mdata-list binaries.
- No test suite

- No subprocesses spawned
- Nearly all memory management is checked and safe
- Most file operations have hard-coded pathnames
- No environment use
- Very nice safer-strings helper routines
- Some privileged serial-port handling, looked safe, they even included
  helpful hints to proper documentation
- Some unix-domain networking, name handling looked safe
- Command stream is parsed with a simple and careful hand-written scanner
- No WebKit
- Since hypervisor will handle serial, MITM attacks are unlikely
- No PolicyKit
- No Javascript

mdata-tools is high-quality code programmed in a professional manner.

Some unit testing code would be nice to have, but the actual function of
the utilities would be quite difficult to test in any automated fashion.

I found several minor issues while reviewing the code that I thought I
should call out in the hopes that it is useful to someone:

- plat_init() unchecked calloc() call
- open_md(), unix_open_serial() O_EXCL used without O_CREAT; O_EXCL
  doesn't influence character device opens. Undefined behavior, but
  probably harmless.

(Issues have been filed at https://github.com/joyent/mdata-client/issues)

Security team ACK for promoting to main.

Thanks

Worked with upstream to fix copyright headers. Packaging now uses a watch file as well.

We're ready for the SRU team to review.

Regarding the crc32.c copyright issue, the new branch uses a new implementation of crc32.c that is _different_ from the the Gary S. Brown. implementation.

So, I'm accepting this directly to main with a few notes to be fixed in the next upload:

1) EXPAT is spelled "Expat" in dep-5
2) Licensing your debian/* (which may contain patches) with a non-upstream-compatible license probably isn't clever, I'd recommend switching from GPL-2 to Expat for the packaging too, so you don't have to explicitly call out patches later as being safe for upstream consumption.
3) Please make sure there's a team bug subscription on the package as soon as it's accepted and LP knows it exists.
4) Please toss it in some supported seed so it doesn't fall out of main.
5) Please talk to upstream about that weird non-FHS legacy path. If they can live without it, we really don't want stuff like that setting a precedent for other people thinking it's hip and cool to litter all over standards and the filesystem.

6) kill off the last lintian warning on your next upload too:

I: joyent-mdata-client source: binary-control-field-duplicates-source field "homepage" in package joyent-mdata-client

On Fri, Nov 08, 2013 at 03:57:12AM -0000, Adam Conrad wrote:
> 5) Please talk to upstream about that weird non-FHS legacy path. If
> they can live without it, we really don't want stuff like that setting
> a precedent for other people thinking it's hip and cool to litter all
> over standards and the filesystem.

Could we, for example, use cloud-init's user data vendor merging thing to
have a runcmd create the symlink only when it's needed, to keep it out
of the packaging? Just a thought.

What is the use case for needing the symlink?

This bug was fixed in the package joyent-mdata-client - 0.0.1-0ubuntu1

---------------
joyent-mdata-client (0.0.1-0ubuntu1) trusty; urgency=low

  * Initial release (LP: #1248000).
 -- Ben Howard <email address hidden> Thu, 07 Nov 2013 08:14:52 -0700

Having the symlink outside the packaging is even worse than shipping it, as removing the package would then leave a dangling link. Ideally, we shouldn't be shipping (or creating) non-FHS clutter at all, unless it's absolutely, undeniably REQUIRED by this platform. In which case, we should convince them to fix their platform, and grudgingly ship the link for now.

joyent-mdata-client replaces a bash script that has traditionally on SmartOS instances resided in /lib/smartdc. On non-Cloud-init booted systems, this file is required to get the meta-data. The inclusion of the link in /lib/smartdc is for historical reasons and to support users that have written scripts against it.

Building packages into PPA for SRU, and validating builds.

Confirmed PPA packages, pending upload to -proposed.

Packages are now waiting in the new queue for Precise, Quantal, Raring and Saucy.

We are now ready for the SRU processing.

Hello Ben, or anyone else affected,

Accepted joyent-mdata-client into precise-proposed. The package will build now and be available at http://launchpad.net/ubuntu/+source/joyent-mdata-client/0.0.1-0ubuntu2~12.04.1 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 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, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

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

Hello Ben, or anyone else affected,

Accepted joyent-mdata-client into quantal-proposed. The package will build now and be available at http://launchpad.net/ubuntu/+source/joyent-mdata-client/0.0.1-0ubuntu2~12.10.1 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 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, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

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

Hello Ben, or anyone else affected,

Accepted joyent-mdata-client into raring-proposed. The package will build now and be available at http://launchpad.net/ubuntu/+source/joyent-mdata-client/0.0.1-0ubuntu2~13.04.1 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 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, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

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

Hello Ben, or anyone else affected,

Accepted joyent-mdata-client into saucy-proposed. The package will build now and be available at http://launchpad.net/ubuntu/+source/joyent-mdata-client/0.0.1-0ubuntu2~13.10.1 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 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, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

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

The verification of this Stable Release Update has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regresssions.

This bug was fixed in the package joyent-mdata-client - 0.0.1-0ubuntu2~13.10.1

---------------
joyent-mdata-client (0.0.1-0ubuntu2~13.10.1) saucy; urgency=low

  * Backport from trusty to saucy for Joyent IaaS (LP: #1248000).
 -- Ben Howard <email address hidden> Fri, 22 Nov 2013 12:14:36 -0700

This bug was fixed in the package joyent-mdata-client - 0.0.1-0ubuntu2~13.04.1

---------------
joyent-mdata-client (0.0.1-0ubuntu2~13.04.1) raring; urgency=low

  * Backport from trusty to raring for Joyent IaaS (LP: #1248000).
 -- Ben Howard <email address hidden> Fri, 22 Nov 2013 12:14:36 -0700

This bug was fixed in the package joyent-mdata-client - 0.0.1-0ubuntu2~12.10.1

---------------
joyent-mdata-client (0.0.1-0ubuntu2~12.10.1) quantal; urgency=low

  * Backport from trusty to quantal for Joyent IaaS (LP: #1248000).
 -- Ben Howard <email address hidden> Fri, 22 Nov 2013 12:14:36 -0700

This bug was fixed in the package joyent-mdata-client - 0.0.1-0ubuntu2~12.04.1

---------------
joyent-mdata-client (0.0.1-0ubuntu2~12.04.1) precise; urgency=low

  * Backport from trusty to precise for Joyent IaaS (LP: #1248000).
 -- Ben Howard <email address hidden> Fri, 22 Nov 2013 12:14:36 -0700