Bug 891768

Notes:
- License
Upstream has been notified of an absence of a License file.  Since the only script with any code in it has it's license in comments at the top, I figured that would do for now.
- Source1
Upstream had a glitch in their tagging, so the two main scripts are in their git repo, but not tagged into the 1.0.3 release.  This should be fixed in their next release.

I have some concerns :

* There is 3 scripts, and 1 is just a exec "$@" without anything, and the 2nd one is just ruby "$@".

I guess that's a system to integrate scl into origin without using some configuration ? 

Wouldn't it be better to directly link those to bash and ruby instead of starting a process and a fork ?
At least, a comment in the script would help. ( but that's not blocking ).


* Also, there is some missing deps for the 3rd script ( oo-diagnotics ).
I see it use :
- bind-utils ( command host )
- rubygems ( command gem, but maybe that's now part of ruby, not sure about this )

Technically, i think there is also missing requires for 
ruby module for json ( ie rubygems-json ) and rest_client ( rubygems-rest-client ). However, as the script first check if this is a broker, then I guess no error can happen in practice, so I think this can be safely skipped.

That's blocking.


* the changelog is empty for :

  * Wed Nov 21 2012 Dan McPherson <dmcphers> 1.0.3-1
  -

while I guess that not a issue, it would be nice to just fill it :)


* test_broker_httpd_error_log and test_node_mco_log are insecure, as they use file in /tmp/. While this is not a easy predictable name ( as upstream used $$ ), that's still predictable during a very short period of time ( ie, between the moment the script is run and after the script start to use sed and grep redirected to the file ). 

And there is no check to see if the file already exist or anything like that. 

So it could be quite easily created in advance with a simple for loop, or if the script was run in some automatic fashion ( like nagios ), someone could create a file /tmp/oodiag-mco-1234  by getting the process id at the right moment. Usually, attackers tend to brute force by running the script often enough to catch it at the right moment, I do not see how this would be done in practice in this case.

Then, once you have the file name, you can do multiple fun things ( for some value of fun ) :
- 1) create a link to another file ( the script run as root, so it can overwrite almost anything, I am sure the system would not work as well with sorted error log in pam config file )
- 2) create a file with cooked error message to trigger a error for the script without them being real ( and using chattr +i on the file so the file redirection would silently fail ). I guess in a very specific totally invented scenario, this could be used to inject error message ( and that could be bad if that's show in a web page or anything ).

Definitely nothing serious or dangerous that would happen on a properly managed system ( due to kernel protection, etc ) but IMHO, something that could and should be corrected one day. I didn't fill a more formal bug as the package is not yet in Fedora, so no need for a CVE.

Spec URL: http://tdawson.fedorapeople.org/openshift-origin/openshift-origin-util.spec
SRPM URL: http://tdawson.fedorapeople.org/openshift-origin/openshift-origin-util-1.0.3-3.fc18.src.rpm

- Requires added: ruby, rubygems, bind-utils
- Changelog fixed
- Added comment to oo-exec-ruby explaining what it's for.  The same comment is in line to make it into the upstream oo-ruby.  so it will be there in the next release.
- Changed oo-diagnostics to use the ruby Tempfile for it's tempfiles.

Package Review
==============

Key:
[x] = Pass
[!] = Fail
[-] = Not applicable
[?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
- License field in the package spec file matches the actual license.
  Note: Checking patched sources after %prep for licenses. No licenses found.
I guess you can convince upstream to ship a license for next release ?

- Packages should try to preserve timestamps of original installed files.
just add -p at the right place

- SourceX tarball generation or download is documented.
I assume this is just temporary, and will be correct for the next release

So, since everything is temporary and should be easy to fix, package is approved.

===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Package complies to the Packaging Guidelines
[!]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Fully versioned dependency in subpackages, if present.
[x]: Spec file lacks Packager, Vendor, PreReq tags.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 0 bytes in 0 files.
[x]: Packages must not store files under /srv, /opt or /usr/local
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).

===== SHOULD items =====

Generic:
[!]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[-]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[!]: SourceX tarball generation or download is documented.
     Note: Package contains tarball without URL, check comments
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: %check is present and all tests pass.
[!]: Packages should try to preserve timestamps of original installed files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define.

===== EXTRA items =====

Generic:
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: openshift-origin-util-1.0.3-3.fc18.noarch.rpm
openshift-origin-util.noarch: W: no-documentation
openshift-origin-util.noarch: W: no-manual-page-for-binary oo-exec-ruby
openshift-origin-util.noarch: W: no-manual-page-for-binary oo-ruby
openshift-origin-util.noarch: W: no-manual-page-for-binary oo-diagnostics
1 packages and 0 specfiles checked; 0 errors, 4 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint openshift-origin-util
openshift-origin-util.noarch: W: no-documentation
openshift-origin-util.noarch: W: no-manual-page-for-binary oo-exec-ruby
openshift-origin-util.noarch: W: no-manual-page-for-binary oo-ruby
openshift-origin-util.noarch: W: no-manual-page-for-binary oo-diagnostics
1 packages and 0 specfiles checked; 0 errors, 4 warnings.
# echo 'rpmlint-done:'



Requires
--------
openshift-origin-util (rpmlib, GLIBC filtered):
    /bin/bash
    /usr/bin/env
    bind-utils
    ruby
    rubygems



Provides
--------
openshift-origin-util:
    openshift-origin-util



MD5-sum check
-------------
http://mirror.openshift.com/pub/openshift-origin/source/openshift-origin-util/openshift-origin-util-1.0.3.tar.gz :
  CHECKSUM(SHA256) this package     : 93d149da3ba1e2bb58af2675ad7991ecfe9711a0229d4d2bfad1fae2a632a6e3
  CHECKSUM(SHA256) upstream package : 93d149da3ba1e2bb58af2675ad7991ecfe9711a0229d4d2bfad1fae2a632a6e3


Generated by fedora-review 0.2.0 (Unknown) last change: Unknown
Buildroot used: fedora-18-x86_64
Command line :./try-fedora-review -b 891768

New Package SCM Request
=======================
Package Name: openshift-origin-util
Short Description: Utility scripts for the OpenShift Origin
Owners: tdawson maxamillion
Branches: f18 f17
InitialCC:

Git done (by process-git-requests).

openshift-origin-util-1.0.3-3.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/openshift-origin-util-1.0.3-3.fc18

openshift-origin-util-1.0.3-3.fc18 has been pushed to the Fedora 18 testing repository.

openshift-origin-util-1.0.3-3.fc18 has been pushed to the Fedora 18 stable repository.