Skip to content
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: impersonating sender of requests to a local PocketIC instance #4013

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

mraszyk
Copy link
Contributor

@mraszyk mraszyk commented Nov 25, 2024

This PR enables impersonating sender of requests to a local PocketIC instance: dfx canister call, dfx canister status, and dfx canister update-settings take an additional CLI argument --impersonate to specify a principal on behalf of which requests to a local PocketIC instance are sent.

@mraszyk mraszyk marked this pull request as ready for review December 18, 2024 16:43
@mraszyk mraszyk requested a review from a team as a code owner December 18, 2024 16:43
CHANGELOG.md Show resolved Hide resolved
@@ -298,3 +298,82 @@ teardown() {
assert_command dfx canister call inter2_mo read
assert_match '(8 : nat)'
}

@test "impersonate sender" {
[[ ! "$USE_POCKETIC" ]] && skip "skipped for replica: impersonating sender is only supported for PocketIC"

Choose a reason for hiding this comment

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

Can we have this test impersonate an identity other than (or in addition to) aaaaa-aa? It looks like these tests would pass if --impersonate always used the anonymous identity rather than the impersonation mechanism.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The principal aaaaa-aa is the management canister identity, not the anonymous identity 2vxsx-fae, and thus --identity anonymous would not make the tests pass.

Choose a reason for hiding this comment

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

Okay, but that being the case, this test shows that --impersonate can impersonate the management canister identity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test shows that --impersonate can impersonate the management canister identity

indeed and the impersonation mechanism is the only way how to make the management canister identity be the caller

Copy link
Contributor Author

Choose a reason for hiding this comment

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

retry_policy.reset();
request_accepted = true;
let blob = if let Some(pocketic) = env.get_pocketic() {
let msg_id = RawMessageId {

Choose a reason for hiding this comment

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

is this change related to impersonation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Because request status can only be retrieved by the same caller who submitted the update call before.

Choose a reason for hiding this comment

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

This new code path is always taken when env.get_pocketic.is_some(), whether or not --impersonate is involved. Doesn't this mean that when using PocketIC, dfx canister request_status will work no matter which identity is used?

I'm adding a new test in #4047 to demonstrate this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't this mean that when using PocketIC, dfx canister request_status will work no matter which identity is used?

Correct. If we aren't comfortable with this behavior, PocketIC would need to be extended with a check for matching callers (in production, the read state request handler takes care of it, but in PocketIC this was omitted since security is not a requirement and it didn't seem important for the sake of canister testing).

Choose a reason for hiding this comment

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

Here's how this could apply to canister testing:

  • developer writes canister
  • some of developer's maintenance processes include dfx canister call --async and dfx canister request-status, but they forget to pass the same --identity on both
  • this all works locally since they're using PocketIC, but fails on mainnet

src/dfx/src/lib/environment.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants