-
Notifications
You must be signed in to change notification settings - Fork 36
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
fetch ohttp keys through CONNECT tunnel #194
Conversation
fa83e06
to
1ba02c0
Compare
4a82716
to
d22bee4
Compare
d22bee4
to
7957a8f
Compare
26029ec
to
b245008
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the feature seems to be able to work in production as-is, fetching keys via relay is not yet included in integration tests. The Proxy agent does not support the danger_local_https
feature either.
payjoin-cli/src/app/v2.rs
Outdated
@@ -130,27 +139,20 @@ impl AppTrait for App { | |||
} | |||
|
|||
impl App { | |||
fn construct_payjoin_uri( | |||
async fn construct_payjoin_uri( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this need to be async?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops nice catch. It doesn't.
I actually think the Enrolled
struct has all of the information necessary to construct a Uri and should become available from one of its methods.
payjoin-cli/src/app/v2.rs
Outdated
@@ -300,6 +305,43 @@ impl App { | |||
} | |||
} | |||
|
|||
async fn get_ohttp_keys(config: &AppConfig) -> Result<payjoin::OhttpKeys> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that get_ohttp_keys
, fetch_ohttp_keys
and normalize_proxy_url
should be part of the payjoin
crate.
the fetch..
could require the http client the user is using as input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that there should be library code for fetching ohttp keys, however, the strong typing is already part of payjoin::OhttpKeys::decode(bytes)
. That leaves the i/o. Since payjoin
is a no-IO crate, this makes the functionality, including normalize_proxy_url as candidates for a payjoin-io crate. normalize_proxy_url
is a hack for a strange ureq bug I'm hoping can be fixed. I'm happy to begin work on the new crate but would prefer to leave that to a new PR. It would be simple to create a dependency on a local payjoin-io crate as a follow up.
get_ohttp_keys
is an app method that takes config as input, so I'm not sure that can be included in a common library.
let relay_port = find_free_port(); | ||
let relay_url = Url::parse(&format!("http://0.0.0.0:{}", relay_port)).unwrap(); | ||
let pj_endpoint = Url::parse("https://payjo.in:443").unwrap(); | ||
tokio::select! { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw this tokio::select!
few times already and always get confused about it. can please explain whats the motivation of using it in this kind of scenarios?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Select is a way to call concurrent async methods without starting new tasks. It's less boilerplate than tasks when multiple async calls, like multiple servers, need to run in e.g. tests. It also cancels all of the functions as soon as one of them returns.
payjoin-cli/src/app/v2.rs
Outdated
@@ -162,7 +165,7 @@ impl App { | |||
let http = http_agent()?; | |||
let response = spawn_blocking(move || { | |||
http.post(req.url.as_ref()) | |||
.set("Content-Type", "text/plain") | |||
.set("content-type", "message/ohttp-req") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the content-type
is quite important in payjoin
world, maybe we should consider add RequestContentType
enum to the main crate with the available options. I believe these are text/plain
and message/ohttp-req
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see two ergonomic TODOs in this comment
- define header key/value constants so these can't be misinterpreted during implementation
- include headers in
req
from which to set them rather than set them manually
Beyond these TODOs, these are exactly the confusing implementation details that a payjoin-io
crate could abstract.
edit: since these are ergonomic in nature and don't affect the logic of the fetch I'd rather leave them to another PR
c9f69a9
to
4a3dd68
Compare
@jbesraa wonderful review. You raised some great points which I've addressed. It's looking like the time for the IO crate to be developed is upon us. |
@@ -28,7 +29,7 @@ pub struct Request { | |||
#[derive(Debug, Clone)] | |||
pub struct V2Context { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should document the properties here, probably for a different PR
@@ -37,20 +38,14 @@ pub struct V2Context { | |||
#[derive(Debug, Clone)] | |||
pub struct Enroller { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should document the properties here, probably for a different PR
@@ -108,39 +103,24 @@ fn subdirectory(pubkey: &bitcoin::secp256k1::PublicKey) -> String { | |||
base64::encode_config(pubkey, b64_config) | |||
} | |||
|
|||
#[derive(Debug, Clone)] | |||
#[derive(Debug, Clone, PartialEq, Eq)] | |||
pub struct Enrolled { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should document the properties here, probably for a different PR
@jbesraa I'm leaving a note as to why that hasn't been merged yet. Thanks for all the reviews. I'd like to merge, but not without that danger-local-https feature support and basic tests. In order to do that testing I've been working on #207 to refactor tests to run |
4a3dd68
to
9dd5e0d
Compare
9dd5e0d
to
52e2691
Compare
This PR now tests the CONNECT bootstrap mechanism by introducing an |
8ff3497
to
ba69abc
Compare
72a0fc8
to
8c6b360
Compare
Testing the networking between local environment and CI has been frustrating. This branch seems to pass v2 tests: https://github.com/DanGould/rust-payjoin/tree/fetch-ohttp-keys-ci but making requests to |
8c6b360
to
a055d62
Compare
The most recent test suggests that ohttp-relay is resolving localhost to ipv6 |
fbb4e7e
to
1ab0f8e
Compare
This required a wrapper struct to make OhttpKeys de/serializable
1ab0f8e
to
a422b9f
Compare
@jbesraa I took your feedback into account and e2e tested the results. Our test framework is way better after this as a side effect. |
Previously we were supplying the OHTTP Gateway's Key Configuration
OhttpKeys
manually with configuration.Now that the Rust ohttp-relay supports fetching it via https-in-https CONNECT tunnel, we can fetch it from the server while revealing our IP address only to an ohttp relay.
While ureq and reqwest advertise https-in-https CONNECT functionality, in practice neither properly tunnels TLS in TLS, though I'm trying to change this. For this reason this version uses https-in-http CONNECT which still gets an authenticated response from the gateway (payjoin directory)
Now we only e2e test v1 payjoin cli, and this is complex behavior we probably want to test for which means standing up an ohttp-relay, payjoin directory, and provisioning https certs for them in testing.
refactored and built on #198
Regarding Configuration
This PR makes it possible to provide an optional default ohttp-keys config to override fetching one.
In the future, ohttp-keys may be cached per-directory and only refreshed when they fail.