Bug 217691 - net/chrony: add nss option + other cleanups
Summary: net/chrony: add nss option + other cleanups
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Rodrigo Osorio
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-11 01:32 UTC by John Hein
Modified: 2017-11-20 13:50 UTC (History)
2 users (show)

See Also:
bugzilla: maintainer-feedback? (yonas)


Attachments
[patch] NSS option + other cleanup (2.96 KB, patch)
2017-03-11 01:32 UTC, John Hein
yonas: maintainer-approval+
Details | Diff
[patch] add missed lib dependency specification (366 bytes, patch)
2017-11-19 22:25 UTC, John Hein
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Hein 2017-03-11 01:32:55 UTC
Created attachment 180709 [details]
[patch] NSS option + other cleanup

The attached patch adds an NSS option and some other cleanup.

portlint - ok
stage-qa - ok
testport - ok (10/stable)

Add NSS option:

Before this patch, if nss is installed when chrony is built, there's a silent lib dependency on nss, and if nss is subsequently uninstalled chrony breaks due to a now missing library.

I decided to turn it on by default:

   - it adds support for a number of more modern hashing algorithms (instead of only the default and less secure md5)

   - if NSS option is turned off, explicitly disable via configure option

   - nss is well maintained

   - I see the case for having NSS off by default.  Many users of chrony just want the basic features, and don't need the extra security.  Turning NSS off by default reduces dependency proliferation that is not necessary for many users.  So feel free to remove 'OPTIONS_DEFAULT=NSS' before committing this patch.

   - Override default NSS_DESC since it's generic text is not very helpful for chrony's usage.  The updated description is more specific regarding chrony's use of NSS.


Other cleanup:

 - --infodir is not a valid configure option (since 2.3 I think)
 - USES=localbase instead of LDFLAGS
 - add explicit --without-tomcrypt [1]
 - add support for passing chronyd_flags to chronyd in rc.d script
 - fix some hard-coded /usr/local in examples

[1] We could add a TOMCRYPT option which adds even more hashing algorithms.  But libtomcrypt does not have wide exposure.  There's some upstream security updates (also backported to debian's package) that have been around for years that were never added to freebsd's port.  The added benefit of some extra less common hashing algorithms didn't seem worth adding an option.  If we do add an option in the future, I believe it should be off by default in preference to nss.
Comment 1 Yonas Yanfa 2017-03-11 01:38:09 UTC
(In reply to John Hein from comment #0)

Thanks John! I'll take a look at this.
Comment 2 Yonas Yanfa 2017-03-18 11:47:57 UTC
(In reply to John Hein from comment #0)

Great patch. I agree about the dependency proliferation concern. The NSS package pulls in sqlite, which might seem strange and unnecessary to chrony users.

On the other hand, quite a few popular packages depend on NSS, including Chromium, Firefox, Thunderbird, GDM, and LibreOffice, so they likely already have it installed if they're a desktop user.

Since MD5 is such a weak hash, why is chrony still using it? Would disabling NSS essentially expose chrony users to security vulnerabilities? If so, what kind?
Comment 3 John Hein 2017-03-27 17:09:25 UTC
Regarding MD5, I haven't looked into its usage in chrony to see how safe or unsafe it is to use.  In some cases like the MD5 flavor of HMAC, MD5 is coupled with an additional private key which makes the MD5 weaknesses much less important.  Similarly MD5 + salt (as in /etc/passwd) with lots of iterations isn't as weak as a single md5 pass.  But I haven't looked at chrony to see exactly how it uses md5.  But, yes, even with other crypto sprinkled in, md5 is weaker, partially because it's less collision resistant and partially because it's a fast algorithm (which makes it somewhat easier to use brute force techniques), although a key generated with good entropy will mitigate that.

Anyway, I don't have a problem leaving a user with only MD5.  If that's what fits their use case, that's fine.

I'd feel better leaving NSS on by default, but I haven't done enough analysis to feel strongly.  If someone digs into the chrony code a bit to see how it uses md5, that would help inform the decision better.

Either way, the user should understand the implications of the different options.  As port maintainer, you can just make the call.  Lots of people use unauthenticated ntp, so the crypto users will likely be in the minority and are more likely to be the ones who will investigate their options.  Having it be an option is the most important first step.  Tweaking the default setting can be done later.
Comment 4 John Hein 2017-09-07 14:09:32 UTC
Ping.
Comment 5 Yonas Yanfa 2017-09-12 01:18:57 UTC
Comment on attachment 180709 [details]
[patch] NSS option + other cleanup

I've been wrestling on having the NSS dependency or not. I think we should, but if anyone has any strong reasons not to, please feel free to let us know.
Comment 6 Yonas Yanfa 2017-09-12 01:22:10 UTC
I tried approving the patch, but it's not working for some reason...
Comment 7 John Hein 2017-09-13 14:47:10 UTC
Comment on attachment 180709 [details]
[patch] NSS option + other cleanup

Yonas, I'm not sure what you tried and what is "not working" for you (re: approving the patch).  But I tried selecting the maintainer-approval (setting it to +) on the patch attachment, and it seemed to work fine.  But since I am not the maintainer, I have now changed that maintainer-approval field (on the patch attachment) to ? with your email.  If you want to approve the patch, just change the ? to + on the maintainer-approval field in the patch and submit.  If that doesn't work, provide details about the failure.
Comment 8 Yonas Yanfa 2017-09-13 22:46:46 UTC
(In reply to John Hein from comment #7)

Thanks John, it seems to work now. I've approved the patch.
Comment 9 commit-hook freebsd_committer 2017-11-18 22:41:52 UTC
A commit references this bug:

Author: rodrigo
Date: Sat Nov 18 22:41:34 UTC 2017
New revision: 454465
URL: https://svnweb.freebsd.org/changeset/ports/454465

Log:
  Makes the NSS dependency explicit, and perform cleanup
  Bump PORTREVISION

  Before this patch if nss is installed when crony is built, there's a silent lib
  dependency on nss, and if nss is subsequently uninstalled chrony breaks.

  NSS is now turned on by default adding support for a number of more modern
  hashing algorithms than md5.

  Cleanup:
   - --infodir is not a valid configure option (since 2.3 I think)
   - USES=localbase instead of LDFLAGS
   - add explicit --without-tomcrypt [1]
   - add support for passing chronyd_flags to chronyd in rc.d script
   - fix some hard-coded /usr/local in examples

  PR:		217691
  Submitted by:	John Hein <z7dr6ut7gs@snkmail.com>
  Approved by:	Yonas Yanfa <yonas@fizk.net> (maintainer)

Changes:
  head/net/chrony/Makefile
  head/net/chrony/files/chronyd.in
  head/net/chrony/files/patch-examples_chrony.conf.example2
  head/net/chrony/files/patch-examples_chrony.conf.example3
Comment 10 Rodrigo Osorio freebsd_committer 2017-11-18 22:42:50 UTC
Committed with minor changes, thanks.
Comment 11 John Hein 2017-11-19 22:25:44 UTC
Created attachment 188127 [details]
[patch] add missed lib dependency specification

(In reply to Rodrigo Osorio from comment #10)
Thanks.  I just realized that a dependency is missing.  Patch attached.
Comment 12 John Hein 2017-11-19 22:26:59 UTC
Re-open - see comment 11.
Comment 13 Rodrigo Osorio freebsd_committer 2017-11-19 22:53:56 UTC
@yonas : waiting for maintainer approval
Comment 14 Rodrigo Osorio freebsd_committer 2017-11-20 13:50:24 UTC
Committed, thanks.
Comment 15 commit-hook freebsd_committer 2017-11-20 13:50:56 UTC
A commit references this bug:

Author: rodrigo
Date: Mon Nov 20 13:50:00 UTC 2017
New revision: 454559
URL: https://svnweb.freebsd.org/changeset/ports/454559

Log:
  Add missing lib dependency in the original patch
  Bump PORTREVISION

  PR:		217691
  Submitted by:	John Hein  <z7dr6ut7gs@snkmail.com>
  Approved by:	Yonas Yanfa <yonas@fizk.net>

Changes:
  head/net/chrony/Makefile