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

Secure Enclave wallet support #4244

Merged
merged 6 commits into from
Jun 29, 2018
Merged

Secure Enclave wallet support #4244

merged 6 commits into from
Jun 29, 2018

Conversation

spoonincode
Copy link
Contributor

@spoonincode spoonincode commented Jun 20, 2018

Add Secure Enclave wallet support. When Secure Enclave is available, a wallet named SecureEnclave is available for use with cleos’ wallet commands. Unlocking the wallet is based on local user authentication — for modern MacBook Pros unlocking the wallet can use your fingerprint. For other macOS devices, your local username/password is required. This means that the password sent to the wallet from cleos is ignored — cleos has been modified to not ask for the password on operations against SecureEnclave wallet.

Once unlocked, you can issue wallet create_key -n SecureEnclave commands to create keys within the Secure Enclave wallet. You may not import keys.

Secure Enslave support requires the application to be signed with “Mac App Store style signing”. This is technically possible to self-sign provided you have an AppleID however you will need to use Xcode to generate the provisioning profile.

Completes #2252

Add Secure Enclave wallet support. When Secure Enclave is available, a wallet named SecureEnclave is available for use with cleos’ wallet commands. Unlocking the wallet is based on local user authentication — for modern MacBook Pros unlocking the wallet can use your fingerprint. For other macOS devices, your local username/password is required.

Once unlocked, you can issue wallet create_key -n SecureEnclave commands to create keys within the Secure Enclave wallet. You may not import keys.

Secure Enslave support requires the application to be signed with “Mac App Store style signing”. This is technically possible to self-sign provided you have an AppleID however you will need to use Xcode to generate the provisioning profile.
Can't get this working yet; just disallow it for now
... But there appears to be no way to query this beyond waiting for a tricky error message from one of the find/add APIs??? For now just use the model number of the system which of coruse is totally bogus and fragile
promise<bool> prom;
future<bool> fut = prom.get_future();
macos_user_auth(detail::auth_callback, &prom, CFSTR("unlock your EOSIO wallet"));
if(!fut.get())
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 will block execution until the user interacts with the GUI. Lousy for keosd but still manageable I think. It’s probably more of a problem if using wallet inside of nodeos

se_wallet::se_wallet() : my(new detail::se_wallet_impl()) {
detail::check_signed();

//How to figure out of SE is available?!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realize how incredibly bogus this is. Will buy lunch to someone who can point out a quality way of detecting Secure Enclave support (beyond attempting to create a SE key and looking for a particular return code... which also isn't defined AFAIK)

Copy link
Contributor

Choose a reason for hiding this comment

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

According to the interwebs, only Macs with an IOKit registry value for "AppleEmbeddedOSSupportHost" have secure enclaves. That name leads me to believe it will be valid even if there's such a thing as a new Macbook with no touch bar but still a secure enclave.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this AppleEmbeddedOSSupportHost is not present on iMacPros, which I have verified have a Secure Enclave and work with this wallet code.

return pub_key_data;
}

static public_key_type get_public_key(SecKeyRef key) {
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 is somewhat icky, but public_key class doesn’t have a way to conjure up an instance otherwise (unless going through private_key, which of course we don’t have in this case!) There is something similar for a signature.

An alternative would be for us to make a public_key & signature_type ctor that allows me to create said objects with raw x/y and r/s values. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Providing an way to do this if you know the wrapped type (like R1) makes sense. I'm not sure given that you are dealing with a wrapper and stubs in public_key_type that it makes sense to suggest there should always be a way to take raw values and form a type.

@@ -38,6 +36,8 @@ void wallet_plugin::set_program_options(options_description& cli, options_descri
void wallet_plugin::plugin_initialize(const variables_map& options) {
ilog("initializing wallet plugin");

wallet_manager_ptr = std::make_unique<wallet_manager>();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved just so that wlog() would be functional in wallet_manager’s construction

@@ -249,6 +249,15 @@ chain::action generate_nonce_action() {
return chain::action( {}, config::null_account_name, "nonce", fc::raw::pack(fc::time_point::now().time_since_epoch().count()));
}

void prompt_for_wallet_password(string& pw, const string& name) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

SecureEnclave wallet ignores the password field for the unlock & remove_key actions because it always uses the local system's authentication (this way we can use fingerprint authentication for it). This cheesy check makes sure we don't prompt for a useless password for this wallet

@spoonincode
Copy link
Contributor Author

Two other review notes:

  1. I elected not to wrap all the CoreFoundation references with C++ wrappers like we do in fc::openssl, for example. I am willing to revisit this if we feel it will markedly improve code readability and robustness.
  2. No unit tests. Not entirely clear how to implement unit tests here when even unlocking requires farming out to a UI prompt

@wanderingbort wanderingbort added this to the Version 1.1 milestone Jun 27, 2018
return pub_key_data;
}

static public_key_type get_public_key(SecKeyRef key) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Providing an way to do this if you know the wrapped type (like R1) makes sense. I'm not sure given that you are dealing with a wrapper and stubs in public_key_type that it makes sense to suggest there should always be a way to take raw values and form a type.

@spoonincode spoonincode merged commit 6759c79 into release/1.1 Jun 29, 2018
@spoonincode spoonincode deleted the se_wallet branch July 3, 2018 22:07
@wanderingbort wanderingbort mentioned this pull request Jul 13, 2018
14 tasks
@ghost ghost mentioned this pull request Aug 18, 2018
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants