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

feat(rpc): Cookie auth system for the RPC endpoint #8900

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Sep 30, 2024

Motivation

We want to authenticate the RPC method by the zcashd cookie method.

Solution

  • Generate cookie when the RPC endpoint starts.
  • Start the RPC endpoint server with that generated cookie.
  • In the middleware, check for basic HTTP auth header.
    • Redirect to request if the user provided auth matches the one generated that the server has.
    • Else redirect to as dummy unauthenticated method.

Tests

  • Manual
  • All tests should remain working as the cookie is created for localhost when zebrad starts.

PR Author's Checklist

  • The PR name will make sense to users.
  • The PR provides a CHANGELOG summary.
  • The solution is tested.
  • The documentation is up to date.
  • The PR has a priority label.

PR Reviewer's Checklist

  • The PR Author's checklist is complete.
  • The PR resolves the issue.

@oxarbitrage oxarbitrage added C-security Category: Security issues A-rpc Area: Remote Procedure Call interfaces A-compatibility Area: Compatibility with other nodes or wallets, or standard rules labels Sep 30, 2024
@oxarbitrage oxarbitrage requested a review from a team as a code owner September 30, 2024 22:16
@oxarbitrage oxarbitrage requested review from upbqdn and removed request for a team September 30, 2024 22:16
@github-actions github-actions bot added the C-feature Category: New features label Sep 30, 2024
@mpguerra mpguerra linked an issue Oct 1, 2024 that may be closed by this pull request
@oxarbitrage
Copy link
Contributor Author

Requests from a remote host that don't have the cookie generated at startup will be rejected. In zcashd, the zcash-cli can be used in a remote host that has the cookie and authenticate. However, we are not targeting a specific application, we want to use the authentication method in a generic way from a remote client. One option can be to use curl with the cookie file and compare server side. Other options are welcome, i am still brainstorming it.

@oxarbitrage
Copy link
Contributor Author

We had a chat today about this with @upbqdn and @arya2, i am adding some more research here.

About the cookie auth method:

... Read access to this file controls who can access through RPC ...

https://bitcoin.org/en/release/v0.12.0#rpc-random-cookie-rpc-authentication

That means the cookie method is actually for local access. I think we should focus on that in this PR.

For remote access, we thought in username/password over TLS/SSL as an option. Bitcoin supported this for its RPC endpoint in the past however they don't do it anymore claiming that the RPC access should only be shared with trusted environments.

https://en.bitcoin.it/wiki/Enabling_SSL_on_original_client_daemon#:~:text=SSL%20for%20RPC%20in%20Bitcoin,was%20criticized%20for%20some%20time.

https://bitcoin.stackexchange.com/questions/108293/why-does-bitcoin-cores-rpc-interface-not-use-encryption

It seems that the remote access should be a combination of username and password, with the additional rpcallowip config option plus a vpn layer.

@oxarbitrage
Copy link
Contributor Author

Continuing with the cookie auth method, the zcash-cli sends the cookie content as basic HTTP credentials to the server: https://github.com/zcash/zcash/blob/master/src/bitcoin-cli.cpp#L251
https://github.com/zcash/zcash/blob/master/src/bitcoin-cli.cpp#L266

We want to do that but just with curl, we need to intercept the Authorization in the zebra RPC middleware and compare it with the cookie in the server side.

I got confused thinking the cookie method will work for remote access, my apologies for that.

@upbqdn
Copy link
Member

upbqdn commented Oct 2, 2024

That means the cookie method is actually for local access. I think we should focus on that in this PR.

Do we have any use cases that require authentication for local access, though?

@oxarbitrage
Copy link
Contributor Author

Do we have any use cases that require authentication for local access, though?

It's a security measure. You can't access the resources if you don't have read access to the cookie, even if you are in the same machine.

@oxarbitrage oxarbitrage requested a review from a team as a code owner October 7, 2024 12:02
@@ -94,6 +99,9 @@ impl Default for Config {

// Debug options are always off by default.
debug_force_finished_sync: false,

//
Copy link
Member

@upbqdn upbqdn Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//
// Use the default cache dir for the auth cookie.

Comment on lines +318 to +319
#[rpc(name = "unauthenticated")]
/// A dummy RPC method that just returns a non-authenticated RPC error.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional
Suggested change
#[rpc(name = "unauthenticated")]
/// A dummy RPC method that just returns a non-authenticated RPC error.
/// A dummy RPC method that just returns a non-authenticated RPC error.
#[rpc(name = "unauthenticated")]

/// Default name for auth cookie file */
pub const COOKIEAUTH_FILE: &str = ".cookie";

/// Generate a new auth cookie and return the encoded password.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Generate a new auth cookie and return the encoded password.
/// Generate a new auth cookie and store it in the given `cookie_dir`.

pub const COOKIEAUTH_FILE: &str = ".cookie";

/// Generate a new auth cookie and return the encoded password.
pub fn generate(cookie_dir: PathBuf) -> Option<()> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional

Returning Result would be more expressive.

}

/// Delete the auth cookie.
pub fn delete() -> Option<()> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional

Similarly here.

.map(|password| password.to_string())
})
.map_or(false, |password| {
if let Some(cookie_password) = cookie::get(self.0.cookie_dir.clone()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We read the cookie from the disk on each check here and only rely on the OS caching the disk reads. We could keep the cookie in the memory instead. For example, by using lazy_static! in cookie::get.

@@ -141,4 +149,51 @@ impl FixHttpRequestMiddleware {
);
}
}

/// Change the method name in the JSON request.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional

Could we move the contents of this fn to unauthenticated and remove the fn since we call it only from there, and it only uses a single-use, hard-coded method name?

Comment on lines +508 to +509
let auth_cookie = cookie::get(RpcConfig::default().cookie_dir).expect("cookie should exist");
let client = RpcRequestClient::new(zebra_rpc_address, auth_cookie);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We write the cookie to the disk, and then read it even in tests that are not related to the authentication. We could generate the cookie for tests without writing it to the disk at all, and instantiate it only through lazy_static!.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compatibility Area: Compatibility with other nodes or wallets, or standard rules A-rpc Area: Remote Procedure Call interfaces C-feature Category: New features C-security Category: Security issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Protect the Zebra RPC endpoint
2 participants