Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config.hardware.onlykey configuration option #33794

Merged
merged 1 commit into from Apr 9, 2018

Conversation

yrashk
Copy link
Contributor

@yrashk yrashk commented Jan 12, 2018

By default, OnlyKey device (https://crp.to/p/) won't work on Linux (and,
therefore, NixOS). This is unintuitive and requires one to search for a
solution in the documentation.

This change allows one to enable OnlyKey device support directly from
their NixOS configuration.

Motivation for this change

Adding straightforward support for OnlyKey to NixOS.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Tested in a VM
  • Fits CONTRIBUTING.md.

config = mkIf config.hardware.onlykey.enable {
services.udev.extraRules =
''
# UDEV Rules for Teensy boards, http://www.pjrc.com/teensy/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole block appears to be copied from a file. Either:

  1. download the file to the containing directory. Use builtins.readFile.
  2. remove the superfluous comments about how to install the file. Not relevant.

#
ATTRS{idVendor}=="16c0", ATTRS{idProduct}=="04[789B]?", ENV{ID_MM_DEVICE_IGNORE}="1"
ATTRS{idVendor}=="16c0", ATTRS{idProduct}=="04[789A]?", ENV{MTP_NO_PROBE}="1"
SUBSYSTEMS=="usb", ATTRS{idVendor}=="16c0", ATTRS{idProduct}=="04[789ABCD]?", MODE:="0666"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mode "0666" is questionable. That adds RW access to all users.

Typically udev rules add RW access to all users in the "plugdev" group. Which is users who are authorized to "plug in devices".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the new style is to use TAG+="uaccess" instead of using GROUP= and MODE=.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bjornfor I just tested with TAG+="uaccess" on my local system and the device didn't work, switch back to GROUP made it work. Not yet sure why.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at other packages it looks like that'd require creating a new udev file with an earlier precedence. Is this what you want me to do?

@yrashk
Copy link
Contributor Author

yrashk commented Mar 18, 2018

Who'd be the best person to talk to about this change? Thanks!

@yrashk
Copy link
Contributor Author

yrashk commented Mar 18, 2018

@coreyoconnor thank you for the feedback, will address shortly.

By default, OnlyKey device (https://crp.to/p/) won't work on Linux (and,
therefore, NixOS). This is unintuitive and requires one to search for a
solution in the documentation.

This change allows one to enable OnlyKey device support directly from
their NixOS configuration.
@yrashk
Copy link
Contributor Author

yrashk commented Mar 18, 2018

@coreyoconnor I've updated this PR to address your feeedback. Hopefully I've got it right now :)

@yrashk
Copy link
Contributor Author

yrashk commented Mar 27, 2018

Anything else I should address in this patch?

@matthewbauer matthewbauer merged commit 07886aa into NixOS:master Apr 9, 2018
@CMCDragonkai
Copy link
Member

CMCDragonkai commented May 4, 2018

It really should be in the docs that users should be part of the "plugdev" group if we want them to be able to access various USB devices. I have to create a rules for ledger nano s. We should also standardise across the different USB devices to use the "plugdev" group as the main group. Currently it's just a magic string that you have to know about by going through the nixpkgs source.

@d10v
Copy link

d10v commented Jul 25, 2019

@yrashk Hi, Onlykey updated it's vendor and product id's, I'm not sure if it's set in hw or firmware, it would be nice to include new values and maybe preserve old ones. Does current NixOS config work for you at the moment?

@jtojnar jtojnar mentioned this pull request Oct 15, 2020
@ranfdev
Copy link
Contributor

ranfdev commented Oct 23, 2020

Indeed the onlykey now has a different vendor and product id, so the current udev rule doesn't work. https://docs.crp.to/linux.html

By looking at the commit history of the official onlykey udev rule, I see that the version of this PR was kept from May 14 2018 to Nov 12 2018, so only for 6 months.
https://github.com/trustcrypto/trustcrypto.github.io/commits/master/49-onlykey.rules
Some days later, onlykey released a firmware update:
https://crp.to/2018/11/onlykey-beta-7-release-blockchains-bootloaders-and-better-onlykey-experience/, probably changing the product id.

Users can upgrade their onlykey by using the official app. I think they even get a notification, so the users who bought the onlykey from May 14 to Nov 12 2018 probably have already upgraded by now.
Considering all of this, I think it's pretty safe to do another PR to use the new vendor and product id.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants