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

Remove all key management from the LCD #1138

Closed
adrianbrink opened this issue Jun 5, 2018 · 16 comments
Closed

Remove all key management from the LCD #1138

adrianbrink opened this issue Jun 5, 2018 · 16 comments

Comments

@adrianbrink
Copy link
Contributor

I discussed this with @ebuchman today.
The LCD should not expose any keys, instead the UI should handle keys. The LCD can help to assemble a valid transaction and accepts a signed transaction to be submitted to a full-node.

The LCD should only be responsible for handling light-client verification and not also double as a signing oracle. A lot of the early Ethereum vulnerabilities came from the fact that a full-node could expose unlocked private keys to potentially any network interface.

By removing key management from the LCD we make the code cleaner and make it easier in the future to migrate the LCD to Rust or JavaScript to be used in websites. The problem is in any case that we cannot start a go process from within a website.

gaiacli should ideally connect to the LCD, and gaiacli handles the key management. We need to ensure that it is easy enough to integrate the ledger with an electron app.

@nylira @faboweb What are the effects on Voyager of this?

@cwgoes @zmanian @rigelrozanski What are your thoughts on this?

@cwgoes
Copy link
Contributor

cwgoes commented Jun 5, 2018

Strongly in favor.

I think we should target removing the light client daemon as a dependency for wallets completely. It introduces an inefficient blocking dependency of frontend code on the SDK daemon, makes writing a secure LCD harder, and will never work for any third-party in-browser wallets (or even local wallets which aren't willing to ship the gaiacli binary). A hypothetical Rust-to-WASM LCD is an interesting solution, but I think that will be way more work in the near term than just porting transaction construction, light client verification, and key management logic to common Javascript libraries (which Voyager can use) - and the latter is likely to result in easier-to-use libraries for developers in any case.

Not sure how viable this is on the Voyager end, open to brainstorming ways the SDK team might be able to help. Maybe separating gaiacli is an impractical requirement for launch right now, but I think we should aim for this in the near term.

@adrianbrink
Copy link
Contributor Author

Another point is that the TokenAPI will not leverage any of the key management functionality of the LCD anyway since it only accepts signed transactions.

ref #1137

@rigelrozanski
Copy link
Contributor

Okay so what happens if I don't want to use HSM to sign messages? Would the key management occur in Voyager? If so, I think this is a great idea - it introduces a bit of code dup - but as long as we're explicit enough in our keys-spec then this shouldn't be a problem :)

@zmanian
Copy link
Member

zmanian commented Jun 5, 2018

Isn't our ledger integration via LCD tho?

@NodeGuy
Copy link
Contributor

NodeGuy commented Jun 5, 2018

I agree that key management does not belong in the LCD.

Jessy posted an excellent presentation by Google in #security that talks about the growing importance of building stronger isolation into our system components: https://docs.google.com/presentation/d/17bKudNDduvN-7hWv7S84MiHUj2AnOPNbwjTM8euDC8w/edit#slide=id.p1

For the same reason, key management doesn't belong in the UI either. This is, in fact, the whole point of an HSM: It's isolated from the rest of the system.

If we're going to do key management in software then we should create a software version of an HSM, i.e., single-purpose.

@jaekwon already argued this point well in #324 (comment).

@okwme
Copy link
Contributor

okwme commented Jun 5, 2018

would voyager be responsible for writing txs or just signing them? would this be a sec issue if/when voyager moved to the browser?

@rigelrozanski
Copy link
Contributor

Okay cool - what we need is a detailed spec for ckeystore described in #324 - ultimately I see there being 3 implementations:

  • HSM
  • cli (in golang)
  • UI, seperate from voyager, but hopefully very closely linked, I'd imagine maybe a popup or something, however the application would still be separate and easily audit-able

CC: @faboweb

@jbibla
Copy link
Contributor

jbibla commented Jun 5, 2018

though i understand the merits of the different arguments here - i want to weigh in by saying that a couple weeks ago, the voyager team agreed to work on LCD.js after launch. most of the changes discussed here would be in line with the requirements for making LCD.js to happen - which is great.

however, making major changes like this, this close to launch probably isn't the right move. unless there are known bugs or vulnerabilities with the current implementation and unless these changes will provide tangible user value - it really seems like this is not a good time to make a change like this.

if we feel like we need to make all these changes a pre-requisite for launch, we are realistically looking at adding months of development time to our project.

@ebuchman
Copy link
Member

ebuchman commented Jun 5, 2018

most of the changes discussed here would be in line with the requirements for making LCD.js to happen

This proposal would not require LCD.js. The LCD would still do all the querying and proofs and could still actually create transactions.

All we're talking about here is moving the signing itself elsewhere.

Isn't our ledger integration via LCD tho?

Currently yes, its the same code used by gaiacli. We certainly still want the command line tools to be able to do signing, eg for offline signing too.

Basically this proposal would require adding support in voyager for creating keys (either in the JS or by connecting to an external HSM from the JS) and signing txs with them (again, either in the JS or by connecting to an external HSM from the JS).

@ebuchman
Copy link
Member

ebuchman commented Jun 5, 2018

would voyager be responsible for writing txs or just signing them? would this be a sec issue if/when voyager moved to the browser?

For this proposal, just signing them. The LCD can still create the txs

@ebuchman
Copy link
Member

ebuchman commented Jun 5, 2018

If we're going to do key management in software then we should create a software version of an HSM, i.e., single-purpose.

I think part of the rationale here is to not have a separate process for key-mgmt that binds a socket because of the inevitable issues of people accidentally not restricting access to that socket. So having a separate go process just for signing doesn't really address the issue, even though it does help on the separation of concerns front.

@NodeGuy
Copy link
Contributor

NodeGuy commented Jun 5, 2018

I think part of the rationale here is to not have a separate process for key-mgmt that binds a socket because of the inevitable issues of people accidentally not restricting access to that socket. So having a separate go process just for signing doesn't really address the issue, even though it does help on the separation of concerns front.

Does this concern apply equally well to using an HSM?

@ebuchman
Copy link
Member

ebuchman commented Jun 5, 2018

Does this concern apply equally well to using an HSM?

No, the ideal is for everyone to use an HSM. If folks are using an HSM, I think it matters much less if it's managed from the LCD or from Voyager or a third process. Third process would be great from separation of concerns front, and wouldn't have the other problems associated with it re binding sockets since the user authorization is required on the HSM regardless of who can talk to the process,

The problem as I see it is really only when the keys are not on a separate machine.

@NodeGuy
Copy link
Contributor

NodeGuy commented Jun 6, 2018

I see.

[it] wouldn't have the other problems associated with it re binding sockets since the user authorization is required on the HSM regardless of who can talk to the process ...

This could be true for a software HSM as well if it has its own UI as proposed by @rigelrozanski.

@faboweb
Copy link
Contributor

faboweb commented Jun 6, 2018

The proposal is to slowly migrate to not create unexpected work prelaunch. We would implement the /txs/broadcast endpoint for general purpose tx transmission and endpoints to build transactions as planned. Then later we remove the build -> sign -> send endpoints in favor of extracting the KMS for separation of concerns in cost of more implementation needed in integrating applications.

@adrianbrink
Copy link
Contributor Author

adrianbrink commented Jun 21, 2018

ICS1 (KeyAPI) will implement the key management. It is an optional feature of the LCD and clients can choose to use it or not. No other APIs depend on ICS1.

ICS1 will support HMS and plain-text keys.

I'll close this in favour of #1314 .

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

No branches or pull requests

9 participants