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
Open
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
7a32461
feat: DelegationChain audits for delegation targets and maximum deleg…
krpeacock Aug 21, 2023
96e90b9
changelog
krpeacock Aug 21, 2023
298d373
wip
krpeacock Aug 25, 2023
ef6c173
continuing work on not repeating delegations
krpeacock Sep 5, 2023
3a89fc0
checks to prevent repeated delegations to public keys
krpeacock Sep 6, 2023
0cb0acb
Merge branch 'main' into kyle/SDK-1193-delegation-enforcement
krpeacock Sep 6, 2023
78e6e56
chore: lock npm version for CI (#762)
krpeacock Sep 6, 2023
139356c
feat: DelegationChain audits for delegation targets and maximum deleg…
krpeacock Aug 21, 2023
4f87fe1
changelog
krpeacock Aug 21, 2023
95588ea
wip
krpeacock Aug 25, 2023
c0dab1b
continuing work on not repeating delegations
krpeacock Sep 5, 2023
d6d5a3c
checks to prevent repeated delegations to public keys
krpeacock Sep 6, 2023
26249e3
Merge branch 'kyle/SDK-1193-delegation-enforcement' of github.com:dfi…
krpeacock Sep 6, 2023
8b80563
fixing test case
krpeacock Sep 6, 2023
32aad22
fixing auth-client test around expired delegations
krpeacock Sep 6, 2023
9e38f30
npm version pinning was accomplished in a separate PR
krpeacock Sep 20, 2023
ed64572
Update packages/identity/src/identity/delegation.ts
krpeacock Sep 20, 2023
194007f
Update packages/identity/src/identity/delegation.ts
krpeacock Sep 20, 2023
b60c736
Update packages/identity/src/identity/delegation.ts
krpeacock Sep 20, 2023
f5a798e
Update packages/identity/src/identity/delegation.ts
krpeacock Sep 20, 2023
bab42fb
refactoring to use reduce function
krpeacock Sep 20, 2023
f5b1c97
upgrading node version to 16 as a baseline for CI
krpeacock Sep 20, 2023
ba9025d
pinning node 9 again
krpeacock Sep 20, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ jobs:
spec:
- release-0.16 # https://github.com/dfinity-lab/ic-ref/tree/release-0.16
node:
- 14
- 16
steps:
- uses: actions/checkout@v2
- name: Use Node.js ${{ matrix.node }}
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/prettier.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ jobs:
spec:
- release-0.16 # https://github.com/dfinity-lab/ic-ref/tree/release-0.16
node:
- 14
- 16
steps:
- uses: actions/checkout@v2
- name: Use Node.js ${{ matrix.node }}
Expand Down
1 change: 0 additions & 1 deletion .github/workflows/unit-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ jobs:
spec:
- '0.16.1'
node:
- 14
- 16

steps:
Expand Down
7 changes: 7 additions & 0 deletions docs/generated/changelog.html
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,16 @@ <h1>Agent-JS Changelog</h1>
<section>
<h2>Version x.x.x</h2>
<ul>
<li>chore: limit npm version to 9 in ci for compatibility with node 16</li>
<li>
feat: DelegationChain audits for delegation targets and maximum delegations during
initialization
</li>
<li>
Adds more helpful error message for when principal is undefined during actor creation
</li>
<li>feat: DelegationChain audits for delegation targets and maximum delegations during
initialization</li>
</ul>
<h2>Version 0.19.2</h2>
<ul>
Expand Down
8 changes: 6 additions & 2 deletions packages/auth-client/src/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -704,10 +704,14 @@ describe('Migration from Ed25519Key', () => {
jest.setSystemTime(new Date('2020-01-01T00:00:00.000Z'));

// two days ago
const expiration = new Date('2019-12-30T00:00:00.000Z');
const properExpiration = new Date('2020-01-03T00:00:00.000Z');
const expiredExpiration = new Date('2019-12-30T00:00:00.000Z');

const key = await Ed25519KeyIdentity.fromJSON(JSON.stringify(testSecrets));
const chain = DelegationChain.create(key, key.getPublicKey(), expiration);
const chain = await DelegationChain.create(key, key.getPublicKey(), properExpiration);
// Override the expiration to be expired before storing
(chain.delegations[0].delegation as any).expiration =
BigInt(Number(expiredExpiration)) * BigInt(10000);
const fakeStore: Record<any, any> = {};
fakeStore[KEY_STORAGE_DELEGATION] = JSON.stringify((await chain).toJSON());
fakeStore[KEY_STORAGE_KEY] = JSON.stringify(testSecrets);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Delegation can include multiple targets 1`] = `
Object {
"expiration": "176bb3e7000",
"pubkey": "010203",
"targets": Array [
"00000000002000030101",
"000000000020008E0101",
],
}
`;
167 changes: 136 additions & 31 deletions packages/identity/src/identity/delegation.test.ts
Original file line number Diff line number Diff line change
@@ -1,31 +1,34 @@
import { SignIdentity } from '@dfinity/agent';
import { Principal } from '@dfinity/principal';
import { DelegationChain } from './delegation';
import { Delegation, DelegationChain } from './delegation';
import { Ed25519KeyIdentity } from './ed25519';

function createIdentity(seed: number): SignIdentity {
const s = new Uint8Array([seed, ...new Array(31).fill(0)]);
return Ed25519KeyIdentity.generate(s);
}

function randomPrincipal(): Principal {
const bytes = new Uint8Array(29);
crypto.getRandomValues(bytes);
return Principal.fromUint8Array(bytes);
}

const expiry_date = new Date(1609459200000);
const current_date = new Date(1609459200000 - 10000);

jest.useFakeTimers();
jest.setSystemTime(current_date);

test('delegation signs with proper keys (3)', async () => {
const root = createIdentity(2);
const middle = createIdentity(1);
const bottom = createIdentity(0);

const rootToMiddle = await DelegationChain.create(
root,
middle.getPublicKey(),
new Date(1609459200000),
);
const middleToBottom = await DelegationChain.create(
middle,
bottom.getPublicKey(),
new Date(1609459200000),
{
previous: rootToMiddle,
},
);
const rootToMiddle = await DelegationChain.create(root, middle.getPublicKey(), expiry_date);
const middleToBottom = await DelegationChain.create(middle, bottom.getPublicKey(), expiry_date, {
previous: rootToMiddle,
});

const golden = {
delegations: [
Expand Down Expand Up @@ -60,23 +63,13 @@ test('DelegationChain can be serialized to and from JSON', async () => {
const middle = createIdentity(1);
const bottom = createIdentity(0);

const rootToMiddle = await DelegationChain.create(
root,
middle.getPublicKey(),
new Date(1609459200000),
{
targets: [Principal.fromText('jyi7r-7aaaa-aaaab-aaabq-cai')],
},
);
const middleToBottom = await DelegationChain.create(
middle,
bottom.getPublicKey(),
new Date(1609459200000),
{
previous: rootToMiddle,
targets: [Principal.fromText('u76ha-lyaaa-aaaab-aacha-cai')],
},
);
const rootToMiddle = await DelegationChain.create(root, middle.getPublicKey(), expiry_date, {
targets: [Principal.fromText('jyi7r-7aaaa-aaaab-aaabq-cai')],
});
const middleToBottom = await DelegationChain.create(middle, bottom.getPublicKey(), expiry_date, {
previous: rootToMiddle,
targets: [Principal.fromText('u76ha-lyaaa-aaaab-aacha-cai')],
});

const rootToMiddleJson = JSON.stringify(rootToMiddle);
// All strings in the JSON should be hex so it is clear how to decode this as different versions
Expand All @@ -97,3 +90,115 @@ test('DelegationChain can be serialized to and from JSON', async () => {
const middleToBottomActual = DelegationChain.fromJSON(middleToBottomJson);
expect(middleToBottomActual).toEqual(middleToBottom);
});

test('DelegationChain has a maximum length of 20 delegations', async () => {
const identities = new Array(21).fill(0).map((_, i) => createIdentity(i));
let prevDelegationChain: DelegationChain | undefined;

for (let i = 0; i < identities.length - 1; i++) {
const identity = identities[i];
const nextIdentity = identities[i + 1];
let signedDelegation: DelegationChain;
if (i === 0) {
signedDelegation = await DelegationChain.create(
identity,
nextIdentity.getPublicKey(),
expiry_date,
);
} else {
signedDelegation = await DelegationChain.create(
identity,
nextIdentity.getPublicKey(),
expiry_date,
{
previous: prevDelegationChain,
},
);
}
prevDelegationChain = signedDelegation;
}
expect(prevDelegationChain?.delegations.length).toEqual(20);

const secondLastIdentity = identities[identities.length - 2];
const lastIdentity = identities[identities.length - 1];
expect(
DelegationChain.create(secondLastIdentity, lastIdentity.getPublicKey(), expiry_date, {
previous: prevDelegationChain,
}),
).rejects.toThrow('Delegation chain cannot exceed 20');
});

test('Delegation can include multiple targets', async () => {
const pubkey = new Uint8Array([1, 2, 3]);
const expiration = BigInt(Number(expiry_date));
const targets = [
Principal.fromText('jyi7r-7aaaa-aaaab-aaabq-cai'),
Principal.fromText('u76ha-lyaaa-aaaab-aacha-cai'),
];
const delegation = new Delegation(pubkey, expiration, targets);
expect(delegation.targets).toEqual(targets);
expect(delegation.toJSON()).toMatchSnapshot();
});

test('Delegation targets cannot exceed 1_000', () => {
const targets = new Array(1_001).fill(randomPrincipal());
expect(() => new Delegation(new Uint8Array([1, 2, 3]), BigInt(0), targets)).toThrow(
'Delegation targets cannot exceed 1000',
);
});

test('Delegation chains cannot repeat public keys', async () => {
const root = createIdentity(2);
const middle = createIdentity(1);
const bottom = createIdentity(0);

const rootToMiddle = await DelegationChain.create(root, middle.getPublicKey(), expiry_date);
const middleToBottom = await DelegationChain.create(middle, bottom.getPublicKey(), expiry_date, {
previous: rootToMiddle,
});

// Repeating middle's public key in the delegation chain should throw an error.
expect(
DelegationChain.create(middle, bottom.getPublicKey(), expiry_date, {
previous: middleToBottom,
}),
).rejects.toThrow('Delegation target cannot be repeated in the chain.');

// Repeating root's public key in the delegation chain should throw an error.
expect(
DelegationChain.create(root, bottom.getPublicKey(), expiry_date, {
previous: middleToBottom,
}),
).rejects.toThrow('Delegation target cannot be repeated in the chain.');
});

test('Cannot create a delegation chain with an outdated expiry', async () => {
const root = createIdentity(2);
const middle = createIdentity(1);
const bottom = createIdentity(0);

const rootToMiddle = await DelegationChain.create(root, middle.getPublicKey(), expiry_date);

expect(
DelegationChain.create(middle, bottom.getPublicKey(), new Date(0), {
previous: rootToMiddle,
}),
).rejects.toThrow('Delegation expiration date cannot be in the past.');
});

test('Cannot create a delegation chain with an outdated delegation', async () => {
const root = createIdentity(2);
const middle = createIdentity(1);
const bottom = createIdentity(0);

const rootToMiddle = await DelegationChain.create(root, middle.getPublicKey(), expiry_date);

// Modify the delegation to have an expiry of 0.
(rootToMiddle.delegations[0].delegation as any).expiration = BigInt(0);

expect(
DelegationChain.create(middle, bottom.getPublicKey(), expiry_date, {
previous: rootToMiddle,
}),
).rejects.toThrow('Previous delegation in the chain has expired.');
});
60 changes: 55 additions & 5 deletions packages/identity/src/identity/delegation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,16 @@ import {
import { Principal } from '@dfinity/principal';
import * as cbor from 'simple-cbor';
import { fromHexString, toHexString } from '../buffer';
import { DelegationError } from './errors';

const domainSeparator = new TextEncoder().encode('\x1Aic-request-auth-delegation');
const requestDomainSeparator = new TextEncoder().encode('\x0Aic-request');
const MAXIMUM_NUMBER_OF_TARGETS = 1_000;
const MAXIMUM_DELEGATION_CHAIN_LENGTH = 20;

function _parseBlob(value: unknown): ArrayBuffer {
if (typeof value !== 'string' || value.length < 64) {
throw new Error('Invalid public key.');
throw new DelegationError('Invalid public key.');
}

return fromHexString(value);
Expand All @@ -32,7 +35,11 @@ export class Delegation {
public readonly pubkey: ArrayBuffer,
public readonly expiration: bigint,
public readonly targets?: Principal[],
) {}
) {
if (targets && targets?.length > MAXIMUM_NUMBER_OF_TARGETS) {
throw new DelegationError(`Delegation targets cannot exceed ${MAXIMUM_NUMBER_OF_TARGETS}`);
}
}

public toCBOR(): cbor.CborValue {
// Expiration field needs to be encoded as a u64 specifically.
Expand Down Expand Up @@ -97,6 +104,16 @@ async function _createSingleDelegation(
expiration: Date,
targets?: Principal[],
): Promise<SignedDelegation> {
// Validate inputs
if (targets && targets?.length > MAXIMUM_NUMBER_OF_TARGETS) {
throw new DelegationError(`Delegation targets cannot exceed ${MAXIMUM_NUMBER_OF_TARGETS}`);
}
if (!from.sign) {
throw new DelegationError('The from identity does not have a sign method.');
}
if (!to.toDer) {
throw new DelegationError('The to public key does not have a toDer method.');
}
const delegation: Delegation = new Delegation(
to.toDer(),
BigInt(+expiration) * BigInt(1000000), // In nanoseconds.
Expand Down Expand Up @@ -177,6 +194,38 @@ export class DelegationChain {
targets?: Principal[];
} = {},
): Promise<DelegationChain> {
/**
* Validations
*/

// Validate expiration
if (expiration.getTime() < Date.now()) {
throw new DelegationError('Delegation expiration date cannot be in the past.');
}
if (options.previous) {
// Ensure we aren't extending beyond the maximum delegation chain length.
if (options.previous.delegations.length >= MAXIMUM_DELEGATION_CHAIN_LENGTH) {
throw new DelegationError(
`Delegation chain cannot exceed ${MAXIMUM_DELEGATION_CHAIN_LENGTH}`,
);
}

// 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).

if (current.delegation.expiration < BigInt(Date.now()) * BigInt(1000000)) {
throw new DelegationError(
'Previous delegation in the chain has expired.',
current.delegation.expiration,
);
}
if (previous.has(current.delegation.pubkey)) {
throw new DelegationError('Delegation target cannot be repeated in the chain.');
}
previous.add(current.delegation.pubkey);
return previous;
}, new Set<ArrayBuffer>([to.toDer()]));
}

const delegation = await _createSingleDelegation(from, to, expiration, options.targets);
return new DelegationChain(
[...(options.previous?.delegations || []), delegation],
Expand All @@ -192,14 +241,14 @@ export class DelegationChain {
public static fromJSON(json: string | JsonnableDelegationChain): DelegationChain {
const { publicKey, delegations } = typeof json === 'string' ? JSON.parse(json) : json;
if (!Array.isArray(delegations)) {
throw new Error('Invalid delegations.');
throw new DelegationError('Invalid delegations.');
}

const parsedDelegations: SignedDelegation[] = delegations.map(signedDelegation => {
const { delegation, signature } = signedDelegation;
const { pubkey, expiration, targets } = delegation;
if (targets !== undefined && !Array.isArray(targets)) {
throw new Error('Invalid targets.');
throw new DelegationError('Invalid targets.');
}

return {
Expand All @@ -209,7 +258,7 @@ export class DelegationChain {
targets &&
targets.map((t: unknown) => {
if (typeof t !== 'string') {
throw new Error('Invalid target.');
throw new DelegationError('Invalid target.');
}
return Principal.fromHex(t);
}),
Expand Down Expand Up @@ -237,6 +286,7 @@ export class DelegationChain {
protected constructor(
public readonly delegations: SignedDelegation[],
public readonly publicKey: DerEncodedPublicKey,
public readonly previous?: DelegationChain,
) {}

public toJSON(): JsonnableDelegationChain {
Expand Down
Loading
Loading