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

Implement client e2ee using HPKE #355

Merged
merged 7 commits into from
Sep 26, 2024
Merged

Conversation

DanGould
Copy link
Contributor

@DanGould DanGould commented Sep 7, 2024

This PR effectively integrates bitcoin-hpke, replacing the custom aead algorithm for point #216 point 4

The biggest review points of this PR are the design of the abstraction layers between payjoin and bitcoin-hpke (and to a lesser extent payjoin to bitcoin-ohttp to bitcoin-hpke).

Some things to consider:

  • payjoin implements serde::{Serialize,Deserialize}forbitcoin-hpkekeys where they could be enabled with a feature, though that would further depart fromrust-hpke`'s main branch. I think it makes sense to upstream it. reviewer, what do you think?
  • hpke pubkey compression for both client e2ee and OhttpKeys is done manually from bytes. While this is custom to the payjoin protocol, might it be useful to expose the ctx.public_key() or ohttp_keys.public_key() for easier extraction? I feel like what I've developed here to solve Consider serializing compressed keys in ?pj= parameters #344 is a bit of a hack but also a significant quality of life improvement for anyone handling bip21 URIs. Similar to upstreaming to bitcoin-hpke, what do you think of upstream change that departs from the main ohttp branch @Spacebear?

@DanGould DanGould force-pushed the bitcoin-hpke branch 2 times, most recently from 9ea26f4 to db1f485 Compare September 10, 2024 16:44
@@ -138,9 +138,6 @@ fn init_ohttp() -> Result<ohttp::Server> {

// create or read from file
let server_config = ohttp::KeyConfig::new(KEY_ID, KEM, Vec::from(SYMMETRIC))?;
let encoded_config = server_config.encode()?;
let b64_config = BASE64_URL_SAFE_NO_PAD.encode(encoded_config);
info!("ohttp-keys server config base64 UrlSafe: {:?}", b64_config);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fetch ohttp-keys by using GET /ohttp-keys

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this documented somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is in the bip 77 spec but not elsewhere in our coodebase.

We could call info! and print "fetch ohttp-keys by using GET /ohttp-keys" as an instruction for the operator.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool, I forgot it was in the spec. I like that suggestion too.

@DanGould DanGould marked this pull request as ready for review September 11, 2024 04:57
@DanGould DanGould mentioned this pull request Sep 12, 2024
@DanGould DanGould added this to the payjoin-0.21.0 milestone Sep 16, 2024
Copy link
Collaborator

@spacebear21 spacebear21 left a comment

Choose a reason for hiding this comment

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

payjoin implements serde::{Serialize,Deserialize}forbitcoin-hpkekeys where they could be enabled with a feature, though that would further depart fromrust-hpke`'s main branch. I think it makes sense to upstream it. reviewer, what do you think?

I don't feel strongly either way, but this issue and this writeup highlight the reasoning for moving away from serde in rust-hpke. It might make sense just to keep it that way.

hpke pubkey compression for both client e2ee and OhttpKeys is done manually from bytes. While this is custom to the payjoin protocol, might it be useful to expose the ctx.public_key() or ohttp_keys.public_key() for easier extraction? I feel like what I've developed here to solve
#344 is a bit of a hack but also a significant quality of life improvement for anyone handling bip21 URIs. Similar to upstreaming to bitcoin-hpke, what do you think of upstream change that departs from the main ohttp branch @Spacebear?

Yeah this seems like it probably belongs upstream. Maybe better to be explicit by exposing the methods as .compressed_public_key()?

payjoin-cli/src/app/config.rs Outdated Show resolved Hide resolved
@@ -138,9 +138,6 @@ fn init_ohttp() -> Result<ohttp::Server> {

// create or read from file
let server_config = ohttp::KeyConfig::new(KEY_ID, KEM, Vec::from(SYMMETRIC))?;
let encoded_config = server_config.encode()?;
let b64_config = BASE64_URL_SAFE_NO_PAD.encode(encoded_config);
info!("ohttp-keys server config base64 UrlSafe: {:?}", b64_config);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this documented somewhere?

payjoin/src/receive/v2/mod.rs Outdated Show resolved Hide resolved
payjoin/src/v2.rs Outdated Show resolved Hide resolved
@DanGould
Copy link
Contributor Author

I've addressed your main concerns, but I'd still like to add that compressed_public_key() accessor to bitcoin-hpke, need to update the lock files, and want to make sure I'm satisfied with the src/v2.rs mod function names and params

This way an ohttp relay sees uniform ciphertexts
Almost all of the replaced field serializers have serde impls so macros just work.
This reduces the size of the subdirectory pj= string.
Payjoin-cli reads ohttp-keys from a binary file as a consequence.
Before, it was passed as aad, but it has already been transmitted from
the URI and can be passed from the typestate machine.
@DanGould
Copy link
Contributor Author

compressed key was tabled for now as non-critical and documented in an bitcoin-hpke repo issue. Otherwise this is ready for review

Copy link
Collaborator

@spacebear21 spacebear21 left a comment

Choose a reason for hiding this comment

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

ACK 81202fa

I haven't smoke tested due to existing payjoin directories not supporting this hpke configuration, but the e2e test seems happy.

Left minor suggestions in spots to revert to s.secret/public_key instead of tuple indices.

payjoin/src/receive/v2/mod.rs Outdated Show resolved Hide resolved
payjoin/src/receive/v2/mod.rs Outdated Show resolved Hide resolved
payjoin/src/receive/v2/mod.rs Outdated Show resolved Hide resolved
payjoin/src/receive/v2/mod.rs Outdated Show resolved Hide resolved
@DanGould
Copy link
Contributor Author

Nice catches. Why even have the abstraction if you're not going to use the helpers. DOH!

I haven't smoke tested due to existing payjoin directories not supporting this hpke configuration

since the integration and e2e tests spin up payjoin directories with this configuration, that won't be a problem, would it? We must discuss deploying the new directory. I don't know that anyone is relying on it in production, so "just do it" might be the strategy.

@spacebear21
Copy link
Collaborator

Agreed on shipping the new directory.

@DanGould
Copy link
Contributor Author

image

@DanGould DanGould merged commit 1d7feb1 into payjoin:master Sep 26, 2024
4 checks passed
@DanGould DanGould deleted the bitcoin-hpke branch September 26, 2024 18:38
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.

2 participants