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)
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
(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'
Created attachment 207433 [details] Poudriere output build flavour python 3.6 on 12.0-RELEASE i386
Created attachment 207434 [details] Poudriere output build flavour python 3.6 on 12.0-RELEASE amd64
Created attachment 207435 [details] Poudriere output build flavour python 3.6 on 11.2-RELEASE amd64
Created attachment 207436 [details] Poudriere output build flavour python 2.7 on 12.0-RELEASE i386
Created attachment 207437 [details] Poudriere output build flavour python 2.7 on 12.0-RELEASE amd64
Created attachment 207438 [details] Poudriere output build flavour python 2.7 on 11.2-RELEASE amd64
Created attachment 207439 [details] Output of portlint -a security/py-fido2
Created attachment 207441 [details] Patch to support fido/u2f on FreeBSD, now with unit test support
Created attachment 207444 [details] Patch to support fido/u2f on FreeBSD, now with unit test support
Created attachment 207445 [details] Patch to support fido/u2f on FreeBSD, now with unit test support
(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?
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
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
(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 :)
Created attachment 207486 [details] Patch to support fido/u2f on FreeBSD, requiring uhid-freebsd 1.2.1
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 on attachment 207486 [details] Patch to support fido/u2f on FreeBSD, requiring uhid-freebsd 1.2.1 Approved by: koobs (python: maintainer)
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
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.