Deuxfleurs/garage
48
56
Fork 73
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 +33 -20
1 changed files with 33 additions and 20 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 63948190e4 - Show all commits

53
src/api/s3_post_object.rs
View file

@ -56,21 +56,29 @@ pub async fn handle_post_object(
} else {
continue;
};
if name != "file" {
if let Ok(content) = HeaderValue::from_str(&field.text().await?) {
match name.as_str() {
"tag" => (/* tag need to be reencoded, but we don't support them yet anyway */),
"acl" => {
params.append("x-amz-acl", content);
if name == "file" {
break field;
}
if let Ok(content) = HeaderValue::from_str(&field.text().await?) {
match name.as_str() {
"tag" => (/* tag need to be reencoded, but we don't support them yet anyway */),
"acl" => {
if params.insert("x-amz-acl", content).is_some() {
return Err(Error::BadRequest(
"Field 'acl' provided more than one time".to_string(),
));
}
_ => {
// TODO actually that's illegal to have the same param multiple times
params.append(name, content);
}
_ => {
if params.insert(&name, content).is_some() {
return Err(Error::BadRequest(format!(
"Field '{}' provided more than one time",
name
)));
}
}
}
} else {
break field;
}
};
@ -100,7 +108,7 @@ pub async fn handle_post_object(
let key = if key.contains("${filename}") {
let filename = field.file_name();
// is this correct? Maybe we should error instead of default?
// TODO is this correct? Maybe we should error instead of default?
key.replace("${filename}", filename.unwrap_or_default())
} else {
key.to_owned()
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

:/

:/
@ -130,7 +138,7 @@ pub async fn handle_post_object(
));
}
let conditions = decoded_policy.into_conditions()?;
let mut conditions = decoded_policy.into_conditions()?;
for (param_key, value) in params.iter() {
let mut param_key = param_key.to_string();
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
@ -154,13 +162,13 @@ pub async fn handle_post_object(
}
}
"key" => {
let conds = conditions.params.get("key").ok_or_else(|| {
let conds = conditions.params.remove("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),
Operation::Equal(s) => s == 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
Operation::StartsWith(s) => key.starts_with(&s),
};
if !ok {
return Err(Error::BadRequest(format!(
@ -171,13 +179,13 @@ pub async fn handle_post_object(
}
}
_ => {
let conds = conditions.params.get(&param_key).ok_or_else(|| {
let conds = conditions.params.remove(&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),
Operation::Equal(s) => s.as_str() == value,
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`
Operation::StartsWith(s) => value.to_str()?.starts_with(s.as_str()),
};
if !ok {
return Err(Error::BadRequest(format!(
@ -190,7 +198,12 @@ pub async fn handle_post_object(
}
}
// TODO check that each policy item is used
if let Some((param_key, _)) = conditions.params.iter().next() {
return Err(Error::BadRequest(format!(
"Key '{}' is required in policy, but no value was provided",
param_key
)));
}
let content_type = field
.content_type()