Skip to content
This repository was archived by the owner on Sep 13, 2023. It is now read-only.

feat: chunks certification #24

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

feat: chunks certification #24

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5d63c89
added body decoding before validation
3cL1p5e7 Feb 27, 2022
cf04a01
cleanup
3cL1p5e7 Feb 27, 2022
d158dff
check of chunk certificates + while streaming
3cL1p5e7 Feb 28, 2022
439f555
review fixes for chunks validation
3cL1p5e7 Mar 1, 2022
cbe66b9
chunk certification: make backward compatibility
3cL1p5e7 Mar 1, 2022
346fbdd
lint + fmt + now decode_body returns body sha
3cL1p5e7 Mar 2, 2022
01f1c6a
added body decoding + refactoring
3cL1p5e7 Mar 4, 2022
e320560
Merge branch 'feature/decode-body' of github.com:3cL1p5e7/icx-proxy i…
3cL1p5e7 Mar 4, 2022
e55fca2
chunk_tree to Vec<u8>
3cL1p5e7 Mar 11, 2022
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
29 changes: 29 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ anyhow = "1.0.34"
base64 = "0.13"
candid = { version = "0.7.11", features = ["mute_warnings"] }
clap = { version = "3", features = ["cargo", "derive"] }
flate2 = "1.0.0"
garcon = { version = "0.2.3", features = ["async"] }
hex = "0.4.3"
hyper = { version = "0.14.13", features = ["full"] }
Expand Down
178 changes: 146 additions & 32 deletions src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ use std::{
Arc, Mutex,
},
};
use std::io::prelude::{Read};
use flate2::read::{GzDecoder, DeflateDecoder};

mod config;
mod logging;
Expand Down Expand Up @@ -282,13 +284,20 @@ async fn forward_request(

let mut certificate: Option<Result<Vec<u8>, ()>> = None;
let mut tree: Option<Result<Vec<u8>, ()>> = None;
let mut chunk_tree: Option<Vec<u8>> = None;
let mut chunk_index: String = String::from("0");
let mut encoding: Option<String> = None;

let mut builder = Response::builder().status(StatusCode::from_u16(http_response.status_code)?);
for HeaderField(name, value) in http_response.headers {
if name.eq_ignore_ascii_case("IC-CERTIFICATE") {
for field in value.split(',') {
if let Some((_, name, b64_value)) = regex_captures!("^(.*)=:(.*):$", field.trim()) {
slog::trace!(logger, ">> certificate {}: {}", name, b64_value);
if name == "chunk_index" {
chunk_index = b64_value.to_string();
continue;
}
let bytes = base64::decode(b64_value).map_err(|e| {
slog::warn!(
logger,
Expand Down Expand Up @@ -335,9 +344,24 @@ async fn forward_request(
bytes
}
});
} else if name == "chunk_tree" {
chunk_tree = match (chunk_tree, bytes) {
(None, bytes) => bytes.ok(),
(Some(chunk_tree), Ok(bytes)) => {
slog::warn!(logger, "duplicate chunk_tree field: {:?}", bytes);
Some(chunk_tree)
},
(Some(chunk_tree), Err(_)) => {
slog::warn!(logger, "duplicate chunk_tree field (failed to decode)");
Some(chunk_tree)
},
};
}
}
}
} else if name.eq_ignore_ascii_case("CONTENT-ENCODING") {
let enc = value.trim().to_string();
encoding = Some(enc);
}

builder = builder.header(&name, value);
Expand All @@ -348,6 +372,40 @@ async fn forward_request(
} else {
None
};

let decoded_body = decode_body(&http_response.body, encoding.clone());
let body_valid = match (certificate.clone(), tree.clone()) {
Copy link
Contributor

@mstein-dfinity mstein-dfinity Feb 28, 2022

Choose a reason for hiding this comment

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

Is some kind of zip-bomb attack possible here, so that a max size needs to be prescribed?

Copy link
Contributor Author

@3cL1p5e7 3cL1p5e7 Mar 1, 2022

Choose a reason for hiding this comment

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

Thank you! Added MAX_BYTES_SIZE_TO_DECOMPRESS param

Copy link
Contributor

@Daniel-Bloom-dfinity Daniel-Bloom-dfinity Mar 1, 2022

Choose a reason for hiding this comment

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

can we go further actually and not pass the body around, but just the sha?

What I'm thinking is we just decode 1K chunk of the body, add that 1K to the sha. Lather rinse and repeat up to EOF or MAX_BYTES_SIZE_TO_DECOMPRESS (or maybe do 0..MAX_CHUNKS_TO_DECOMPRESS). Then return the body_sha and pass that into validate_body

Copy link
Contributor Author

@3cL1p5e7 3cL1p5e7 Mar 2, 2022

Choose a reason for hiding this comment

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

Great idea!
Left the possibility of partial hash calculation

(Some(Ok(certificate)), Some(Ok(tree))) => match validate_body(
&certificate,
&tree,
chunk_tree,
chunk_index,
&canister_id,
&agent,
&uri,
&decoded_body,
logger.clone(),
) {
Ok(valid) => valid,
Err(e) => {
return Ok(Response::builder()
.status(StatusCode::INTERNAL_SERVER_ERROR)
.body(format!("Certificate validation failed: {}", e).into())
.unwrap());
}
},
(Some(_), _) | (_, Some(_)) => false,
// Canisters don't have to provide certified variables
(None, None) => true,
};

if !body_valid && !cfg!(feature = "skip_body_verification") {
return Ok(Response::builder()
.status(StatusCode::INTERNAL_SERVER_ERROR)
.body("Body does not pass verification".into())
.unwrap());
}

let is_streaming = http_response.streaming_strategy.is_some();
let response = if let Some(streaming_strategy) = http_response.streaming_strategy {
let (mut sender, body) = body::Body::channel();
Expand All @@ -359,6 +417,7 @@ async fn forward_request(
let streaming_canister_id_id = callback.callback.principal;
let method_name = callback.callback.method;
let mut callback_token = callback.token;
let chunk_index = callback_token.index.0.to_str_radix(10);
let logger = logger.clone();
tokio::spawn(async move {
let canister = HttpRequestCanister::create(&agent, streaming_canister_id_id);
Expand All @@ -376,7 +435,41 @@ async fn forward_request(
.call()
.await
{
Ok((StreamingCallbackHttpResponse { body, token },)) => {
Ok((StreamingCallbackHttpResponse { body, token, chunk_tree },)) => {
let body_valid = match (certificate.clone(), tree.clone(), chunk_tree) {
(Some(Ok(certificate)), Some(Ok(tree)), Some(chunk_tree)) => {
let decoded_chunk_tree = base64::decode(chunk_tree).map_err(|e| {
slog::warn!(
logger,
"Unable to decode chunk_tree from base64: {}",
e
);
}).ok();


let decoded_body = decode_body(&body, encoding.clone());
validate_body(
&certificate,
Copy link
Contributor

@mstein-dfinity mstein-dfinity Feb 28, 2022

Choose a reason for hiding this comment

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

So many parameters. Maybe there's a ChunckedBody object there?

Copy link
Contributor Author

@3cL1p5e7 3cL1p5e7 Mar 1, 2022

Choose a reason for hiding this comment

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

Added Certificates struct

&tree,
decoded_chunk_tree,
chunk_index.clone(),
&canister_id,
&agent,
&uri,
&decoded_body,
logger.clone(),
)
.ok()
.unwrap_or(false)
},
_ => false,
};

if !body_valid && !cfg!(feature = "skip_body_verification") {
sender.abort();
break;
}

if sender.send_data(Bytes::from(body)).await.is_err() {
sender.abort();
break;
Expand All @@ -400,35 +493,6 @@ async fn forward_request(

builder.body(body)?
} else {
let body_valid = match (certificate, tree) {
(Some(Ok(certificate)), Some(Ok(tree))) => match validate_body(
&certificate,
&tree,
&canister_id,
&agent,
&uri,
&http_response.body,
logger.clone(),
) {
Ok(valid) => valid,
Err(e) => {
return Ok(Response::builder()
.status(StatusCode::INTERNAL_SERVER_ERROR)
.body(format!("Certificate validation failed: {}", e).into())
.unwrap());
}
},
(Some(_), _) | (_, Some(_)) => false,
// Canisters don't have to provide certified variables
(None, None) => true,
};

if !body_valid && !cfg!(feature = "skip_body_verification") {
return Ok(Response::builder()
.status(StatusCode::INTERNAL_SERVER_ERROR)
.body("Body does not pass verification".into())
.unwrap());
}
builder.body(http_response.body.into())?
};

Expand Down Expand Up @@ -467,9 +531,30 @@ async fn forward_request(
Ok(response)
}

fn decode_body(body: &[u8], encoding: Option<String>) -> Vec<u8> {
match encoding {
Some(enc) => match enc.as_str() {
"gzip" => {
let decoded: &mut Vec<u8> = &mut vec![];
GzDecoder::new(body).read_to_end(decoded).unwrap();
decoded.to_vec()
},
"deflate" => {
let decoded: &mut Vec<u8> = &mut vec![];
DeflateDecoder::new(body).read_to_end(decoded).unwrap();
decoded.to_vec()
},
_ => body.to_vec(),
},
_ => body.to_vec(),
}
}

fn validate_body(
certificate: &[u8],
tree: &[u8],
chunk_tree: Option<Vec<u8>>,
chunk_index: String,
canister_id: &Principal,
agent: &Agent,
uri: &Uri,
Expand Down Expand Up @@ -532,9 +617,38 @@ fn validate_body(

let mut sha256 = Sha256::new();
sha256.update(response_body);
let body_sha = sha256.finalize();
let body_sha: [u8; 32] = sha256.finalize().into();

if let Some(tree) = chunk_tree {
let chunk_tree: HashTree = serde_cbor::from_slice(&tree).map_err(AgentError::InvalidCborData)?;

Ok(&body_sha[..] == tree_sha)
let chunk_tree_digest = chunk_tree.digest();

if chunk_tree_digest != tree_sha {
slog::trace!(
Copy link
Contributor

@mstein-dfinity mstein-dfinity Feb 28, 2022

Choose a reason for hiding this comment

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

Maybe this is a warning not just trace?

Copy link
Contributor

@mstein-dfinity mstein-dfinity Feb 28, 2022

Choose a reason for hiding this comment

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

Same in 639 below.

Copy link
Contributor Author

@3cL1p5e7 3cL1p5e7 Mar 1, 2022

Choose a reason for hiding this comment

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

A similar design is already used nearby. If it's really necessary, then I'll change it :)

logger,
">> Invalid chunk_tree in the header. Digest does not equal tree_sha",
);
return Ok(false);
}

let index_path = [chunk_index.into()];
let chunk_sha = match chunk_tree.lookup_path(&index_path) {
LookupResult::Found(v) => v,
_ => {
slog::trace!(
logger,
">> Invalid Tree in the header. Does not contain path {:?}",
path
);
return Ok(false);
}
};

Ok(body_sha == chunk_sha)
} else {
Ok(body_sha == tree_sha)
}
}

fn is_hop_header(name: &str) -> bool {
Expand Down