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: opt-in tls for rpc #1767

Merged
merged 31 commits into from
Sep 26, 2023
Merged

feat: opt-in tls for rpc #1767

merged 31 commits into from
Sep 26, 2023

Conversation

greyscaled
Copy link
Contributor

@greyscaled greyscaled commented Sep 15, 2023

Summary

Adds --diable-tls (true by default) to glaredb local and glaredb rpcsrv.

Details

This is opt-in while we're still working on the feature and ensuring end-to-end experience
works as expected. At some point in the near future, it will be on by default and opt-out
will be flag for local use. Note that opting out will only work when running both the rpcsrv
and local with opt-out.

Resolves: #1784

- This is just focusing client-side atm
	- Refactor = done
	- Get CA = wip
		- update URL
		- Test(s)
		- Error handling
	- Make RemoteClient using CA cert

- Then I'll update the server-side
@adhish20

This comment was marked as resolved.

@greyscaled greyscaled changed the title wip(rpcproxy tls): obtain ca wip: apply certs to gRPC proxy conn Sep 19, 2023
@greyscaled greyscaled self-assigned this Sep 19, 2023
crates/glaredb/src/args/mod.rs Outdated Show resolved Hide resolved
crates/glaredb/src/local.rs Outdated Show resolved Hide resolved
crates/sqlexec/src/remote/client.rs Outdated Show resolved Hide resolved
crates/glaredb/src/local.rs Outdated Show resolved Hide resolved
@greyscaled greyscaled changed the title wip: apply certs to gRPC proxy conn feat: opt-in apply certs to gRPC proxy conn Sep 22, 2023
@greyscaled greyscaled changed the title feat: opt-in apply certs to gRPC proxy conn feat: opt-in tls for rpc Sep 22, 2023
@greyscaled greyscaled marked this pull request as ready for review September 22, 2023 22:32
py-glaredb/src/lib.rs Outdated Show resolved Hide resolved
We know we're changing to disable_tls in the future, so we'll pass down
!enable_tls
crates/sqlexec/src/remote/client.rs Outdated Show resolved Hide resolved
crates/glaredb/src/local.rs Outdated Show resolved Hide resolved
crates/glaredb/src/rpc_proxy.rs Show resolved Hide resolved
crates/glaredb/src/rpc_proxy.rs Show resolved Hide resolved
crates/sqlexec/src/remote/client.rs Show resolved Hide resolved
py-glaredb/src/lib.rs Outdated Show resolved Hide resolved
crates/glaredb/src/args/local.rs Outdated Show resolved Hide resolved
crates/sqlexec/src/remote/client.rs Show resolved Hide resolved
Comment on lines +196 to +198

let client = reqwest::Client::new();
let res = client
Copy link
Contributor

Choose a reason for hiding this comment

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

client creation is expensive (ish) and part of my "can we do this closer to the startup or when we construct the remote client". I'm not sure how re-connections are handled, but

another option would be to pass the credential config in as a future, if you didn't want to resolve the certs until you try to connect. This way (I think) subsequent attempts to resolve the future would get an effectively cached value, and you would end up doing the request only once.

crates/glaredb/src/args/local.rs Outdated Show resolved Hide resolved
crates/glaredb/src/local.rs Outdated Show resolved Hide resolved
crates/glaredb/src/rpc_proxy.rs Show resolved Hide resolved
let ca = std::fs::read_to_string(tls_conf.ca_cert_path)?;

let mut api_url = "https://console.glaredb.com/api/internal/authenticate/client";
let host = dst.uri().host().expect("invalid host");
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
let host = dst.uri().host().expect("invalid host");
let host = dst.uri().host()?;

To not panic here

Copy link
Contributor Author

@greyscaled greyscaled Sep 26, 2023

Choose a reason for hiding this comment

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

FYI: I merged without this change, because it required updating the error/signature (ie: this change as-is would fail to compile). If I was more familiar with Rust and the cb I would've been more able to make that change, but I just wanted to get the ball rolling for e2e.

crates/sqlexec/src/remote/client.rs Outdated Show resolved Hide resolved
crates/sqlexec/src/remote/client.rs Show resolved Hide resolved
Copy link
Contributor

@tychoish tychoish left a comment

Choose a reason for hiding this comment

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

let's add the TODOs to the epic?

crates/glaredb/src/args/local.rs Outdated Show resolved Hide resolved
@greyscaled greyscaled enabled auto-merge (squash) September 26, 2023 17:14
@greyscaled greyscaled merged commit d95ae28 into main Sep 26, 2023
@greyscaled greyscaled deleted the grey/tls branch September 26, 2023 17:16
@greyscaled greyscaled mentioned this pull request Sep 26, 2023
@greyscaled
Copy link
Contributor Author

let's add the TODOs to the epic?

Issues created and added.

Issues now on the epci:

Additional issues:

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

Successfully merging this pull request may close these issues.

Extract and apply certificate(s) to gRPC proxy connection
5 participants