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>, | ||
tnull marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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?
Contributor
There was a problem hiding this comment. How is 16KiB ? just from intuition after some brief googling, we are fetching a URL 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); | ||
|
Contributor
There was a problem hiding this comment. I'd put a 16KiB limit here too, this should be a short response. |
||
| 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?
Contributor
There was a problem hiding this comment. IIUC i would expect
Contributor
Author
There was a problem hiding this comment.
I'm not quite sure? The and then go ahead and use |
||
| 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.