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

Merged
merged 31 commits into from
Jan 6, 2025

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
src/dfx/src/commands/canister/request_status.rs Outdated Show resolved Hide resolved
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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To unblock this PR, I've removed changes to the request-status command in this commit. I'll come back to this functionality once PocketIC supports checking caller when requesting ingress status.

src/dfx/src/lib/environment.rs Outdated Show resolved Hide resolved
@mraszyk mraszyk merged commit 79de1d0 into master Jan 6, 2025
298 checks passed
@mraszyk mraszyk deleted the mraszyk/pic-impersonate branch January 6, 2025 17:53
rikonor pushed a commit that referenced this pull request Feb 3, 2025
…4013)

* feat: impersonating sender of requests to a local PocketIC instance

* windows

* no max_request_time_ms

* bump pocket-ic

* request-status

* cleanup

* cleanup

* fix

* typo

* impersonate.bash

* dfx canister request-status

* smoke

* smoke

* smoke

* smoke

* chore: update replica version to d9fe2076f677a08734bed90c67b1c3f4056ed621

* no e2e.yml changes

* move impersonate.bash to call.bash

* docs

* unnecessary DfxResult

* harden tests

* fix dfx identity new in tests

* no changes to request-status command
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