Bug 220869 - sysutils/py-salt : Update to 2017.7.1
Summary: sysutils/py-salt : Update to 2017.7.1
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Ben Woods
URL: https://docs.saltstack.com/en/latest/...
Keywords: patch, patch-ready, security
Depends on:
Blocks: 221547
  Show dependency treegraph
 
Reported: 2017-07-19 22:39 UTC by Christer Edwards
Modified: 2017-08-26 08:46 UTC (History)
7 users (show)

See Also:
christer.edwards: maintainer-feedback+
woodsb02: merge-quarterly-


Attachments
patch (964 bytes, text/plain)
2017-07-19 22:39 UTC, Christer Edwards
no flags Details
patch (21.08 KB, patch)
2017-08-15 19:55 UTC, Christer Edwards
christer.edwards: maintainer-approval+
Details | Diff
sysutils/py-salt-common QA (530.33 KB, text/plain)
2017-08-15 19:56 UTC, Christer Edwards
no flags Details
sysutils/py-salt-tcp QA (21.21 KB, text/plain)
2017-08-15 19:56 UTC, Christer Edwards
no flags Details
sysutils/py-salt-zmq QA (20.90 KB, text/plain)
2017-08-15 19:56 UTC, Christer Edwards
no flags Details
Patch to update sysutils/py-salt to 2017.7.1 (4.97 KB, patch)
2017-08-20 08:49 UTC, Ben Woods
christer.edwards: maintainer-approval+
Details | Diff
patch-salt_modules_freebsdservice.py (530 bytes, patch)
2017-08-20 14:00 UTC, Ben Woods
christer.edwards: maintainer-approval+
Details | Diff
Makefile: py-futures no longer needed in Python 3.2+ (249 bytes, text/plain)
2017-08-24 08:05 UTC, Sven R
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Dani 2017-07-20 10:23:25 UTC
(In reply to Christer Edwards from comment #0)
W
ouldn't it make more sense to have two version's avi? One with the 2016.11.x and one with the 2017.7.x branch?

There are still some very critical issues open with the 2017.7.0 release, so i think updating right now is not a good idea...

E.g. see:
https://github.com/saltstack/salt/issues/42389
https://github.com/saltstack/salt/issues/42404
https://github.com/saltstack/salt/issues/42410
Comment 2 Kubilay Kocak freebsd_committer freebsd_triage 2017-07-23 11:21:05 UTC
Thank you Christer, please:

- Use the maintainer-approval flag on attachments (set to +) to signify maintainer submission/approval
- Confirm this change passes QA (ideally including run time/tests)
Comment 3 Ben Woods freebsd_committer 2017-07-23 13:54:04 UTC
Hi Christer,
Given Dani's reply, would you like to defer this update, or proceed?
If defer, please cancel the bug report.
Regards,
Ben
Comment 4 Christer Edwards 2017-07-29 20:13:17 UTC
We can wait for the 2017.7.1 release. Hopefully these will be fixed then.
Comment 5 Kubilay Kocak freebsd_committer freebsd_triage 2017-08-01 06:07:27 UTC
Comment on attachment 184521 [details]
patch

Obsolete patch pending update
Comment 6 Christer Edwards 2017-08-15 19:55:20 UTC
This is a big patch..

sysutils/py-salt : splitting package into -zmq, -tcp, -common
-------------------------------------------------------------

This patch moves sysutils/py-salt and splits it into meta-packages by transport method. I've done this for a few reasons:

1) I am also submitting security/py-hubble [#221547]. This port relies on SaltStack but no transport method is required. This, therefore, required a split by transport method.
2) This supports simple installation by transport method. ZMQ[2] or TCP[2]. Note: I built a RAET[2] transport package, but RAET is broken on FreeBSD; not submitting yet.
3) Desire to support transport-layer flexibility in SaltStack[1] on FreeBSD.
4) Challenge myself and extend my porting skills.

Note:
This will require a MOVED entry:
MOVED: sysutils/py-salt|sysutils/py-salt-zmq|2017-08-15|Supporting multiple transports; default -zmq

In addition to the above change, this release also patches a CVE (CVE-2017-12791)[3]:

CVE-2017-12791
--------------
Maliciously crafted minion IDs can cause unwanted directory traversals on the Salt-master

-----
Correct a flaw in minion id validation which could allow certain minions to authenticate to a master despite not having the correct credentials. To exploit the vulnerability, an attacker must create a salt-minion with an ID containing characters that will cause a directory traversal.  Credit for discovering the security flaw goes to: Vernhk@qq.com
-----


I realize that the previously submitted upstream bugs are not all patched (one closed, one not reproducible, and one appears waiting for feedback), but I want to go ahead with this release.

These three ports (-common, -zmq, -tcp) all pass QA (attached)

[1] https://www.youtube.com/watch?v=XjkTfBpen8M
[2] https://docs.saltstack.com/en/latest/topics/transports/
[3] http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-12791
Comment 7 Christer Edwards 2017-08-15 19:55:43 UTC
Created attachment 185456 [details]
patch
Comment 8 Christer Edwards 2017-08-15 19:56:15 UTC
Created attachment 185457 [details]
sysutils/py-salt-common QA
Comment 9 Christer Edwards 2017-08-15 19:56:37 UTC
Created attachment 185458 [details]
sysutils/py-salt-tcp QA
Comment 10 Christer Edwards 2017-08-15 19:56:56 UTC
Created attachment 185459 [details]
sysutils/py-salt-zmq QA
Comment 11 Ben Woods freebsd_committer 2017-08-16 10:32:36 UTC
Without having had a detailed look, I would have thought a better way to handle this to align with the Policy Of Least Astonishment (POLA) would be to:
- add 2 options to the existing sysutils/py-salt port (TCP and ZMQ) with ZMQ enabled by default and TCP disabled by default (OPTIONS_RADIO so that one and only one must be selected).
- Add a new slave port sysutils/py-salt-tcp which is the same as the master port but with ZMQ disabled by default and TCP enabled by default.

Doing it this way will mean:
- for most users a simple "pkg upgrade" will work with no involvement
- when the ports tree gets flavors support the TCP port could easily be deleted and converted to a flavor

Thoughts?
Comment 12 Christer Edwards 2017-08-16 15:42:02 UTC
The current version of the port does have make options to select the transport (although it's currently ZMQ & RAET). That could be updated to support TCP and I suppose we could have slave ports. I'm still learning when it comes to packaging, so I welcome the feedback.

I would still like to see a transport-less option to support the security/py-hubble port submission. Can we achieve all three in a non-disruptive way?
Comment 13 Ben Woods freebsd_committer 2017-08-20 07:51:31 UTC
Hi Christer,

Thanks for putting in a lot of work into this port, and it is good to see you extending your porting skills.

That said, I don't see the driver for having a "transportless" version of the port for other ports to depend on. This will bring additional maintenance overhead, and the 3 proposed ports duplicate 80% of their Makefiles.

The "transportless" sysutils/py-salt-common port which you have proposed installs almost the exact same files as the existing sysutils/py-salt which has the ZEROMQ option enabled by default with the following minor differences:
- does not install the salt rc.d scripts (which do nothing if not enabled)
- does not install master/minion config files (which do nothing if not enabled)
- has 6 fewer dependencies (142 instead of 148), as it does not depend on:
  - math/gmp
  - net/libzmq4
  - net/norm
  - net/openpgm
  - net/py-pyzmq
  - security/py-pycrypto

Other than this, all of the installed salt files are the same.

In my opinion, these differences are minor enough that the dedicated "transportless" version of the port is not warranted, and that any ports which depend on salt could just depend on the version of salt with the default transport option ZEROMQ enabled.

Similar arguments apply for the different transport options. These only install a few different runtime dependencies, and add 2 single line files which configure the transport option. This could easily be done by the sysadmin as it is described in the salt documentation:
https://docs.saltstack.com/en/latest/topics/transports/tcp.html

What are your thoughts?

I am about to propose a different patch, which simply updates the existing sysutils/py-salt port with a few minor tweaks. Could you please have a look and let me know what you think?

Thanks again for maintaining salt on FreeBSD!
Comment 14 Ben Woods freebsd_committer 2017-08-20 08:49:19 UTC
Created attachment 185600 [details]
Patch to update sysutils/py-salt to 2017.7.1

Update sysutils/py-salt to 2017.7.1

- Add TCP transport option
- Clarify the port options for transports only install the runtime dependencies
- Add note to pkg-message explaining how to change to non-default transports
- Change supported python releases to exclude 2.6 and allow python3 [1]
- Only depend on py-enum34 if python version is < 3.4 (included in python >= 3.4)
- Reorder Makefile to move OPTIONS after USES/USE/standard variables [2]
- Ensure Makefile lists are sorted alphabetically

[1] https://docs.saltstack.com/en/latest/topics/releases/2017.7.0.html#python-3
[2] https://www.freebsd.org/doc/en/books/porters-handbook/porting-order.html

Changes this release:
  https://docs.saltstack.com/en/latest/topics/releases/2017.7.0.html
  https://docs.saltstack.com/en/latest/topics/releases/2017.7.1.html
Comment 15 Ben Woods freebsd_committer 2017-08-20 14:00:48 UTC
Created attachment 185603 [details]
patch-salt_modules_freebsdservice.py

I have found that salt 2017.7.1 has a nasty bug on FreeBSD which prevents the service module from working (including any service.running lines in states).

The attached file patch-salt_modules_freebsdservice.py is a patch which, if included in the sysutils/py-salt/files/ directory, fixes the issue.

Further discussion on this issue can be found here:
https://github.com/saltstack/salt/issues/36675#issuecomment-323586323

Christer: with your approval I would like to also include this patch in the port update to 2017.7.1 unless a better fix becomes available.
Comment 16 commit-hook freebsd_committer 2017-08-22 23:02:36 UTC
A commit references this bug:

Author: woodsb02
Date: Tue Aug 22 23:02:22 UTC 2017
New revision: 448586
URL: https://svnweb.freebsd.org/changeset/ports/448586

Log:
  Update sysutils/py-salt to 2017.7.1

  - Includes fix for security vulnerability CVE-2017-12791
  - Include patch to fix bug in the freebsdservice module [1]
  - Add TCP transport option
  - Clarify the port options for transports only install the runtime dependencies
  - Add note to pkg-message explaining how to change to non-default transports
  - Change supported python releases to exclude 2.6 and allow python3 [2]
  - Only depend on py-enum34 if python version is < 3.4 (included in python >= 3.4)
  - Reorder Makefile to move OPTIONS after USES/USE/standard variables [3]
  - Ensure Makefile lists are sorted alphabetically

  [1] https://github.com/saltstack/salt/issues/36675#issuecomment-323586323
  [2] https://docs.saltstack.com/en/latest/topics/releases/2017.7.0.html#python-3
  [3] https://www.freebsd.org/doc/en/books/porters-handbook/porting-order.html

  Changes this release:
    https://docs.saltstack.com/en/latest/topics/releases/2017.7.0.html
    https://docs.saltstack.com/en/latest/topics/releases/2017.7.1.html

  PR:		220869
  Reported by:	Christer Edwards <christer.edwards@gmail.com> (maintainer)
  Approved by:	Christer Edwards <christer.edwards@gmail.com> (maintainer)
  Security:	CVE-2017-12791
  Security:	https://vuxml.freebsd.org/freebsd/3531141d-a708-477c-954a-2a0549e49ca9.html

Changes:
  head/sysutils/py-salt/Makefile
  head/sysutils/py-salt/distinfo
  head/sysutils/py-salt/files/patch-salt_modules_freebsdservice.py
  head/sysutils/py-salt/files/pkg-message.in
Comment 17 Ben Woods freebsd_committer 2017-08-22 23:10:18 UTC
Committed to head.
Waiting on feedback from ports-secteam for merge to 2017Q3 quarterly branch
Comment 18 Sven R 2017-08-24 08:05:41 UTC
Created attachment 185711 [details]
Makefile: py-futures no longer needed in Python 3.2+
Comment 19 commit-hook freebsd_committer 2017-08-26 07:52:53 UTC
A commit references this bug:

Author: woodsb02
Date: Sat Aug 26 07:51:59 UTC 2017
New revision: 448753
URL: https://svnweb.freebsd.org/changeset/ports/448753

Log:
  sysutils/py-salt: Only depend on devel/py-futures with python2

  (futures is built into python3)

  PR:		220869
  Submitted by:	admin@netzmacher.net

Changes:
  head/sysutils/py-salt/Makefile
Comment 20 commit-hook freebsd_committer 2017-08-26 08:44:36 UTC
A commit references this bug:

Author: woodsb02
Date: Sat Aug 26 08:44:15 UTC 2017
New revision: 448754
URL: https://svnweb.freebsd.org/changeset/ports/448754

Log:
  sysutils/py-salt: Update to 2016.11.7

  - Includes fix for security vulnerability CVE-2017-12791
  - Use @sample for the master and minion config files.

  PR:		220869
  PR:		217780
  Approved by:	ports-secteam (delphij)
  Security:	CVE-2017-12791
  Security:	https://vuxml.freebsd.org/freebsd/3531141d-a708-477c-954a-2a0549e49ca9.html

Changes:
  branches/2017Q3/sysutils/py-salt/Makefile
  branches/2017Q3/sysutils/py-salt/distinfo
  branches/2017Q3/sysutils/py-salt/pkg-plist
Comment 21 Ben Woods freebsd_committer 2017-08-26 08:46:18 UTC
All committed - thanks everyone.
Ports-secteam advised me to keep the ports r2017Q3 quarterly branch on the salt 2016.11 tree if possible, so I updated it to 2016.11.7 to fix the security vulnerability.