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: enhanced delegation enforcement checks #761

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

krpeacock
Copy link
Contributor

Description

So far, the JavaScript agent has not enforced checks around delegation constraints. This meant that developers had to encounter errors later in the development process, often with less helpful certificate errors.

These changes introduce the following constraints, which now throw errors during the creation of a delegation chain:

  • Expiration
  • Max number of targets
  • Max delegation chain length
  • No longer allows repeats of public keys in the chain

This complies with the constraints indicated in the Internet Computer Interface Specification

Fixes # (issue)

How Has This Been Tested?

New and updated unit tests

Checklist:

  • My changes follow the guidelines in CONTRIBUTING.md.
  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

@krpeacock krpeacock requested a review from a team as a code owner September 6, 2023 17:34
@krpeacock krpeacock force-pushed the kyle/SDK-1193-delegation-enforcement branch from adaf5d6 to 3a89fc0 Compare September 6, 2023 17:36
@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2023

size-limit report 📦

Path Size
@dfinity/agent 87.38 KB (0%)
@dfinity/candid 13.52 KB (0%)
@dfinity/principal 5.15 KB (0%)
@dfinity/auth-client 92.83 KB (+0.32% 🔺)
@dfinity/assets 89.96 KB (0%)
@dfinity/identity 90.12 KB (+0.35% 🔺)
@dfinity/identity-secp256k1 232.51 KB (0%)

Copy link
Member

@ericswanson-dfinity ericswanson-dfinity left a comment

Choose a reason for hiding this comment

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

for (it in collection) {
    do something with it
}

is easier to understand than

let current = something else;
for (it in collection) {
    do something with current
    current = it
}
now check current for something // last value in collection

packages/identity/src/identity/delegation.ts Outdated Show resolved Hide resolved
packages/identity/src/identity/delegation.ts Outdated Show resolved Hide resolved
packages/identity/src/identity/delegation.ts Outdated Show resolved Hide resolved
packages/identity/src/identity/delegation.ts Outdated Show resolved Hide resolved
@krpeacock
Copy link
Contributor Author

for (it in collection) {
    do something with it
}

is easier to understand than

let current = something else;
for (it in collection) {
    do something with current
    current = it
}
now check current for something // last value in collection

Valid point! I've refactored to use a reduce function which is more semantically appropriate

}

// Ensure that public keys are not repeated in the chain.
options.previous.delegations.reduce((previous, current) => {

Choose a reason for hiding this comment

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

Given that options.previous is a DelegationChain, is it really necessary to check that options.previous.delegations has no repeated delegations?

Asked another way, does DelegationChain have an invariant that delegations has no repeated public keys?

If so, it should be sufficient (and simpler) here to check that to is not found among options.previous.delegations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If TypeScript was the runtime language of the web, we could be confident that the other chain is a valid DelegationChain. However, if we want runtime validation, we have to do the checks ourself

Copy link
Member

@ericswanson-dfinity ericswanson-dfinity Sep 20, 2023

Choose a reason for hiding this comment

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

good thing MAXIMUM_DELEGATION_CHAIN_LENGTH is only 20. Pretty sure this is O(n^2) or possibly O(n^2 log n).

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