Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions object_store/src/aws/credential.rs
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,7 @@ async fn web_identity(
])
.retryable(retry_config)
.idempotent(true)
.sensitive(true)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if similar treatment is needed for ?

.append_pair("X-Amz-Security-Token", token);

.append_pair("X-Amz-Signature", &signature);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No neither are credentials and are designed to be exposed to end-users - https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-query-string-auth.html

.send()
.await?
.bytes()
Expand Down
60 changes: 60 additions & 0 deletions object_store/src/client/retry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ pub struct RetryableRequest {
retry_timeout: Duration,
backoff: Backoff,

sensitive: bool,
idempotent: Option<bool>,
payload: Option<PutPayload>,
}
Expand All @@ -190,6 +191,14 @@ impl RetryableRequest {
}
}

/// Set whether this request contains sensitive data
///
/// This will avoid printing out the URL in error messages
#[allow(unused)]
pub fn sensitive(self, sensitive: bool) -> Self {
Self { sensitive, ..self }
}

/// Provide a [`PutPayload`]
pub fn payload(self, payload: Option<PutPayload>) -> Self {
Self { payload, ..self }
Expand All @@ -206,6 +215,11 @@ impl RetryableRequest {
.idempotent
.unwrap_or_else(|| self.request.method().is_safe());

let sanitize_err = move |e: reqwest::Error| match self.sensitive {
true => e.without_url(),
false => e,
};

loop {
let mut request = self
.request
Expand Down Expand Up @@ -238,6 +252,7 @@ impl RetryableRequest {
};
}
Err(e) => {
let e = sanitize_err(e);
let status = r.status();
if retries == max_retries
|| now.elapsed() > retry_timeout
Expand Down Expand Up @@ -280,6 +295,8 @@ impl RetryableRequest {
}
},
Err(e) => {
let e = sanitize_err(e);

let mut do_retry = false;
if e.is_connect()
|| e.is_body()
Expand Down Expand Up @@ -365,6 +382,7 @@ impl RetryExt for reqwest::RequestBuilder {
backoff: Backoff::new(&config.backoff),
idempotent: None,
payload: None,
sensitive: false,
}
}

Expand Down Expand Up @@ -565,6 +583,48 @@ mod tests {
"{e}"
);

let url = format!("{}/SENSITIVE", mock.url());
for _ in 0..=retry.max_retries {
mock.push(
Response::builder()
.status(StatusCode::BAD_GATEWAY)
.body("ignored".to_string())
.unwrap(),
);
}
let res = client.request(Method::GET, url).send_retry(&retry).await;
let err = res.unwrap_err().to_string();
assert!(err.contains("SENSITIVE"), "{err}");

let url = format!("{}/SENSITIVE", mock.url());
for _ in 0..=retry.max_retries {
mock.push(
Response::builder()
.status(StatusCode::BAD_GATEWAY)
.body("ignored".to_string())
.unwrap(),
);
}

// Sensitive requests should strip URL from error
let req = client
.request(Method::GET, &url)
.retryable(&retry)
.sensitive(true);
let err = req.send().await.unwrap_err().to_string();
assert!(!err.contains("SENSITIVE"), "{err}");

for _ in 0..=retry.max_retries {
mock.push_fn(|_| panic!());
}

let req = client
.request(Method::GET, &url)
.retryable(&retry)
.sensitive(true);
let err = req.send().await.unwrap_err().to_string();
assert!(!err.contains("SENSITIVE"), "{err}");

// Shutdown
mock.shutdown().await
}
Expand Down