Implement client e2ee using HPKE #355
Conversation
DanGould
force-pushed
the
bitcoin-hpke
branch
2 times, most recently
from
9ea26f4 to
db1f485
Compare
| @@ -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); | |||
There was a problem hiding this comment.
fetch ohttp-keys by using GET /ohttp-keys
There was a problem hiding this comment.
Is this documented somewhere?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Cool, I forgot it was in the spec. I like that suggestion too.
DanGould
force-pushed
the
bitcoin-hpke
branch
from
db1f485 to
8c87fe4
Compare
There was a problem hiding this comment.
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()?
| @@ -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); | |||
There was a problem hiding this comment.
Is this documented somewhere?
DanGould
force-pushed
the
bitcoin-hpke
branch
from
0726817 to
ff95b4c
Compare
|
I've addressed your main concerns, but I'd still like to add that |
DanGould
force-pushed
the
bitcoin-hpke
branch
from
ff95b4c to
45710f6
Compare
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
force-pushed
the
bitcoin-hpke
branch
from
45710f6 to
81202fa
Compare
|
compressed key was tabled for now as non-critical and documented in an bitcoin-hpke repo issue. Otherwise this is ready for review |
There was a problem hiding this comment.
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.
DanGould
force-pushed
the
bitcoin-hpke
branch
from
81202fa to
3b9769b
Compare
|
Nice catches. Why even have the abstraction if you're not going to use the helpers. DOH!
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. |
|
Agreed on shipping the new directory. |
|
|
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
payjoinandbitcoin-hpke(and to a lesser extentpayjointobitcoin-ohttptobitcoin-hpke).Some things to consider:
payjoin implementsserde::{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?ctx.public_key()orohttp_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 mainohttpbranch @Spacebear?