Implement simple password-based gRPC authentication #4189
Conversation
* Renamed CliCommand to RpcCommand, differentiating it from cmd string token(s) * Added new CliConfig, based on :common Config * Injected Config into CoreApi to make bisq.properties and cmd line args available to server * Added cleartext username:password BisqCallCredentials to :cli, and an AuthenticationInterceptor to :core (server) * Moved :daemon resources folder to expected location under src/main * Duplicated CompositeOptionSet, ConfigException and BisqException in :cli because they are not visible from :common. (TODO refactor?) CompositeOptionSet will be used to let command line parameters override params set in config file. * Removed outdated references to :cli in a couple of comments in :common Config * gRPC parameters & command names are lowercase to mimic bitcoind/rpc TBD * Decide what rpc auth param names to use, to differentiate them from bitcoind rpc user/passord param names
|
TODO change rpcuser/rpcpassword params to grpcuser/grpcpassword, as per the request made by @cbeams to me in keybase |
There is no need or benefit to injecting Config into CoreApi for the purpose of accessing it indirectly through BisqGrpcServer.
Final fields should be favored wherever possible to advertise and enforce immutability. Also as a matter of style, favor declaring fields one per line instead of multiple on the same line. This style guideline is not consistent throughout the codebase, but is favored because it makes diffs / patches cleaner and is the more widely accepted style througout most modern Java codebases.
|
Thanks, @ghubstan. Please hold off on further changes for the moment, I have a number of review commits incoming. |
There is actually no use case for both username and password forming the authentication token. This change simplifies the arrangement such that a single token is passed. --rpcUser and --rpcPassword are no longer used and are replaced by --apiToken on both cli and daemon sides.
|
@ghubstan, you'll need to check the 'allow edits from maintainers' checkbox on this PR. Until then, I'm unable to push my review commits. See https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork. Thanks. |
@cbeams, 'allow edits from maintainers' was checked when I logged in a few minutes ago: I wonder if the problem (or bug) has anything to do with this PR being created as a draft. I think the last time you pushed commits to one of my PRs, it had not yet been changed to a draft. |
|
@cbeams , I clicked "ready for review" to temporarily change this from a draft to a normal pull request to see if that will allow you to push changes. |
|
@ghubstan the inability to push was an issue on my side with my local It's worth looking through each of the commits, in some of them I've explained exactly why changes were made. Overall, though, the idea is to radically simplify things. One of the main purposes of the cli is to serve as a reference client for working with Bisq's rpc api, and as such it should be dead simple to understand. It's probably a bit too simple now, I would do some additional refactoring to reduce certain redundant code, but I wouldn't go very far here. The implementation will certainly need to get more sophisticated as we go implementing all the endpoints, but we can do that on an as-needed basis. Hope this helps / makes sense. Let me know if you have questions / feedback. It might be a little bit before I get back to this, Monday possibly. By the way, here's how you can invoke Where The |
|
And you can / should change this PR back to a draft now. Thanks. |
|
@cbeams, Thanks for the feedback, notes on style conventions and new commits. Glad this work can gain some momentum now... The only remark I have is that the reason I broke main into RpcCommand, CliConfig & CommandParser was to avoid the main class from getting very large (a personal itch), but of course I want to follow the conventions. |
When e.g. a given option is not recognized, print a clean error message and exit 1 as opposed to printing a stack trace.
Fail fast if the specified command is unknown
No StatusRuntimeException can be thrown from this code, so it is not necessary to include in the try block.
Co-authored-by: ghubstan <[email protected]>
- Ensure script runs from root directory regardless of where it is invoked from - Touch up documentation and whitespace
Don't instruct the user to create a fresh data directory every time, as this takes quite a bit longer to initialize the wallet than running against the same data directory repeatedly. Just be clear that the requirement is an unencrypted wallet with 0 BTC balance.
|
@ghubstan, you'll see I've added a number of additional commits, including adding your expect-based test suite. Please give everything a look and give a thumbs-up from your side and we'll get this committed. It's an ACK from my side. |
Stop attempting to align Option and Method description columns with one another. It's a moving target as we add options and method names, and this help output format will probably change in the future anyway.
This is a partial reversion of the earlier commit. Marking the password option as required at the parser level made it impossible to run ./bisq-cli without options or arguments and get the help text. This is a useful thing to do, and not worth creating a bad user experience to get the free required option error handling and error messaging.
|
ACK |
There was a problem hiding this comment.
ACK. @bisq-network/bisq-maintainers, can we get this merged asap? I have additional changes that build on these commits, and I'd like to get them available to @ghubstan in a separate PR.
We might want to discuss allowing me to merge these gRPC-related PRs after peer review with @ghubstan. I'm not sure if it's worth the wait to have one of you go through them as well. I've debated doing this work together in my own fork to speed stuff up, but I like the idea of keeping it all here in the main repo, and doing everything incrementally in small PRs. Just not sure if the normal review process will keep up with that. Thoughts?
|
I just removed the bats commit and its reversion and force pushed the branch back to its original state when I gave my final ACK. |
|
I'm good with @cbeams merging this work |
A 2nd attempt at gRPC authentication after finding out in #4129 that TLS + Macaroons is too much overhead for
:cliour purposes at this point (edit: @cbeams). Most of the changes not related to refactoring :cli are listed below.Renamed CliCommand to RpcCommand, differentiating it from cmd
string token(s)
Added new CliConfig, based on :common Config
Injected Config into CoreApi to make bisq.properties and cmd line
args available to server
Added cleartext username:password BisqCallCredentials to :cli, and
an AuthenticationInterceptor to :core (server)
Moved :daemon resources folder to expected location under src/main
Duplicated CompositeOptionSet, ConfigException and BisqException
in :cli because they are not visible from :common. (TODO refactor?)
CompositeOptionSet will be used to let command line parameters
override params set in config file.
Removed outdated references to :cli in a couple of comments in
:common Config
gRPC parameters & command names are lowercase to mimic bitcoind/rpc
TODO: Decide what rpc auth param names to use, to differentiate them from bitcoind rpc user/password param names. Until that decision is made, the existing bitcoind rpcUser/rpcPassword param names are used for gRPC authentication. As you know, you can define rpcUser/rpcPassword creds in bisq.properties and/or pass rpc login creds on command line: