CLI and REST rewrite #324
Comments
|
Great start. The idea was that the rest api and the cli mirror each other very closely. Pub/sub for events (new block, new tx) is needed on the rest api only and not included yet as part of this spec. We will make a second issue/proposal for it later to extend the functionality, but this should provide a solid basis to use apps. |
|
Cli equivalents to the GET/POST commands....
|
|
Thought was that keys doesn't need init (all If this is unpopular, we can easily embed those commands under |
|
Seems like a great start. Love the idea of the CLI and REST mirroring each other. Seems you've left out the command to actually start the REST server. I think we want to make sure the Does this mean we'll be dropping Will the gaia CLI still support the |
|
It might not make for the best CLI experience if all the things currently under the So while I like the idea of mirroring the REST API, it might actually make the CLI a little harder to use. Could use more outline on how some of those queries will look like, seems they've been left out above (ie. querying accounts and other application state). |
|
I also like having a pure |
|
Finally, what should this binary actually be called? |
|
chub? |
|
I'd go for But won't we want to also use this tool to access other zones? Not unreasonable for it to be called |
|
|
|
Chub is cool. Most of these commands would be in the Sdk, so they would be similar in all zones that build on the Sdk.
As to the queries... Fabo wanted to make them top level. It is shorter. If it is confusing, let us put them all under |
|
Proxy was removed as all needed proxy items should be exposed in the rest. Rpc calls that we found actually useful were made top level. Block (for Block, header, and commit), validatorset, and status. If there are other rpc calls we need in the ui apps, then we can add them one by one. Keep the interface clean, add when needed, not just a kitchen sink. All query stuff has custom commands that can parse. All TX Actions have custom commands. What is left? Commits are handled internally by the LCD... Net_info??? |
|
Regarding RPC: Regarding CLI:
Regarding resources: Regarding doubts about special usecases: @ebuchman |
|
i've read through this thread a couple times and am impressed and pleased. |
|
lol The above sounds good, thanks @faboweb and @ethanfrey Though indeed, using the REST verbs in the CLI commands would make it much harder to write, so let's not do that. Re the re-write, we basically have to do a lot of re-writing to target the new SDK anyways, so it shouldn't be much more work to get the CLI and REST spec to match too. |
|
The organization of the cli is easy, I have to implement all the logic against the new sdk. Most of the pieces are available in tendermint/lite or in cosmos-sdk/client (which needs to be ported). The big piece is porting the integration of client from old sdk to sdk2. The reorg of cli is just a couple days on top of that. Jae wanted to personally review all this and approve it before I start work on it. But he is busy writing code. He sorta deferred to bucky for preliminary approval, but I have no energy to start code that will be ripped apart later. I would like the spec approved before starting. |
|
Also realize that without query being implemented in sdk2, no query functionality can be implemented, which while only a small part of the code, is essential to test it works. |
|
Some thoughts about |
|
Can we just stick with one binary for now? No one has ever been satisfied breaking things up into multiple binaries. We've consolidated binaries multiple times. What's the reason to not use a single binary ?
So how's this actually supposed to work w.r.t the UI? The UI boots and the user is prompted for a password - then what ? Obviously the best approach is for the user to use an HSM and then no password needed but some folks wont have an HSM and will need to encrypt on disk - then what ? |
|
@jaekwon Before spec-ing anything out, I want to address the high level design decisions first. I think we may agree on much, and maybe I just don't understand your post fully. If you could respond with your thoughts on these points, it would help me clarify. The location of passwords is a bit unclear to me, but I am flexible. Point 4 below seems most essential to the usability, and I am not sure if we agree or disagree there, so it would be very helpful to see clear responses on those points.
The REST app is designed to bind to localhost and only be used locally, providing the same functionality as the cli, but in a form more acceptable to the UI. This was a design decision since Feb/March. We also talk of using unix sockets possibly to make it more secure, which is how keybase seems to do it. If you have other ideas to increase the security of the connection between the UI and the LCD (rest daemon), we should implement them as much as possible. The REST server is the client, and I think everyone agrees it is safer to manage private keys in go than in electron.
4.a. constructing binary bytes for the transactions 4.b. signing the transactions 4.c. parsing the key/value used and returned by ABCIQuery 4.d. exposing simplified subset of rpc |
Maybe gaiap and gaiad can be merged, under a "gaiad proxy" subcommand with no further nesting of subcommands. But definitely not gaiacli or ckeystore. There's nothing wrong with multiple commands, but there is something wrong with nested subcommands. https://en.wikipedia.org/wiki/Unix_philosophy
If the UI can write to disk, then maybe it can encrypt and store. Just don't use a network request. In the bare minimum, the user can be asked to enter their 12 words every time they need to sign something. But really, they shouldn't be entering their 12 words onto an online computer, so this would only be for the case where they're signing a transaction w/ an offline-computer -- e.g. writing a transaction to a USB drive, loading it onto another computer with the transaction and the UI (which isn't connected to the computer), signing the transaction by entering the 12 words, saving the signed tx to disk (but not the 12 words), transferring the 12 words back to the online computer via a USB drive, loading it to the online-UI, and broadcasting it. There's never a need to transmit passwords, encrypted or not, via TCP/IP. |
There's good reason to split these binaries. dockerd != docker. redis-server != redis-cli. We'll want/need to split it sooner or later, so lets save ourselves a lot of pain down the road by splitting them now. Every blockchain project seems to go through the pain of realizing that they shouldn't add key/wallet management w/ the daemon binaries. Lets learn from their mistakes and not conflate functions. Lets not add them to begin with, and discourage others from adding key/wallet management to these daemons. Good point, we shouldn't pass them in via flags for that reason.
If we can use electron to create a unix socket, and we understand all the nuances of managing the security of unix sockets, then OK. Without a comprehensive analysis, we can't be confident about the security of this design. For example, what happens if the unix socket times out for any reason, and meanwhile a malicious external program swaps out the .sock file for another one which proxies the the old one? https://superuser.com/questions/484671/can-i-monitor-a-local-unix-domain-socket-like-tcpdump . OTOH if Electron can access the filesystem, then we can have it bcrypt and store the private key if we wanted it to. It has the private key in its runtime already so I don't see any significant additional risk in signing transactions right there. Using a socket to transfer the password in plaintext, on the other hand, is introducing a new surface of vulnerability. Either we put work into understanding the full extent of this new surface of vulnerability, or we do the more conservative thing. Even if the proxy node is malicious or gets man-in-the-middle'd, at least we have a chance of not losing our private key. The highest-priority thing to protect is the integrity of the keys. If we get double-spent because the proxy node wasn't being honest, it's (hopefully) not the end of the world.
Not if the private key came from the UX. It's strictly worse to send the private key to go, if it came from electron.
Lets use that shim for now and replace it with js-wire in the near future.
Wouldn't it make more sense for the HSM (ala plugnplay devices as Myetherwallet does it w/ the ledger) to interface directly with the Javascript? I don't see the point of going into the console to connect to the HSM. I mean, I do because I'm extra paranoid, but if we assume that the HSM itself is secure, then there's no need to go to golang to interface with the HSM.
No, that sounds good, and seems to be the point of having gaia* in the first place. In the future maybe we'll have the option to do most of that in the client, and people can use stock tendermint w/ a containerized app.
Yes, if we can refactor Tendermint RPC to improve it, I'm in favor of doing it if it's simple. We can continue to upgrade/refactor it after launch as well.
We just need to make broadcast_* require a PUT or POST verb. Want to do it and update the docs? Otherwise I don't think it's unrestful to allow POST/PUTs for GETs. We also do a passing job of showing all the RPC commands when you hit "http://localhost:46657/", so I'd say it's restful enough to get started. We could improve our docs and get a little more intelligent with cache response headers.
We're going to start off by requiring github.com/tendermint/go-wire to encode and decode JSON (the go-wire JSON support is coming, requires a merge from Emmanuel). Would love to discuss why (as opposed to go-wire/data or encoding/json), but that's going to require an hour or so and another issue to discuss. We also have cmn/HexBytes now, pretty much only used for go-crypto/Address at the moment, but otherwise we'll use base-64 going forward for most byte slices as per encoding/json, so that we may support encoding/json support as well in the future (though we may not, as it's got all kinds of issues given the design goals of go-wire, namely interface support). I understand that you're against forking encoding/json, but OTOH it's not uncommon to do so (see protobuf has its own encoder as well), and more importantly, transparent interface-support is incompatible with the standard encoding/json Marshaller/Unmarshaller interface (since there's no way to implement those methods without first registering interfaces & concrete types at init time to some global scope, and that is bad(tm) ).
Lets use go-wire binary and go-wire json for (a), (c). |
|
Question: does the cli touch the keys? but the REST not? or we push all key handling to js? |
|
Basically, is it the design of pushing so much functionality into this REST daemon that you find insecure? And if so, can you give feedback on the The REST will need a complete overhaul obviously. |
How about this...
Ckeystore is in Golang... Maybe we can start this as a console-only tool for now and later also make it an electron app? This app would be designed with the goal that it doesn't touch the internet and is expected to run offline. (though it should still be audited etc because some people will inevitably run it online.)
I'm trying to get us to avoid creating kitchen sink binaries, but the insecurity is from surface-area of attack. For example, sending sending data over a (unix) socket to another process exposes the secret to +1 extra process and +1 communication layer. Trying to create firewalls. Having a single binary with so many functions is maybe less of an issue in terms of security.
"gaiacli get accounts ..." -> "gaiacli accounts ..." We don't need to follow the REST conventions in commands, so we can drop "get". Everything should be in |
|
@jolesbi @nylira @mappum ^^ What do you think? Trying to make the UX more secure by enforcing a separate process where one enters the key. We can have another electron app. If we could check for existence of internet (not by pinging our own servers... I wonder if there's a better way, like pinging DNS servers), and quit with a warning message if internet is found. ckeystore will show the transaction JSON visually so one can inspect what they are about to sign. |
|
Okay, I would like to hear the response of the UI team that needs to expose this to the user. The first design was gaiacli accounts. Otherwise, it is clear feedback for the cli tools. The separation of TX generation, TX signing, and TX posting mirrors the current 0.7 design. There was the request to simplify this from the UI team. Basically, for the cli it makes sense and is not that different than the older design I had, with some improvements. There was lots of feedback from the UI team and they should get involved in this conversation |
|
One more point: Above:
Later:
I agree it is important to show the JSON tx before signing (or display important info on the ledger) To show the JSON, ckeystore needs to have the specific tx structs for this application. To decode go-wire binary to json. Thus, If Then you can feed the original unsigned tx json along with one or more signature files into the cli, which will combine them into a signed tx, encode them in binary, and post to the node. I think this is the cleanest way to handle the separate app, where the communication between the two local components is go-wire JSON, and can be inspected by a user (or electron app). |
How about sdk.Tx GetSignBytes() is just expected to return JSON bytes? If it doesn't then ckeystore can show the hex bytes, or otherwise try to load a codec on it. If the first bytes of binary GetSignBytes() were to include the hash of a schema file, then ckeystore can safely convert to JSON... hmm... |
|
How about, GetSignBytes() is always JSON, and for binary the convention is:
But otherwise for now we can punt on this by just serializing the whole Tx/Msg to JSON using go-wire MarshalJSON. |
this is a cool idea. and i think in the future it would be neat to experiment with this. for launch (and for UX) i think we should be comfortable with hardware support for those who are not ok with storing their seed locally... if i'm understanding correctly. |
|
Multiple binaries: Signing transactions: UI communicating with the SDK: Finally:
|
|
Sounds like there are some compelling thought towards breaking up binaries again in this thread... I don't really care if there is a logical decision between others here to use multiple binaries. I'll leave it to the great thinkers here |
|
Re: signing UX If I'm getting this right, Jae is proposing that we release Cosmos UI as two binaries, an online one to view and create transactions -- CosmosUI, and an offline to hold keys and sign transactions -- CKeystore. This means to send coins, the user will have to:
If that is the primary send flow for users, I think a lot of people are going to be turned off from using Cosmos due to the time investment and complexity. I understand the security advantages. But --
Here are my ideas:
|
|
please include tx UID's in this ticket #318 |
|
I refactored this huge thread:
I tryied to add key quotes from this thread. Please forgive me, if I missed information. I will close this thread in favor of the others. Feel free to reopen it, if you disagree. |
They can use a hardware HSM, which most people should be using, great. Otherwise the offline computer is the HSM. I'm maybe ok with allowing users to enter their password or whatever on an online computer, but we absolutely must provide the offline signing binary. We certainly need it. We won't necessarily be relying on an HSM. |
No, those don't help w/ private key management. It's possible to do more, but not something for us to tackle right now. |
The idea is to remove a lot of unused functions from the CLI.
Additionally we want to base the CLI and the REST server around actual REST. This means basing all the calls around data-resources and using a standard set of properties to query the entities. The actual manipulation of the data and signing of the transactions is transparent to the developer using the client.
Resources:
Defined on these resources are REST actions (GET, POST, PUT, DELETE). This way we unify how developers interact with both ways and minimize the code needed to serve both ways.
GET i.e.:
REST:
GET /Blocks/{block-height}CLI:
gaia client blocks {block-height}POST i.e.:
REST:
curl -x POST /keys --data {name: 'Fabos', password: '1234567890', seed: '...........'}CLI:
gaia client keys add --name='Fabos' --password='1234567890' --seed='...........'POST TX i.e.:
REST:
curl -x POST /txs/send --data {_name: 'Fabos', _password: '1234567890', ...payload}CLI:
gaia client txs send --name='Fabos' --password='1234567890' --seed='...........'PUT/UPDATE i.e.:
REST:
curl -x PUT /keys/Fabos --data {password: '1234567890', new_password: '0987654321'}CLI:
gaia client keys update Fabos --password='1234567890' --new_password='0987654321'Querying i.e.: (search TXs by a certain tag)
REST:
curl -x GET /txs?tag={tag}CLI:
gaia client txs --tag={tag}REST:
curl -x GET /txs/{hash}CLI:
gaia client txs {hash}Limiting returned data i.e.:
REST:
GET /Blocks/{height}?select="metainfo.time"CLI:
gaia client blocks {height} --select="metainfo.time"Summing up:
Top level (
gaia {x})node - Entry to running a full node
client - Entry to data gathering and manipulation
init - Initialize genesis files for a blockchain
unsafe_reset_all - Reset all blockchain data
actions: GET, POST, PUT, DELETE / get, add, update, remove
/version
/status
/keys
/{key_name}
/accounts/{address}
/blocks/{height}|'latest'
/validatorsets/{height}|'latest'
/txs?tag=string
/{hash}
/send
/unbond
/bond
...
// maybe use /txs with Content-Type="SendTransaction" as it is more REST
pulling in @ethanfrey
The text was updated successfully, but these errors were encountered: