Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

libyubihsm2 upgrade & YubiHSM 2.1 support #6733

Merged
merged 30 commits into from
Feb 20, 2019
Merged

libyubihsm2 upgrade & YubiHSM 2.1 support #6733

merged 30 commits into from
Feb 20, 2019

Conversation

spoonincode
Copy link
Contributor

@spoonincode spoonincode commented Feb 13, 2019

Change Description

Upgrade the yubihsm wallet to use libyubihsm2. This brings about two user facing features:

  • Support for newly shipping YubiHSM 2.1 firmware
  • Support for talking to a YubiHSM directly through USB instead of through the connector (use yubihsm-url = yhusb://)

libyubihsm2 is open source so now we build and statically link to the library internal to the eosio project. This makes using YubiHSM for users much easier as they don't have to chase down Yubico's installer and dump the .sos in the correct directory (correct can be tricky here). However, this also makes eosio have three new dependencies: libusb, libcurl, and pkg-config. All three of these are pretty standard fare so I don't feel too guilty adding them as dependencies.

CI is currently failing because of the inability to have libusb available in the test instance (I link to it dynamically since I consider it much like other system level packages like openssl)

Consensus Changes

API Changes

Documentation Additions

LTO objects confuse some of the older platforms/compilers we target
Something really wonky here; it's like the script tries to unlink everything it installs?? Get this working for now with just the libusb that I added
EXCLUDE_FROM_ALL blocks out all non-dependant targets but this doesn't prevent add_test() from being called on the now non-existant targets. I tried several approaches to remove these tests (without changing upstream cmake) and this approach was the best when factoring in that it's documented and simple
if [[ "$DEP" == "llvm@4" ]]; then
"${BREW}" unlink ${DEP}
elif [[ -z "$DEP" ]]; then
true
elif ! "${BREW}" unlink ${DEP} && "${BREW}" link --force ${DEP}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This here is all sorts of WTFBBQ. The script seems written such that for any dependencies we're installing unlink them, but if we fail to unlink force link them. That just really boggles my mind. Also bizarre is that DEP appears to be a list of items. So the if [[ "$DEP" == "llvm@4" ]]; checks seems... not right. How could DEP ever be "llvm@4" when the line to change DEP is DEP=$DEP"${brewname} " -- it has a space after every DEP.

I need libusb to remain linked, which is its default, but that unlink ${DEP} burns me. So add some stuff here to remove libusb from the DEP list and skip the unlink/force link if DEP is then empty because of that removal.

@heifner
Copy link
Contributor

heifner commented Feb 20, 2019

Looks like you need to rebase to develop.

@NorseGaud
Copy link
Contributor

mac builders -
cicdm012-b1-v2-1
cicdm011-b1-v2-1
mac testers -
cicdm003-t1-v2-1
cicdm008-t1-v2-1

@spoonincode spoonincode merged commit acb3ac1 into develop Feb 20, 2019
@spoonincode spoonincode deleted the libyubihsm2 branch February 20, 2019 21:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants