Bug 240476 - security/py-fido2: Add FreeBSD support
Summary: security/py-fido2: Add FreeBSD support
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Many People
Assignee: Michael Gmelin
URL: https://github.com/Yubico/python-fido...
Keywords:
Depends on:
Blocks:
 
Reported: 2019-09-10 15:40 UTC by Michael Gmelin
Modified: 2019-09-14 15:22 UTC (History)
3 users (show)

See Also:
bugzilla: maintainer-feedback? (python)
grembo: merge-quarterly-


Attachments
Patch to support fido/u2f on FreeBSD (3.72 KB, patch)
2019-09-10 15:40 UTC, Michael Gmelin
no flags Details | Diff
Poudriere output build flavour python 3.6 on 12.0-RELEASE i386 (26.23 KB, text/plain)
2019-09-12 22:20 UTC, Michael Gmelin
no flags Details
Poudriere output build flavour python 3.6 on 12.0-RELEASE amd64 (26.53 KB, text/plain)
2019-09-12 22:21 UTC, Michael Gmelin
no flags Details
Poudriere output build flavour python 3.6 on 11.2-RELEASE amd64 (26.21 KB, text/plain)
2019-09-12 22:22 UTC, Michael Gmelin
no flags Details
Poudriere output build flavour python 2.7 on 12.0-RELEASE i386 (26.48 KB, text/plain)
2019-09-12 22:23 UTC, Michael Gmelin
no flags Details
Poudriere output build flavour python 2.7 on 12.0-RELEASE amd64 (26.45 KB, text/plain)
2019-09-12 22:24 UTC, Michael Gmelin
no flags Details
Poudriere output build flavour python 2.7 on 11.2-RELEASE amd64 (26.46 KB, text/plain)
2019-09-12 22:25 UTC, Michael Gmelin
no flags Details
Output of portlint -a security/py-fido2 (44 bytes, text/plain)
2019-09-12 22:27 UTC, Michael Gmelin
no flags Details
Patch to support fido/u2f on FreeBSD, now with unit test support (14.58 KB, patch)
2019-09-12 22:35 UTC, Michael Gmelin
no flags Details | Diff
Patch to support fido/u2f on FreeBSD, now with unit test support (14.56 KB, patch)
2019-09-12 23:18 UTC, Michael Gmelin
no flags Details | Diff
Patch to support fido/u2f on FreeBSD, now with unit test support (14.63 KB, patch)
2019-09-12 23:39 UTC, Michael Gmelin
no flags Details | Diff
Patch to support fido/u2f on FreeBSD that was merged upstream (+ port skeleton changes) (15.31 KB, patch)
2019-09-13 16:00 UTC, Michael Gmelin
no flags Details | Diff
Patch to support fido/u2f on FreeBSD, requiring uhid-freebsd 1.2.1 (15.09 KB, patch)
2019-09-14 12:01 UTC, Michael Gmelin
koobs: maintainer-approval+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Gmelin freebsd_committer 2019-09-10 15:40:41 UTC
Created attachment 207347 [details]
Patch to support fido/u2f on FreeBSD

The attached patch adds support for the FIDO protocol on FreeBSD. The motivation for doing so was to be able to use `ykman fido' commands from security/py-yubikey-manager.

I tested using `ykman fido (list|info|reset|set-pin)' with a Yubikey 5 NFC (Yubico Yubikey OTP+FIDO+CCID)
Comment 1 Kubilay Kocak freebsd_committer freebsd_triage 2019-09-11 01:40:38 UTC
Thank you for the report and patch Michael

Could you:

- Confirm the changes pass QA (portlint, poudriere)
- If there's a test suite for fido2, confirm it passes with this patch (and adding TEST_DEPENDS / test: target support to port)
- Submit the patches in an upstream PR and add the PR title/URL as a comment in the patch header
Comment 2 Kubilay Kocak freebsd_committer freebsd_triage 2019-09-11 01:47:27 UTC
(In reply to Kubilay Kocak from comment #1)

Also, could you register/publish uhid-freebsd (under that name) in PyPI, so that upstream can depend on it in setup.py:install_requires using an environment marker ; sys_platform == 'freebsd'
Comment 3 Michael Gmelin freebsd_committer 2019-09-12 22:20:07 UTC
Created attachment 207433 [details]
Poudriere output build flavour python 3.6 on 12.0-RELEASE i386
Comment 4 Michael Gmelin freebsd_committer 2019-09-12 22:21:16 UTC
Created attachment 207434 [details]
Poudriere output build flavour python 3.6 on 12.0-RELEASE amd64
Comment 5 Michael Gmelin freebsd_committer 2019-09-12 22:22:28 UTC
Created attachment 207435 [details]
Poudriere output build flavour python 3.6 on 11.2-RELEASE amd64
Comment 6 Michael Gmelin freebsd_committer 2019-09-12 22:23:42 UTC
Created attachment 207436 [details]
Poudriere output build flavour python 2.7 on 12.0-RELEASE i386
Comment 7 Michael Gmelin freebsd_committer 2019-09-12 22:24:33 UTC
Created attachment 207437 [details]
Poudriere output build flavour python 2.7 on 12.0-RELEASE amd64
Comment 8 Michael Gmelin freebsd_committer 2019-09-12 22:25:23 UTC
Created attachment 207438 [details]
Poudriere output build flavour python 2.7 on 11.2-RELEASE amd64
Comment 9 Michael Gmelin freebsd_committer 2019-09-12 22:27:43 UTC
Created attachment 207439 [details]
Output of portlint -a security/py-fido2
Comment 10 Michael Gmelin freebsd_committer 2019-09-12 22:35:53 UTC
Created attachment 207441 [details]
Patch to support fido/u2f on FreeBSD, now with unit test support
Comment 11 Michael Gmelin freebsd_committer 2019-09-12 23:18:39 UTC
Created attachment 207444 [details]
Patch to support fido/u2f on FreeBSD, now with unit test support
Comment 12 Michael Gmelin freebsd_committer 2019-09-12 23:39:51 UTC
Created attachment 207445 [details]
Patch to support fido/u2f on FreeBSD, now with unit test support
Comment 13 Michael Gmelin freebsd_committer 2019-09-12 23:51:28 UTC
(In reply to Kubilay Kocak from comment #1)

Thanks for your response and the clear instructions Kubilay.

> - Confirm the changes pass QA (portlint, poudriere)
Confirmed, please find attached the output of `poudriere testport security/py-fido2' runs for various combinations of releases and architectures.

portlint -a is happy (output also attached).

> - If there's a test suite for fido2, confirm it passes with this patch (and adding TEST_DEPENDS / test: target support to port)
The test suite didn't work, as it would unconditionally import all test sources, including the linux tests, which require pyfakefs, which isn't available in ports (I tried a quick port, but it's quite linux specific and would've required even more work, just to support a unit test that won't run anyway). To fix this, setup.py now uses pytest, which allows exclusion of files in conftest.py (also gets rid of "you're not on darwin" warnings).

Test target support is now in the port skeleton, so `make test' does the Right Thing(tm). I also added unit tests for the FreeBSD specific bits and ran `make test' in all the poudriere combinations mentioned above manually (using a shell after invoking `poudriere testport -i').

> - Submit the patches in an upstream PR and add the PR title/URL as a comment in the patch header

To to so, some more work was necessary (like adding FreeBSD unit tests, changing readme, etc.). You can find the pull request here https://github.com/Yubico/python-fido2/pull/64

> Also, could you register/publish uhid-freebsd (under that name) in PyPI, so that upstream can depend on it in setup.py:install_requires using an environment marker ; sys_platform == 'freebsd'

PyPI account created and package published (https://pypi.org/project/uhid-freebsd/). devel/py-freebsd-uhid now pulls from there instead of github.

Anything else?
Comment 14 Michael Gmelin freebsd_committer 2019-09-13 16:00:51 UTC
Created attachment 207468 [details]
Patch to support fido/u2f on FreeBSD that was merged upstream (+ port skeleton changes)

The patch was accepted upstream. There were a few more changes, as they have reasons to not introduce a dependency on pytest at this point in time.

So hopefully this version of the patch is final.

See https://github.com/Yubico/python-fido2/pull/64
and https://github.com/Yubico/python-fido2/commit/19c86d5459931b8a76d1adc76420a8a1e0c0cf2e
Comment 15 Kubilay Kocak freebsd_committer freebsd_triage 2019-09-14 03:52:55 UTC
Thanks Michael, nice work upstreaming. 

A couple of leftovers:

- RUN_DEPENDS don't also need to be in TEST_DEPENDS (uhid-freebsd)
- setup.py says uhid-freebsd>=1.2.1, RUN_DEPENDS says >=1.0. *_DEPENDS should match upstream requirements as closely as possible modulo any syntax limitations we have (we cant foo!=x.y right now for example)

On the test suite with OS specific tests, upstreams should always 'skip' these tests if the environment isn't appropriate for the test. pytest and nose have very easy decorators upstreams can use to do this with various condition support)

When test suites 'dont' skip inappropriate tests, one can usually (with most unittest frameworks), skip individual tests explicitly in the test invocation.

For example with pytest you can -k 'not <expression>' and with nose you can -e{xclude} <regex>

With unittest (in Python > 3.1) you can skipIf [1], i cant recall if there's a command line arg for skipping tests. Also, if the tests use 'unittest', then the Python package should probably tests_require on "unittest2; python_version < 3"

[1] https://docs.python.org/3/library/unittest.html#unittest.skipIf
Comment 16 Michael Gmelin freebsd_committer 2019-09-14 11:26:24 UTC
(In reply to Kubilay Kocak from comment #15)

Hi Koobs,

> A couple of leftovers:

> - RUN_DEPENDS don't also need to be in TEST_DEPENDS
>   (uhid-freebsd)
> - setup.py says uhid-freebsd>=1.2.1, RUN_DEPENDS
>   says >=1.0. *_DEPENDS should match upstream
>   requirements as closely as possible modulo any
>   syntax limitations we have (we cant foo!=x.y
>   right now for example)

I will change those. Interestingly I couldn't find any mention of that fact in the porters handbook, also, this sounds like something that could be added to `portlint' quite easily.

Do we have any tooling for extracting such details from setup.py automatically?

> On the test suite with OS specific tests,
> upstreams should always 'skip' these tests
> if the environment isn't appropriate for
> the test. pytest and nose have very easy 
> decorators upstreams can use to do this
> with various  condition support)

Yes, that is what the pytest change did using conftest.py, by excluding whole files, as linux_test.py was incompatible on import (not just when running the tests) as pyfakefs was missing.

> When test suites 'dont' skip inappropriate
> tests, one can usually (with most unittest
> frameworks), skip individual tests explicitly
> in the test invocation.
> For example with pytest you can
> -k 'not <expression>' and with nose you can
> - > {xclude} <regex>

My first attempt was to do that in `do-test', by running `pytest -k' on the tests directory and excluding those offending files, which worked okay.

But running `setup.py test' seems cleaner (IMHO) as it will pick up more things specific to the project and once it became clear that I'll aim to upstream it, I wanted a solution that worked outside of the ports framework and allowed those who want to improve the library to start right away.

Also, obviously the added FreeBSD unit tests shouldn't interfere with upstream's CI. Introducing pytest or nose to the project would've made that easier, but they had valid reasons not to do that.

> With unittest (in Python > 3.1) you can
> skipIf [1], i cant recall if there's
> a command line arg for skipping tests.

This is exactly what the patch that was upstreamed does - it uses @skipIf on the platform specific test case classes:

  @unittest.skipIf(not sys.platform.startswith('linux'),
                   'Linux specific test case')
  class LinuxTest(unittest.TestCase):

That, plus an extra catch on importing pyfakefs that ignores the exception if the file is imported on a platform != Linux.

  try:
    from fakefs import fake_filesystem
  except ImportError:
    if sys.platform.startswith('linux'):
      raise

Not as clean as excluding the file completely, but it works. 

> Also, if the tests use 'unittest',
> then the Python package should
> probably tests_require on
> "unittest2;python_version < 3"

unittest2 is only required for versions of python < 2.7 in this project, so it should be okay (poudriere on FreeBSD and Travis CI on Linux agree):

  if sys.version_info[:2] < (2, 7):
      import unittest2 as unittest  # pylint: disable=g-import-not-at-top
  else:
      import unittest

So one more iteration to improve *_DEPENDS and we should be done for now :)
Comment 17 Michael Gmelin freebsd_committer 2019-09-14 12:01:33 UTC
Created attachment 207486 [details]
Patch to support fido/u2f on FreeBSD, requiring uhid-freebsd 1.2.1
Comment 18 Kubilay Kocak freebsd_committer freebsd_triage 2019-09-14 12:07:16 UTC
I'm happy if you are and it still passes QA after the last round(s) of changes

The only other thing I'd do (not a deal breaker, but desirable), is add patch comment headers with upstream references/links to issues/PR's so our future selves and others are clear on their provenance/status upstream. Please also include those "[1] references" in the commit log message to.

If you consider the lack of this freebsd support and the subsequent changes to be "fixing something that's broken/doesn't work", please also MFH the changes so that our quarterly users can benefit too. Otherwise, set the merge-quarterly flag to - to 'not accept' the MFH request/question

Lastly, thank you for all of this work to level up FIDO support on FreeBSD
Comment 19 Kubilay Kocak freebsd_committer freebsd_triage 2019-09-14 12:08:00 UTC
Comment on attachment 207486 [details]
Patch to support fido/u2f on FreeBSD, requiring uhid-freebsd 1.2.1

Approved by: koobs (python: maintainer)
Comment 20 commit-hook freebsd_committer 2019-09-14 15:09:59 UTC
A commit references this bug:

Author: grembo
Date: Sat Sep 14 15:09:18 UTC 2019
New revision: 512019
URL: https://svnweb.freebsd.org/changeset/ports/512019

Log:
  Add fido/u2f support for FreeBSD.

  Also adds test target.

  This was merged upstream, so patches can be removed from files/
  on the next release of python-fido2.

  See:
  https://github.com/Yubico/python-fido2/pull/64
  https://github.com/Yubico/python-fido2/commit/19c86d5459931b8a76d1adc76420a8a1e0c0cf2e

  PR:		240476
  Approved by:	koobs (python: maintainer)

Changes:
  head/security/py-fido2/Makefile
  head/security/py-fido2/files/
  head/security/py-fido2/files/patch-README.adoc
  head/security/py-fido2/files/patch-fido2___pyu2f_____init____.py
  head/security/py-fido2/files/patch-fido2___pyu2f_freebsd.py
  head/security/py-fido2/files/patch-setup.py
  head/security/py-fido2/files/patch-test___pyu2f_freebsd__test.py
  head/security/py-fido2/files/patch-test___pyu2f_linux__test.py
  head/security/py-fido2/files/patch-test___pyu2f_macos__test.py
Comment 21 Michael Gmelin freebsd_committer 2019-09-14 15:22:17 UTC
Double-checked QA before committing.

I added a comment about the source of the patches and that they can most likely be removed on the next release (NEWS already mentions FreeBSD support for the next release upstream) to all patches in files/ and in the commit message.

Given that 2019Q4 is just two weeks away, I decided against MFHing.

Thank you for your review and the good advise. I learned quite a bit about python packaging (CheeseShop, setuptools etc.) in the process. Quick turnaround times really help making contributing a positive experience.