Deuxfleurs/garage
44
56
Fork 74
Code Issues 100 Pull requests 14 Releases 30 Wiki Activity

Support for PostObject #222

Merged
lx merged 14 commits from trinity-1686a/garage:post-object into main 2022-02-21 22:02:31 +00:00
Conversation 1 Commits 14 Files changed 12 +187 -184
1 changed files with 187 additions and 184 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit e0aee72a9c - Show all commits

371
src/api/s3_post_object.rs
View file

@ -43,7 +43,14 @@ pub async fn handle_post_object(
let mut multipart = Multipart::with_constraints(body, boundary, constraints);
let mut params = HeaderMap::new();
while let Some(field) = multipart.next_field().await? {
let field = loop {
trinity-1686a marked this conversation as resolved Outdated
lx commented 2022-02-08 22:05:42 +00:00
Outdated
Review
Copy link

I think we can avoid putting almost the entire code of this function in the while loop (and remove 1 indentation level almost everywhere) by doing something like this:

let file = loop {
    let field = match multipart.next_field().await? {
        None => return Err(no file field found),
        Some(x) => x,
    };
    let name = ...;
    if name == "file" {
         break field;
    }
    // here handle header field adding it to the headermap
};
// here do all of the rest of the processing once we have all headers and are now reading the file body

This looks much nicer to me, especially as in the current version we have a for inside the while, which looks a bit like a nested loop but is in fact not at all.

I think we can avoid putting almost the entire code of this function in the `while` loop (and remove 1 indentation level almost everywhere) by doing something like this: ```rust let file = loop { let field = match multipart.next_field().await? { None => return Err(no file field found), Some(x) => x, }; let name = ...; if name == "file" { break field; } // here handle header field adding it to the headermap }; // here do all of the rest of the processing once we have all headers and are now reading the file body ``` This looks much nicer to me, especially as in the current version we have a `for` inside the `while`, which looks a bit like a nested loop but is in fact not at all.
let field = if let Some(field) = multipart.next_field().await? {
field
} else {
return Err(Error::BadRequest(
"Request did not contain a file".to_owned(),
));
};
let name: HeaderName = if let Some(Ok(name)) = field.name().map(TryInto::try_into) {
name
} else {
@ -62,198 +69,194 @@ pub async fn handle_post_object(
}
}
}
continue;
}
// Current part is file. Do some checks before handling to PutObject code
let key = params
.get("key")
.ok_or_bad_request("No key was provided")?
.to_str()?;
let credential = params
.get("x-amz-credential")
.ok_or_else(|| {
Error::Forbidden("Garage does not support anonymous access yet".to_string())
})?
.to_str()?;
let policy = params
.get("policy")
.ok_or_bad_request("No policy was provided")?
.to_str()?;
let signature = params
.get("x-amz-signature")
.ok_or_bad_request("No signature was provided")?
.to_str()?;
let date = params
.get("x-amz-date")
.ok_or_bad_request("No date was provided")?
.to_str()?;
let key = if key.contains("${filename}") {
let filename = field.file_name();
// is this correct? Maybe we should error instead of default?
key.replace("${filename}", filename.unwrap_or_default())
} else {
key.to_owned()
};
let date = parse_date(date)?;
let api_key = verify_v4(&garage, credential, &date, signature, policy.as_bytes()).await?;
let bucket_id = resolve_bucket(&garage, &bucket, &api_key).await?;
if !api_key.allow_write(&bucket_id) {
return Err(Error::Forbidden(
"Operation is not allowed for this key.".to_string(),
));
break field;
}
};
let decoded_policy = base64::decode(&policy)?;
let decoded_policy: Policy =
serde_json::from_slice(&decoded_policy).ok_or_bad_request("Invalid policy")?;
// Current part is file. Do some checks before handling to PutObject code
let key = params
.get("key")
.ok_or_bad_request("No key was provided")?
.to_str()?;
let credential = params
.get("x-amz-credential")
.ok_or_else(|| {
Error::Forbidden("Garage does not support anonymous access yet".to_string())
})?
.to_str()?;
let policy = params
.get("policy")
.ok_or_bad_request("No policy was provided")?
.to_str()?;
let signature = params
.get("x-amz-signature")
.ok_or_bad_request("No signature was provided")?
.to_str()?;
let date = params
.get("x-amz-date")
.ok_or_bad_request("No date was provided")?
.to_str()?;
let expiration: DateTime<Utc> = DateTime::parse_from_rfc3339(&decoded_policy.expiration)
.ok_or_bad_request("Invalid expiration date")?
.into();
if Utc::now() - expiration > Duration::zero() {
return Err(Error::BadRequest(
"Expiration date is in the paste".to_string(),
));
}
let key = if key.contains("${filename}") {
let filename = field.file_name();
// is this correct? Maybe we should error instead of default?
key.replace("${filename}", filename.unwrap_or_default())
} else {
key.to_owned()
};
let conditions = decoded_policy.into_conditions()?;
let date = parse_date(date)?;
let api_key = verify_v4(&garage, credential, &date, signature, policy.as_bytes()).await?;
for (param_key, value) in params.iter() {
let mut param_key = param_key.to_string();
param_key.make_ascii_lowercase();
match param_key.as_str() {
"policy" | "x-amz-signature" => (), // this is always accepted, as it's required to validate other fields
"content-type" => {
for cond in &conditions.content_type {
let ok = match cond {
Operation::Equal(s) => value == s,
Operation::StartsWith(s) => {
value.to_str()?.split(',').all(|v| v.starts_with(s))
}
};
if !ok {
return Err(Error::BadRequest(format!(
"Key '{}' has value not allowed in policy",
param_key
)));
}
}
}
"key" => {
let conds = conditions.params.get("key").ok_or_else(|| {
Error::BadRequest(format!("Key '{}' is not allowed in policy", param_key))
})?;
for cond in conds {
let ok = match cond {
Operation::Equal(s) => s == &key,
Operation::StartsWith(s) => key.starts_with(s),
};
if !ok {
return Err(Error::BadRequest(format!(
"Key '{}' has value not allowed in policy",
param_key
)));
}
}
}
_ => {
let conds = conditions.params.get(&param_key).ok_or_else(|| {
Error::BadRequest(format!("Key '{}' is not allowed in policy", param_key))
})?;
for cond in conds {
let ok = match cond {
Operation::Equal(s) => s == value,
Operation::StartsWith(s) => value.to_str()?.starts_with(s),
};
if !ok {
return Err(Error::BadRequest(format!(
"Key '{}' has value not allowed in policy",
param_key
)));
}
}
}
}
}
let bucket_id = resolve_bucket(&garage, &bucket, &api_key).await?;
// TODO check that each policy item is used
let content_type = field
.content_type()
.map(AsRef::as_ref)
.map(HeaderValue::from_str)
.transpose()
.ok_or_bad_request("Invalid content type")?
.unwrap_or_else(|| HeaderValue::from_static("blob"));
params.append(header::CONTENT_TYPE, content_type);
let headers = get_headers(&params)?;
let stream = field.map(|r| r.map_err(Into::into));
let (_, md5) = save_stream(
garage,
headers,
StreamLimiter::new(stream, conditions.content_length),
bucket_id,
&key,
None,
None,
)
.await?;
let etag = format!("\"{}\"", md5);
// TODO get uri
// get Host
// append www-form-urlencoded key
let location = "todo";
let resp = if let Some(mut target) = params
.get("success_action_redirect")
.and_then(|h| h.to_str().ok())
.and_then(|u| url::Url::parse(u).ok())
.filter(|u| u.scheme() == "https" || u.scheme() == "http")
{
target
.query_pairs_mut()
.append_pair("bucket", &bucket)
.append_pair("key", &key)
.append_pair("etag", &etag);
let target = target.to_string();
Response::builder()
.status(StatusCode::SEE_OTHER)
.header(header::LOCATION, target.clone())
.header(header::ETAG, etag)
.body(target.into())?
} else {
let action = params
.get("success_action_status")
.and_then(|h| h.to_str().ok())
.unwrap_or("204");
let builder = Response::builder()
.status(StatusCode::OK)
.header(header::LOCATION, location)
.header(header::ETAG, etag);
match action {
"200" => builder.status(StatusCode::OK).body(Body::empty())?,
"201" => {
// TODO body should be an XML document, not sure which yet
builder.status(StatusCode::CREATED).body(Body::from(""))?
}
_ => builder.status(StatusCode::NO_CONTENT).body(Body::empty())?,
}
};
return Ok(resp);
if !api_key.allow_write(&bucket_id) {
trinity-1686a marked this conversation as resolved Outdated
lx commented 2022-02-14 16:24:43 +00:00
Outdated
Review
Copy link

This looks like it should rather be an error case: if the application builder put ${filename} in the key field but the browser for some reason didn't include the name of the uploaded file, we probably want to reject the request.

This looks like it should rather be an error case: if the application builder put `${filename}` in the key field but the browser for some reason didn't include the name of the uploaded file, we probably want to reject the request.
trinity-1686a commented 2022-02-15 17:14:31 +00:00
Outdated
Review
Copy link

that was actually how a previous iteration worked. This behavior was added because it's exactly how AWS behave (not that I can say I agree with that behavior)

that was actually how a previous iteration worked. This behavior was added because it's exactly how AWS behave (not that I can say I agree with that behavior)
lx commented 2022-02-15 17:33:52 +00:00
Outdated
Review
Copy link

:/

:/
return Err(Error::Forbidden(
"Operation is not allowed for this key.".to_string(),
));
}
Err(Error::BadRequest(
"Request did not contain a file".to_owned(),
))
let decoded_policy = base64::decode(&policy)?;
let decoded_policy: Policy =
serde_json::from_slice(&decoded_policy).ok_or_bad_request("Invalid policy")?;
let expiration: DateTime<Utc> = DateTime::parse_from_rfc3339(&decoded_policy.expiration)
.ok_or_bad_request("Invalid expiration date")?
.into();
if Utc::now() - expiration > Duration::zero() {
return Err(Error::BadRequest(
"Expiration date is in the paste".to_string(),
));
}
let conditions = decoded_policy.into_conditions()?;
for (param_key, value) in params.iter() {
let mut param_key = param_key.to_string();
param_key.make_ascii_lowercase();
match param_key.as_str() {
"policy" | "x-amz-signature" => (), // this is always accepted, as it's required to validate other fields
"content-type" => {
for cond in &conditions.content_type {
let ok = match cond {
Operation::Equal(s) => value == s,
Operation::StartsWith(s) => {
lx marked this conversation as resolved Outdated
lx commented 2022-02-14 16:31:39 +00:00
Outdated
Review
Copy link

I feel like we should add unit tests for the policy decoding logic (not just for into_conditions but end-to-end starting with JSON)

I feel like we should add unit tests for the policy decoding logic (not just for into_conditions but end-to-end starting with JSON)
lx commented 2022-02-14 16:54:03 +00:00
Outdated
Review
Copy link

I'll write a test and post it in the comments of the PR so that you can copy and paste

I'll write a test and post it in the comments of the PR so that you can copy and paste
value.to_str()?.split(',').all(|v| v.starts_with(s))
}
};
if !ok {
return Err(Error::BadRequest(format!(
"Key '{}' has value not allowed in policy",
param_key
)));
}
}
}
"key" => {
let conds = conditions.params.get("key").ok_or_else(|| {
Error::BadRequest(format!("Key '{}' is not allowed in policy", param_key))
})?;
for cond in conds {
let ok = match cond {
Operation::Equal(s) => s == &key,
Operation::StartsWith(s) => key.starts_with(s),
};
if !ok {
return Err(Error::BadRequest(format!(
"Key '{}' has value not allowed in policy",
param_key
)));
}
lx marked this conversation as resolved Outdated
lx commented 2022-02-14 16:45:12 +00:00
Outdated
Review
Copy link

Can you point me to the documentation section which says that there must be a policy specified for the key field?

Can you point me to the documentation section which says that there must be a policy specified for the `key` field?
trinity-1686a commented 2022-02-15 17:17:57 +00:00
Outdated
Review
Copy link

this is not specified, it's however the behavior of AWS. policy and x-amz-signature are the only two fields I found to not be required in the policy. Even x-amz-credential, which is definitelly required to make a valid v4 signature, must be allowed in policy
this is in fact specified somewhere

Each form field that you specify in a form (except x-amz-signature, file, policy, and field names that have an x-ignore- prefix) must appear in the list of conditions.

Which means I have to add some code to ignore x-ignore-*, others are already ignored

~~this is not specified, it's however the behavior of AWS. `policy` and `x-amz-signature` are the only two fields I found to not be required in the policy. Even `x-amz-credential`, which is definitelly required to make a valid v4 signature, must be allowed in policy~~ this is in fact [specified somewhere](https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-HTTPPOSTConstructPolicy.html#sigv4-PolicyConditions) > Each form field that you specify in a form (except x-amz-signature, file, policy, and field names that have an x-ignore- prefix) must appear in the list of conditions. Which means I have to add some code to ignore `x-ignore-*`, others are already ignored
👍 1
}
}
_ => {
let conds = conditions.params.get(&param_key).ok_or_else(|| {
Error::BadRequest(format!("Key '{}' is not allowed in policy", param_key))
})?;
for cond in conds {
let ok = match cond {
Operation::Equal(s) => s == value,
Operation::StartsWith(s) => value.to_str()?.starts_with(s),
};
if !ok {
return Err(Error::BadRequest(format!(
"Key '{}' has value not allowed in policy",
param_key
)));
}
trinity-1686a marked this conversation as resolved Outdated
lx commented 2022-02-14 16:45:36 +00:00
Outdated
Review
Copy link

Same here, can you point me to the doc which says a policy must be given for all fields?

Same here, can you point me to the doc which says a policy must be given for all fields?
trinity-1686a commented 2022-02-15 17:18:40 +00:00
Outdated
Review
Copy link

see comment on key

see comment on `key`
}
}
}
}
// TODO check that each policy item is used
let content_type = field
.content_type()
.map(AsRef::as_ref)
.map(HeaderValue::from_str)
.transpose()
.ok_or_bad_request("Invalid content type")?
.unwrap_or_else(|| HeaderValue::from_static("blob"));
params.append(header::CONTENT_TYPE, content_type);
let headers = get_headers(&params)?;
let stream = field.map(|r| r.map_err(Into::into));
let (_, md5) = save_stream(
garage,
headers,
StreamLimiter::new(stream, conditions.content_length),
bucket_id,
&key,
None,
None,
)
.await?;
let etag = format!("\"{}\"", md5);
// TODO get uri
lx marked this conversation as resolved Outdated
lx commented 2022-02-14 16:38:46 +00:00
Outdated
Review
Copy link

This definitely looks like it should have been done before the policy check

This definitely looks like it should have been done before the policy check
👍 1
trinity-1686a commented 2022-02-16 22:25:27 +00:00
Outdated
Review
Copy link

turns out AWS ignore this, and only consider content type set in what I called param, not in the field metadata

turns out AWS ignore this, and only consider content type set in what I called param, not in the field metadata
// get Host
trinity-1686a commented 2022-02-07 18:51:55 +00:00
Outdated
Review
Copy link

the actual response should be

<PostResponse>
    <Location>https://bucketname.garage.tld/key</Location>
    <Bucket>bucketname</Bucket>
    <Key>key</Key>
    <ETag>"0123456789abcdef0123456789abcdef"</ETag>
</PostResponse>

with corresponding etag and location http headers (these headers are also here for 200 and 204, but not the body)

When using success_action_redirect, etag is set as usual, and location is set to ${success_action_redirect}?bucket=bucketname&key=key&etag=%220123456789abcdef0123456789abcdef%22

the actual response should be ```xml <PostResponse> <Location>https://bucketname.garage.tld/key</Location> <Bucket>bucketname</Bucket> <Key>key</Key> <ETag>"0123456789abcdef0123456789abcdef"</ETag> </PostResponse> ``` with corresponding `etag` and `location` http headers (these headers are also here for 200 and 204, but not the body) When using `success_action_redirect`, `etag` is set as usual, and location is set to `${success_action_redirect}?bucket=bucketname&key=key&etag=%220123456789abcdef0123456789abcdef%22`
// append www-form-urlencoded key
let location = "todo";
let resp = if let Some(mut target) = params
.get("success_action_redirect")
.and_then(|h| h.to_str().ok())
.and_then(|u| url::Url::parse(u).ok())
.filter(|u| u.scheme() == "https" || u.scheme() == "http")
{
target
.query_pairs_mut()
.append_pair("bucket", &bucket)
.append_pair("key", &key)
.append_pair("etag", &etag);
let target = target.to_string();
Response::builder()
.status(StatusCode::SEE_OTHER)
.header(header::LOCATION, target.clone())
trinity-1686a marked this conversation as resolved Outdated
lx commented 2022-02-08 22:09:52 +00:00
Outdated
Review
Copy link

Looks like we don't need this .status() as we are calling it in all branches below

Looks like we don't need this `.status()` as we are calling it in all branches below
.header(header::ETAG, etag)
.body(target.into())?
} else {
let action = params
.get("success_action_status")
.and_then(|h| h.to_str().ok())
.unwrap_or("204");
let builder = Response::builder()
.header(header::LOCATION, location)
.header(header::ETAG, etag);
match action {
"200" => builder.status(StatusCode::OK).body(Body::empty())?,
"201" => {
// TODO body should be an XML document, not sure which yet
builder.status(StatusCode::CREATED).body(Body::from(""))?
}
_ => builder.status(StatusCode::NO_CONTENT).body(Body::empty())?,
}
};
Ok(resp)
}
#[derive(Deserialize)]