Bug 1310092

Here's a quick spec file review:

- Typographical: Space in name tag "Name :" 

- Why x86_64 only?

- The %prep section isn't empty as it contains %setup, just do:
%prep
%setup

- %pre is optional, just drop it if you don't need any scripts.

- Use of %post questions:
-- chmod in %post: Why can it not be installed in this mode? Or does the binary modify this when run?
-- Why copy the binary in %post? Can't it just be installed in bin directly?
-- No installing to /usr/local, this package would be part of Fedora and should install to the system location, using the proper macro, %{_bindir}.

- %clean shouldn't be needed unless you have multiple Source files.

- %attr use in %files: This is usually only used to set users/groups for non-root. Setting the mode should be done correctly by %make_install. Additionally it's more common to fix permissions in %install rather than in %files.

- In the case where you really want /usr/lib on x86_64 the better method is to use %{_prefix}/lib.

Running tests for functionality and ease..


Will advise any function / design quarks / suggests within the next few days

(In reply to Corey Sheldon from comment #2)
> Running tests for functionality and ease..
> 
> 
> Will advise any function / design quarks / suggests within the next few days

Corey,

thank you for running the tests. Please keep in mind that someone who'd use the RPM from the web site would probalbly know of the fundamental ideas behind the Crypto Bone, because of the context given there. In particular they would have
read the installation page.

Maybe I have to include some of this information in the new Fedora package?

(In reply to Richard Shaw from comment #1)
> Here's a quick spec file review:

> - Why x86_64 only?

Richard, 

thank you for your spec file review. 

The permission issues you found may require a re-organisation of the
installation process (%make_install) and a change of the source code. 
I'm working on this.

Let me explain why some of these decisions are important for the cryptobone package.

The numerous (not-standard) file permissions follow from the necessity to 
restrict the use of the crypto bone to the root user. Also the location in
/usr/lib where secrets are held is essential, because it is checked by the
binaries that use them, in particular the cryobone daemon. 

For now, I decided to only prepare a tar archive of the newly build software
that does not install but contains everything needed. And I did the fiddly work
in the spec file. I'm now trying to put as much as possible back into the 
"make install" in the source code that would clean up the spec file.

> - Why x86_64 only?

Well, in principle, this system should work on all architectures. But I decided 
to restrict it to x86_64 at the moment, because the full testing cycle on an
arch is quite expensive and I don't have access to all other arches to test the
final result there. For a security-critical system like the cryptobone it is 
indispensable to run these tests manually, and I simply cannot do this alone.
It should be possible to expand the scope to i386 or arm but I don't see me testing
ppc and co. So let me stick to this restriction for the moment. 

I'll post new spec file and SRPM later.

Ralf.
~

senderek's scratch build of cryptobone-1.0.1-2.fc20.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=13066306

Richard, 

I have changed the installation process in the source code which led to a substantial reduction in the spec file. Here are the updated SRPM and spec file:

Spec URL: https://crypto-bone.com/fedora/cryptobone.spec
SRPM URL: https://crypto-bone.com/fedora/cryptobone-1.0.1-2.fc23.src.rpm

The new KOJI build is here:

http://koji.fedoraproject.org/koji/taskinfo?taskID=13067106

Please let me know if the changes are ok.


Ralf.

senderek's scratch build of cryptobone-1.0.1-2.fc20.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=13068144

Ok, another round of spec comments. You may have to do some explaining as this is not an area I'm familiar with but you're doing a lot of interesting things with this package.

1. Have you considered a systemd timer rather than a cron job?

2. %global cryptobonedir %{_prefix}/lib/%{name}

It's odd to place this between %description and %prep. I usually throw all this stuff at the top of the spec file so it sticks out.

3. It would be good to add a comment on why this is necessary:

     if ! systemctl is-active postfix > /dev/null ; then
          systemctl enable postfix
     fi 

4. This should be done in %install

     cp %{cryptobonedir}/GUI/cryptobone.jpg /usr/share/icons/default
     desktop-file-install --dir /usr/share/applications -m 755 %{cryptobonedir}/GUI/cryptobone.desktop

5. chkconfig --add cryptoboned 

SysV init scripts are just redirected to systemd, I would think this would only be useful on EL 6.

6. echo "Please reboot your computer as the crypto bone daemon will start only while booting."

Generally output during install is discouraged and anyone using a GUI installer won't see it.

7. What is the purpose of this? Placing systemd unit files in /etc is intended for the end user so they can override the files in /usr/lib/systemd/system. I'm pretty sure mucking around with /etc/systemd/system is forbidden.

     if [ -f /etc/systemd/system/cryptoboned.service ] ; then
          # re-install symlinks
	  systemctl disable cryptoboned
	  rm -f /etc/systemd/system/cryptoboned.service 2> /dev/null
          systemctl enable cryptoboned 
     fi

8. %files

/etc/init.d/cryptoboned -> %{_sysconfdir}/init.d/cryptoboned

/usr/share/man/man8/cryptoboned.8.gz -> %{_mandir}/man8/cryptoboned.8.gz

I think that's enough for now!

(In reply to Richard Shaw from comment #8)
> Ok, another round of spec comments. You may have to do some explaining as this
> is not an area I'm familiar with but you're doing a lot of interesting things
> with this package.

I'd be glad to explain every aspect of the cryptobone to you. In order to make
things easy for the user and at the same time not compromising on security, that
leads to a system with a few bells and whistles that need explanation.


> 1. Have you considered a systemd timer rather than a cron job?

Not yet, a cron job running every minute seemed a good trade-off to me, as I try
to get new incoming messages fast without putting too much strain on the system.
The user should see a new message popping up in the RAM disk automatically. I could have avoided the cron job if I burden the user with triggering the mail polling which I don't want to do. Switching to systemd timers might be a good idea if I wanted to increase the frequency of incoming email polling.

Do you have a reason to suggest switching to a systemd timer that has any advantage in this context?


> 2. %global cryptobonedir %{_prefix}/lib/%{name}
done.

> 3. It would be good to add a comment on why this is necessary:
> 
>      if ! systemctl is-active postfix > /dev/null ; then
>           systemctl enable postfix
>      fi

That is something I have thought about recently. The main reason is to make sure
that email is sent out. I've decided to require postfix for this package, but anyone already using another MTA would not really need that and what's worse postfix might create a conflict with the installed alternative MTA.

The best solution would be to require any MTA be it postfix or other.

If "Requires: sendmail" would work I'd be happy enough. But I need to test that
before I can change this aspect of my spec file.

There's only one place in the source code where it is needed: 
(line 67 in the script "sendmail")

67   /usr/sbin/sendmail  -f "$SENDER" "$RECIPIENT" < /usr/lib/cryptobone/cryptobone/encryptedmessage.asc

No other dependency is required for outgoing messages.

 
> 4. This should be done in %install
Of course.

> 5. chkconfig --add cryptoboned 
As the package now requires systemd, this is unnecessary.


> 6. echo "Please reboot your computer as the crypto bone daemon will start only
> while booting."

The cryptobone daemon is unusual in the sense that it needs access to secret information that is only available for a tiny period while the system is booting, I tried to inform the user that a re-boot is necessary after installation. Users will find that the GUI alerts them if the cryptobone daemon does not run, which will also be the case, if it is stopped and restarted like ordinary daemons. 
So the GUI will alert the user in any case. If the message can remain in the spec file it might be better to place it in a %posttrans script so that it gets displayed as the last line?



> 7. What is the purpose of this? Placing systemd unit files in /etc is intended
> for the end user so they can override the files in /usr/lib/systemd/system. I'm
> pretty sure mucking around with /etc/systemd/system is forbidden.
> 
>      if [ -f /etc/systemd/system/cryptoboned.service ] ; then
>           # re-install symlinks
>       systemctl disable cryptoboned
>       rm -f /etc/systemd/system/cryptoboned.service 2> /dev/null
>           systemctl enable cryptoboned
>      fi

Yes, because I have previously (mistakenly) installed the unit file under /etc
I included this code only to get a clean system after update. In future versions I will delete this code as it is no longer necessary. be cause I use %{_unitdir} now.


> 8. %files
done.

> I think that's enough for now!

Thanks for your helpful comments.

I have updated the spec file (now release 4) and the source code remains unchanged for now.

https://crypto-bone.com/fedora/cryptobone.spec

When the postfix/sendmail issue is resolved, I'll post the new SRPM and KOJI build.

(In reply to Ralf Senderek from comment #9)
> (In reply to Richard Shaw from comment #8)
> > 1. Have you considered a systemd timer rather than a cron job?
> 
> Not yet, a cron job running every minute seemed a good trade-off to me, as I
> try
> to get new incoming messages fast without putting too much strain on the
> system.
> The user should see a new message popping up in the RAM disk automatically.
> I could have avoided the cron job if I burden the user with triggering the
> mail polling which I don't want to do. Switching to systemd timers might be
> a good idea if I wanted to increase the frequency of incoming email polling.
> 
> Do you have a reason to suggest switching to a systemd timer that has any
> advantage in this context?

Nothing other than like SysV -> Systemd, systemd timers are the "latest" way to do it. I could help you develop the file if you like but we'll save that for another day.


> > 3. It would be good to add a comment on why this is necessary:
> > 
> >      if ! systemctl is-active postfix > /dev/null ; then
> >           systemctl enable postfix
> >      fi
> 
> That is something I have thought about recently. The main reason is to make
> sure
> that email is sent out. I've decided to require postfix for this package,
> but anyone already using another MTA would not really need that and what's
> worse postfix might create a conflict with the installed alternative MTA.

Both sendmail and postfix provide the MTA capability (look at rpm -q --provies <package>) but that will only make sure it's installed, not enabled or started. This is an interesting one.


> The best solution would be to require any MTA be it postfix or other.
> 
> If "Requires: sendmail" would work I'd be happy enough. But I need to test
> that
> before I can change this aspect of my spec file.
> 
> There's only one place in the source code where it is needed: 
> (line 67 in the script "sendmail")
> 
> 67   /usr/sbin/sendmail  -f "$SENDER" "$RECIPIENT" <
> /usr/lib/cryptobone/cryptobone/encryptedmessage.asc
> 
> No other dependency is required for outgoing messages.

You may want to ask about this on the devel mailing list. I'm sure there will be plenty of opinions on the matter though :)


> > 6. echo "Please reboot your computer as the crypto bone daemon will start only
> > while booting."
> 
> The cryptobone daemon is unusual in the sense that it needs access to secret
> information that is only available for a tiny period while the system is
> booting, I tried to inform the user that a re-boot is necessary after
> installation. Users will find that the GUI alerts them if the cryptobone
> daemon does not run, which will also be the case, if it is stopped and
> restarted like ordinary daemons. 

Well when you submit bodhi updates it will give you the option to suggest rebooting, but of course that only works with PackageKit, not direct dnf install/updates.


> So the GUI will alert the user in any case. If the message can remain in the
> spec file it might be better to place it in a %posttrans script so that it
> gets displayed as the last line?

Doesn't make any different as far as the packaging guidelines go but probably more appropriate overall.

 
> > 7. What is the purpose of this? Placing systemd unit files in /etc is intended
> > for the end user so they can override the files in /usr/lib/systemd/system. I'm
> > pretty sure mucking around with /etc/systemd/system is forbidden.
> > 
> >      if [ -f /etc/systemd/system/cryptoboned.service ] ; then
> >           # re-install symlinks
> >       systemctl disable cryptoboned
> >       rm -f /etc/systemd/system/cryptoboned.service 2> /dev/null
> >           systemctl enable cryptoboned
> >      fi
> 
> Yes, because I have previously (mistakenly) installed the unit file under
> /etc

You can fix this in %install as well. Basically anything that needs to happen after "make install" to fix things should be done there. The script macros are only for things that need to happen during install/uninstall/update/removal, etc.

senderek's scratch build of cryptobone-1.0.1-4.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=13188782

To resolve the postfix issue, I changed the requirement to MTA.
On a system without an MTA installed the default "exim" will be installed alongside the cryptobone package.
If postfix is already installed the dependency is satisfied.
Both exim and postfix are active after installation, so the cryptobone works fine in both situations.

New spec file: https://crypto-bone.com/fedora/cryptobone.spec
New SRPM:      https://crypto-bone.com/fedora/cryptobone-1.0.1-4.fc23.src.rpm

And the KOJI build is here: 
http://koji.fedoraproject.org/koji/taskinfo?taskID=13188783

Please let me know if anything else is missing.

Ralf.

I'm assuming the sudogetuser in %post creates an interactive prompt?

Unfortunately the guidelines strictly forbid interactive installs, it's one of the biggest differences between Fedora/Redhat and Debian philosophies. 

I haven't checked yet but there should be instructions on any setup required before running the daemon. 

Also, this is probably not compliant:


     if ! systemctl is-active sshd > /dev/null ; then
          systemctl enable sshd 
     fi

However, as long as the dependencies are setup correctly in the service file for cryptobone, it will start sshd unless the user has masked it. Disabling a service only stops it from being started by default, if something requires it, it will be started unless masked.

Some other script feedback:

Daemons are not allowed to be enabled on install unless they have been approved to do so. You should be using the systemd macros which take care of this for you:

$ rpm -E %systemd_post

if [ $1 -eq 1 ] ; then 
        # Initial installation 
        systemctl preset  >/dev/null 2>&1 || : 
fi 

$ rpm -E %systemd_preun

if [ $1 -eq 0 ] ; then 
        # Package removal, not upgrade 
        systemctl --no-reload disable  > /dev/null 2>&1 || : 
        systemctl stop  > /dev/null 2>&1 || : 
fi 

$ rpm -E %systemd_postun

systemctl daemon-reload >/dev/null 2>&1 || : 


Other odds-and-ends:

# You want to use mkdir -p and always use the directory macros
mkdir $RPM_BUILD_ROOT/usr/share/icons
mkdir $RPM_BUILD_ROOT/usr/share/icons/default

# I prefer %{buildroot} but either is acceptable:
mkdir -p %{buildroot}%{_datadir}/default

# Is a jpg the only one available? PNG/SVGs are preferred, usually XPM is the backup.
cp $RPM_BUILD_ROOT%{cryptobonedir}/GUI/cryptobone.jpg $RPM_BUILD_ROOT/usr/share/icons/default

# Is there a reason the desktop file needs to be executable? 
desktop-file-install --dir $RPM_BUILD_ROOT/usr/share/applications -m 755 $RPM_BUILD_ROOT%{cryptobonedir}/GUI/cryptobone.desktop

(In reply to Richard Shaw from comment #14)
> I'm assuming the sudogetuser in %post creates an interactive prompt?
> 
> Unfortunately the guidelines strictly forbid interactive installs, it's one
> of the biggest differences between Fedora/Redhat and Debian philosophies. 
> 
> I haven't checked yet but there should be instructions on any setup required
> before running the daemon. 

I need to find a solution for this issue, because it is an essential point.

The reason why the installation is interactive at the moment is that there
can only be one single user for the cryptobone. This user must contact the
cryptobone daemon (/usr/lib/cryptobone/cryptoboned) using only a very limited
number of well-defined programs as the root user. I am using the sudo mechanism
to allow the owner of the cryptobone to execute /usr/lib/cryptobone/cbcontrol
as root. 

The usability of the system depends on this feature, that a user can have 
controlled access to the secret message key data base via the daemon without
having to know (and use) anything but his login password.

That's why I need to set the user name in the file /etc/sudoers.d/cbcontrol
when the software is being installed. That's why I chose a GUI to ask for the
user name.

I cannot come up with an alternative to this setup. Because if the installation
was silent, some other root process must write this crucial file.
I need to exit the GUI until the user has manually created the sudo file.
It seems much more natural to me to request the user name while the package
is being installed, because at that time, the user who is in control of the
machine can make the right choice. 

So if this is absolutely a no-go, what do you suggest?

I have changed the source code and updated the spec file (now release 5):

Spec URL: https://crypto-bone.com/fedora/cryptobone.spec
SRPM URL: https://crypto-bone.com/fedora/cryptobone-1.0.1-5.fc23.src.rpm



(In reply to Richard Shaw from comment #14)
> I'm assuming the sudogetuser in %post creates an interactive prompt?
> 
> Unfortunately the guidelines strictly forbid interactive installs, it's one
> of the biggest differences between Fedora/Redhat and Debian philosophies. 
> 

OK, I've made the whole installation process non-interactive now!


> 
> Also, this is probably not compliant:
> 
> 
>      if ! systemctl is-active sshd > /dev/null ; then
>           systemctl enable sshd 
>      fi

I have added a line "Requires=sshd.service" to the cryptoboned.service file
and removed the code above from the spec file.


> 
> Some other script feedback:
> 
> Daemons are not allowed to be enabled on install unless they have been
> approved to do so. You should be using the systemd macros which take care of
> this for you:

OK, I have resolved these issues by transferring the activation of my daemons
to the source code (/usr/lib/cryptobone/sudogetuser). The spec file now has
a %prosttrans section, which informs the user to run this script.
This can be done any time, as long as the user has knowledge of the 
root password, to set the sudoers.d/cbcontrol file and to activate the deamon.

Cron jobs are gone now in favour of a systemd timer, as you advised.

> 
> Other odds-and-ends:
> 
> # You want to use mkdir -p and always use the directory macros
> mkdir $RPM_BUILD_ROOT/usr/share/icons
> mkdir $RPM_BUILD_ROOT/usr/share/icons/default

done.

> 
> # I prefer %{buildroot} but either is acceptable:
> mkdir -p %{buildroot}%{_datadir}/default

done.

> 
> # Is a jpg the only one available? PNG/SVGs are preferred, usually XPM is
> the backup.
> cp $RPM_BUILD_ROOT%{cryptobonedir}/GUI/cryptobone.jpg
> $RPM_BUILD_ROOT/usr/share/icons/default

I've changed that to a PNG file instead.

> 
> # Is there a reason the desktop file needs to be executable? 

No, of course not, I've changed that to 0644 mode now.


I hope the cryptobone package is in an acceptable shape now.

Thank you for your review so far.

Ralf

(In reply to Ralf Senderek from comment #16)
>> (In reply to Richard Shaw from comment #14)
> > I'm assuming the sudogetuser in %post creates an interactive prompt?
> > 
> > Unfortunately the guidelines strictly forbid interactive installs, it's one
> > of the biggest differences between Fedora/Redhat and Debian philosophies. 
> > 
> 
> OK, I've made the whole installation process non-interactive now!

Ok, good. While I understand why you wanted it, I was worried about gui based installs, I'm not even sure what would happen.


> > 
> > Also, this is probably not compliant:
> > 
> > 
> >      if ! systemctl is-active sshd > /dev/null ; then
> >           systemctl enable sshd 
> >      fi
> 
> I have added a line "Requires=sshd.service" to the cryptoboned.service file
> and removed the code above from the spec file.

OK.
 
 
> > Some other script feedback:
> > 
> > Daemons are not allowed to be enabled on install unless they have been
> > approved to do so. You should be using the systemd macros which take care of
> > this for you:
> 
> OK, I have resolved these issues by transferring the activation of my daemons
> to the source code (/usr/lib/cryptobone/sudogetuser). The spec file now has
> a %prosttrans section, which informs the user to run this script.
> This can be done any time, as long as the user has knowledge of the 
> root password, to set the sudoers.d/cbcontrol file and to activate the
> deamon.

Ok, I may have to dig into this one a bit. There is actually a process to get permission to be enabled by default, I believe it requires an FPC ticket but really I don't for this kind of process that it's unreasonable to have them read a little documentation so they know why they're getting into and enable the daemon explicitly.

This is a pretty invasive package so I appreciate your patience with getting me up to speed and making all the requisite changes.

I'll start on the full review as soon as I have a few moments.

(In reply to Richard Shaw from comment #17)

> > I have added a line "Requires=sshd.service" to the cryptoboned.service file
> > and removed the code above from the spec file.
> 
> OK.

In hindsight you've been quite right actually to complain about this feature.
I made a mistake by thinking that the ssh daemon is a requirement. On the
client side I really do need the ssh client to be present, the daemon would
be necessary on a Fedora system that implements the external Crypto Bone.
So I think I'd remove this requirement from the source code.

The next release (6) is in the pipeline, but needs further testing before 
it'll hit the road.

> >                                              The spec file now has
> > a %prosttrans section, which informs the user to run this script.
> > This can be done any time, as long as the user has knowledge of the 
> > root password, to set the sudoers.d/cbcontrol file and to activate the
> > deamon.
> 
> Ok, I may have to dig into this one a bit. There is actually a process to
> get permission to be enabled by default, I believe it requires an FPC ticket
> but really I don't for this kind of process that it's unreasonable to have
> them read a little documentation so they know why they're getting into and
> enable the daemon explicitly.
> 

I've already worked further on this problem, and you're right that explicit
user consent should be necessary to activate the daemon. So the best way
to make sure that everything needed is up and running is the GUI. I modified
the GUI to ask the user for permission to activate the Crypto Bone and to
define the login name of the user that should (exclusively) use this
Crypto Bone. It is essential that only a user who can acquire root permissions
is able to make this initial setup (via "sudogetuser"). All other users
must be blocked from changing anything. I think the GUI does that now. (rel 6)

> This is a pretty invasive package so I appreciate your patience with getting
> me up to speed and making all the requisite changes.
 
> I'll start on the full review as soon as I have a few moments.

Thank you for your time and effort.

PS: I know that rpmlint has a few issues with the rpm, but the non-standard
file permissions are all a result of security measures not ignorance.

Before you post a new spec and SRPM go ahead and remove the chkconfig stuff. No need to add it just to silence rpmlint. We have to review all rpmlint errors but it is sometimes wrong and we can choose to ignore it.

(In reply to Richard Shaw from comment #19)
> Before you post a new spec and SRPM go ahead and remove the chkconfig stuff.
> No need to add it just to silence rpmlint. We have to review all rpmlint
> errors but it is sometimes wrong and we can choose to ignore it.

OK, here is the new (release 6) SRPM and spec file:

Spec URL: https://crypto-bone.com/fedora/cryptobone.spec
SRPM URL: https://crypto-bone.com/fedora/cryptobone-1.0.1-6.fc23.src.rpm

The new KOJI build is here:

http://koji.fedoraproject.org/koji/taskinfo?taskID=13381952

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

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


Issues:
=======
  - gtk-update-icon-cache is invoked in %postun and %posttrans if package
  contains icons.
  Note: icons in cryptobone
  See: http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache

  - Permissions on files are set properly.
  Note: See rpmlint output
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#FilePermissions
  This is a special case I think we're good here.

  - 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 %license.
  Note: License file COPYING is marked as %doc instead of %license
  See:
  http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text
  This is due to a Fedora specific guideline to put licenses in /usr/share/license
  instead of /usr/share/doc to reduce install size for space limited targets like arm.
  Might be best to remove the license stuff from your makefile and use relative paths 
  instead.
  
  - cryptobone.x86_64: E: missing-call-to-setgroups-before-setuid /usr/lib/cryptobone/libcl.so.3.4.3
  $ rpmlint -I missing-call-to-setgroups-before-setuid
  missing-call-to-setgroups-before-setuid:
  This executable is calling setuid and setgid without setgroups or initgroups.
  There is a high probability this means it didn't relinquish all groups, and
  this would be a potential security issue to be fixed. Seek POS36-C on the web
  for details about the problem.
  
  - Some files are licensed MIT:
	  MIT/X11 (BSD like)
	------------------
	cryptobone-1.0.1/src/cryptoboned/b64.c
	cryptobone-1.0.1/src/cryptoboned/b64.h
	cryptobone-1.0.1/src/openpgp/b64.c
	cryptobone-1.0.1/src/openpgp/b64.h
	
	I think just updating your license tag to "BSD and MIT" should be good enough here.




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

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

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.
[!]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "MIT/X11 (BSD like)", "BSD (2 clause)", "GPL (v3 or later)",
     "Unknown or generated", "BSD (4 clause)". 12 files have unknown
     license. Detailed output of licensecheck in /home/build/fedora-
     review/1310092-cryptobone/licensecheck.txt
[!]: Package requires other packages for directories it uses.
     Note: No known owner of /usr/share/doc/cryptobone
[!]: Package must own all directories that it creates.
     Note: Directories without known owners: /etc/init.d,
     /usr/share/icons/default, /usr/share/doc/cryptobone\
	 Do we need the init.d file since we have a systemd service file?
	 I think we can ignore all but /usr/share/doc/cryptobone which can be added as:
	 %dir %{_docdir}/cryptobone in %files
[x]: %build honors applicable compiler flags or justifies otherwise.
[-]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: 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.
[x]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 102400 bytes in 5 files.
[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]: 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 contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install or
     desktop-file-validate if there is such a file.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[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]: Packages must not store files under /srv, /opt or /usr/local

===== 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.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: %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]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Uses parallel make %{?_smp_mflags} macro.
[x]: SourceX is a working URL.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: Spec use %global instead of %define unless justified.

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

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


Rpmlint
-------
Checking: cryptobone-1.0.1-6.fc25.x86_64.rpm
          cryptobone-1.0.1-6.fc25.src.rpm
cryptobone.x86_64: E: missing-call-to-setgroups-before-setuid /usr/lib/cryptobone/libcl.so.3.4.3
cryptobone.x86_64: E: non-readable /usr/lib/cryptobone/sudogetuser 700
cryptobone.x86_64: E: non-standard-executable-perm /usr/lib/cryptobone/sudogetuser 700
cryptobone.x86_64: E: non-readable /usr/lib/cryptobone/cbcontrol 700
cryptobone.x86_64: E: non-standard-executable-perm /usr/lib/cryptobone/cbcontrol 700
cryptobone.x86_64: E: non-readable /usr/lib/cryptobone/createmasterkey 700
cryptobone.x86_64: E: non-standard-executable-perm /usr/lib/cryptobone/createmasterkey 700
cryptobone.x86_64: E: non-standard-dir-perm /usr/lib/cryptobone/keys 700
cryptobone.x86_64: E: non-standard-dir-perm /usr/lib/cryptobone 700
cryptobone.x86_64: E: non-standard-dir-perm /usr/lib/cryptobone/cryptobone 700
cryptobone.x86_64: E: non-readable /usr/lib/cryptobone/cryptoboned 700
cryptobone.x86_64: E: non-standard-executable-perm /usr/lib/cryptobone/cryptoboned 700
cryptobone.x86_64: E: non-readable /usr/lib/cryptobone/getlocalsecret 700
cryptobone.x86_64: E: non-standard-executable-perm /usr/lib/cryptobone/getlocalsecret 700
cryptobone.x86_64: E: non-standard-dir-perm /usr/lib/cryptobone/bin 700
cryptobone.x86_64: E: non-readable /usr/lib/cryptobone/getIP 700
cryptobone.x86_64: E: non-standard-executable-perm /usr/lib/cryptobone/getIP 700
cryptobone.x86_64: E: non-readable /usr/lib/cryptobone/rc.local 700
cryptobone.x86_64: E: non-standard-executable-perm /usr/lib/cryptobone/rc.local 700
cryptobone.x86_64: E: non-readable /usr/lib/cryptobone/cbcontrol.functions 700
cryptobone.x86_64: E: non-standard-executable-perm /usr/lib/cryptobone/cbcontrol.functions 700
cryptobone.x86_64: E: non-readable /etc/init.d/cryptoboned 700
cryptobone.x86_64: E: non-standard-executable-perm /etc/init.d/cryptoboned 700
cryptobone.x86_64: W: dangerous-command-in-%preun rm
cryptobone.x86_64: W: dangerous-command-in-%postun rm
cryptobone.x86_64: E: postin-without-chkconfig /etc/init.d/cryptoboned
cryptobone.x86_64: E: preun-without-chkconfig /etc/init.d/cryptoboned
cryptobone.x86_64: W: service-default-enabled /etc/init.d/cryptoboned
cryptobone.x86_64: W: service-default-enabled /etc/init.d/cryptoboned
cryptobone.x86_64: E: subsys-not-used /etc/init.d/cryptoboned
cryptobone.src:1: E: hardcoded-library-path in %{_prefix}/lib/%{name}
cryptobone.src:54: W: setup-not-quiet
cryptobone.src:116: W: macro-in-comment %{cryptobonedir}
2 packages and 0 specfiles checked; 27 errors, 6 warnings.




Rpmlint (debuginfo)
-------------------
Checking: cryptobone-debuginfo-1.0.1-6.fc25.x86_64.rpm
cryptobone-debuginfo.x86_64: E: debuginfo-without-sources
1 packages and 0 specfiles checked; 1 errors, 0 warnings.





Rpmlint (installed packages)
----------------------------
cryptobone.x86_64: E: missing-call-to-setgroups-before-setuid /usr/lib/cryptobone/libcl.so.3.4.3
cryptobone.x86_64: E: non-readable /usr/lib/cryptobone/getIP 700
cryptobone.x86_64: E: non-standard-executable-perm /usr/lib/cryptobone/getIP 700
cryptobone.x86_64: E: non-readable /usr/lib/cryptobone/getlocalsecret 700
cryptobone.x86_64: E: non-standard-executable-perm /usr/lib/cryptobone/getlocalsecret 700
cryptobone.x86_64: E: non-readable /usr/lib/cryptobone/sudogetuser 700
cryptobone.x86_64: E: non-standard-executable-perm /usr/lib/cryptobone/sudogetuser 700
cryptobone.x86_64: E: non-standard-dir-perm /usr/lib/cryptobone/bin 700
cryptobone.x86_64: E: non-readable /usr/lib/cryptobone/cbcontrol.functions 700
cryptobone.x86_64: E: non-standard-executable-perm /usr/lib/cryptobone/cbcontrol.functions 700
cryptobone.x86_64: E: non-readable /etc/init.d/cryptoboned 700
cryptobone.x86_64: E: non-standard-executable-perm /etc/init.d/cryptoboned 700
cryptobone.x86_64: E: non-standard-dir-perm /usr/lib/cryptobone/cryptobone 700
cryptobone.x86_64: E: non-readable /usr/lib/cryptobone/rc.local 700
cryptobone.x86_64: E: non-standard-executable-perm /usr/lib/cryptobone/rc.local 700
cryptobone.x86_64: E: non-standard-dir-perm /usr/lib/cryptobone/keys 700
cryptobone.x86_64: E: non-readable /usr/lib/cryptobone/cryptoboned 700
cryptobone.x86_64: E: non-standard-executable-perm /usr/lib/cryptobone/cryptoboned 700
cryptobone.x86_64: E: non-standard-dir-perm /usr/lib/cryptobone 700
cryptobone.x86_64: E: non-readable /usr/lib/cryptobone/cbcontrol 700
cryptobone.x86_64: E: non-standard-executable-perm /usr/lib/cryptobone/cbcontrol 700
cryptobone.x86_64: E: non-readable /usr/lib/cryptobone/createmasterkey 700
cryptobone.x86_64: E: non-standard-executable-perm /usr/lib/cryptobone/createmasterkey 700
cryptobone.x86_64: W: dangerous-command-in-%preun rm
cryptobone.x86_64: W: dangerous-command-in-%postun rm
cryptobone.x86_64: E: postin-without-chkconfig /etc/init.d/cryptoboned
cryptobone.x86_64: E: preun-without-chkconfig /etc/init.d/cryptoboned
cryptobone.x86_64: W: service-default-enabled /etc/init.d/cryptoboned
cryptobone.x86_64: W: service-default-enabled /etc/init.d/cryptoboned
cryptobone.x86_64: E: subsys-not-used /etc/init.d/cryptoboned
cryptobone-debuginfo.x86_64: E: debuginfo-without-sources
2 packages and 0 specfiles checked; 27 errors, 4 warnings.



Requires
--------
cryptobone (rpmlib, GLIBC filtered):
    /bin/bash
    /bin/ksh
    /bin/sh
    /usr/bin/python
    MTA
    base64
    bash
    cryptsetup
    fetchmail
    ksh
    libbsd.so.0()(64bit)
    libbsd.so.0(LIBBSD_0.0)(64bit)
    libc.so.6()(64bit)
    libcl.so.3.4.3()(64bit)
    libdl.so.2()(64bit)
    libpthread.so.0()(64bit)
    libresolv.so.2()(64bit)
    nmap
    openssh
    openssh-askpass
    python
    rtld(GNU_HASH)
    socat
    systemd
    tkinter



Provides
--------
cryptobone:
    application()
    application(cryptobone.desktop)
    bundled(libcl.so)
    cryptobone
    cryptobone(x86-64)
    libcl.so.3.4.3()(64bit)



Source checksums
----------------
https://crypto-bone.com/release/source/cryptobone-1.0.1.tar.gz :
  CHECKSUM(SHA256) this package     : bf01ab08a171ad433769119c461a5ef4f856e17c58ba5605eb17ef3d507db9b9
  CHECKSUM(SHA256) upstream package : bf01ab08a171ad433769119c461a5ef4f856e17c58ba5605eb17ef3d507db9b9


Generated by fedora-review 0.6.0 (3c5c9d7) last change: 2015-05-20
Command line :/bin/fedora-review -m fedora-rawhide-x86_64 -b 1310092
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6

(In reply to Richard Shaw from comment #21)
> Package Review
> 
> Issues:
> =======
>   - gtk-update-icon-cache is invoked in %postun and %posttrans if package
>   contains icons.
>   Note: icons in cryptobone
>   See: http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache

I have included the necessary update scripts in %post %postun and %posttrans
sections in the new (release 7) spec file


> 
>   - Permissions on files are set properly.
>   Note: See rpmlint output
>   See: http://fedoraproject.org/wiki/Packaging/Guidelines#FilePermissions
>   This is a special case I think we're good here.

OK.

> 
>   - 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 %license.
>   Note: License file COPYING is marked as %doc instead of %license
>   See:
>   http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text
>   This is due to a Fedora specific guideline to put licenses in
> /usr/share/license
>   instead of /usr/share/doc to reduce install size for space limited targets
> like arm.
>   Might be best to remove the license stuff from your makefile and use
> relative paths 
>   instead.

To be honest, I don't know how to handle this. The COPYING file is already
marked as %license. Would it be necessary to move them to 
/usr/share/license and leave the mark as %license?

What else should be changed?

>   
>   - cryptobone.x86_64: E: missing-call-to-setgroups-before-setuid
> /usr/lib/cryptobone/libcl.so.3.4.3
>   $ rpmlint -I missing-call-to-setgroups-before-setuid
>   missing-call-to-setgroups-before-setuid:
>   This executable is calling setuid and setgid without setgroups or
> initgroups.
>   There is a high probability this means it didn't relinquish all groups, and
>   this would be a potential security issue to be fixed. Seek POS36-C on the
> web
>   for details about the problem.

For weeks I have been trying to find out what rpmlint thinks the problem
may be here, and I have found nothing substantial on the web since that could
shed some light on what's required. I suppose this is a false-positive.
I'm inclined to ignore this error.

>   
>   - Some files are licensed MIT:
> 	  MIT/X11 (BSD like)
> 	------------------
> 	cryptobone-1.0.1/src/cryptoboned/b64.c
> 	cryptobone-1.0.1/src/cryptoboned/b64.h
> 	cryptobone-1.0.1/src/openpgp/b64.c
> 	cryptobone-1.0.1/src/openpgp/b64.h
> 	
> 	I think just updating your license tag to "BSD and MIT" should be good
> enough here.

I've done that.


 
> ===== MUST items =====

> [!]: License field in the package spec file matches the actual license.
>      Note: Checking patched sources after %prep for licenses. Licenses
>      found: "MIT/X11 (BSD like)", "BSD (2 clause)", "GPL (v3 or later)",
>      "Unknown or generated", "BSD (4 clause)". 12 files have unknown
>      license. Detailed output of licensecheck in /home/build/fedora-
>      review/1310092-cryptobone/licensecheck.txt

Should be good with the new updated license tag.

> [!]: Package requires other packages for directories it uses.
>      Note: No known owner of /usr/share/doc/cryptobone

I've added 
%dir       %{_docdir}/cryptobone


> [!]: Package must own all directories that it creates.
>      Note: Directories without known owners: /etc/init.d,
>      /usr/share/icons/default, /usr/share/doc/cryptobone\
> 	 Do we need the init.d file since we have a systemd service file?

Well yes, we need /etc/init.d so I added 
%dir /etc/init.d
(see comments in the spec file (release 7))


> 	 I think we can ignore all but /usr/share/doc/cryptobone which can be added
> as:
> 	 %dir %{_docdir}/cryptobone in %files

done.


This is the updated (release 7) spec file. No changes to the source.

https://crypto-bone.com/fedora/cryptobone.spec

Please let me know if I have to change anything else.

(In reply to Richard Shaw from comment #21)
> Package Review

My analysis of the RPMLINT-issue:
  This executable is calling setuid and setgid without setgroups or initgroups.
  There is a high probability this means it didn't relinquish all groups, and
  this would be a potential security issue to be fixed. Seek POS36-C on the web
  for details about the problem.

The problem is described in the following URL:
https://www.securecoding.cert.org/confluence/display/c/POS36C.+Observe+correct+revocation+order+while+relinquishing+privileges

The relevant part of the source code file "random/unix.c" (lines 1258 and 1259)
uses setresuid to set all three uids (real, effective and saved) as __linux__ is set.
The author notes that dropping the privileges here occurs as a precautionary measure.
I cannot see that there is any chance that there are supplementary groups set that
will not be relinquished, so I don't see any reason to bother the author to include
setgroups or initgroups to address this issue.


source code:
1215                 /* If we're root, give up our permissions to make sure that we don't
1216                    inadvertently read anything sensitive.  If the getpwnam() fails
1217                    (this can happen if we're chrooted with no "nobody" entry in the
1218                    local passwd file) we default to -1, which is usually nobody.
1219                    The newer setreXid() and setresXid() calls use a parameter value
1220                    of -1 to indicate "don't change this value" so this isn't
1221                    possible any longer, but then there's not really much else that
1222                    we can do at this point.
1223 
1224                    We don't check whether the change succeeds since it's not a major
1225                    security problem but just a precaution (in theory an attacker
1226                    could do something like fork()ing until RLIMIT_NPROC is reached,
1227                    at which point it'd fail, but that doesn't really give them
1228                    anything) */
1229                 if( geteuid() == 0 )

1248                         if( gathererUID != ( uid_t ) -1 )
1249                                 {
1250 #if 0                   /* Not available on some OSes */
1251                                 ( void ) setuid( gathererUID );
1252                                 ( void ) seteuid( gathererUID );
1253                                 ( void ) setgid( gathererGID );
1254                                 ( void ) setegid( gathererGID );
1255 #else
1256   #if( defined( __linux__ ) || ( defined( __FreeBSD__ ) && OSVERSION >= 5 ) || \
1257            ( defined( __hpux ) && OSVERSION >= 11 ) )
1258                                 ( void ) setresuid( gathererUID, gathererUID, gathererUID );
1259                                 ( void ) setresgid( gathererGID, gathererGID, gathererGID );
1260   #else
1261                                 ( void ) setreuid( gathererUID, gathererUID );
1262                                 ( void ) setregid( gathererGID, gathererGID );
1263   #endif /* OSses with setresXid() */
1264 #endif /* 0 */
1265                                 }
1266                         }

(In reply to Ralf Senderek from comment #23)
> (In reply to Richard Shaw from comment #21)
> > Package Review

URL:
https://www.securecoding.cert.org/confluence/display/c/POS36-C.+Observe+correct+revocation+order+while+relinquishing+privileges

(In reply to Ralf Senderek from comment #22)
> (In reply to Richard Shaw from comment #21)
> > 
> >   - 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 %license.
> >   Note: License file COPYING is marked as %doc instead of %license
> >   See:
> >   http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text
> >   This is due to a Fedora specific guideline to put licenses in
> > /usr/share/license
> >   instead of /usr/share/doc to reduce install size for space limited targets
> > like arm.
> >   Might be best to remove the license stuff from your makefile and use
> > relative paths 
> >   instead.
> 
> To be honest, I don't know how to handle this. The COPYING file is already
> marked as %license. Would it be necessary to move them to 
> /usr/share/license and leave the mark as %license?

I'm pretty sure the license file must be installed to /usr/share/license... The easiest solution would be to move them in %install after make install and update your paths in %files


> >   
> >   - cryptobone.x86_64: E: missing-call-to-setgroups-before-setuid
> > /usr/lib/cryptobone/libcl.so.3.4.3
> >   $ rpmlint -I missing-call-to-setgroups-before-setuid
> >   missing-call-to-setgroups-before-setuid:
> >   This executable is calling setuid and setgid without setgroups or
> > initgroups.
> >   There is a high probability this means it didn't relinquish all groups, and
> >   this would be a potential security issue to be fixed. Seek POS36-C on the
> > web
> >   for details about the problem.
> 
> For weeks I have been trying to find out what rpmlint thinks the problem
> may be here, and I have found nothing substantial on the web since that could
> shed some light on what's required. I suppose this is a false-positive.
> I'm inclined to ignore this error.

Yeah, I just wanted to bring it up since we need to address it (even if the decision is to do nothing).


> > [!]: Package must own all directories that it creates.
> >      Note: Directories without known owners: /etc/init.d,
> >      /usr/share/icons/default, /usr/share/doc/cryptobone\
> > 	 Do we need the init.d file since we have a systemd service file?
> 
> Well yes, we need /etc/init.d so I added 
> %dir /etc/init.d
> (see comments in the spec file (release 7))

Ok, I see your comment in the spec file... I'll need some time to digest this but I'm pretty sure it's against the guidelines to provide both a systemd and SysV startup file. 

Is the problem that the SysV files does a lot of things that the systemd file does not?

I know there was a lot of pain during the SystemD migration because SysV was supposed to be used for starting and stopping the daemon but because it used shell scripting a lot of upstreams used (some would say abused) that fact to have it do much more.

Ok, I wish I had looked at the systemd file sooner :) 

Since it's basically calling the SysV script I think the best solution is to move it to another location. After reviewing FHS[1] and the Fedora packaging guidelines[2] I'm thinking /usr/libexec/cryptobone would be a good solution assuming you don't want to drop it in /usr/bin :)

The alternative would be /usr/lib64/cryptobone.

[1] https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch04.html
[2] https://fedoraproject.org/wiki/Packaging:Guidelines#Libexecdir

(In reply to Richard Shaw from comment #26)
> Ok, I wish I had looked at the systemd file sooner :) 
> 
> Since it's basically calling the SysV script I think the best solution is to
> move it to another location. After reviewing FHS[1] and the Fedora packaging
> guidelines[2] I'm thinking /usr/libexec/cryptobone would be a good solution
> assuming you don't want to drop it in /usr/bin :)
> 
> The alternative would be /usr/lib64/cryptobone.
> 
> [1] https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch04.html
> [2] https://fedoraproject.org/wiki/Packaging:Guidelines#Libexecdir

The problem with relocating this script is that the cryptobone daemon is
coded in a way to reduce the attack surface, which means it would only work
if located in a certain directory (ie /etc/init.d). So I decided to finally
change the source code again and make sure that /etc/init.d is replaced by
/usr/lib/cryptobone/init.d (in the binary's source code).
Now everything is local to %(cryptobonedir) and tested to work as expected.

So here are the new SRPMS and spec file. (release 8)

Spec URL: https://crypto-bone.com/fedora/cryptobone.spec
SRPM URL: https://crypto-bone.com/fedora/cryptobone-1.0.1-8.fc23.src.rpm

All other issues, you mentioned have been addressed in the new spec file.
I've moved the two COPYING files to /usr/share/license/cryptobone and claimed
the ownership of this directory.

So, what's next?

(In reply to Richard Shaw from comment #25)

> I'm pretty sure the license file must be installed to /usr/share/license...
> The easiest solution would be to move them in %install after make install
> and update your paths in %files

This is changed in release 8.


> > For weeks I have been trying to find out what rpmlint thinks the problem
> > may be here, and I have found nothing substantial on the web since that could
> > shed some light on what's required. I suppose this is a false-positive.
> > I'm inclined to ignore this error.
> 
> Yeah, I just wanted to bring it up since we need to address it (even if the
> decision is to do nothing).

I would think to do nothing is the right thing to do, see my separate analysis
of the cryptlib source code.

(In reply to Ralf Senderek from comment #27)

> So here are the new SRPMS and spec file. (release 8)
> 
> Spec URL: https://crypto-bone.com/fedora/cryptobone.spec
> SRPM URL: https://crypto-bone.com/fedora/cryptobone-1.0.1-8.fc23.src.rpm

And the new KOJI build for rawhide:

http://koji.fedoraproject.org/koji/taskinfo?taskID=13496001

(In reply to Richard Shaw from comment #25)

> I'm pretty sure the license file must be installed to /usr/share/license...

Anyway, we need to use the plural here. (revision 9)


Spec URL: https://crypto-bone.com/fedora/cryptobone.spec
SRPM URL: https://crypto-bone.com/fedora/cryptobone-1.0.1-9.fc23.src.rpm

Ok, I think everything has been addressed so the package is:

*** APPROVED ***

I'll need to sponsor you formally now. I'm not sure if you have interest packaging other pieces of software so let me know. Typically I would require review of other packages before sponsoring but as you are upstream on a pretty complicated project I was making an exception but I would like to review the next couple of package reviews you submit.

Ok, you've been formally sponsored, use your powers for good!

(In reply to Richard Shaw from comment #32)
> Ok, you've been formally sponsored, use your powers for good!

I will, indeed. Thank you very much for sponsoring me and for your time you've
spent to help me improve this package.

(In reply to Richard Shaw from comment #31)
> Ok, I think everything has been addressed so the package is:
> 
> *** APPROVED ***
> 
> I'll need to sponsor you formally now. I'm not sure if you have interest
> packaging other pieces of software so let me know.

I have a few ideas for future packages, but for a couple of days I'll stay
with making me familiar with all the new stuff that comes with using SCM.
And over time I will work on extending the approved package to other arches
as to make it available everywhere.

Probably the next step would be to package the external cryptobone, that uses
a second linux system to store the encryption keys separately.
Then there was the idea of packaging Peter Gutmann's cryptlib as a system
library that could generally be used. But this would require some consultation
with Peter before I know exactly how to deal with the test suites.
But this is definitively on my radar.
 
>                                                   Typically I would require
> review of other packages before sponsoring but as you are upstream on a
> pretty complicated project I was making an exception but I would like to
> review the next couple of package reviews you submit.

I'd like to contribute to and advance code review of security critical software
in the Fedora community. So I looked at the new review requests and did a
preliminary check on the git-evtag package ( https://bugzilla.redhat.com/show_bug.cgi?id=1323214 )

I think I'll follow the developments there first. I'll keep you informed.

Thanks again for your assistance and help.

Ralf.

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/cryptobone

Too late but to make it correct, I'm assigning the bug to myself.

I have concerns about the bundled cryptlib:
* Some of the included ECC curves haven't been approved (see rhbz 1019390) by legal AFAICT: 
- Brainpool p256r1
- Brainpool p384r1
- Brainpool p512r1
* The license needs clarification as while it states (http://www.cryptlib.com/security-software/licensing) it's an opensource license it also states " All cryptlib users must have a valid software license. Please contact the cryptlib sales team for further details.". It also states in the COPYING file that the website takes precedence so it could change at any time without our knowledge and the version shipped would have legal issues.

Why was this classified as "Not applicable"? I don't see why cryptlib shouldn't be shipped separately [2]. Was there a FPC ticket for this exception?
[-]: Package contains no bundled libraries without FPC exception.

A few other things to note:
* on x86_64 only. It explicitly states in the description "This external device can be another Linux computer dedicated to this task or a Beagle Bone or a Raspberry Pi. (https://crypto-bone.com)" which tells me it should be built on at least ARMv7 otherwise the description is misleading. Also note the architecture Packaging guidelines [1]. I don't see any reason to not build this on all arches.
* It packages a zlib, that should be using the distro version
* I also, personally, believe it should be running non root as it's own user. No "secure" application should have any need to run as root. But that is my opinion.

So I'm going to:
* takeover this BZ review
* block the package while we confirm the legal details of the ECC curves and license with legal.

[1] https://fedoraproject.org/wiki/Packaging:Guidelines#Architecture_Support
[2] https://fedoraproject.org/wiki/Bundled_Software_policy

(In reply to Peter Robinson from comment #37)
> Why was this classified as "Not applicable"? I don't see why cryptlib
> shouldn't be shipped separately [2]. Was there a FPC ticket for this
> exception?
> [-]: Package contains no bundled libraries without FPC exception.

Unless things have changed again, the prohibition on bundled libraries were completely removed from Fedora but apparently FPR hasn't been updated in that regard.

https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries

(In reply to Richard Shaw from comment #38)
> (In reply to Peter Robinson from comment #37)
> > Why was this classified as "Not applicable"? I don't see why cryptlib
> > shouldn't be shipped separately [2]. Was there a FPC ticket for this
> > exception?
> > [-]: Package contains no bundled libraries without FPC exception.
> 
> Unless things have changed again, the prohibition on bundled libraries were
> completely removed from Fedora but apparently FPR hasn't been updated in
> that regard.
> 
> https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries

Please note that the policy says:
"All packages whose upstreams have no mechanism to build against system libraries must be contacted publicly about a path to supporting system libraries. If upstream refuses, this must be recorded in the spec file using a persistent mechanism to be clarified in the packaging guidelines. "

I can see no record of this request in the spec file, or any public question of
allowing to unbundle


Also, it says that in this case: "The FPC will maintain the list of bundled provides, please consult with them when adding new provides or in case of disputes.".

Nor can I see any FPC communication reporting this provides to them.

(In reply to Peter Robinson from comment #37)
> I have concerns about the bundled cryptlib:
> * Some of the included ECC curves haven't been approved (see rhbz 1019390)
> by legal AFAICT: 
> - Brainpool p256r1
> - Brainpool p384r1
> - Brainpool p512r1
> * The license needs clarification as while it states
> (http://www.cryptlib.com/security-software/licensing) it's an opensource
> license it also states " All cryptlib users must have a valid software
> license. Please contact the cryptlib sales team for further details.". It
> also states in the COPYING file that the website takes precedence so it
> could change at any time without our knowledge and the version shipped would
> have legal issues.

Well, this is a fun mess. Cryptlib is dual-licensed under the Sleepycat license or a closed-"commercial" license. Since we're fine with the Sleepycat license, Fedora can happily just use it under those terms.

The Brainpool curves are a problem. They need to be cleaned out of cryptlib before it can be included. Not just disabled, or not used, but the source cleaned of implementations. Nothing in cryptobone seems to depend on them. I've made a cleaned zip file for the cryptlib source. You'll need to make a new tarball for cryptobone that either has no bundled cryptlib zip file, or replaces the one it has now with one that does not include brainpool.

https://spot.fedorapeople.org/cl343_beta-no-brainpool.zip

(In reply to Tom "spot" Callaway from comment #40)

> Well, this is a fun mess. Cryptlib is dual-licensed under the Sleepycat
> license or a closed-"commercial" license. Since we're fine with the
> Sleepycat license, Fedora can happily just use it under those terms.

And the Sleepycat license is exactly what's shipped with the cryptobone.

> The Brainpool curves are a problem. 
> I've made a cleaned zip file for the cryptlib source. You'll need to make a
> new tarball for cryptobone that either has no bundled cryptlib zip file, or
> replaces the one it has now with one that does not include brainpool.
> 
> https://spot.fedorapeople.org/cl343_beta-no-brainpool.zip

Thank you Tom for your help. I will change the upstream source code to make
sure that there will be no possibility of a legal issue. To indicate this 
change I will bump the version number to 1.0.2 and proceed after testing
next week.

(In reply to Peter Robinson from comment #37)
> I have concerns about the bundled cryptlib:

In order to resolve the legal issue that has led to the
blocking of the cryptobone package once and for all, I
will not include the original cryptlib source code file
in any future SRPM. Instead, I will store a much reduced
zip archive that has no reference to any brainpool curve
implementation or any reference to brainpool code at all.

For public inspection I will provide this new zip-file 
on the Crypto Bone web site as a permalink:

https://crypto-bone.com/fedora/cl343_beta_fedora.zip

In addition to the work of eradicating brainbool code and
references from the original source code archive, for which
I thank Tom Callaway, I have deleted files that did
not contribute anything to the compilation of the library
for use on linux systems. These files are:

 Test32.vcxproj
 Test32.vcxproj.filters
 Test32.vcxproj.user
 cl32.dll
 cl32.lib
 cl64.dll
 cl64.lib
 crypt32.def
 crypt32.dsp
 crypt32.dsw
 crypt32.ico
 crypt32.rc
 crypt32.sln
 crypt32.vcxproj
 crypt32.vcxproj.filters
 crypt32.vcxproj.user
 crypt32ce.vcp
 crypt32ce.vcw
 test32.dsp
 test32.dsw
 test32.sln
 test32ce.vcp
 test32ce.vcw
 tools/GenPas.pl
 tools/GenPerl.pl
 tools/GenVB.pl

This greatly reduced zip-archive (cl343_beta_fedora.zip) will 
always be used in future versions of the cryptobone, starting
with version 1.0.2

Secondly, as Tom Callaway has pointed out in his Comment #40,
cryplib is licensed under the Sleepycat license, which is 
recognized as a good license for Fedora.

https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#Good_Licenses

As both objections have been addressed to avoid any possible
legal problem, I think the cryptobone package should be unblocked
once the new cryptobone version 1.0.2 is used to build.

Ralf Senderek

(In reply to Peter Robinson from comment #37)
> I have concerns about the bundled cryptlib:

...
 
> Why was this classified as "Not applicable"? I don't see why cryptlib
> shouldn't be shipped separately [2]. Was there a FPC ticket for this
> exception?
> [-]: Package contains no bundled libraries without FPC exception.

I'd like to explain why cryptlib as a bundled library
does not violate the packaging guidelines and why 
Richard's original classification as "Not applicable"
is correct.

Under the heading "Bundling and Duplication of system libraries"
the packaging guidelines state the following:

   Fedora packages should make every effort to avoid having multiple,
   separate, upstream projects bundled together in a single package.

   All packages whose upstreams allow them to be built against system 
   libraries must be built against system libraries.

   All packages whose upstreams have no mechanism to build against
   system libraries may opt to carry bundled libraries, but if they do,
   they must include Provides: bundled(<libname>) = <version> in their
   RPM spec file. In addition, packages whose upstreams have no mechanism
   to build against system libraries must be contacted publicly about a
   path to supporting system libraries. If upstream refuses, this must
   be recorded in the spec file, either in comments placed adjacent to
   the Provides: above, or in an additional file checked into the SCM
   and referenced by a comment placed adjacent to the Provides: above.

The cryptobone and cryptlib are not separable. Even if a separate
cryptlib package existed (which is not the fact) the cryptobone could
use it in principle, but as the cryptobone is based on only a very
small part of cryptlib, essentially the symmetric encryption enveloping
only, and because reduction of complexity is one of the cryptobone's
main goals, the whole point is to be able to link against a minimalistic
version of cryptlib, maybe under a different name like libcl-mini.so
instead of libcl.so.

Now, we don't have this separate cryptlib package yet, and the cryptobone
cannot be built against a system library. And if it could, the reduced
mini library is a better choice from a security point of view.
At the moment there is no alternative to bundle cryptlib with the 
cryptobone.

"In addition, packages whose ..." The cryptobone package has no mechanism
to build against a system library called libcl.so. I have made it clear 
(Comment #34 and devel list) that I (as the cryptobone upstream) want
to build a separate cryptlib, so that linking is possible. I don't refuse
to do that (as a packager). But when this library comes into existence
the mini version as a bundled library is the next thing to do and
to do it right takes time. 

If it helps I'll put this explanation as a comment into the 
spec file, but the cryptobone should not be blocked for this reason.

Ralf Senderek

(In reply to Ralf Senderek from comment #42)
> (In reply to Peter Robinson from comment #37)
> > I have concerns about the bundled cryptlib:

In a private conversation with Peter Gutmann I have confirmed that

a) there is no legal problem with using the Sleepycat license for cryptlib
   as a library at all.

b) with reference to the authors of the brainpool curves, Peter Gutmann does
   not expect any restrictions of the use of brainpool curves.

Even though upstream sees no conflict with (alleged) unapproved brainpool curve parameters with the Fedora legal team, I will make sure that brainpool curve
source code will not be included in the SRPMs used by Fedora. (see Comment #42) 

Ralf Senderek

(In reply to Ralf Senderek from comment #44)
> (In reply to Ralf Senderek from comment #42)
> > (In reply to Peter Robinson from comment #37)
> > > I have concerns about the bundled cryptlib:

These are the new SRPM and spec file for cryptobone version 1.0.2

Included in the SRPM is the reduced cryptlib source code zip file, which
is permalinked for public inspection here:

cryptlib reduced ZIP:        https://crypto-bone.com/fedora/cl343_beta_fedora.zip

SRPM:     https://crypto-bone.com/fedora/cryptobone-1.0.2-1.fc23.src.rpm

Spec file (1.0.2 release 1): https://crypto-bone.com/fedora/cryptobone.spec


The SRPM does not include anything that has legal issues, so there is no
reason to block this version of the cryptobone.

Ralf Senderek

Confirmed. New SRPM has no legal issues. Lifting FE-Legal, please unblock this package in git.

(In reply to Tom "spot" Callaway from comment #46)
> Confirmed. New SRPM has no legal issues. Lifting FE-Legal, please unblock
> this package in git.

Of course when updated versions of cryptlib become available, I will apply
these restrictions and deletions to the updates as well and build new SRPMS.

You should be good to go. Thanks for the help here, much appreciated!

built