Switch from reqwest to bitreq HTTP client
#56
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| use crate::headers::{VssHeaderProvider, VssHeaderProviderError}; | ||
| use async_trait::async_trait; | ||
| use base64::engine::general_purpose::URL_SAFE_NO_PAD; | ||
| use base64::Engine; | ||
|
|
@@ -45,13 +45,14 @@ impl JwtToken { | |
| } | ||
| } | ||
|
|
||
| const DEFAULT_TIMEOUT_SECS: u64 = 10; | ||
|
|
||
| /// Provides a JWT token based on LNURL Auth. | ||
| pub struct LnurlAuthToJwtProvider { | ||
| engine: Secp256k1<SignOnly>, | ||
| parent_key: Xpriv, | ||
| url: String, | ||
| default_headers: HashMap<String, String>, | ||
|
Contributor
There was a problem hiding this comment. Are we sure we want to delete the cached client here ? Thought we could cache it since the first get request in the lnurl auth flow is always to the same URL.
Contributor
Author
There was a problem hiding this comment. AFAIU, we we'll only retrieve a new JWT token if the old one expired, which should be in the order of days or at least ours. It seems to me that keeping a client around (and potentially even keeping a connection open) is a bunch of unnecessary overhead if we just make single requests that rarely. |
||
| cached_jwt_token: RwLock<Option<JwtToken>>, | ||
| } | ||
|
|
||
|
|
@@ -70,47 +71,44 @@ impl LnurlAuthToJwtProvider { | |
| /// with the JWT authorization header for VSS requests. | ||
| pub fn new( | ||
| parent_key: Xpriv, url: String, default_headers: HashMap<String, String>, | ||
| ) -> LnurlAuthToJwtProvider { | ||
| let engine = Secp256k1::signing_only(); | ||
|
|
||
| LnurlAuthToJwtProvider { | ||
| engine, | ||
| parent_key, | ||
| url, | ||
| default_headers, | ||
| cached_jwt_token: RwLock::new(None), | ||
| } | ||
| } | ||
|
|
||
| async fn fetch_jwt_token(&self) -> Result<JwtToken, VssHeaderProviderError> { | ||
| // Fetch the LNURL. | ||
| let lnurl_request = bitreq::get(&self.url) | ||
| .with_headers(self.default_headers.clone()) | ||
| .with_timeout(DEFAULT_TIMEOUT_SECS); | ||
|
Contributor
There was a problem hiding this comment. I know this is a trusted server, but do we want to set max body size here to prevent any possible OOM crashes ? same for the get request below.
Contributor
Author
There was a problem hiding this comment. True, I overlooked that. Do you have a suggestion for a good limit here? |
||
| let lnurl_response = | ||
| lnurl_request.send_async().await.map_err(VssHeaderProviderError::from)?; | ||
| let lnurl_str = String::from_utf8(lnurl_response.into_bytes()).map_err(|e| { | ||
| VssHeaderProviderError::InvalidData { | ||
| error: format!("LNURL response is not valid UTF-8: {}", e), | ||
| } | ||
| })?; | ||
|
|
||
| // Sign the LNURL and perform the request. | ||
| let signed_lnurl = sign_lnurl(&self.engine, &self.parent_key, &lnurl_str)?; | ||
| let auth_request = bitreq::get(&signed_lnurl) | ||
| .with_headers(self.default_headers.clone()) | ||
| .with_timeout(DEFAULT_TIMEOUT_SECS); | ||
| let auth_response = | ||
| auth_request.send_async().await.map_err(VssHeaderProviderError::from)?; | ||
| let lnurl_auth_response: LnurlAuthResponse = | ||
| serde_json::from_slice(&auth_response.into_bytes()).map_err(|e| { | ||
|
Contributor
Author
There was a problem hiding this comment. We could use
Contributor
There was a problem hiding this comment. either is fine to me 🤷♂️
Contributor
There was a problem hiding this comment. @tnull sounds to me like this is something we can upstream to bitreq no ? ie pass the
Contributor
Author
There was a problem hiding this comment.
Yeah, I considered that too. Just haven't fully made my mind up whether not validating UTF8 is okay. Maybe it's here, but not necessarily in the general case? |
||
| VssHeaderProviderError::InvalidData { | ||
| error: format!("Failed to parse LNURL Auth response as JSON: {}", e), | ||
| } | ||
| })?; | ||
|
|
||
| let untrusted_token = match lnurl_auth_response { | ||
| LnurlAuthResponse { token: Some(token), .. } => token, | ||
|
|
@@ -128,13 +126,14 @@ impl LnurlAuthToJwtProvider { | |
| parse_jwt_token(untrusted_token) | ||
| } | ||
|
|
||
| async fn get_jwt_token(&self) -> Result<String, VssHeaderProviderError> { | ||
| let cached_token_str = self | ||
| .cached_jwt_token | ||
| .read() | ||
| .unwrap() | ||
| .as_ref() | ||
| .filter(|t| !t.is_expired()) | ||
| .map(|t| t.token_str.clone()); | ||
| if let Some(token_str) = cached_token_str { | ||
| Ok(token_str) | ||
| } else { | ||
|
|
@@ -150,7 +149,7 @@ impl VssHeaderProvider for LnurlAuthToJwtProvider { | |
| async fn get_headers( | ||
| &self, _request: &[u8], | ||
| ) -> Result<HashMap<String, String>, VssHeaderProviderError> { | ||
| let jwt_token = self.get_jwt_token().await?; | ||
| let mut headers = self.default_headers.clone(); | ||
| headers.insert(AUTHORIZATION.to_string(), format!("Bearer {}", jwt_token)); | ||
| Ok(headers) | ||
|
|
@@ -256,8 +255,8 @@ impl From<bitcoin::bip32::Error> for VssHeaderProviderError { | |
| } | ||
| } | ||
|
|
||
| impl From<bitreq::Error> for VssHeaderProviderError { | ||
| fn from(e: bitreq::Error) -> VssHeaderProviderError { | ||
| VssHeaderProviderError::RequestError { error: e.to_string() } | ||
| } | ||
| } | ||
|
|
||
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.
Now bumped this considerably, as of course 10MB would be much too small. Let me know if you have an opinion on a better value here @TheBlueMatt @tankyleo
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.
Mmm, yea, good question. On the server side we defaulted to 1G because its postgres' limit. Not that "just because its postgres' limit" is a good reason to do anything, though. 500 seems fine enough to me, honestly, though. As long as its documented I don't feel super strongly.