-
Notifications
You must be signed in to change notification settings - Fork 317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(boundary): setup of rate-limit canister #1961
base: master
Are you sure you want to change the base?
feat(boundary): setup of rate-limit canister #1961
Conversation
acca095
to
2d6a3fe
Compare
2d6a3fe
to
db4c0a6
Compare
Err: text; | ||
}; | ||
|
||
type DiscloseRulesByIncidentIdResponse = variant { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that you have a lot of
type XResponse = variant {
Ok;
Err: text;
};
maybe it'd make sense to consolidate those somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These types could be consolidate at some later point. But once consolidated the extensibility/flexibility is lost. Probably some Ok
variant will contain messages, i haven't yet carefully thought through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but couldn't that just be a new Response type? I didn't mean you can only respond with this smaller type. E.g
type EmptyResponse = variant {
Ok;
Err: text;
};
type ListResponse = variant {
Ok: vec RuleId,
Err: text;
};
service: {
disclose: (DiscloseArg) -> EmptyResponse;
list: (ListArg) -> ListResponse;
};
type InputConfig = record { | ||
schema_version: SchemaVersion; // schema version used to serialized the rules | ||
rules: vec InputRule; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be possible when specifying rules to also specify whether they are public or not? Or do you have to basically set the config, and then in another call disclose rules? I guess what I'm asking is "what does a normal user flow look like?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expected flow is:
- Each newly added rule is confidential by default
- To open the rule for public viewing a separate call to
disclose_()
function is needed
We could indeed add an additional fieldpolicy
withprivate/public
, but it seems to be unnecessary. If needed it can be added.
disclose_rules_by_rule_ids: (vec RuleId) -> (DiscloseRulesByRuleIdsResponse); | ||
|
||
// Make the viewing of the specified rules related to an incident ID publicly accessible | ||
disclose_rules_by_incident_id: (IncidentId) -> (DiscloseRulesByIncidentIdResponse); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any thoughts on consolidating these two disclose methods? e.g
type DiscloseArg = variant {
Rules: vec RuleId;
Incidents: vec IncidentId;
};
disclose: (vec DiscloseArg) -> (DiscloseResponse);
and similarly for get_rules
:
type ListArg = variant {
Rules: vec RuleId;
Incidents: vec IncidentId;
};
list: (vec ListArg) -> (ListResponse);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do like the disclose
one! but for get_rules
it would be strange as for a RuleId
it returns a single rule.
mod types; | ||
|
||
#[ic_cdk_macros::update] | ||
fn get_config(version: Option<Version>) -> GetConfigResponse { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Friendly suggestion - you may not agree with it. Notice that canisters don't really have a main
function, so it's hard to gauge "what runs first". This can make it tricky to have a clear understanding of the order of operations happening in the canister upon startup (the various initialization steps, like declaring stable memory, canister methods, etc). For that reason, my suggestion would be to not rush to break up the canister code into separate files. Instead, treat canister/lib.rs
similarly to how you would treat your main.rs
file, meaning configure all your initialization logic there sequentially, like you would do in a main
function. So in this case:
# lib.rs
... define stable memory
... define dependencies that will be used in your canister methods
... define canister methods
then on a per-need basis you'd create new files to host things that get imported into your lib.rs. All this is mostly so that it's easier to understand the flow of the canister. As it is right now, you would have to ask yourself, what's happening first? Stable memory in storage.rs? Canister methods in canister.rs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice that canisters don't really have a main function, so it's hard to gauge "what runs first".
Yes, canister is an actor.
For that reason, my suggestion would be to not rush to break up the canister code into separate files.
I understand. Since I have already prototyped the canister locally, i wanted to have at least a minimum separation of files. For example canister/types.rs
and canister/canister.rs
(lib.rs) make sense already now IMO. Stable memory variables and init
i can add in the next MR. Wdyt? However, I still wanted a working canister at this point.
c90662c
to
9c7347a
Compare
e3945e5
to
42abe63
Compare
const FULL_READ_ID: &str = "2vxsx-fae"; | ||
|
||
pub trait ResolveAccessLevel { | ||
fn get_access_level(&self) -> AccessLevel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not get_access_level(&self, Principal) -> AccessLevel
? Otherwise, I notice below you create a new AccessLevelResolver
every time you want to check someone's access level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it allows me to construct objects nicely in the canister method:
#[query(name = "get_rule_by_id")]
#[candid_method(query)]
fn get_rule_by_id(rule_id: RuleId) -> GetRuleByIdResponse {
let caller_id = ic_cdk::api::caller();
let response = with_state(|state| {
let access_resolver = AccessLevelResolver::new(caller_id);
let formatter =
ConfidentialityFormatterFactory::new(access_resolver).create_rule_formatter();
let fetcher = RuleFetcher::new(state, formatter);
fetcher.fetch(rule_id)
})?;
Ok(response.into())
}
otherwise i'd need to pass caller_id
somehow else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we doing the ACLs so far down the call stack? For example, for add_config
or disclose_rules
, we should not even instantiate these ConfigAdder
and RulesDiscloser
objects. The earlier we can reject the call, the better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO in the canister function it's better to assemble the final object (by either objects composition or using wrappers) and perform just one single call on that object, which executes the whole logic. With this approach we can ensure best testability of the canister. I think this approach is common for canisters. So I would abstain from putting access level call within the canister method. Although it is probably indeed better to introduce wrapper structs: WithLogger
, WithAccessLevel
, WithMetrics
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, for add_config or disclose_rules, we should not even instantiate these ConfigAdder and RulesDiscloser objects.
We might want to log info before/after authorization, so we need a composable and extensible approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it allows me to construct objects nicely in the canister method:
Ah, but there's an even nicer way to achieve this:
// Initialize AccessLevelResolver (let's say call it ACCESS_LEVEL_RESOLVER)
#[query(name = "get_rule_by_id")]
#[candid_method(query)]
fn get_rule_by_id(rule_id: RuleId) -> GetRuleByIdResponse {
let caller_id = ic_cdk::api::caller();
// use ACCESS_LEVEL_RESOLVER with caller_id
...
}
I have some examples for doing this here, here as well as here (last one includes stable structures).
Also, we use a similar approach throughout this codebase.
types::{InputConfig, RuleId, Version}, | ||
}; | ||
|
||
pub const INIT_VERSION: Version = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this value mean? Keep in mind it will get reset back to 1 any time the canister is restarted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the number from which the versions start. Config/version initialization should run only once, when the canister is installed for the first time. I will handle this logic later to make sure config is never initialized twice.
44215b4
to
5127eb5
Compare
e9995ca
to
5c554f2
Compare
|
||
use k256::elliptic_curve::SecretKey; | ||
|
||
const TEST_PRIVATE_KEY: &str = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does it work when the private key is empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should create one or use some existing one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me put it and commit for simplicity of testing
const IC_DOMAIN: &str = "https://ic0.app"; | ||
|
||
use k256::elliptic_curve::SecretKey; | ||
|
||
const TEST_PRIVATE_KEY: &str = ""; | ||
const TEST_PRIVATE_KEY: &str = "-----BEGIN EC PRIVATE KEY----- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also use:
ic/rs/test_utilities/identity/src/lib.rs
Lines 30 to 55 in 34b7182
lazy_static! { | |
// A keypair meant to be used in various test setups, including | |
// but (not limited) to scenario tests, end-to-end tests and the | |
// workload generator. | |
pub static ref TEST_IDENTITY_KEYPAIR: ic_canister_client_sender::Ed25519KeyPair = { | |
let mut rng = ChaChaRng::seed_from_u64(1_u64); | |
ic_canister_client_sender::Ed25519KeyPair::generate(&mut rng) | |
}; | |
// a dedicated identity for when we use --principal-id in the | |
// workload generator | |
pub static ref TEST_IDENTITY_KEYPAIR_HARD_CODED: ic_canister_client_sender::Ed25519KeyPair = { | |
get_pair(None) | |
}; | |
pub static ref PUBKEY : UserPublicKey = UserPublicKey { | |
key: TEST_IDENTITY_KEYPAIR.public_key.to_vec(), | |
algorithm_id: AlgorithmId::Ed25519, | |
}; | |
pub static ref PUBKEY_PID : UserPublicKey = UserPublicKey { | |
key: get_pub(None).serialize_raw().to_vec(), | |
algorithm_id: AlgorithmId::Ed25519, | |
}; | |
} |
// Initialize an empty config with version=1 | ||
init_version_and_config(1); | ||
|
||
let interval = std::time::Duration::from_secs(init_arg.registry_polling_period_secs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since you import std::time::Duration
, can't you just use Duration
?
pub enum AccessLevel { | ||
FullAccess, | ||
FullRead, | ||
RestrictedRead, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should call it something like Default
or Unauthorized
. Now, it sounds like this is still a special permission and there is something like NoRead
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unauthorized
could be a valid variant only for add_config()/disclose()
operations, however get_config()/get_rule_bu_id()
operations are always "authorized" just the access rights can be different. Default
sounds a bit vague to me, a comment could resolve it though. Let's discuss it more. I introduced this enum based on the access level to the stored data, it was meant to be agnostic to specific canister methods.
|
||
use crate::storage::API_BOUNDARY_NODE_PRINCIPALS; | ||
|
||
const FULL_ACCESS_ID: &str = "imx2d-dctwe-ircfz-emzus-bihdn-aoyzy-lkkdi-vi5vw-npnik-noxiy-mae"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also be removed and set through the init/upgrade args. Maybe we can add two vecs to those args: one vec with the principals that should be added and one vec with the principals that should be removed. Also, we might want to add a method that exposes the principals with full access.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also be removed and set through the init/upgrade args.
I can do it, but why? Is this because You wanted this principal to be eventually retrieved via canister call?
} | ||
|
||
#[derive(Debug, thiserror::Error)] | ||
pub enum ConfidentialFormatterError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this Confidential while everything else is Confidentiality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And why is it even needed? Is there a case where access is being denied as you can always get some data even if you request with the anonymous principal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far it is indeed not used, i can remove it. I usually introduce errors to traits at the beginning, and then remove them once I'm certain there's no case for error.
|
||
fn format(&self, config: &OutputConfig) -> Result<OutputConfig, ConfidentialFormatterError> { | ||
let mut config = config.clone(); | ||
if self.access_resolver.get_access_level() == AccessLevel::RestrictedRead { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would switch this such that the default behavior is the one with the least privilege instead of default is full privilege.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't understand, i think it is exactly the case now, formatting takes place for the user with least privileged access level
rule: &OutputRuleMetadata, | ||
) -> Result<OutputRuleMetadata, ConfidentialFormatterError> { | ||
let mut rule = rule.clone(); | ||
if self.access_resolver.get_access_level() == AccessLevel::RestrictedRead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the same: least privilege should be default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
provide a suggestion plz, and i'll apply it
Co-authored-by: r-birkner <103420898+r-birkner@users.noreply.github.com>
rs/boundary_node/rate_limits/canister/confidentiality_formatting.rs
Outdated
Show resolved
Hide resolved
rs/boundary_node/rate_limits/canister/confidentiality_formatting.rs
Outdated
Show resolved
Hide resolved
…ng.rs Co-authored-by: r-birkner <103420898+r-birkner@users.noreply.github.com>
…ng.rs Co-authored-by: r-birkner <103420898+r-birkner@users.noreply.github.com>
9b1f074
to
67cb123
Compare
if !metadata.is_disclosed { | ||
disclose_rules(repository, time, &metadata.rule_ids)?; | ||
metadata.is_disclosed = true; | ||
let _ = repository.update_incident(incident_id, metadata); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: maybe add assert
pub fn new(access_resolver: A) -> Self { | ||
Self { | ||
access_resolver, | ||
phantom: PhantomData, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
write a comment why
How to run e2e canister client tests:
$ rs/boundary_node/rate_limits $ dfx deploy rate_limit_canister --network playground --no-wallet
canister_id
inrs/boundary_node/rate_limits/canister_client/src/main.rs
.rs/boundary_node/rate_limits/canister_client $ cargo run