Spec URL: https://sgallagh.fedorapeople.org/sscg/python-sscg.spec
SRPM URL: https://sgallagh.fedorapeople.org/sscg/python-sscg-0.1-2.fc22.src.rpm


Updating spec because I forgot to include BuildRequires: python-setuptools.

Built successfully in Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=9247922

%py3dir is not to be used anymore since it stores files outside %_builddir where they are not removed by rpmbuild:

  https://fedoraproject.org/w/index.php?title=PackagingDrafts%3APython&diff=403829&oldid=403815
  [ https://fedorahosted.org/fpc/ticket/435 ]


> %build
> # Remove CFLAGS=... for noarch packages (unneeded)
> CFLAGS="$RPM_OPT_FLAGS" %{__python2} setup.py build

The comments raises questions. The package is "noarch", so the CFLAGS definition is really not needed.


> %files
> %doc

An empty %doc line is a no-op and should be deleted. %doc is not a spec file section like %build (an empty %build section could be deleted, too, but the guidelines want it to be empty instead).

Since you are upstream, consider this:
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text

(In reply to Michael Schwendt (Fedora Packager Sponsors Group) from comment #2)
> %py3dir is not to be used anymore since it stores files outside %_builddir
> where they are not removed by rpmbuild:
> 
>  
> https://fedoraproject.org/w/index.
> php?title=PackagingDrafts%3APython&diff=403829&oldid=403815
>   [ https://fedorahosted.org/fpc/ticket/435 ]
> 
> 

OK, I will change that. I copied this bit from firewalld since it was my first time doing a dual-stack build. Thanks for the pointer.

> > %build
> > # Remove CFLAGS=... for noarch packages (unneeded)
> > CFLAGS="$RPM_OPT_FLAGS" %{__python2} setup.py build
> 
> The comments raises questions. The package is "noarch", so the CFLAGS
> definition is really not needed.
> 
> 

Yeah, I generated the spec with rpmdev-newspec -t python and it included that. I can remove it.


> > %files
> > %doc
> 
> An empty %doc line is a no-op and should be deleted. %doc is not a spec file
> section like %build (an empty %build section could be deleted, too, but the
> guidelines want it to be empty instead).
> 

Right, as above this was generated by the newspec and I forgot to remove it.

> Since you are upstream, consider this:
> https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text

Yes, I remembered that just after cutting the 0.1 tarball. As code audit is revealing a couple minor issues, I'm going to cut a 0.2 tarball today with the license text in it.

Thanks for the review!

Spec URL: https://sgallagh.fedorapeople.org/sscg/python-sscg.spec
SRPM URL: https://sgallagh.fedorapeople.org/sscg/python-sscg-0.2.1-1.fc22.src.rpm

Built in Koji:

http://koji.fedoraproject.org/koji/taskinfo?taskID=9252945 (i686)
http://koji.fedoraproject.org/koji/taskinfo?taskID=9252952 (x86_64)
http://koji.fedoraproject.org/koji/taskinfo?taskID=9252950 (armv7hl)

Running 2to3 over the sources does not actually do anything, except remove some imports from __future__. So the 2to3 step can be skipped. And this allows nice simplifications to be made in packaging, because if 2to3 is not run, there's no build step, and the install step for Python 2 and 3 can be run from the source directory.

I also think providing two binaries (for Python 2 and 3) is just confusing. Installing one which runs under whichever version of python is default should be enough.

So the whole procedure could look like:
%prep
%setup ...
rm -rf src/*.egg-info

%build
%{__python2} setup.py build

%install
%{__python2} setup.py install --skip-build --root=%{buildroot}
%{__python3} setup.py install --skip-build --root=%{buildroot}
# or in reverse order

This provides proper #! lines.

--

In the python code, a bare try: ... except: (without any exception name after "except") is used. In some places the exception is re-raised, but in others not. This is a problem because the script might be ended with ^C (or a signal) and silently "swallow" the exception. The script should always exit after being interrupted. It also should not print an error message like "Failed to write..." when killed.

It seems that gettext is called on formatted arguments:
   _("Could not write to {0}. Error: {1}".format(...))
but it should be before:
   _("Could not write to {0}. Error: {1}").format(...)

Why open the temp file in text mode, then decode and re-encode the certificate. Maybe it would be simpler to open it in binary mode and skip the decode step?

It seems that this will be usable a library. I think that calling sys.exit() should not be done, except in main. Errors should not be printed either. Simply raising an exception with an appropriately formatted message and letting the caller handle it would nicely simplify things.

It seems that tempfile.NamedTemporaryFile would do the right thing without requiring all this manual cleanup:

--- src/sscg/__init__.py
+++ src/sscg/__init__.py
@@ -28,36 +28,24 @@ def write_certificate(options, cert, destination):
     # Create the temporary file in the same directory as the destination
     # This ensures that we can atomically move it to the final name.
 
+    f = tempfile.NamedTemporaryFile(dir=os.path.dirname(destination))
     try:
-        (fd, fpath) = tempfile.mkstemp(dir=os.path.dirname(destination))
-        if options.debug:
-            print(_("Creating temporary certificate file at {}".format(fpath)))
-        f = os.fdopen(fd, "w")
-
-        f.write(crypto.dump_certificate(options.cert_format, cert).decode("UTF-8"))
-        f.close()
-    except:
-        # Something went wrong. Remove the temporary file before failing.
-        print(_("Could not write to {0}. Error: {1}".format(
-              fpath, sys.exc_info()[1])),
-              file=sys.stderr)
-        os.unlink(fpath)
-        raise
+        f.write(crypto.dump_certificate(options.cert_format, cert))
+        f.flush()
+    except IOError as e:
+        raise Exception(_("Could not write to {0}. Error: {1}").format(f.name, e))
 
     # Now atomically move the temporary file into place.
     # We use os.rename because this is guaranteed to be atomic if it succeeds
     # This operation can fail on some flavors of UNIX if the source and
     # destination are on different filesystems, but this should not be the case.
+    if options.debug:
+        print(_("Renaming {} to {}").format(f.name, destination))
     try:
-        if options.debug:
-            print(_("Renaming {} to {}".format(fpath, destination)))
         os.rename(fpath, destination)
-    except:
-        # Something went wrong. Remove the temporary file before failing.

-              destination, sys.exc_info()[1])))
-        os.unlink(fpath)
-        raise
+    except IOError as e:
+        raise Exception(_("Could not rename to {0}. Error: {1}").format(destination, e))
+    f.delete = False
 
(sorry, this should have been part of the previous paste.)

(In reply to Zbigniew Jędrzejewski-Szmek from comment #5)
> Running 2to3 over the sources does not actually do anything, except remove
> some imports from __future__. So the 2to3 step can be skipped. And this
> allows nice simplifications to be made in packaging, because if 2to3 is not
> run, there's no build step, and the install step for Python 2 and 3 can be
> run from the source directory.
> 
> I also think providing two binaries (for Python 2 and 3) is just confusing.
> Installing one which runs under whichever version of python is default
> should be enough.

Well, I was trying to address a potential containerized case where either python release might be available. If I was only going to include the script for the default python, it doesn't make sense to build both (this module isn't really intended to be imported by other scripts, though it could be). So if you're building a Dockerfile and want the image to generate a certificate on startup, you probably want to be able to use whichever version of the script matches the version of python already in the environment.

> 
> So the whole procedure could look like:
> %prep
> %setup ...
> rm -rf src/*.egg-info
> 
> %build
> %{__python2} setup.py build
> 
> %install
> %{__python2} setup.py install --skip-build --root=%{buildroot}
> %{__python3} setup.py install --skip-build --root=%{buildroot}
> # or in reverse order
> 
> This provides proper #! lines.
> 

Yeah, actually the sed thing is a leftover, since I don't have any #! lines in my actual code; just in the generated scripts.


> --
> 
> In the python code, a bare try: ... except: (without any exception name
> after "except") is used. In some places the exception is re-raised, but in
> others not. This is a problem because the script might be ended with ^C (or
> a signal) and silently "swallow" the exception. The script should always
> exit after being interrupted. It also should not print an error message like
> "Failed to write..." when killed.
> 


Ah, where did I do that? It wasn't intentional.

> It seems that gettext is called on formatted arguments:
>    _("Could not write to {0}. Error: {1}".format(...))
> but it should be before:
>    _("Could not write to {0}. Error: {1}").format(...)
> 

Typo, thanks for catching it.

> Why open the temp file in text mode, then decode and re-encode the
> certificate. Maybe it would be simpler to open it in binary mode and skip
> the decode step?
> 

I don't do I? I thought the default was binary mode, which is why I was decoding the UTF-8 to the binary representation. Or have I confused myself? (I don't like dealing with encodings...)


> It seems that this will be usable a library. I think that calling sys.exit()
> should not be done, except in main. Errors should not be printed either.
> Simply raising an exception with an appropriately formatted message and
> letting the caller handle it would nicely simplify things.

It's not supposed to be used directly as a library, only as a script.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #6)
> It seems that tempfile.NamedTemporaryFile would do the right thing without
> requiring all this manual cleanup:
> 

The reason I'm using the tempfile.mkstemp() implementation is this:

"Creates a temporary file in the most secure manner possible. There are no race conditions in the file’s creation, assuming that the platform properly implements the os.O_EXCL flag for os.open()."

I very much do not want to worry about a race when creating a certificate, particularly a CA certificate.

(In reply to Stephen Gallagher from comment #9)
> (In reply to Zbigniew Jędrzejewski-Szmek from comment #6)
> > It seems that tempfile.NamedTemporaryFile would do the right thing without
> > requiring all this manual cleanup:
> > 
> 
> The reason I'm using the tempfile.mkstemp() implementation is this:
> 
> "Creates a temporary file in the most secure manner possible. There are no
> race conditions in the file’s creation, assuming that the platform properly
> implements the os.O_EXCL flag for os.open()."
TemporaryFile and NamedTemporaryFile and mkstemp share the implementation, so there's no difference in security, only in the way that file is automatically deleted.

(In reply to Stephen Gallagher from comment #8)
> Well, I was trying to address a potential containerized case where either
> python release might be available. If I was only going to include the script
> for the default python, it doesn't make sense to build both (this module
> isn't really intended to be imported by other scripts, though it could be).
> So if you're building a Dockerfile and want the image to generate a
> certificate on startup, you probably want to be able to use whichever
> version of the script matches the version of python already in the
> environment.
But this actually makes things more complicated for the user, because if the have only the non-default version installed, there's no /usr/bin/sscg. So maybe there shouldn't be packages for both versions, just one, always providing /usr/bin/sscg, and internally it should use either python2 or python3, whichever one is the default for this Fedora version.

> > In the python code, a bare try: ... except: (without any exception na
> > after "except") is used. In some places the exception is re-raised, but in
> > others not. This is a problem because the script might be ended with ^C (or
> > a signal) and silently "swallow" the exception. The script should always
> > exit after being interrupted. It also should not print an error message like
> > "Failed to write..." when killed.
> Ah, where did I do that? It wasn't intentional.
I'm not sure about what one specifically you're asking... "bare" except is used almost everywhere.

In src/sscg/main.py there is this:

           try:
                os.unlink(file)
           except:
                # Nothing we can do if we get an error
                pass

> > It seems that gettext is called on formatted arguments:
> >    _("Could not write to {0}. Error: {1}".format(...))
> > but it should be before:
> >    _("Could not write to {0}. Error: {1}").format(...)
> > 
> 
> Typo, thanks for catching it.
> 
> > Why open the temp file in text mode, then decode and re-encode the
> > certificate. Maybe it would be simpler to open it in binary mode and skip
> > the decode step?
> > 
> 
> I don't do I? I thought the default was binary mode, which is why I was
> decoding the UTF-8 to the binary representation. Or have I confused myself?
> (I don't like dealing with encodings...)
Yes, binary is the default. But decoding goes from bytes to unicode (i.e. text, i.e. str in Python 3). Encoding goes from text (i.e. str) to a specific encoding, for example UTF-8, and the bytes type. So you were writing text to a binary file. This actually does not work in Python 3:

>>> f = tempfile.NamedTemporaryFile()
>>> f.write('blah')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib64/python3.4/tempfile.py", line 399, in func_wrapper
    return func(*args, **kwargs)
TypeError: 'str' does not support the buffer interface

although it does work under Python 2.

> > It seems that this will be usable a library. I think that calling sys.exit()
> > should not be done, except in main. Errors should not be printed either.
> > Simply raising an exception with an appropriately formatted message and
> > letting the caller handle it would nicely simplify things.
> 
> It's not supposed to be used directly as a library, only as a script.
OK.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #10)
> TemporaryFile and NamedTemporaryFile and mkstemp share the implementation,
> so there's no difference in security, only in the way that file is
> automatically deleted.
> 

I generally avoid assuming implementations when the documentation implies otherwise. From the docs, it sounds like mkstemp() is *guaranteed* to behave this way, whereas there is no mention of security from NamedTempFile in the docs. The fact that it's currently implemented the same isn't necessarily relevant.



> But this actually makes things more complicated for the user, because if the
> have only the non-default version installed, there's no /usr/bin/sscg. So
> maybe there shouldn't be packages for both versions, just one, always
> providing /usr/bin/sscg, and internally it should use either python2 or
> python3, whichever one is the default for this Fedora version.
> 

That's a reasonable argument, I suppose. If I went this route, do you think I should rename the package to simply 'sscg' then? It seems to me that, despite installing into the python_sitedir, since I'm only using it as a script I should probably just package it as its upstream name (using the same logic that packages like Transifex and ReviewBoard don't lead with python-). What do you think? The only reason I named it python[3]-sscg was to support dual-mode installation.


> > > In the python code, a bare try: ... except: (without any exception na
> > > after "except") is used. In some places the exception is re-raised, but in
> > > others not. This is a problem because the script might be ended with ^C (or
> > > a signal) and silently "swallow" the exception. The script should always
> > > exit after being interrupted. It also should not print an error message like
> > > "Failed to write..." when killed.
> > Ah, where did I do that? It wasn't intentional.
> I'm not sure about what one specifically you're asking... "bare" except is
> used almost everywhere.
> 
> In src/sscg/main.py there is this:
> 
>            try:
>                 os.unlink(file)
>            except:
>                 # Nothing we can do if we get an error
>                 pass

I should probably log an error there, but no matter what exception I get, there's no solution...

> 
> > > It seems that gettext is called on formatted arguments:
> > >    _("Could not write to {0}. Error: {1}".format(...))
> > > but it should be before:
> > >    _("Could not write to {0}. Error: {1}").format(...)
> > > 
> > 
> > Typo, thanks for catching it.
> > 

> Yes, binary is the default. But decoding goes from bytes to unicode (i.e.
> text, i.e. str in Python 3). Encoding goes from text (i.e. str) to a
> specific encoding, for example UTF-8, and the bytes type. So you were
> writing text to a binary file. This actually does not work in Python 3:
> 
> >>> f = tempfile.NamedTemporaryFile()
> >>> f.write('blah')
> Traceback (most recent call last):
>   File "<stdin>", line 1, in <module>
>   File "/usr/lib64/python3.4/tempfile.py", line 399, in func_wrapper
>     return func(*args, **kwargs)
> TypeError: 'str' does not support the buffer interface
> 
> although it does work under Python 2.

Well, the problem here is that pyOpenSSL behaves differently between python2 and python3. On python2, crypto.dump_certificate() returns a str, but it returns a binary type on python3. To support both language versions, I just always pipe it through a conversion to UTF-8. I could do it the opposite direction if that seems more sensible.

(In reply to Stephen Gallagher from comment #11)
> (In reply to Zbigniew Jędrzejewski-Szmek from comment #10)
> > TemporaryFile and NamedTemporaryFile and mkstemp share the implementation,
> > so there's no difference in security, only in the way that file is
> > automatically deleted.
> > 
> 
> I generally avoid assuming implementations when the documentation implies
> otherwise. From the docs, it sounds like mkstemp() is *guaranteed* to behave
> this way, whereas there is no mention of security from NamedTempFile in the
> docs. The fact that it's currently implemented the same isn't necessarily
> relevant.

I'm a big fan of NamedTemporaryFile and TemporaryFile ;) The docs could use some freshening, that's true. But they do say that "[NamedTemporaryFile] perates exactly as TemporaryFile does, except that the file is guaranteed to have a visible name in the file system", and TemporaryFile "is created using mkstemp". (This is the part that is wrong, because both mkstemp and *TemporaryFile call to a helper function, but TemporaryFile does not actually call mkstemp.)

http://bugs.python.org/issue23725

> If I went this route, do you think I should rename the package to simply 'sscg' then?
Yes, makes sense.

> > > > In the python code, a bare try: ... except: (without any exception na
> > > > after "except") is used. In some places the exception is re-raised, but in
> > > > others not. This is a problem because the script might be ended with ^C (or
> > > > a signal) and silently "swallow" the exception. The script should always
> > > > exit after being interrupted. It also should not print an error message like
> > > > "Failed to write..." when killed.
> > > Ah, where did I do that? It wasn't intentional.
> > I'm not sure about what one specifically you're asking... "bare" except is
> > used almost everywhere.
> > 
> > In src/sscg/main.py there is this:
> > 
> >            try:
> >                 os.unlink(file)
> >            except:
> >                 # Nothing we can do if we get an error
> >                 pass
> 
> I should probably log an error there, but no matter what exception I get,
> there's no solution...
Logging is one thing. I was worried about the following scenario:

       try:
            os.unlink(file)          <----------------- ^C is pressed here
       except:                       <----------------- KeyboadInterruptException is caught here
            # Nothing we can do if we get an error
            pass                     <----------------- nothing happens
                                     <----------------- program continues running

With a slow filesystem, or a hanging network filesystem this kind of race is quite possible.

> 
> > 
> > > > It seems that gettext is called on formatted arguments:
> > > >    _("Could not write to {0}. Error: {1}".format(...))
> > > > but it should be before:
> > > >    _("Could not write to {0}. Error: {1}").format(...)
> > > > 
> > > 
> > > Typo, thanks for catching it.
> > > 
> 
> > Yes, binary is the default. But decoding goes from bytes to unicode (i.e.
> > text, i.e. str in Python 3). Encoding goes from text (i.e. str) to a
> > specific encoding, for example UTF-8, and the bytes type. So you were
> > writing text to a binary file. This actually does not work in Python 3:
> > 
> > >>> f = tempfile.NamedTemporaryFile()
> > >>> f.write('blah')
> > Traceback (most recent call last):
> >   File "<stdin>", line 1, in <module>
> >   File "/usr/lib64/python3.4/tempfile.py", line 399, in func_wrapper
> >     return func(*args, **kwargs)
> > TypeError: 'str' does not support the buffer interface
> > 
> > although it does work under Python 2.
> 
> Well, the problem here is that pyOpenSSL behaves differently between python2
> and python3. On python2, crypto.dump_certificate() returns a str, but it
> returns a binary type on python3. To support both language versions, I just
> always pipe it through a conversion to UTF-8. I could do it the opposite
> direction if that seems more sensible.
UTF-8 *is* binary. Under python 2 you get a sequence of UTF-8 octets which you can directly write to a binary file. Under python 3 you get the same thing. The difference is that Python 3 makes the distinction between bytes and text, and considers this UTF-8-encoded Unicode string to be binary, and Python 2 does not make the distinction, so it allows it to be used as both, in some circumstances. The distinction is blurry, but under Python 2 'str' is binary.

I haven't tested this, but I'm pretty sure that if use the exact same code under Python 2 and 3, and not do any conversions, things should just work.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #12)
> (In reply to Stephen Gallagher from comment #11)

> > Well, the problem here is that pyOpenSSL behaves differently between python2
> > and python3. On python2, crypto.dump_certificate() returns a str, but it
> > returns a binary type on python3. To support both language versions, I just
> > always pipe it through a conversion to UTF-8. I could do it the opposite
> > direction if that seems more sensible.
> UTF-8 *is* binary. Under python 2 you get a sequence of UTF-8 octets which
> you can directly write to a binary file. Under python 3 you get the same
> thing. The difference is that Python 3 makes the distinction between bytes
> and text, and considers this UTF-8-encoded Unicode string to be binary, and
> Python 2 does not make the distinction, so it allows it to be used as both,
> in some circumstances. The distinction is blurry, but under Python 2 'str'
> is binary.
> 
> I haven't tested this, but I'm pretty sure that if use the exact same code
> under Python 2 and 3, and not do any conversions, things should just work.

When I went to print this content in debug mode, I definitely got different output to the screen. You could of course be correct that it would make no difference programatically, so if you think it's sensible to only force the conversion on screen-printing, I don't have a problem with that.

I'll take the review... since I already know the package is OK ;)

Spec URL: https://sgallagh.fedorapeople.org/sscg/sscg.spec
SRPM URL: https://sgallagh.fedorapeople.org/sscg/sscg-0.3.0-1.fc22.src.rpm

Rawhide Koji Build: http://koji.fedoraproject.org/koji/taskinfo?taskID=9310671


This should also include all of the recommended upstream changes. Thanks for the review, both of the package and the code behind it :)

Issues:
- missing ownership of %{python3_sitelib}/%{srcname}/ and %{python2_sitelib}/%{srcname}/
- 2to3 is still used (specified in setup.py), which is not necessary
- The paragraph starting from "# Set any script hashbangs to the appropriate python version" is not necessary, I think. setup.py will set the line by itself.

- It seems that some imports are missing:

$ sscg --package foo --cert-file /tmp/file1 --cert-key-file /tmp/file2 --country pl --state '' --locality None --organization None --organizational-unit www
Traceback (most recent call last):
  File "/usr/bin/sscg", line 9, in <module>
    load_entry_point('sscg==0.3.0', 'console_scripts', 'sscg')()
  File "/usr/lib/python3.4/site-packages/sscg/main.py", line 126, in main
    (ca_cert, ca_key) = create_temp_ca(options)
  File "/usr/lib/python3.4/site-packages/sscg/authority.py", line 20, in create_temp_ca
    print (_("{host} is not a valid FQDN").format(
NameError: name '_' is not defined

- $ sscg
...
sscg: error: the following arguments are required: --package, --cert-file, --cert-key-file, --country, --state, --locality, --organization, --organizational-unit

Just a suggestion: maybe some of those could be made optional... E.g. --state is a US-only thing, and some of the others could default to empty too.


===== 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]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "Unknown or generated". 6 files have unknown license. Detailed output of
     licensecheck in /var/tmp/1202604-sscg/licensecheck.txt
[!]: Package requires other packages for directories it uses.
     Note: No known owner of /usr/lib/python3.4/site-packages/sscg
Please add to %files.

[ ]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/lib/python3.4/site-
     packages/sscg
[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 uses nothing in %doc for runtime.
[x]: Package consistently uses macros (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]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines
[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).
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[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]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[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. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 0 bytes in 0 files.
[x]: Packages must not store files under /srv, /opt or /usr/local

Python:
[x]: Python eggs must not download any dependencies during the build process.
[x]: A package which is used by another package via an egg interface should
     provide egg info.
[x]: Package meets the Packaging Guidelines::Python
[x]: Package contains BR: python2-devel or python3-devel
[x]: Binary eggs must be removed in %prep

===== 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).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: 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.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[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 (not strictly required in GL).
[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 unless justified.

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

Generic:
[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: sscg-0.3.0-1.fc23.noarch.rpm
          sscg-0.3.0-1.fc23.src.rpm
sscg.noarch: W: no-documentation
sscg.noarch: W: no-manual-page-for-binary sscg
sscg.src: W: invalid-url Source0: https://github.com/sgallagher/sscg/releases/download/sscg-0.3.0/sscg-0.3.0.tar.gz HTTP Error 403: Forbidden
2 packages and 0 specfiles checked; 0 errors, 3 warnings.




Rpmlint (installed packages)
----------------------------
Cannot parse rpmlint output:


Requires
--------
sscg (rpmlib, GLIBC filtered):
    /usr/bin/python3
    python(abi) = 3.4
    python3-pyOpenSSL
    python3-pyasn1



Provides
--------
sscg:
    sscg



Source checksums
----------------
https://github.com/sgallagher/sscg/releases/download/sscg-0.3.0/sscg-0.3.0.tar.gz :
  CHECKSUM(SHA256) this package     : b2179af37b631d4ebd518b6cbd9e957548ee73697924454e0903d3e4020e9333
  CHECKSUM(SHA256) upstream package : b2179af37b631d4ebd518b6cbd9e957548ee73697924454e0903d3e4020e9333


Generated by fedora-review 0.5.2 (63c24cb) last change: 2014-07-14
Command line :/usr/bin/fedora-review -m fedora-rawhide-i386 -b 1202604
Buildroot used: fedora-rawhide-i386
Active plugins: Python, Generic, Shell-api
Disabled plugins: Java, C/C++, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, EPEL5, BATCH, DISTTAG

Spec URL: https://sgallagh.fedorapeople.org/sscg/sscg.spec
SRPM URL: https://sgallagh.fedorapeople.org/sscg/sscg-0.4.0-0.fc22.src.rpm

Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=9313294

Packaging looks good, the comments above were all addressed, rpmlint output is OK.

sscg.noarch: W: no-documentation
sscg.noarch: W: no-manual-page-for-binary sscg
1 packages and 0 specfiles checked; 0 errors, 2 warnings.

Package is APPROVED.


I found another small issue, which I think should be resolved before the package is submitted:

DEFAULT_CA_CERT = "/etc/pki/ca-trust/source/anchors", but /etc/pki/ca-trust/source/anchors is a directory, owned by ca-certificates. So renaming the tempfile fails:

$ sscg --package foo --cert-file /tmp/file1 --cert-key-file /tmp/file2 --country pl --state '' --locality None --organization None --organizational-unit www --hostname bupkis.localdomain.
Traceback (most recent call last):
  File "/usr/bin/sscg", line 9, in <module>
    load_entry_point('sscg==0.4.0', 'console_scripts', 'sscg')()
  File "/usr/lib/python3.4/site-packages/sscg/main.py", line 183, in main
    crypto.dump_certificate(options.cert_format, ca_cert))
  File "/usr/lib/python3.4/site-packages/sscg/__init__.py", line 69, in write_secure_file
    delete=False)
  File "/usr/lib64/python3.4/tempfile.py", line 460, in NamedTemporaryFile
    (fd, name) = _mkstemp_inner(dir, prefix, suffix, flags)
  File "/usr/lib64/python3.4/tempfile.py", line 200, in _mkstemp_inner
    fd = _os.open(file, flags, 0o600)
PermissionError: [Errno 13] Permission denied: '/etc/pki/ca-trust/source/tmpmbqqy6qs'

(In reply to Zbigniew Jędrzejewski-Szmek from comment #18)
> Packaging looks good, the comments above were all addressed, rpmlint output
> is OK.
> 
> sscg.noarch: W: no-documentation
> sscg.noarch: W: no-manual-page-for-binary sscg
> 1 packages and 0 specfiles checked; 0 errors, 2 warnings.
> 
> Package is APPROVED.
> 

Thank you for the very thorough review, Zbigniew!


> 
> I found another small issue, which I think should be resolved before the
> package is submitted:
> 
> DEFAULT_CA_CERT = "/etc/pki/ca-trust/source/anchors", but
> /etc/pki/ca-trust/source/anchors is a directory, owned by ca-certificates.
> So renaming the tempfile fails:

Well, I was envisioning that in real-world usage, this would normally be operated by the root user, but I suppose that may not actually be true. I'll change the default location to match the directory where the service certificate is placed.

Please add a the CVS request.

New Package SCM Request
=======================
Package Name: sscg
Short Description: Self-signed Certificate Generator 
Upstream URL: https://github.com/sgallagher/sscg
Owners: sgallagh
Branches: f21 f22 el6 epel7
InitialCC:

Git done (by process-git-requests).