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

Make Airbitz default accounts system #842

Closed
wants to merge 5 commits into from

Conversation

paullinator
Copy link
Contributor

Always popup Airbitz login modal when user clicks SignUp/Login

@coveralls
Copy link

coveralls commented Jan 6, 2017

Coverage Status

Coverage increased (+0.05%) to 74.226% when pulling de57b1d on Airbitz:master into 6e5fde8 on AugurProject:master.

@tinybike
Copy link
Member

tinybike commented Jan 8, 2017

Can the modal header text be changed to something like Augur Login - Airbitz? Right now it says Airbitz Edge Login, but what Airbitz and Edge are aren't explained anywhere, so I think users may be confused by this. Otherwise it looks good!

@tinybike
Copy link
Member

tinybike commented Jan 8, 2017

@paullinator If I register with airbitz, then try and use the login ID (copy/pasted from the Accounts page) to login using the "manual" (non-airbitz) login, I get an invalid username/password error -- any idea what's going on here?

@tinybike
Copy link
Member

tinybike commented Jan 8, 2017

Also, when I do a non-PIN login using the Airbitz modal, the following error appears in the console:

VM7394 abcuiloader.js:32942 Uncaught TypeError: Cannot read property 'setLoading' of undefined
    at VM7394 abcuiloader.js:32942
    at eval (eval at <anonymous> (main.3778062….js:4168), <anonymous>:1882:7)
    at eval (eval at <anonymous> (main.3778062….js:4168), <anonymous>:1395:5)
    at eval (eval at <anonymous> (main.3778062….js:4168), <anonymous>:1214:5)
    at eval (eval at <anonymous> (main.3778062….js:4168), <anonymous>:986:12)
    at XMLHttpRequest.eval (eval at <anonymous> (main.3778062….js:4168), <anonymous>:1761:7)

(The login completes successfully, though.)

@tinybike
Copy link
Member

tinybike commented Jan 8, 2017

Also, could @paullintor or @swansontec explain a bit about what updating is planned to be added to the login-account selector?

	if (loginAccount.airbitzAccount) {
		loginAccount.onAirbitzManageAccount = () => {
			require('../../../selectors').abc.openManageWindow(loginAccount.airbitzAccount, (result, account) => {
				// Possibly update the loginAccount
			});
		};
	}

@paullinator
Copy link
Contributor Author

@tinybike In response to your question: "If I register with airbitz, then try and use the login ID (copy/pasted from the Accounts page) to login using the "manual" (non-airbitz) login, I get an invalid username/password error -- any idea what's going on here?"

I believe in the Augur code, the password for the login ID is hardcoded to something fixed and not the actual Airbitz password since the Airbitz password might change but the encryption of the login ID doesn't change. I'll need to look into the code to see what password is used.

@paullinator
Copy link
Contributor Author

@tinybike The hardcoded password for the LoginID that you pull out of the Account is simply "password". I looked at the code that generates the loginid and it's all in augur.js:loginWithMasterKey(). This was the routine you gave us at accomplishes the equivalent of augur.js:register() but with a privateKey given from the Airbitz SDK. There seems to be a few subtle differences between the two routines now and I'm wondering if they would be a cause for the issue. Specifically the kdfparams are determined a little differently in each. Can you take a look and see what would cause the differences?

@tinybike
Copy link
Member

I'm confused. Why are there hardcoded constant password, IV, and salt variables in loginWithMasterKey? Shouldn't these be passed in from Airbitz? These look like simple placeholders in a routine that I wrote as an example, so you guys could work from it.

var salt = new Buffer("6169fdd07cb61657ad0d1c60f1132eed52c91949d6d85654110b11ede80a6d2e", "hex");
var iv = new Buffer("ef40723ec10d95c4356c8d157ce4308e", "hex");
keys.deriveKey("password", salt, null, function (derivedKey) { ... });

Am I missing something / did I just not review your code thoroughly? This appears to be obviously insecure. Why are salt, iv, and "password" constants?

@paullinator
Copy link
Contributor Author

@tinybike These are constants because Airbitz does its own encryption in its SDK using it's own random salt, IV, and the a strong scrypt hash of the user's username and password. These can't be transferred out of the SDK to Augur. The keys are still secure as they are encrypted by Airbitz. We simply kept the keythereum format internally within Augur to provide compatibility with all your code that expects that format. The keythereum format however requires encryption parameters so we just gave it constant values to get it to work.

@tinybike
Copy link
Member

Ah, alright. So, the issue with this is that the "keystore" json that is generated can be trivially decrypted to reveal the plaintext private key (which is not supposed to be the case). However it shouldn't be necessary for the Airbitz login to mimic the "manual" augur login 100%. There are basically two things the derived key is used for by the front-end:

  1. The whole keystore json is made available for download (export); we can either disable this entirely for Airbitz accounts or else prompt the user to re-enter their password if they want to export their account to file (so that we can use the password to encrypt the private key at that point).

  2. The derived key is used to encrypt reports so that they can be revealed from a different machine (by using the same key to decrypt the reports, which are stored, encrypted, on the chain). The simplest option here is to just use the private key for report encryption/decryption. IIRC symmetric ciphers are about 2x as strong as ECC for a given key length; we use AES-128 for report encryption, so that should be at roughly the same security level as 256-bit ECDSA. So, dumb question...is it ok to re-use a key in this way?

@paullinator
Copy link
Contributor Author

@tinybike #1 (prompting the user to re-enter a password) is perfect if you don't mind the user doing that. Note that it's possible that the user enters a different password for #1 than they actually had in their Airbitz account.

#2. I wouldn't directly use the private key for encryption but instead apply a HMAC_SHA256 of the private key and use that for the AES128 encryption. @swansontec is more an expert on this than me and can comment further.

@tinybike
Copy link
Member

tinybike commented Jan 10, 2017

Addendum to point (1) re: exporting the keystore JSON to file. Is this a capability you are planning to add to Airbitz itself? The reason we have this as an option in augur is so users can export their "augur account" to a file which can be seamlessly read by geth (or other local ethereum node). Since Airbitz is now an Ethereum wallet, this would IMO be a nice feature for it to have anyway, to make it fully interoperable with other Ethereum clients.

If Airbitz wanted to offer this export functionality, we could just disable that export altogether from augur for Airbitz accounts, and instead it would be done thru the Airbitz interface. Alternately, Airbitz could just pass augur the keystore JSON.

(The "keythereum format" you refer to isn't actually something peculiar to keythereum -- it's an implementation of https://github.com/ethereum/wiki/wiki/Web3-Secret-Storage-Definition, which is something that geth and other eth clients also implement.)

@paullinator
Copy link
Contributor Author

#1 We do plan to offer key export eventually and even natively support ethereum in the Airbitz mobile wallet but not in the near future and not before Augur launch. For export from the Airbitz mobile wallet, we would probably prefer the 12 word mnemonic format vs the keystore JSON however since exporting a JSON file from a mobile device is pretty problematic. From within the JS SDK we could offer both options pretty easily.

#2 BTW. We currently store in the user's account a random string as the raw entropy for the master seed for creating the private key. If we instead stored a BIP44 word list, is there a standard way to go from word list to JSON keystore that's compatible will most ethereum dapps? I think future dapps may only want to support the word list and we'd like those wallets to be compatible with Augur. We should always be able to go from word list to JSON keystore, but if the JSON keystore is derived from a random string/buffer, we can't ever go back to a word list.

@tinybike
Copy link
Member

tinybike commented Jan 10, 2017

For (1), would it be an issue for you guys to import keythereum into Airbitz and add a function to your JS SDK that just does this:

Airbitz.getKeystoreJSON = function (password, privateKey, salt, iv, callback) {
    return keythereum.dump(password, privateKey, salt, iv, {}, callback);
};

(where password, privateKey, salt, and iv can be provided as either strings or Buffers, whichever is easier.) This function would be called on register/login, and the JSON output would be sent to augur.

@tinybike
Copy link
Member

If we instead stored a BIP44 word list, is there a standard way to go from word list to JSON keystore that's compatible will most ethereum dapps? I think future dapps may only want to support the word list and we'd like those wallets to be compatible with Augur.

In BIP44, the word list is used in combination with a user-supplied password to derive the private key, correct? As I understand it, the JSON keystore is sort of a level above that -- you've already generated the private key, then you encrypt it (with a password), and the JSON keystore is just the encrypted private key (+ parameters for the encryption). Or, am I misunderstanding your question?

@tinybike
Copy link
Member

If you're asking if Ethereum has a standard way of going from the mnemonic to the private key, unfortunately I don't think so at the moment -- googled a bit and it seems like it's still in the discussion phase. Here's a "pre-EIP" and some discussion: https://www.reddit.com/r/ethereum/comments/55i25c/preeip_password_protected_paper_wallets_feedback/ And here's an actual EIP that doesn't appear to have been acted on yet: ethereum/EIPs#75

I know Jaxx, MetaMask, Ledger Nano S, et al do use mnemonics, but I'm unfortunately not knowledgeable enough about them to know if they're compatible with each other. I hope they are :/

@tinybike
Copy link
Member

The immediate issue is some of the stuff related to our accounts won't work with Airbitz logins -- specifically, the giant goofy login ID (which is the keystore JSON in base58) and the account export features. What would you think of, instead of having a big blue "Manage Airbitz Account" button and modal, just replacing the top section on the account page with the airbitz account manager (for airbitz logins only, of course)? You'd have to display the account address, but other than that, it should be pretty much a drop-in thing.

@swansontec
Copy link
Contributor

swansontec commented Jan 10, 2017

Regarding the BIP44 mnemonic, @paullinator, I don't think we should go that route. Ethereum as an ecosystem has settled on an encrypted file as their key storage mechanism, so we should accept that choice and move on. There are usability tradeoffs either way, and besides, you yourself are always talking about how sucky mnemonics are.

Airbitz is just a key store. In fact, you can see the code in Augur that creates the ethereum key and puts it into Airbitz. Getting the key out is the same way. Airbitz's job is to manage and secure this key, which may or may not involve a password. In the case of Edge login, it's an explicit design goal not to know the password on the client.

Given an active Airbitz login, you can implement getKeystoreJSON pretty easily using airbitzAccount.getFirstWallet(walletType) like we do, and then just read out the key as a hex string.

This key, being 256 bits long, can be used both for EC stuff and with AES-256. The 256 refers to the expected key length, so this is the right crypto algorithm. Actually, you want AES-256-CBC + HMAC-SHA256 to ensure message integrity, but proper AEAD constructions are a whole other can of worms (which I am happy to open if you care to).

If you were really paranoid, you could maybe use aesKey = sha256(ethereumKey). That way, if there is a break in AES, an attacker can't learn the Ethereum key to steal funds. This is what Paul menat by "I wouldn't directly use the private key for encryption but instead apply a HMAC_SHA256 of the private key and use that for the AES128 encryption." OTOH, a break in EC is far more likely than one in AES-256, so you don't gain much. I'd just use the ethereumKey directly for encryption and not worry about it.

@tinybike
Copy link
Member

tinybike commented Jan 11, 2017

Given an active Airbitz login, you can implement getKeystoreJSON pretty easily using airbitzAccount.getFirstWallet(walletType) like we do, and then just read out the key as a hex string.

@swansontec The things needed to construct the keystore JSON are 1) the private key, 2) the user's password, and 3) the salt (+ initialization vector, if applicable). The password is presumably not stored anywhere by Airbitz, so there are two options for constructing the JSON:

  • Automatically create the keystore JSON when the user logs in (and provides their password), then save the JSON in memory. If Airbitz did this, it could just pass this object to Augur. (FWIW, this is how augur.js does it.)
  • If Airbitz isn't literally constructing the keystore JSON, it could pass augur equivalent information. @paullinator mentioned above that Airbitz is internally deriving a secret key from the password and salt using scrypt. When the user logs in, can Airbitz include this secret key (along with the private key), along with the salt and the encryption parameters? If we have that secret key, we don't need the password directly!

@paullinator
Copy link
Contributor Author

paullinator commented Jan 11, 2017

@tinybike Of those two above options:

  1. We don't always have the 'password' when the user logs in. They may login with a PIN or Edge Login via barcode so the password wouldn't be available.
  2. There are several keys decrypted by the password that we are guaranteed to have access to when the user logs in. However, if we use one of those as the JSON password, the user won't know what it is if trying to use the LoginID JSON bundle to login w/o Airbitz. Really the only useful password we can use is a fixed string like "password" and rely on the Airbitz SDK for encryption when data is at rest. Note, we STRONGLY recommend that you do NOT use the "derivedKey" for encrypting your data on the blockchain as that potentially puts password encrypted data on a public blockchain. Ideally you only want publicly accessible encrypted data to be encrypted by a fully random 256bit key such as the actual private key. I think you already recommended making this change and I think @swansontec would strongly recommend this as his first point as a security auditor.

@tinybike
Copy link
Member

@paullinator: I'm closing this b/c it conflicts with our recent auth refactoring / reskinning. Does the current setup at https://app.augur.net look alright to you? We thought the auto-popup wasn't great UX, so instead, we put the "login with airbitz" button at the top, and put the "manual" login below it. (Once you're logged in with airbitz, you receive the popup automatically, which we thought was reasonable since the user has already decided to use Airbitz at that point.) Let me know what you think!

@tinybike tinybike closed this Feb 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants