From 4998108a1ac86bad14cdb9b2332a629f2e23b7e4 Mon Sep 17 00:00:00 2001 From: DanGould Date: Wed, 20 Nov 2024 17:44:07 -0500 Subject: [PATCH] Audit function signatures Co-authored-by: spacebear21 Co-authored-by: nothingmuch --- payjoin/src/hpke.rs | 18 +++++++++++------- payjoin/src/io.rs | 2 +- payjoin/src/ohttp.rs | 8 +++++--- payjoin/src/psbt.rs | 8 +++++--- payjoin/src/receive/mod.rs | 26 ++++++++++---------------- payjoin/src/receive/v2/mod.rs | 26 ++++++++++++-------------- payjoin/src/request.rs | 4 ++++ payjoin/src/send/mod.rs | 24 ++++++++++-------------- payjoin/src/uri/mod.rs | 5 ++++- 9 files changed, 62 insertions(+), 59 deletions(-) diff --git a/payjoin/src/hpke.rs b/payjoin/src/hpke.rs index b88434f7..5dc2caa2 100644 --- a/payjoin/src/hpke.rs +++ b/payjoin/src/hpke.rs @@ -1,7 +1,9 @@ use std::ops::Deref; use std::{error, fmt}; -use bitcoin::key::constants::{ELLSWIFT_ENCODING_SIZE, UNCOMPRESSED_PUBLIC_KEY_SIZE}; +use bitcoin::key::constants::{ + ELLSWIFT_ENCODING_SIZE, PUBLIC_KEY_SIZE, UNCOMPRESSED_PUBLIC_KEY_SIZE, +}; use bitcoin::secp256k1::ellswift::ElligatorSwift; use hpke::aead::ChaCha20Poly1305; use hpke::kdf::HkdfSha256; @@ -97,7 +99,7 @@ impl<'de> serde::Deserialize<'de> for HpkeSecretKey { pub struct HpkePublicKey(pub PublicKey); impl HpkePublicKey { - pub fn to_compressed_bytes(&self) -> [u8; 33] { + pub fn to_compressed_bytes(&self) -> [u8; PUBLIC_KEY_SIZE] { let compressed_key = bitcoin::secp256k1::PublicKey::from_slice(&self.0.to_bytes()) .expect("Invalid public key from known valid bytes"); compressed_key.serialize() @@ -148,7 +150,7 @@ impl<'de> serde::Deserialize<'de> for HpkePublicKey { /// Message A is sent from the sender to the receiver containing an Original PSBT payload #[cfg(feature = "send")] pub fn encrypt_message_a( - body: Vec, + body: Vec, // FIXME: could be &[u8] reply_pk: &HpkePublicKey, receiver_pk: &HpkePublicKey, ) -> Result, HpkeError> { @@ -172,7 +174,7 @@ pub fn encrypt_message_a( #[cfg(feature = "receive")] pub fn decrypt_message_a( message_a: &[u8], - receiver_sk: HpkeSecretKey, + receiver_sk: HpkeSecretKey, // FIXME: could be &HpkeSecretKey ) -> Result<(Vec, HpkePublicKey), HpkeError> { use std::io::{Cursor, Read}; @@ -203,7 +205,7 @@ pub fn decrypt_message_a( /// Message B is sent from the receiver to the sender containing a Payjoin PSBT payload or an error #[cfg(feature = "receive")] pub fn encrypt_message_b( - mut plaintext: Vec, + mut plaintext: Vec, // FIXME: could be &[u8] receiver_keypair: &HpkeKeyPair, sender_pk: &HpkePublicKey, ) -> Result, HpkeError> { @@ -227,8 +229,8 @@ pub fn encrypt_message_b( #[cfg(feature = "send")] pub fn decrypt_message_b( message_b: &[u8], - receiver_pk: HpkePublicKey, - sender_sk: HpkeSecretKey, + receiver_pk: HpkePublicKey, // FIXME: could be &HpkePublicKey + sender_sk: HpkeSecretKey, // FIXME: could be &HpkeSecretKey ) -> Result, HpkeError> { let enc = message_b.get(..ELLSWIFT_ENCODING_SIZE).ok_or(HpkeError::PayloadTooShort)?; let enc = encapped_key_from_ellswift_bytes(enc)?; @@ -242,6 +244,8 @@ pub fn decrypt_message_b( Ok(plaintext) } +// FIXME: could be &mut [u8; padded_length] and return &[u8; padded_length] +// ACTYUALLY function should not exist at all fn pad_plaintext(msg: &mut Vec, padded_length: usize) -> Result<&[u8], HpkeError> { if msg.len() > padded_length { return Err(HpkeError::PayloadTooLarge { actual: msg.len(), max: padded_length }); diff --git a/payjoin/src/io.rs b/payjoin/src/io.rs index 1886e6e1..7b537420 100644 --- a/payjoin/src/io.rs +++ b/payjoin/src/io.rs @@ -9,7 +9,7 @@ use crate::{OhttpKeys, Url}; /// /// * `payjoin_directory`: The payjoin directory from which to fetch the ohttp keys. This /// directory stores and forwards payjoin client payloads. -/// +/// // FIXME split into two functions so that the docstring can be right /// * `cert_der` (optional): The DER-encoded certificate to use for local HTTPS connections. This /// parameter is only available when the "danger-local-https" feature is enabled. #[cfg(feature = "v2")] diff --git a/payjoin/src/ohttp.rs b/payjoin/src/ohttp.rs index 9bd7d147..7e955cd4 100644 --- a/payjoin/src/ohttp.rs +++ b/payjoin/src/ohttp.rs @@ -4,7 +4,9 @@ use std::{error, fmt}; use bitcoin::base64::prelude::BASE64_URL_SAFE_NO_PAD; use bitcoin::base64::Engine; -pub fn ohttp_encapsulate( +pub const PADDED_MESSAGE_BYTES: usize = 8192; + +pub(crate) fn ohttp_encapsulate( ohttp_keys: &mut ohttp::KeyConfig, method: &str, target_resource: &str, @@ -38,7 +40,7 @@ pub fn ohttp_encapsulate( } /// decapsulate ohttp, bhttp response and return http response body and status code -pub fn ohttp_decapsulate( +pub(crate) fn ohttp_decapsulate( res_ctx: ohttp::ClientResponse, ohttp_body: &[u8], ) -> Result>, OhttpEncapsulationError> { @@ -57,7 +59,7 @@ pub fn ohttp_decapsulate( /// Error from de/encapsulating an Oblivious HTTP request or response. #[derive(Debug)] -pub enum OhttpEncapsulationError { +pub(crate) enum OhttpEncapsulationError { Http(http::Error), Ohttp(ohttp::Error), Bhttp(bhttp::Error), diff --git a/payjoin/src/psbt.rs b/payjoin/src/psbt.rs index 46b6cdf1..610b8c34 100644 --- a/payjoin/src/psbt.rs +++ b/payjoin/src/psbt.rs @@ -36,9 +36,9 @@ pub(crate) trait PsbtExt: Sized { fn proprietary_mut(&mut self) -> &mut BTreeMap>; fn unknown_mut(&mut self) -> &mut BTreeMap>; fn input_pairs(&self) -> Box> + '_>; - // guarantees that length of psbt input matches that of unsigned_tx inputs and same + /// guarantees that length of psbt input matches that of unsigned_tx inputs and same /// thing for outputs. - fn validate(self) -> Result; + fn validate(self) -> Result; // FIXME a ValidPsbt wrapper makes more semantic sense fn validate_input_utxos(&self, treat_missing_as_error: bool) -> Result<(), PsbtInputsError>; } @@ -70,6 +70,8 @@ impl PsbtExt for Psbt { } fn validate(self) -> Result { + // TODO try to simplify trait to handle all validation + // TODO use validate_input_utxos here let tx_ins = self.unsigned_tx.input.len(); let psbt_ins = self.inputs.len(); let tx_outs = self.unsigned_tx.output.len(); @@ -135,7 +137,7 @@ impl InternalInputPair<'_> { pub fn validate_utxo( &self, - treat_missing_as_error: bool, + treat_missing_as_error: bool, // FIXME never used! remove! ) -> Result<(), InternalPsbtInputError> { match (&self.psbtin.non_witness_utxo, &self.psbtin.witness_utxo) { (None, None) if treat_missing_as_error => diff --git a/payjoin/src/receive/mod.rs b/payjoin/src/receive/mod.rs index 071c744d..349a88fd 100644 --- a/payjoin/src/receive/mod.rs +++ b/payjoin/src/receive/mod.rs @@ -109,7 +109,7 @@ pub struct UncheckedProposal { impl UncheckedProposal { pub fn from_request( - mut body: impl std::io::Read, + mut body: impl std::io::Read, // FIXME: could be &[u8] bc you need the whole thing query: &str, headers: impl Headers, ) -> Result { @@ -142,9 +142,7 @@ impl UncheckedProposal { let params = Params::from_query_pairs(pairs).map_err(InternalRequestError::SenderParams)?; log::debug!("Received request with params: {:?}", params); - // TODO check that params are valid for the request's Original PSBT - - Ok(UncheckedProposal { psbt, params }) + Ok(Self { psbt, params }) } /// The Sender's Original PSBT transaction @@ -354,7 +352,7 @@ impl WantsOutputs { pub fn substitute_receiver_script( self, output_script: &Script, - ) -> Result { + ) -> Result { let output_value = self.original_psbt.unsigned_tx.output[self.change_vout].value; let outputs = vec![TxOut { value: output_value, script_pubkey: output_script.into() }]; self.replace_receiver_outputs(outputs, output_script) @@ -369,7 +367,7 @@ impl WantsOutputs { self, replacement_outputs: Vec, drain_script: &Script, - ) -> Result { + ) -> Result { let mut payjoin_psbt = self.original_psbt.clone(); let mut outputs = vec![]; let mut replacement_outputs = replacement_outputs.clone(); @@ -427,7 +425,7 @@ impl WantsOutputs { // Update the payjoin PSBT outputs payjoin_psbt.outputs = vec![Default::default(); outputs.len()]; payjoin_psbt.unsigned_tx.output = outputs; - Ok(WantsOutputs { + Ok(Self { original_psbt: self.original_psbt, payjoin_psbt, params: self.params, @@ -494,7 +492,7 @@ impl WantsInputs { /// A simple consolidation is otherwise chosen if available. pub fn try_preserving_privacy( &self, - candidate_inputs: impl IntoIterator, + candidate_inputs: impl IntoIterator, // FIXME &InputPair & clone out ) -> Result { let mut candidate_inputs = candidate_inputs.into_iter().peekable(); if candidate_inputs.peek().is_none() { @@ -521,7 +519,7 @@ impl WantsInputs { /// https://eprint.iacr.org/2022/589.pdf fn avoid_uih( &self, - candidate_inputs: impl IntoIterator, + candidate_inputs: impl IntoIterator, // FIXME &InputPair & clone out ) -> Result { let min_original_out_sats = self .payjoin_psbt @@ -559,7 +557,7 @@ impl WantsInputs { fn select_first_candidate( &self, - candidate_inputs: impl IntoIterator, + candidate_inputs: impl IntoIterator, // FIXME &InputPair & clone out ) -> Result { candidate_inputs.into_iter().next().ok_or(InternalSelectionError::NotFound.into()) } @@ -569,7 +567,7 @@ impl WantsInputs { pub fn contribute_inputs( self, inputs: impl IntoIterator, - ) -> Result { + ) -> Result { let mut payjoin_psbt = self.payjoin_psbt.clone(); // The payjoin proposal must not introduce mixed input sequence numbers let original_sequence = self @@ -609,7 +607,7 @@ impl WantsInputs { return Err(InternalInputContributionError::ValueTooLow.into()); } - Ok(WantsInputs { + Ok(Self { original_psbt: self.original_psbt, payjoin_psbt, params: self.params, @@ -897,10 +895,6 @@ pub struct PayjoinProposal { } impl PayjoinProposal { - pub fn utxos_to_be_locked(&self) -> impl '_ + Iterator { - self.payjoin_psbt.unsigned_tx.input.iter().map(|input| &input.previous_output) - } - pub fn is_output_substitution_disabled(&self) -> bool { self.params.disable_output_substitution } diff --git a/payjoin/src/receive/v2/mod.rs b/payjoin/src/receive/v2/mod.rs index c62bc8c4..5526bce0 100644 --- a/payjoin/src/receive/v2/mod.rs +++ b/payjoin/src/receive/v2/mod.rs @@ -113,7 +113,7 @@ impl Receiver { /// indicating no UncheckedProposal is available yet. pub fn process_res( &mut self, - mut body: impl std::io::Read, + mut body: impl std::io::Read, // FIXME: could be &[u8], not streamed context: ohttp::ClientResponse, ) -> Result, Error> { let mut buf = Vec::new(); @@ -139,10 +139,12 @@ impl Receiver { ohttp_encapsulate(&mut self.context.ohttp_keys, "GET", fallback_target.as_str(), None) } + // FIXME: could be &str fn extract_proposal_from_v1(&mut self, response: String) -> Result { Ok(self.unchecked_from_payload(response)?) } + // FIXME: could be &[u8] because we've got a known length fn extract_proposal_from_v2(&mut self, response: Vec) -> Result { let (payload_bytes, e) = decrypt_message_a(&response, self.context.s.secret_key().clone())?; self.context.e = Some(e); @@ -152,7 +154,7 @@ impl Receiver { fn unchecked_from_payload( &mut self, - payload: String, + payload: String, // FIXME: could be &str ) -> Result { let (base64, padded_query) = payload.split_once('\n').unwrap_or_default(); let query = padded_query.trim_matches('\0'); @@ -347,9 +349,9 @@ impl WantsOutputs { pub fn substitute_receiver_script( self, output_script: &Script, - ) -> Result { + ) -> Result { let inner = self.inner.substitute_receiver_script(output_script)?; - Ok(WantsOutputs { inner, context: self.context }) + Ok(Self { inner, context: self.context }) } /// Replace **all** receiver outputs with one or more provided outputs. @@ -359,11 +361,11 @@ impl WantsOutputs { /// receiver needs to pay for additional miner fees (e.g. in the case of adding many outputs). pub fn replace_receiver_outputs( self, - replacement_outputs: Vec, + replacement_outputs: Vec, // FIXME could be Iterator drain_script: &Script, - ) -> Result { + ) -> Result { let inner = self.inner.replace_receiver_outputs(replacement_outputs, drain_script)?; - Ok(WantsOutputs { inner, context: self.context }) + Ok(Self { inner, context: self.context }) } /// Proceed to the input contribution step. @@ -395,7 +397,7 @@ impl WantsInputs { /// https://eprint.iacr.org/2022/589.pdf pub fn try_preserving_privacy( &self, - candidate_inputs: impl IntoIterator, + candidate_inputs: impl IntoIterator, // FIXME &InputPair & clone out ) -> Result { self.inner.try_preserving_privacy(candidate_inputs) } @@ -404,7 +406,7 @@ impl WantsInputs { /// Any excess input amount is added to the change_vout output indicated previously. pub fn contribute_inputs( self, - inputs: impl IntoIterator, + inputs: impl IntoIterator, // Own and place in payjoin_psbt ) -> Result { let inner = self.inner.contribute_inputs(inputs)?; Ok(WantsInputs { inner, context: self.context }) @@ -450,10 +452,6 @@ pub struct PayjoinProposal { } impl PayjoinProposal { - pub fn utxos_to_be_locked(&self) -> impl '_ + Iterator { - self.inner.utxos_to_be_locked() - } - pub fn is_output_substitution_disabled(&self) -> bool { self.inner.is_output_substitution_disabled() } @@ -509,7 +507,7 @@ impl PayjoinProposal { /// choose to broadcast the original PSBT. pub fn process_res( &self, - res: Vec, + res: Vec, // FIXME could be &[u8] ohttp_context: ohttp::ClientResponse, ) -> Result<(), Error> { let res = ohttp_decapsulate(ohttp_context, &res)?; diff --git a/payjoin/src/request.rs b/payjoin/src/request.rs index a093aa10..1ad32d83 100644 --- a/payjoin/src/request.rs +++ b/payjoin/src/request.rs @@ -27,10 +27,14 @@ pub struct Request { } impl Request { + // FIXME: could be &Url and clone inside to help caller + // FIXME: could be &[u8] and clone inside to help caller pub fn new_v1(url: Url, body: Vec) -> Self { Self { url, content_type: V1_REQ_CONTENT_TYPE, body } } + // FIXME: could be &Url and clone inside to help caller + // FIXME: could be &[u8] and clone inside to help caller #[cfg(feature = "v2")] pub fn new_v2(url: Url, body: Vec) -> Self { Self { url, content_type: V2_REQ_CONTENT_TYPE, body } diff --git a/payjoin/src/send/mod.rs b/payjoin/src/send/mod.rs index f31bb6f0..7e7f3803 100644 --- a/payjoin/src/send/mod.rs +++ b/payjoin/src/send/mod.rs @@ -277,7 +277,7 @@ impl Sender { #[cfg(feature = "v2")] pub fn extract_v2( &self, - ohttp_relay: Url, + ohttp_relay: Url, // FIXME: could be &Url and clone inside to help caller ) -> Result<(Request, V2PostContext), CreateRequestError> { use crate::uri::UrlExt; if let Some(expiry) = self.endpoint.exp() { @@ -342,7 +342,7 @@ pub struct V1Context { impl V1Context { pub fn process_response( self, - response: &mut impl std::io::Read, + response: &mut impl std::io::Read, // FIXME: could be &[u8] bc you need the whole thing ) -> Result { self.psbt_context.process_response(response) } @@ -360,11 +360,9 @@ pub struct V2PostContext { impl V2PostContext { pub fn process_response( self, - response: &mut impl std::io::Read, + response: &[u8], // FIXME: could be &[u8] bc caller doesn't need to track read cursor ) -> Result { - let mut res_buf = Vec::new(); - response.read_to_end(&mut res_buf).map_err(InternalValidationError::Io)?; - let response = ohttp_decapsulate(self.ohttp_ctx, &res_buf) + let response = ohttp_decapsulate(self.ohttp_ctx, response) .map_err(InternalValidationError::OhttpEncapsulation)?; match response.status() { http::StatusCode::OK => { @@ -392,7 +390,7 @@ pub struct V2GetContext { impl V2GetContext { pub fn extract_req( &self, - ohttp_relay: Url, + ohttp_relay: Url, // FIXME: could be &Url and clone inside to help caller ) -> Result<(Request, ohttp::ClientResponse), CreateRequestError> { use crate::uri::UrlExt; let mut url = self.endpoint.clone(); @@ -417,12 +415,10 @@ impl V2GetContext { pub fn process_response( &self, - response: &mut impl std::io::Read, + response: &[u8], // FIXME: could be &[u8] bc caller doesn't need to track read cursor ohttp_ctx: ohttp::ClientResponse, ) -> Result, ResponseError> { - let mut res_buf = Vec::new(); - response.read_to_end(&mut res_buf).map_err(InternalValidationError::Io)?; - let response = ohttp_decapsulate(ohttp_ctx, &res_buf) + let response = ohttp_decapsulate(ohttp_ctx, response) .map_err(InternalValidationError::OhttpEncapsulation)?; let body = match response.status() { http::StatusCode::OK => response.body().to_vec(), @@ -465,7 +461,7 @@ struct HpkeContext { #[cfg(feature = "v2")] impl HpkeContext { - pub fn new(receiver: HpkePublicKey) -> Self { + fn new(receiver: HpkePublicKey) -> Self { Self { receiver, reply_pair: HpkeKeyPair::gen_keypair() } } } @@ -496,7 +492,7 @@ impl PsbtContext { #[inline] pub fn process_response( self, - response: &mut impl std::io::Read, + response: &mut impl std::io::Read, // FIXME: could be &[u8] or &str ref IF POSSIBLE ) -> Result { let mut res_str = String::new(); response.read_to_string(&mut res_str).map_err(InternalValidationError::Io)?; @@ -859,7 +855,7 @@ fn serialize_v2_body( } fn serialize_url( - endpoint: Url, + endpoint: Url, // FIXME: could be &Url and clone inside to help caller disable_output_substitution: bool, fee_contribution: Option<(bitcoin::Amount, usize)>, min_fee_rate: FeeRate, diff --git a/payjoin/src/uri/mod.rs b/payjoin/src/uri/mod.rs index 6281446c..ac067807 100644 --- a/payjoin/src/uri/mod.rs +++ b/payjoin/src/uri/mod.rs @@ -61,6 +61,8 @@ pub trait UriExt<'a>: sealed::UriExt { } impl<'a> UriExt<'a> for Uri<'a, NetworkChecked> { + // FIXME custom enum since error is actually a default fallback for pj unsupported + // enumerate reasons why this might fail fn check_pj_supported(self) -> Result, Box>> { match self.extras { MaybePayjoinExtras::Supported(payjoin) => { @@ -116,7 +118,7 @@ impl PjUriBuilder { pub fn new( address: Address, origin: Url, - #[cfg(feature = "v2")] receiver_pubkey: Option, + #[cfg(feature = "v2")] receiver_pubkey: Option, // FIXME make Option<(pk, keys, exp)> #[cfg(feature = "v2")] ohttp_keys: Option, #[cfg(feature = "v2")] expiry: Option, ) -> Self { @@ -149,6 +151,7 @@ impl PjUriBuilder { } /// Set whether or not payjoin output substitution is allowed. + #[cfg(not(feature = "v2"))] // TODO ensure v2 options are set imply pjos=true pub fn pjos(mut self, pjos: bool) -> Self { self.pjos = pjos; self