Bug 1442547 - Review Request: gsignond - GSignOn daemon
Summary: Review Request: gsignond - GSignOn daemon
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Neal Gompa
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: PantheonDesktopPackageReviews
TreeView+ depends on / blocked
 
Reported: 2017-04-15 12:14 UTC by Fabio Valentini
Modified: 2017-11-11 17:12 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-04-27 05:51:58 UTC
Type: ---
Embargoed:
ngompa13: fedora-review+


Attachments (Terms of Use)

Description Fabio Valentini 2017-04-15 12:14:48 UTC
Spec URL: https://decathorpe.fedorapeople.org/packages/gsignond.spec

SRPM URL: https://decathorpe.fedorapeople.org/packages/gsignond-1.0.6-1.fc26.src.rpm

Description: The GSignOn daemon is a D-Bus service which performs user authentication on behalf of its clients. There are currently authentication plugins for OAuth 1.0 and 2.0, SASL, Digest-MD5, and plain username/password combination.

Fedora Account System Username: decathorpe


koji scratch build for rawhide: https://koji.fedoraproject.org/koji/taskinfo?taskID=19008549

This package is one of the dependencies for pantheon-mail, pantheon-online-accounts / switchboard-plug-onlineaccounts - which are among the last missing pieces for a complete Pantheon desktop experience on fedora.

Comment 1 Neal Gompa 2017-04-15 12:17:44 UTC
Taking this review.

Comment 2 Neal Gompa 2017-04-15 17:39:42 UTC
Going through fedora-review and analyzing the spec, I've noticed several issues:

* fedora-review complains about gsignond being setuid. Is there a reason for this? If so, why? If anything, I don't see a good reason for it at all. If there *is* a good reason for it, it needs to be documented...

* Your default-config subpackage doesn't make a ton of sense. First, why does it require the libs, when the libs don't care? It would probably make sense for the default-config subpackage to provide "%{name}-config" and have gsignond require it.

* Your doc subpackage shouldn't require the libs, since it's development documentation. If you want it to require anything, it should require the devel subpackage, as it is developer documentation.

* At the very least, the doc subpackage should be noarch, but as the config file doesn't appear to have any arch specific information, the default-config subpackage can also be noarch.

* Some dependencies seem to have failed to be detected. Of note, it looked like it was looking for CHECK and LIBECRYPTFS, and those failed to be found. At least with those two, it looks like you don't have the appropriate pkgconfig() BuildRequires (pkgconfig(check) and pkgconfig(libecryptfs)). It was also unable to locate /usr/include/trousers/tss.h (provided by trousers-devel).

* Since you're generating the autofoo, you need automake, autoconf, and autoconf-archive as BuildRequires. I'm not sure how the autogen.sh is working at the moment (maybe something else is pulling it in when it shouldn't), but in this case, it's better to be explicit about it.

Comment 3 Fabio Valentini 2017-04-16 15:55:27 UTC
1) It seems even elementaryOS ships gsignond without the setuid bit. I'll include a patch to not set the setuid bit.

2) I've changed it around so the main package depends on gsignond-config, which the -default-config package provides (and other packages can also provide in the future). I've also removed the dependency on the -libs subpackage (I think that was a copy-paste-error). I've also marked the -default-config subpackage as noarch.

3+4) I've removed the dependency on the -libs subpackage and marked the -docs package as noarch as well.

5) I've added pkgconfig(check), pkgconfig(libecryptfs) and trousers-devel as BuildRequires. The only problem is that - now that more plugins are built - some of them FTBFS (I've reported this issue upstream at [1]). I could also just disable the offending plugin, since it seems to be related to Intel's IoT OS (Ostro).

6) automake and autoconf are explicit dependencies of libtool - so adding them explicitly as BRs is not necessary.


[1]: https://gitlab.com/accounts-sso/gsignond/issues/9

Comment 4 Neal Gompa 2017-04-16 16:11:45 UTC
> I could also just disable the offending plugin, since it seems to be related to Intel's IoT OS (Ostro).

Since you've reported the issue upstream and it doesn't appear to be useful, disabling the broken plugin should be fine.

Comment 5 Fabio Valentini 2017-04-17 09:27:36 UTC
I think I have incorporated fixes for all the issues you pointed out. Also, I have disabled the Ostro OS and Tizen extensions. Links are the same.

New scratch build, with noarch -doc and -defautl-config subpackages:
https://koji.fedoraproject.org/koji/taskinfo?taskID=19040060

Comment 6 Neal Gompa 2017-04-17 12:47:57 UTC
Issues:


>[!]: Package must own all directories that it creates.
>     Note: Directories without known owners: /usr/share/gir-1.0, /usr/share
>     /gtk-doc, /usr/share/dbus-1, /usr/share/vala/vapi,
>     /usr/share/dbus-1/services, /usr/share/gtk-doc/html/gsignond,
>     /usr/share/gtk-doc/html, /usr/lib64/girepository-1.0,
>     /usr/share/dbus-1/interfaces, /usr/share/vala

Some of the ownership issues can be ignored (like /usr/share/vala, /usr/share/vala/vapi, /usr/share/gtk-doc, /usr/share/gtk-doc/html, /usr/lib64/girepository-1.0).

However, gsignond needs a runtime requires for dbus%{?_isa}, as it is necessary for its functionality to be useful.

The doc subpackage file entry needs the asterisk removed, because it's preventing RPM from considering that it should own /usr/share/gtk-doc/html/gsignond too.

>Rpmlint: 
> gsignond.x86_64: E: missing-call-to-setgroups-before-setuid /usr/bin/gsignond

Apparently, gsignond was a setuid binary on purpose. In the code, it uses seteuid() in daemon/main.c and common/gsignond-storage-manager.c.

However, it seems to gracefully fail when it does that. That said, apparently it's not doing setegid() before using seteuid() in common/gsignond-storage-manager.c (or in the ostro/tizen code, but I don't care much about them). From what I can tell, it's setuid so that it can drop privileges as a daemon and set storage directories to be individually owned by specific users.

In Fedora, we prefer if this can function with file capabilities, as they are more granular and when used well, can limit the damage caused by vulnerabilities to privileged applications. Please file a bug upstream to see if this can be appropriately resolved.

Action items:

* Fix the directory ownership issues
* File a bug upstream about the rpmlint error and to request gsignond to work with file caps instead.
- https://fedoraproject.org/wiki/Features/RemoveSETUID
- https://www.mankier.com/7/capabilities

Comment 7 Fabio Valentini 2017-04-17 13:55:11 UTC
- added: Requires: dbus%{?isa}
- removed asterisk from doc directory
- filed bug about setuid / file caps upstream at [1]

[1]: https://gitlab.com/accounts-sso/gsignond/issues/10

Comment 8 Neal Gompa 2017-04-17 15:33:47 UTC
Aside from the already known issues, fedora-review is happy with it.

PACKAGE APPROVED.

Comment 9 Gwyn Ciesla 2017-04-17 16:41:46 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/gsignond

Comment 10 Fabio Valentini 2017-04-17 17:30:29 UTC
I've reported a bug about the setgroups / setuid usage upstream at: https://gitlab.com/accounts-sso/gsignond/issues/11

Comment 11 Fedora Update System 2017-04-17 17:36:51 UTC
gsignond-1.0.6-1.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-4cfec99e64

Comment 12 Fedora Update System 2017-04-17 17:49:30 UTC
gsignond-1.0.6-1.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-85a6084eee

Comment 13 Fedora Update System 2017-04-18 13:22:41 UTC
gsignond-1.0.6-1.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-85a6084eee

Comment 14 Fedora Update System 2017-04-18 16:41:54 UTC
gsignond-1.0.6-2.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-edda6d990d

Comment 15 Fedora Update System 2017-04-18 19:53:29 UTC
gsignond-1.0.6-1.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-4cfec99e64

Comment 16 Fedora Update System 2017-04-19 10:56:46 UTC
gsignond-1.0.6-2.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-edda6d990d

Comment 17 Fedora Update System 2017-04-27 05:51:58 UTC
gsignond-1.0.6-2.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.


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