Skip to content
This repository has been 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

Conversation

3cL1p5e7
Copy link
Contributor

@3cL1p5e7 3cL1p5e7 commented Feb 28, 2022

That is a part of proposal about chunks certification (with backward compatibility).

These improvements are a proposal to improve the certification infrastructure around IC and might be considered as a recommendation for dfinity-team.

Goal

Make it possible to certify asset chunks. Validate chunk certificates on the service-worker and icx-proxy.

Why

At the moment, the service-worker and icx-proxy does not support the certification of chunkified files. Moreover, right now it is not possible to correctly stream chunkified and large audio and video files to the front-end.
This problems could be solved independently if it would be possible to install an additional service-worker in the certified zone of the domain ic0.app (for 206 partial http-request handling). But is is impossible because there is unable to place custom worker on ic0.app domain.
Making your own custom player for audio and video is extremely difficult due to the large number of formats and non-native implementation.

Details

To make this possible, support for HTTP-range requests for http_request query method has been added. This is done to support native html audio/video element (which uses 206 partial http-request) and to determine the index of the chunk throught 206 partial http-request.
Using 206 partial http-requests allows you to focus only on certification in the worker and icx-proxy.

Steps

  1. It all starts with PR for certified-assets-canister in cdk-rs
  2. Did file was updated in PR for certified-assets-canister
  3. Service-worker started supporting chunk_tree certificate verification in PR for ic
  4. (Here) icx-proxy started supporting chunk_tree certificate verification here
  5. Added support for new certified-assets-canister did in PR for agent-rs

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated
@@ -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

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

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

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

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

src/main.rs Outdated Show resolved Hide resolved
let chunk_tree_digest = chunk_tree.digest();

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

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

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

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 :)

src/main.rs Outdated

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

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

Choose a reason for hiding this comment

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

Added Certificates struct

@3cL1p5e7
Copy link
Contributor Author

3cL1p5e7 commented Mar 2, 2022

Tests are not passing due to PR to agent-rs

@Daniel-Bloom-dfinity
Copy link
Contributor

I really think we should separate the unzip change from this. Ideally also the refactors like decode_hash_tree. If you could move those changes into their own PRs separate from this (1 for each would be best), they would be much easier to approve. The chunks change is a protocol change and thus has many more moving parts (like agent-rs changes) and requires more thorough consideration and review.

Really appreciate all this work though!

@3cL1p5e7 3cL1p5e7 mentioned this pull request Mar 4, 2022
@3cL1p5e7
Copy link
Contributor Author

3cL1p5e7 commented Mar 4, 2022

@Daniel-Bloom-dfinity body decoding is separated to another PR

@paulyoung
Copy link
Contributor

Are there still plans to add this feature?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants