Bug 970009 - Review Request: stoken - Token code generator compatible with RSA SecurID 128-bit (AES) token
Review Request: stoken - Token code generator compatible with RSA SecurID 128...
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: David Woodhouse
Fedora Extras Quality Assurance
:
Depends On: 970002
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-03 06:07 EDT by Simone Caronni
Modified: 2013-08-05 00:56 EDT (History)
5 users (show)

See Also:
Fixed In Version: stoken-0.2-4.fc18
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-06-16 01:32:40 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
dwmw2: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Simone Caronni 2013-06-03 06:07:04 EDT
Spec URL: http://slaanesh.fedorapeople.org/stoken.spec
SRPM URL: http://slaanesh.fedorapeople.org/stoken-0.2-1.fc19.src.rpm
Description: Software Token for Linux/UNIX. It's a token code generator compatible with RSA SecurID 128-bit (AES) tokens. It is a hobbyist project, not affiliated with or endorsed by RSA Security.
Fedora Account System Username: slaanesh
Comment 1 Simone Caronni 2013-06-03 06:09:11 EDT
Note:

This package is required for enabling RSA software token support in openconnect, the Cisco AnyConnect VPN client.
Comment 2 David Woodhouse 2013-06-03 08:32:38 EDT
I could commit the libtomcrypt changes for you in the short term, as a provenpackager. But I think we'd be better off ditching libtomcrypt and using something else like gnutls if possible.
Comment 3 Simone Caronni 2013-06-03 08:39:56 EDT
(In reply to David Woodhouse from comment #2)
> I could commit the libtomcrypt changes for you in the short term, as a
> provenpackager. But I think we'd be better off ditching libtomcrypt and
> using something else like gnutls if possible.

Well, for me is the same; I would simply like to have stoken in Fedora. The code needs to be patched for gnutls support. though.
Comment 4 Simone Caronni 2013-06-03 09:13:08 EDT
Woah, installed the token with stoken, rebuilt openconnect with stoken-devel, launch from the command line with "--token-mode=rsa" and now I'm asked the pin instead of the passcode (one number for another... don't know if it's a big achievement or not... :D).

I assume that for enabling the graphial interface there are additional changes required in NetworkManager-openconnect.
Comment 5 David Woodhouse 2013-06-03 10:36:22 EDT
Well, it might be one number for another, but at least it's now a *constant* number so it can be scripted. And with NetworkManager-openconnect, it can be stored in the keyring. Yes, some changes will be required in NetworkManager-openconnect but I think they're relatively simple. And I think Kevin had already done them but I was slacking and hadn't yet merged them, or something like that?
Comment 6 David Woodhouse 2013-06-03 10:43:05 EDT
Ah, no. The NM-openconnect parts should all be there already. Now you have a version of libopenconnect which admits to having stoken support, you should see the corresponding options in the configuration GUI.
Comment 7 David Woodhouse 2013-06-03 11:28:27 EDT
Why set CFLAGS=$RPM_OPT_FLAGS in %build? That's done by default anyway. Is that a leftover from an experiment with *changing* CFLAGS?

Perhaps you did that when you noticed the errant -O0 that you get by adding --enable-debug? We'll want upstream to give us a way to build with -O2 -ggdb which is our normal CFLAGS, rather than forcibly either overriding it to -O0 or stripping the debug info.

If you apply the patch I just sent you for using GnuTLS, you'll need to run autoreconf. And then you don't need to hack the libtool script with sed to fix the stupid rpath brokenness. Is that something you (should) have reported to the upstream maintainer, to make sure it's not present in the next release?
Comment 8 Simone Caronni 2013-06-03 11:45:48 EDT
(In reply to David Woodhouse from comment #6)
> Ah, no. The NM-openconnect parts should all be there already. Now you have a
> version of libopenconnect which admits to having stoken support, you should
> see the corresponding options in the configuration GUI.

Nope, I don't have them. I see support comes from the 0.9.8 release; though I don't know if NetworkManager-openconnect-0.9.7.0-2.git20120918.fc19 is a snapshot of 0.9.8 or not.

Can you update the current git snapshot in Fedora 18+ to 0.9.8?

=======================================================
network-manager-openconnect-0.9.8
Overview of changes since network-manager-openconnect-0.9.6.2
=======================================================

This is a new stable release of network-manager-openconnect.  Notable changes include:

* Updated translations
* Builds against the GNOME 3.8 versions of GLib and Gtk+
* Software Tokens are now supported (if libopenconnect has support for them)
* Fixed typos in several messages
* nm-openconnect-service now returns translated error messages

(In reply to David Woodhouse from comment #7)
> Perhaps you did that when you noticed the errant -O0 that you get by adding
> --enable-debug? We'll want upstream to give us a way to build with -O2 -ggdb
> which is our normal CFLAGS, rather than forcibly either overriding it to -O0
> or stripping the debug info.

Exactly for this actually.

> If you apply the patch I just sent you for using GnuTLS, you'll need to run
> autoreconf. And then you don't need to hack the libtool script with sed to
> fix the stupid rpath brokenness.

I was planning to make the minimum changes, but since the GnuTLS requires autoreconf to be rerun I removed the rpath script and CFLAGS override as well.

> Is that something you (should) have
> reported to the upstream maintainer, to make sure it's not present in the
> next release?

Before doing anything I would have liked to see all the changes that are required coming from the review.

Is removing entirely the "enable debugging option" (lines 25-39) from configure.ac the best approach?

Thanks,
--Simone
Comment 9 David Woodhouse 2013-06-03 12:16:13 EDT
NM-openconnect needs to be built against openconnect 5.00 or newer in order to get stoken support. I've updated it in rawhide and will update f19 once https://admin.fedoraproject.org/updates/openconnect-5.01-1.fc19 makes it to stable (and hence the buildroots).

We should probably look at enabling OATH support at the same time...
Comment 10 Simone Caronni 2013-06-03 15:08:49 EDT
Spec URL: http://slaanesh.fedorapeople.org/stoken.spec
SRPM URL: http://slaanesh.fedorapeople.org/stoken-0.2-2.fc19.src.rpm

Intermediate package until upstream replies to your mail about GnuTLS and removal of the conditional CFLAGS.

- Removed CFLAGS override
- Removed rpath sed hacks
- Added GnuTLS patch
- Patch away "debug" section in configure.ac

Btw, as you said I rebuilt NetworkManager-openconnect-0.9.8 on openconnect-5.01-1 and now I have the token settings on Fedora 19.

Thanks.
Comment 11 Kevin Cernekee 2013-06-03 15:42:47 EDT
> Woah, installed the token with stoken, rebuilt openconnect with
> stoken-devel, launch from the command line with "--token-mode=rsa"
> and now I'm asked the pin instead of the passcode (one number for
> another... don't know if it's a big achievement or not... :D).

You can use "stoken setpin" to cache the pin in ~/.stokenrc, allowing fully unattended operation.

Depending on your account lockout policies, you may be able to run openconnect in a loop to auto-reconnect on error:

while :; do openconnect -u user --non-inter --token-mode=rsa vpn.example.com; sleep 5; done

> NM-openconnect needs to be built against openconnect 5.00 or newer
> in order to get stoken support. I've updated it in rawhide and will
> update f19 once
> https://admin.fedoraproject.org/updates/openconnect-5.01-1.fc19
> makes it to stable (and hence the buildroots).
>
> We should probably look at enabling OATH support at the same time...

The current NM-openconnect head of tree supports RSA but not OATH.  I submitted OATH patches here (needs testing):

http://lists.infradead.org/pipermail/openconnect-devel/2013-March/001007.html
http://lists.infradead.org/pipermail/openconnect-devel/2013-March/001006.html
http://lists.infradead.org/pipermail/openconnect-devel/2013-March/001008.html

> Intermediate package until upstream replies to your mail about
> GnuTLS and removal of the conditional CFLAGS.

Replied privately with a couple of follow-on questions.
Comment 12 Kevin Cernekee 2013-06-04 02:19:10 EDT
I installed the FC19 beta tonight and made some additional progress:

stoken only really needs a tiny piece of libtomcrypt, so I copied the necessary libtomcrypt files right into the stoken tree.  autoconf will still look for a shared version (since presumably it will get bugfixes and updates) but lacking that, it will use the local copy.  Users who do have libtomcrypt installed can bypass it using "configure --without-libtomcrypt".

Incorporating the ~5 files from libtomcrypt allowed us to drop the GnuTLS dependency and the various concurrency / initialization problems that arose.

If all of this works OK, and doesn't completely break the Debian builds, I'll make it a formal v0.3 release.

Unfortunately I still wasn't able to get rpmbuild to stop complaining about the /usr/lib64 rpath, even after running autogen.sh on a much newer Linux installation than usual.  So the autoreconf line remains in the spec file.

My latest SRPM: https://dl.dropboxusercontent.com/u/169702767/stoken/stoken-0.3-1.fc19.src.rpm

Feature branch: https://github.com/cernekee/stoken/commits/fedora-v1
Comment 13 Simone Caronni 2013-06-04 02:59:48 EDT
(In reply to Kevin Cernekee from comment #12)
> stoken only really needs a tiny piece of libtomcrypt, so I copied the
> necessary libtomcrypt files right into the stoken tree.  autoconf will still
> look for a shared version (since presumably it will get bugfixes and
> updates) but lacking that, it will use the local copy.  Users who do have
> libtomcrypt installed can bypass it using "configure --without-libtomcrypt".
> 
> Incorporating the ~5 files from libtomcrypt allowed us to drop the GnuTLS
> dependency and the various concurrency / initialization problems that arose.

Unfortunately, the packaging guidelines forbid the inclusion of bundled libraries in the code; so at least in the Fedora case we have to use the libtomcrypt packages:

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

Spec URL: http://slaanesh.fedorapeople.org/stoken.spec
SRPM URL: http://slaanesh.fedorapeople.org/stoken-0.3-2.fc19.src.rpm

- Re-enabled libtomcrypt system library

Thanks,
--Simone
Comment 14 David Woodhouse 2013-06-04 04:23:39 EDT
If run on a machine with the Intel AES-NI instructions, does this make use of them? I'd like to make sure it does. Give me a SSH public key and a preferred username if you need an account on a suitable machine.
Comment 15 David Woodhouse 2013-06-04 08:40:44 EDT
Hm, you don't *really* need to update libtomcrypt; you could just configure with TOMCRYPT_CFLAGS=-I%{_includedir}/tomcrypt.

Well, that *was* true with the 0.2 tarball. Now it seems to define LOCAL_TOMCRYPT if there's no pkg-config package, and it doesn't fall back to using the system libtomcrypt unless there's a corresponding .pc file.
Comment 16 Simone Caronni 2013-06-04 08:48:59 EDT
(In reply to David Woodhouse from comment #15)
> Hm, you don't *really* need to update libtomcrypt; you could just configure
> with TOMCRYPT_CFLAGS=-I%{_includedir}/tomcrypt.
> 
> Well, that *was* true with the 0.2 tarball. Now it seems to define
> LOCAL_TOMCRYPT if there's no pkg-config package, and it doesn't fall back to
> using the system libtomcrypt unless there's a corresponding .pc file.

Yep. So it still needs the update. Actually I preferred the GnuTLS patch, including the 5 files from libtomcrypt makes things more complicated when packaging.
Comment 17 David Woodhouse 2013-06-04 11:32:55 EDT
The GnuTLS patch needs to be fixed to call gnutls_cipher_deinit(), and you need to call gnutls_global_init() somewhere.

And you need to *not* care about the tiny possibility of a race condition with multiple libraries in different threads each calling gnutls_global_init() at the same time. Which I suppose is OKish.
Comment 18 David Woodhouse 2013-06-04 12:19:42 EDT
[!]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf %{buildroot} present but not required

[!]: update-desktop-database is invoked when required
     Note: desktop file(s) in stoken-gui

[!]: Package installs a %{name}.desktop using desktop-file-install if there is
     such a file.

[!]: Sources can be downloaded from URI in Source: tag
     Note: Could not download Source0
Comment 19 Simone Caronni 2013-06-04 14:59:19 EDT
(In reply to David Woodhouse from comment #18)
> [!]: Package do,es not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
>      beginning of %install.
>      Note: rm -rf %{buildroot} present but not required

Fixed this, it is created by rpmdev-newspec even on new (rpm 4.11) templates.

> [!]: update-desktop-database is invoked when required
>      Note: desktop file(s) in stoken-gui

This is no mime type installed, so this should not be required [1].

> [!]: Package installs a %{name}.desktop using desktop-file-install if there
> is
>      such a file.

SPEC file already calls desktop-file-validate, as specified by the packaging guidelines desktop-file-install should be run only if the package does not install the desktop file on its own [2].

> [!]: Sources can be downloaded from URI in Source: tag
>      Note: Could not download Source0

I will revert to version 0.2, since it's released and the changes introduced still do not allow us to avoid libtomcrypt. I don't see any benefit now for using version 0.3; unless it's released before the end of the review.

I would also have to use the SourceURL for github prereleases according to the guidelines [3] which I would rather avoid.

[1] http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#desktop-database

[2] http://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage

[3] https://fedoraproject.org/wiki/Packaging:SourceURL#Github
Comment 20 Simone Caronni 2013-06-04 15:20:15 EDT
Here is the updated package. Version is 0.2, I updated the changelog accordingly.

No rpath sed hacks, proper CFLAGS. Requires proper libtomcrypt version.

Spec URL: http://slaanesh.fedorapeople.org/stoken.spec
SRPM URL: http://slaanesh.fedorapeople.org/stoken-0.2-3.fc19.src.rpm
Comment 21 Kevin Cernekee 2013-06-04 20:17:08 EDT
(In reply to Simone Caronni from comment #13)
> Unfortunately, the packaging guidelines forbid the inclusion of bundled
> libraries in the code; so at least in the Fedora case we have to use the
> libtomcrypt packages

If you do manage to revive the libtomcrypt package, it might be a good idea to see if dropbear (which currently bundles libtomcrypt) can use it.  If dropbear is set up as an internet-facing sshd, it could be vulnerable to library problems like this:

https://github.com/libtom/libtomcrypt/commit/2cd666f2849d62b11469fc876f51e07f327cee3b

or the various side-channel attacks that people keep finding in crypto libraries.

> Actually I preferred the GnuTLS patch, including the 5 files from
> libtomcrypt makes things more complicated when packaging.

OK - I don't have a strong opinion one way or the other.  The "5 files" code was experimental and I can just kill off that branch.


(In reply to David Woodhouse from comment #14)
> If run on a machine with the Intel AES-NI instructions, does this make
> use of them?

I don't see any architecture-specific optimizations in libtomcrypt.

It would be nice to have AES-NI support but from a practical standpoint it probably doesn't matter much for such an infrequent calculation.
Comment 22 Kevin Cernekee 2013-06-05 02:35:33 EDT
BTW - related question about x86 crypto features: what is the preferred way to utilize the Ivy Bridge RDRAND instruction from an application?

Should the GnuTLS/libtomcrypt/OpenSSL/... library authors assume that the user is already running rngd in the background to seed the kernel's /dev/random pool?  Or would it be better to integrate RDRAND support right into the library?

Fedora's rng-tools package is up to date, but Debian and Ubuntu are using an old fork which lacks RDRAND support.
Comment 23 David Woodhouse 2013-06-05 03:39:11 EDT
Hm.

For an application the question is a bit simpler. If the app needs to generate random numbers often and fast, then using rdrand directly is the way to go. Otherwise, just use the library and don't worry about it.

However, it's a bit harder for the library. I suppose it wants to be optional, and the distribution packager needs to do the right thing. Or perhaps this is just another case of Debian and Ubuntu being a bit behind the curve. We update the libraries to assume that rngd is doing it, and when we finally have current software in .deb form it'll all work out OK.

Getting back vaguely on-topic... it occurs to me that stoken could probably just open /dev/random and read from it to get a random number. You don't do this often, right? It's only for 'stoken import --random'? And thus it is /dev/random not /dev/urandom that you need?

In that case I could have just used nettle instead of gnutls, and not had to worry about the library initialisation issues.
Comment 24 Simone Caronni 2013-06-05 05:42:31 EDT
(In reply to Kevin Cernekee from comment #21)
> (In reply to Simone Caronni from comment #13)
> If you do manage to revive the libtomcrypt package, it might be a good idea
> to see if dropbear (which currently bundles libtomcrypt) can use it.

True, I will file a bug on it.

> > Actually I preferred the GnuTLS patch, including the 5 files from
> > libtomcrypt makes things more complicated when packaging.
> 
> OK - I don't have a strong opinion one way or the other.  The "5 files" code
> was experimental and I can just kill off that branch.

Actually libtomcrypt is used now only by 1 package (2 if we count dropbear not using the bundled library):

# repoquery --whatrequires "libtomcrypt.so.0()(64bit)"
libtomcrypt-devel-0:1.17-14.fc18.x86_64
libtomcrypt-devel-0:1.17-15.fc18.x86_64
olpc-os-builder-0:6.0.0-1.fc19.x86_64
Comment 25 David Woodhouse 2013-06-05 06:09:25 EDT
Package Review
==============

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



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

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig called in %post and %postun if required.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.
[x]: Development (unversioned) .so files in -devel subpackage, if present.

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]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[x]: Development files must be in a -devel package
[x]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Fully versioned dependency in subpackages, if present.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in stoken-
     devel , stoken-libs , stoken-cli , stoken-gui
[x]: Package complies to the Packaging Guidelines
[x]: License field in the package spec file matches the actual license.
[x]: License file installed when any subpackage combination is installed.
[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[x]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[x]: Package contains systemd file(s) if in need.
[x]: update-desktop-database is invoked when required
[x]: Useful -debuginfo package or justification otherwise.
[x]: Large documentation must go in a -doc subpackage.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install if there is
     such a file.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: Package 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]: Packages must not store files under /srv, /opt or /usr/local
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.


I would prefer to see BuildRequires: pkgconfig(libtomcrypt) pkgconfig(gtk+-2.0) instead of the package names libtomcrypt-devel and gtk2-devel.

In fact, I'd much rather see pkgconfig(gtk+-3.0) and it doesn't look particularly hard, but neither of those observations make it a review failure.

Review passed; thanks for contributing to Fedora.
Comment 26 Simone Caronni 2013-06-05 07:44:20 EDT
(In reply to David Woodhouse from comment #25)
> Package Review
> ==============
> 
> Legend:
> [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
> [ ] = Manual review needed
> 
> 
> 
> ===== MUST items =====
> 
> C/C++:
> [x]: Package does not contain kernel modules.
> [x]: Package contains no static executables.
> [x]: Header files in -devel subpackage, if present.
> [x]: ldconfig called in %post and %postun if required.
> [x]: Package does not contain any libtool archives (.la)
> [x]: Rpath absent or only used for internal libs.
> [x]: Development (unversioned) .so files in -devel subpackage, if present.
> 
> 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]: %build honors applicable compiler flags or justifies otherwise.
> [x]: Package contains no bundled libraries without FPC exception.
> [x]: Changelog in prescribed format.
> [x]: Sources contain only permissible code or content.
> [x]: Development files must be in a -devel package
> [x]: Package requires other packages for directories it uses.
> [x]: Package uses nothing in %doc for runtime.
> [x]: Package is not known to require ExcludeArch.
> [x]: Fully versioned dependency in subpackages, if present.
>      Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in stoken-
>      devel , stoken-libs , stoken-cli , stoken-gui
> [x]: Package complies to the Packaging Guidelines
> [x]: License field in the package spec file matches the actual license.
> [x]: License file installed when any subpackage combination is installed.
> [x]: Package consistently uses macro is (instead of hard-coded directory
>      names).
> [x]: Package is named according to the Package Naming Guidelines.
> [x]: Package does not generate any conflict.
> [x]: Package obeys FHS, except libexecdir and /usr/target.
> [x]: If the package is a rename of another package, proper Obsoletes and
>      Provides are present.
> [x]: Package must own all directories that it creates.
> [x]: Package does not own files or directories owned by other packages.
> [x]: Requires correct, justified where necessary.
> [x]: Spec file is legible and written in American English.
> [x]: Package contains systemd file(s) if in need.
> [x]: update-desktop-database is invoked when required
> [x]: Useful -debuginfo package or justification otherwise.
> [x]: Large documentation must go in a -doc subpackage.
> [x]: All build dependencies are listed in BuildRequires, except for any that
>      are listed in the exceptions section of Packaging Guidelines.
> [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
>      beginning of %install.
> [x]: Each %files section contains %defattr if rpm < 4.4
> [x]: Macros in Summary, %description expandable at SRPM build time.
> [x]: Package contains desktop file if it is a GUI application.
> [x]: Package installs a %{name}.desktop using desktop-file-install if there
> is
>      such a file.
> [x]: Package does not contain duplicates in %files.
> [x]: Permissions on files are set properly.
> [x]: If (and only if) the source package includes the text of the license(s)
>      in its own file, then that file, containing the text of the license(s)
>      for the package is included in %doc.
> [x]: Package 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]: Packages must not store files under /srv, /opt or /usr/local
> [x]: Package successfully compiles and builds into binary rpms on at least
> one
>      supported primary architecture.
> [x]: Package installs properly.
> [x]: Rpmlint is run on all rpms the build produces.
> 
> 
> I would prefer to see BuildRequires: pkgconfig(libtomcrypt)
> pkgconfig(gtk+-2.0) instead of the package names libtomcrypt-devel and
> gtk2-devel.

Done.

> In fact, I'd much rather see pkgconfig(gtk+-3.0) and it doesn't look
> particularly hard, but neither of those observations make it a review
> failure.

Looking into it, is not my domain of expertise and could be good for learning on something simple like this gui.

I will post something here before asking for fedora-cvs?.
 
> Review passed; thanks for contributing to Fedora.

Thank you very much as well for all your contributions :)
Comment 27 David Woodhouse 2013-06-05 07:51:29 EDT
(In reply to Simone Caronni from comment #26)
> > In fact, I'd much rather see pkgconfig(gtk+-3.0) and it doesn't look
> > particularly hard, but neither of those observations make it a review
> > failure.
> 
> Looking into it, is not my domain of expertise and could be good for
> learning on something simple like this gui.
> 
> I will post something here before asking for fedora-cvs?.

No, go ahead and ship it as-is, matching the upstream release. We can update to gtk3 later, when there's an upstream release that does so.

FWIW I think there's only one build *failure*; the rest is just warnings about using deprecated gtk functions that do still work anyway.
Comment 28 Simone Caronni 2013-06-05 08:59:59 EDT
Spec URL: http://slaanesh.fedorapeople.org/stoken.spec
SRPM URL: http://slaanesh.fedorapeople.org/stoken-0.2-4.fc19.src.rpm

- Change gtk and libtomcrypt build requirements (pkgconfig).
- Remove useless "--with-libtomcrypt" parameter from %configure.
Comment 29 Simone Caronni 2013-06-05 09:01:24 EDT
New Package SCM Request
=======================
Package Name: stoken
Short Description: Token code generator compatible with RSA SecurID 128-bit (AES) token
Owners: slaanesh
Branches: f18 f19 el6
InitialCC:
Comment 30 Gwyn Ciesla 2013-06-05 09:15:56 EDT
Git done (by process-git-requests).
Comment 31 Fedora Update System 2013-06-06 02:56:13 EDT
stoken-0.2-4.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/stoken-0.2-4.fc18
Comment 32 Fedora Update System 2013-06-06 02:56:40 EDT
stoken-0.2-4.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/stoken-0.2-4.fc19
Comment 33 Simone Caronni 2013-06-06 11:44:06 EDT
Package Change Request
======================
Package Name: stoken
New Branches: f17 f18
Owners: slaanesh
Comment 34 Simone Caronni 2013-06-06 11:44:34 EDT
Package Change Request
======================
Package Name: stoken
New Branches: f17
Owners: slaanesh
Comment 35 Gwyn Ciesla 2013-06-06 11:59:02 EDT
Git done (by process-git-requests).
Comment 36 Fedora Update System 2013-06-06 12:16:10 EDT
stoken-0.2-4.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/stoken-0.2-4.fc17
Comment 37 Fedora Update System 2013-06-06 22:58:30 EDT
stoken-0.2-4.fc17 has been pushed to the Fedora 17 testing repository.
Comment 38 Kevin Cernekee 2013-06-08 00:33:04 EDT
(In reply to David Woodhouse from comment #23)
> For an application the question is a bit simpler. If the app needs to
> generate random numbers often and fast, then using rdrand directly is the
> way to go. Otherwise, just use the library and don't worry about it.
> 
> However, it's a bit harder for the library. I suppose it wants to be
> optional, and the distribution packager needs to do the right thing. Or
> perhaps this is just another case of Debian and Ubuntu being a bit behind
> the curve. We update the libraries to assume that rngd is doing it, and when
> we finally have current software in .deb form it'll all work out OK.

Well, this bothered me enough to try fixing the Debian rng-tools package:

http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=692450

> Getting back vaguely on-topic... it occurs to me that stoken could probably
> just open /dev/random and read from it to get a random number. You don't do
> this often, right? It's only for 'stoken import --random'? And thus it is
> /dev/random not /dev/urandom that you need?

/dev/random is really slow if there's not a lot of entropy saved up - to the point where "stoken-gui --random" lags for a minute or two unless I ask the user to furiously move his mouse around.  Either way makes for a substandard user experience.

The RNG is used for two purposes in stoken:

1) Generating random tokens - per the man page this is for novelty purposes only.  We can't convert our ctf seeds back into sdtid seeds yet, and even if we could, we can't generate the RSA signature needed to make the official SecurID server believe they are authentic.  Maybe someday libstoken will be able to help perform service-side authentication, but we're not there yet so the random tokens are fairly useless except for testing.

2) Creating a random IV to encrypt the PIN, if the user elects to encrypt his seed + PIN in ~/.stokenrc.

At this point /dev/urandom is probably good enough for both use cases, and that is what libtomcrypt uses (at least on Debian).

> In fact, I'd much rather see pkgconfig(gtk+-3.0) and it doesn't look
> particularly hard, but neither of those observations make it a review failure.

Will revisit this in the next few months - also planning to switch over to GtkBuilder and add a basic "import token" dialog to the GUI.
Comment 39 Fedora Update System 2013-06-14 23:08:13 EDT
stoken-0.2-4.fc19 has been pushed to the Fedora 19 stable repository.
Comment 40 Fedora Update System 2013-06-16 01:32:40 EDT
stoken-0.2-4.fc17 has been pushed to the Fedora 17 stable repository.
Comment 41 Fedora Update System 2013-06-16 01:33:37 EDT
stoken-0.2-4.fc18 has been pushed to the Fedora 18 stable repository.
Comment 42 Christopher Meng 2013-08-05 00:56:23 EDT
I'm cleaning old emails and found this.

Seems dropbear has big issues, I'll handle this.

Note You need to log in before you can comment on or make changes to this bug.