Skip to content

Commit

Permalink
Merge branch 'gdemay/CRP-1965-limit-number-of-targets-in-delegations'…
Browse files Browse the repository at this point in the history
… into 'master'

fix(crypto): CRP-1965 limit the number of targets per delegation in HTTP requests

Follow-up on [MR-11129](https://gitlab.com/dfinity-lab/public/ic/-/merge_requests/11129) to limit the number of [targets](https://internetcomputer.org/docs/current/references/ic-interface-spec#authentication) (collection of canister IDs) to 1000 that can be specified in each delegation inside an HTTP request.
HTTP request containing a single delegation with more than 1000 targets will be declared invalid. 

See merge request dfinity-lab/public/ic!11723
  • Loading branch information
gregorydemay committed Apr 11, 2023
2 parents 3d0b069 + 3f0d860 commit ef3dd0f
Show file tree
Hide file tree
Showing 9 changed files with 280 additions and 56 deletions.
2 changes: 1 addition & 1 deletion Cargo.lock

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

6 changes: 3 additions & 3 deletions rs/http_endpoints/public/src/read_state.rs
Expand Up @@ -562,7 +562,7 @@ mod test {
&sre,
&user_test_id(1),
&[Path::from(Label::from("time"))],
&CanisterIdSet::All,
&CanisterIdSet::all(),
canister_test_id(1),
)
.await,
Expand All @@ -584,7 +584,7 @@ mod test {
Label::from("reply")
])
],
&CanisterIdSet::All,
&CanisterIdSet::all(),
canister_test_id(1),
)
.await,
Expand All @@ -597,7 +597,7 @@ mod test {
Path::new(vec![Label::from("request_status"), [0; 32].into()]),
Path::new(vec![Label::from("request_status"), [1; 32].into()])
],
&CanisterIdSet::All,
&CanisterIdSet::all(),
canister_test_id(1),
)
.await
Expand Down
4 changes: 4 additions & 0 deletions rs/types/types/src/messages/http.rs
Expand Up @@ -517,6 +517,10 @@ impl Delegation {
}
}
}

pub fn number_of_targets(&self) -> Option<usize> {
self.targets.as_ref().map(Vec::len)
}
}

impl SignedBytesWithoutDomainSeparator for Delegation {
Expand Down
14 changes: 12 additions & 2 deletions rs/validator/BUILD.bazel
@@ -1,4 +1,4 @@
load("@rules_rust//rust:defs.bzl", "rust_library", "rust_test")
load("@rules_rust//rust:defs.bzl", "rust_doc", "rust_doc_test", "rust_library", "rust_test")

package(default_visibility = ["//visibility:public"])

Expand All @@ -11,6 +11,7 @@ DEPENDENCIES = [
"//rs/types/types",
"@crate_index//:chrono",
"@crate_index//:hex",
"@crate_index//:thiserror",
]

DEV_DEPENDENCIES = [
Expand All @@ -19,7 +20,6 @@ DEV_DEPENDENCIES = [
"//rs/test_utilities",
"@crate_index//:assert_matches",
"@crate_index//:base64",
"@crate_index//:maplit",
"@crate_index//:rand_0_8_4",
]

Expand All @@ -46,3 +46,13 @@ rust_test(
srcs = ["tests/ingress_validation.rs"],
deps = DEPENDENCIES + DEV_DEPENDENCIES + [":validator"],
)

rust_doc(
name = "validator_doc",
crate = ":validator",
)

rust_doc_test(
name = "validator_doc_test",
crate = ":validator",
)
2 changes: 1 addition & 1 deletion rs/validator/Cargo.toml
Expand Up @@ -13,6 +13,7 @@ ic-interfaces = { path = "../interfaces" }
ic-types = { path = "../types/types" }
ic-crypto = { path = "../crypto" }
ic-crypto-sha = { path = "../crypto/sha" }
thiserror = "1.0"

[dev-dependencies]
assert_matches = "1.3.0"
Expand All @@ -21,7 +22,6 @@ hex = "0.4.2"
ic-crypto-sha = { path = "../crypto/sha"}
ic-crypto-test-utils-reproducible-rng = { path = "../crypto/test_utils/reproducible_rng" }
ic-test-utilities = { path = "../test_utilities" }
maplit = "1.0.2"
rand = "0.8"

[features]
Expand Down
47 changes: 47 additions & 0 deletions rs/validator/ingress_message/src/internal/tests.rs
Expand Up @@ -379,13 +379,15 @@ mod validate_request {
mod authenticated_requests_delegations {
use super::*;
use crate::AuthenticationError::DelegationContainsCyclesError;
use crate::AuthenticationError::DelegationTargetError;
use crate::AuthenticationError::InvalidBasicSignature;
use crate::AuthenticationError::InvalidCanisterSignature;
use crate::HttpRequestVerifier;
use crate::RequestValidationError::InvalidDelegation;
use crate::RequestValidationError::InvalidDelegationExpiry;
use crate::RequestValidationError::{CanisterNotInDelegationTargets, InvalidSignature};
use ic_crypto_test_utils_reproducible_rng::reproducible_rng;
use ic_types::messages::{HttpRequest, SignedIngressContent};
use ic_types::time::GENESIS;
use ic_types::{CanisterId, Time};
use ic_validator_http_request_test_utils::{
Expand All @@ -395,6 +397,7 @@ mod validate_request {
use std::time::Duration;

const MAXIMUM_NUMBER_OF_DELEGATIONS: usize = 20; // !changing this number might be breaking!//
const MAXIMUM_NUMBER_OF_TARGETS: usize = 1_000; // !changing this number might be breaking!//
const CURRENT_TIME: Time = GENESIS;

#[test]
Expand Down Expand Up @@ -854,6 +857,50 @@ mod validate_request {
if public_key == duplicated_key_pair.public_key_der())
}

#[test]
fn should_fail_when_too_many_distinct_targets_in_delegation() {
let mut targets = Vec::with_capacity(MAXIMUM_NUMBER_OF_TARGETS + 1);
for i in 0..MAXIMUM_NUMBER_OF_TARGETS + 1 {
targets.push(CanisterId::from_u64(i as u64))
}
let mut rng = reproducible_rng();
let request = request_authenticated_by_delegation_with_targets(targets, &mut rng);

let result = verifier_at_time(CURRENT_TIME).validate_request(&request);

assert_matches!(result, Err(InvalidDelegation(DelegationTargetError(e))) if e.contains("expected at most 1000 targets"))
}

#[test]
fn should_fail_when_too_many_same_targets_in_delegation() {
let mut targets = Vec::with_capacity(MAXIMUM_NUMBER_OF_TARGETS + 1);
for _ in 0..MAXIMUM_NUMBER_OF_TARGETS + 1 {
targets.push(CanisterId::from_u64(0_u64))
}
let mut rng = reproducible_rng();
let request = request_authenticated_by_delegation_with_targets(targets, &mut rng);

let result = verifier_at_time(CURRENT_TIME).validate_request(&request);

assert_matches!(result, Err(InvalidDelegation(DelegationTargetError(e))) if e.contains("expected at most 1000 targets"))
}

fn request_authenticated_by_delegation_with_targets<R: Rng + CryptoRng>(
targets: Vec<CanisterId>,
rng: &mut R,
) -> HttpRequest<SignedIngressContent> {
assert!(!targets.is_empty());
HttpRequestBuilder::default()
.with_ingress_expiry_at(CURRENT_TIME)
.with_canister_id(Blob(targets[0].get().to_vec()))
.with_authentication(AuthenticationScheme::Delegation(
DelegationChain::rooted_at(random_user_key_pair(rng))
.delegate_to_with_targets(random_user_key_pair(rng), CURRENT_TIME, targets)
.build(),
))
.build()
}

fn delegation_chain_of_length<R: Rng + CryptoRng>(
number_of_delegations: usize,
delegation_expiration: Time,
Expand Down

0 comments on commit ef3dd0f

Please sign in to comment.