rpcclient: Add cookie auth #1460
Conversation
rpcclient/infrastructure.go
Outdated
|
|
||
| // retrieveCookie is a function that returns the cookie username and | ||
| // passphrase. | ||
| retrieveCookie func() (username, passphrase string, err error) |
There was a problem hiding this comment.
Is there any reasoning why this is a member closure? It seems like this is just keeping track of lastCheckTime and lastModTime.
Could this somehow be replaced with
func (config *ConnConfig) retrieveCookie() (username, passphrase string, err error) {
// access config.lastModTime and config.lastCheckTime ...
}that way the path does not need to be passed in and config.CookiePath can be accessed instead?
| // this connection. This will be the result of checking the cookie if a cookie | ||
| // path is configured; if not, it will be the user-configured username and | ||
| // passphrase. | ||
| func (config *ConnConfig) getAuth() (username, passphrase string, err error) { |
There was a problem hiding this comment.
Bitcoin Core falls back to cookie authentication if a password is not found, we might want to replicate that functionality rather than fall back to username/password authentication if a cookie is not found.
Additionally, there is no example of cookie creation - this is a relevant feature, and it is currently undefined.
There was a problem hiding this comment.
Additionally, there is no example of cookie creation - this is a relevant feature, and it is currently undefined.
Hmm, what functionality are you looking for exactly? Making the btcd daemon support creating a cookie file?
There was a problem hiding this comment.
Yup - something like in https://github.com/bitcoin/bitcoin/blob/master/src/rpc/request.cpp
Just so users don't have to generate using core first, and this can be a little more feature complete.
There was a problem hiding this comment.
Out of scope for this PR though
rpcclient/cookiefile.go
Outdated
| return | ||
| } | ||
|
|
||
| s := strings.TrimSpace(string(b)) |
There was a problem hiding this comment.
Considering Core uses base64 encoding for their client cookie authentication, those libraries could be used instead of a binary => string cast
There was a problem hiding this comment.
You're talking about https://github.com/bitcoin/bitcoin/blob/f4a0d27e85754d60804ffa36e415b67c263180b9/src/bitcoin-cli.cpp#L345 , right? AFAICT from looking at the Bitcoin Core source code, while it's true that the Core cookie reader doesn't try to parse out a separate username and password (and instead simply submits the line extracted from the cookie file verbatim), the code that actually creates the cookie file does distinguish between the username and password components.
I think it's generally a good thing to try to parse out the username and password separately, for 2 reasons:
- It simplifies the integration with existing code that expects authentication to use a username+password rather than a combined blob.
- It will better detect cases where the cookie file doesn't look like a standard cookie file, which are probably an indication of some kind of error.
There was a problem hiding this comment.
That makes sense, I'm cool with the user/pass separation. I still have an issue with this line though - why are the spaces being trimmed? I just added a space in a cookie file with Core and it worked.
Parsing only the first line of the file makes sense, and it is what core does in https://github.com/bitcoin/bitcoin/blob/42d0eca725a83c999e2b67e33dfc7bcc96288dc3/src/rpc/request.cpp#L108, but spaces in general seem a little arbitrary
There was a problem hiding this comment.
Replaced TrimSpace with reading the first line.
|
Thanks for the feedback @Rjected , I'll address the feedback ASAP. Things are a bit busy here at the moment, so I might take a little while. |
|
Updated PR to address feedback. |
2b1eed8 Switch to new rpcclient based on latest upstream Conformal (JeremyRand) Pull request description: Namecoin's fork of Conformal's RPC client library is ancient and unmaintained; switching to upstream eliminates a major source of potential bugs (and known bugs, e.g. the ConsensusJ and Electrum compatibility bugs that we had to patch). Fixes #9 TODO: - [ ] Wait for Conformal to merge btcsuite/btcd#1457 - [ ] Wait for Conformal to merge btcsuite/btcd#1460 - [x] Push `name_show` support for `ncjson` and `ncrpcclient` - [x] Implement cookie authentication - [x] Test cookie authentication ACKs for commit 2b1eed: Tree-SHA512: 157780613661af240d83a78d66386c66fcfeed0700088d263a20389c45b200db1b651ea985f345de2ac2f250bdafcc4cd5901579e9f5a97ddc13e77e7a7bcf39
|
Tested this locally worked like a charm. LGTM |
|
@jcvernaleo (as per #1530)
Well tested, mergable with minimal review |
Based on Hugo Landau's cookie auth implementation for Namecoin's ncdns. Fixes btcsuite#1054
|
Rebased and fixed merge conflicts. |
Fixes #1054