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

fix(agent): Check subnet canister ranges #580

Merged
merged 18 commits into from Jun 28, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/agent/src/canisterStatus/index.ts
Expand Up @@ -87,8 +87,8 @@ export const request = async (options: {
const response = await agent.readState(canisterId, {
paths: [encodedPaths[index]],
});
const cert = new Certificate(response, agent);
const verified = await cert.verify();
const cert = new Certificate(response.certificate, agent.fetchRootKey());
Copy link
Contributor

Choose a reason for hiding this comment

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

agent.fetchRootKey() is asynchronous, so this would need an await - otherwise, the value that gets passed to the Certificate constructor is a Promise.

const rootKey = await agent.fetchRootKey();
const cert = new Certificate(response.certificate, rootKey);

Copy link
Member Author

Choose a reason for hiding this comment

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

The Certiificate constructor takes a Promise, it's later awaited on in Certificate.verify, where it's used - I didn't want to block before the point where it was needed (not that it makes much of a difference in practice, as the user pretty much has to call verify to do anything with the certificate).

const verified = await cert.verify(canisterId);
Copy link
Contributor

Choose a reason for hiding this comment

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

quick check here - this changes the interface of verify. We should bump agent-js to the next minor version instead of the next patch version.

also, is there anywhere else in the codebase that calls verify() without arguments?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have the same concern. Not all certificates contain a path with canister_id. How do we verify the certificate if we only want to read non canister related paths, e.g. /time, /subnet/ and /request_status?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point - I'll bump the version up. We could also consider rejigging the type of verify (or getting rid of it) - see the answer to your next comment.

For the other point (other places calling verify), I'd hope that the Typescript type system is sound enough to catch this? However, I did miss one place, revealed by the failed PR tests (I didn't check these, my bad), in e2e, because I never called the TS compiler on that package. Let's discuss with Kyle to add a command that builds the whole monorepo in the right order and catches all such errors? I tried to do this as we discussed a couple of weeks ago, but didn't get anywhere in an hour or so that I spent on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

For @chenyan-dfinity 's point: any call to read_state (including those to get time, subnet and request status) has to have an effective canister ID, and the scope of the certificate must be checked against this ID:
https://internetcomputer.org/docs/current/references/ic-interface-spec/#http-read-state
Is there another way to obtain such a certificate?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd hope that the Typescript type system is sound enough to catch this?

Nope. TS type system is NOT sound. Never rely on its type checker.

any call to read_state (including those to get time, subnet and request status) has to have an effective canister ID, and the scope of the certificate must be checked against this ID

Good point. Then we could use effective_canister_id as the argument name? There is also an exception that provisional_create_canister_with_cycles allows effective canister id to be any id (https://docs.dfinity.systems/spec/public/#http-effective-canister-id). So we need to at least disable the check for the provisional API.

Also, can this effective_canister_id be a member of the Certificate class, so that we don't have to pass the canister id when calling verify? This also makes it a non-breaking change. We can disable the check if the effective_canister_id is missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

So we need to at least disable the check for the provisional API.

I think the interface spec is actually wrong on this, as it says that any canister is an effective canister ID for provisional_create_canister_with_cycles, but then also requires that the delegation is checked for the call to read_state. I think we should disable the check for the case when aaaaa-aa is the effective canister ID, as per the spec it can only be the effective canister ID for the provisional_create_canister_with_cycles call. I'll discuss this with Jens once he's back from vacation.

Also, can this effective_canister_id be a member of the Certificate class, so that we don't have to pass the canister id when calling verify? This also makes it a non-breaking change. We can disable the check if the effective_canister_id is missing.

Since we're fixing a security flaw, I think we have to make a breaking change (callers that don't change the call would stay vulnerable). After discussing with Jason, since it's a breaking change already, I've changed the API to perform the verification immediately in the constructor - we couldn't find a use case where you'd want to delay the verification (do you maybe see one?). Also, I unified the error reporting to always use exceptions, instead of a mixture of exceptions and Booleans as was previously the case for verify

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the interface spec is actually wrong on this

I think the spec is fine. We just need to disable the check when the effective canister id is aaaaa-aa. agent-rs does the same thing: https://github.com/dfinity/agent-rs/blob/main/ic-agent/src/agent/mod.rs#L1214

Since we're fixing a security flaw, I think we have to make a breaking change (callers that don't change the call would stay vulnerable)

Not sure it has to be a breaking change. Callers that didn't upgrade agent-js to the fixed version stays vulnerable. It doesn't have to be a breaking change to fix a vulnerability.

I've changed the API to perform the verification immediately in the constructor - we couldn't find a use case where you'd want to delay the verification (do you maybe see one?).

There might be a use case that the caller doesn't want to do a verification. If we verify in the constructor, it can be harder to skip verification? The problem is that bls verify is written in Wasm, some platforms don't support wasm execution, e.g. mobile platform, chrome plugins. In those environments, they either have to use a slower JS implementation or skip the verification completely. I know a grant awardee is writing a Chrome plugin to inspect all IC traffic, and cannot run verify in the plugin setting. Since it's a debugging tool, it's not too bad to disable the verification.

if (!verified) {
throw new Error(
'There was a problem certifying the response data. Please verify your connection to the mainnet, or be sure to call fetchRootKey on your agent if you are developing locally',
Expand Down
45 changes: 45 additions & 0 deletions packages/agent/src/certificate.test.ts
Expand Up @@ -3,9 +3,12 @@
* an instance of ArrayBuffer).
* @jest-environment node
*/
import { fromHexString } from '@dfinity/candid';
import * as cbor from './cbor';
import * as Cert from './certificate';
import { fromHex, toHex } from './utils/buffer';
import { Principal } from '@dfinity/principal';
import { verify } from 'crypto';

function label(str: string): ArrayBuffer {
return new TextEncoder().encode(str);
Expand All @@ -15,6 +18,13 @@ function pruned(str: string): ArrayBuffer {
return fromHex(str);
}

// Root public key for the IC main net, encoded as hex
const IC_ROOT_KEY =
'308182301d060d2b0601040182dc7c0503010201060c2b0601040182dc7c05030201036100814' +
'c0e6ec71fab583b08bd81373c255c3c371b2e84863c98a4f1e08b74235d14fb5d9c0cd546d968' +
'5f913a0c0b2cc5341583bf4b4392e467db96d65b9bb4cb717112f8472e0d5a4d14505ffd7484' +
'b01291091c5f87b98883463f98091a0baaae';

test('hash tree', async () => {
const cborEncode = fromHex(
'8301830183024161830183018302417882034568656c6c6f810083024179820345776f726c64' +
Expand Down Expand Up @@ -124,3 +134,38 @@ test('lookup', () => {
expect(toText(Cert.lookup_path([fromText('d')], tree))).toEqual('morning');
expect(Cert.lookup_path([fromText('e')], tree)).toEqual(undefined);
});

// The sample certificate for testing delegation is extracted from the response used in agent-rs tests, where they were taken
// from an interaction with the IC mainnet.
const SAMPLE_CERT: string =
'd9d9f7a364747265658301830182045820250f5e26868d9c1ea7ab29cbe9c15bf1c47c0d7605e803e39e375a7fe09c6ebb830183024e726571756573745f7374617475738301820458204b268227774ec77ff2b37ecb12157329d54cf376694bdd59ded7803efd82386f83025820edad510eaaa08ed2acd4781324e6446269da6753ec17760f206bbe81c465ff528301830183024b72656a6563745f636f64658203410383024e72656a6563745f6d6573736167658203584443616e69737465722069766733372d71696161612d61616161622d61616167612d63616920686173206e6f20757064617465206d6574686f64202772656769737465722783024673746174757382034872656a65637465648204582097232f31f6ab7ca4fe53eb6568fc3e02bc22fe94ab31d010e5fb3c642301f1608301820458203a48d1fc213d49307103104f7d72c2b5930edba8787b90631f343b3aa68a5f0a83024474696d65820349e2dc939091c696eb16697369676e6174757265583089a2be21b5fa8ac9fab1527e041327ce899d7da971436a1f2165393947b4d942365bfe5488710e61a619ba48388a21b16a64656c65676174696f6ea2697375626e65745f6964581dd77b2a2f7199b9a8aec93fe6fb588661358cf12223e9a3af7b4ebac4026b6365727469666963617465590231d9d9f7a26474726565830182045820ae023f28c3b9d966c8fb09f9ed755c828aadb5152e00aaf700b18c9c067294b483018302467375626e6574830182045820e83bb025f6574c8f31233dc0fe289ff546dfa1e49bd6116dd6e8896d90a4946e830182045820e782619092d69d5bebf0924138bd4116b0156b5a95e25c358ea8cf7e7161a661830183018204582062513fa926c9a9ef803ac284d620f303189588e1d3904349ab63b6470856fc4883018204582060e9a344ced2c9c4a96a0197fd585f2d259dbd193e4eada56239cac26087f9c58302581dd77b2a2f7199b9a8aec93fe6fb588661358cf12223e9a3af7b4ebac402830183024f63616e69737465725f72616e6765738203581bd9d9f781824a000000000020000001014a00000000002fffff010183024a7075626c69635f6b657982035885308182301d060d2b0601040182dc7c0503010201060c2b0601040182dc7c050302010361009933e1f89e8a3c4d7fdcccdbd518089e2bd4d8180a261f18d9c247a52768ebce98dc7328a39814a8f911086a1dd50cbe015e2a53b7bf78b55288893daa15c346640e8831d72a12bdedd979d28470c34823b8d1c3f4795d9c3984a247132e94fe82045820996f17bb926be3315745dea7282005a793b58e76afeb5d43d1a28ce29d2d158583024474696d6582034995b8aac0e4eda2ea16697369676e61747572655830ace9fcdd9bc977e05d6328f889dc4e7c99114c737a494653cb27a1f55c06f4555e0f160980af5ead098acc195010b2f7';

test('delegation works for canisters within the subnet range', async () => {
const canisterId = Principal.fromText('ivg37-qiaaa-aaaab-aaaga-cai');
const cert = new Cert.Certificate(fromHex(SAMPLE_CERT), Promise.resolve(fromHex(IC_ROOT_KEY)));
const result = await cert.verify(canisterId);
expect(result).toEqual(true);
});

test('delegation check fails for canisters outside of the subnet range', async () => {
// Use a different principal than the happy path, which isn't in the delegation ranges.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would have to check the allowed ranges myself here. Would it help to provide the allowed range(s) as a comment here?

also, since it would be cheap: would it make sense to add tests for the boundary cases at the (closed) ends of a range?

Copy link
Member Author

Choose a reason for hiding this comment

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

TBH I don't even know what the subnet ranges in this certificate are - I just copied these over from the Rust tests. But even if I did know what they were, I don't know if our textual encoding preserves the ordering?

Boundary cases totally make sense, I'll have a look at those.

Choose a reason for hiding this comment

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

You can actually decode this (it's cbor, https://cbor.me comes in handy).

So the first level of decoding reveals

"delegation": {"subnet_id": h'D77B2A2F7199B9A8AEC93FE6FB588661358CF12223E9A3AF7B4EBAC402', "certificate": h'D9D9F7A26474726565830182045820AE023F28C3B9D966C8FB09F9ED755C828AADB5152E00AAF700B18C9C067294B483018302467375626E6574830182045820E83BB025F6574C8F31233DC0FE289FF546DFA1E49BD6116DD6E8896D90A4946E830182045820E782619092D69D5BEBF0924138BD4116B0156B5A95E25C358EA8CF7E7161A661830183018204582062513FA926C9A9EF803AC284D620F303189588E1D3904349AB63B6470856FC4883018204582060E9A344CED2C9C4A96A0197FD585F2D259DBD193E4EADA56239CAC26087F9C58302581DD77B2A2F7199B9A8AEC93FE6FB588661358CF12223E9A3AF7B4EBAC402830183024F63616E69737465725F72616E6765738203581BD9D9F781824A000000000020000001014A00000000002FFFFF010183024A7075626C69635F6B657982035885308182301D060D2B0601040182DC7C0503010201060C2B0601040182DC7C050302010361009933E1F89E8A3C4D7FDCCCDBD518089E2BD4D8180A261F18D9C247A52768EBCE98DC7328A39814A8F911086A1DD50CBE015E2A53B7BF78B55288893DAA15C346640E8831D72A12BDEDD979D28470C34823B8D1C3F4795D9C3984A247132E94FE82045820996F17BB926BE3315745DEA7282005A793B58E76AFEB5D43D1A28CE29D2D158583024474696D6582034995B8AAC0E4EDA2EA16697369676E61747572655830ACE9FCDD9BC977E05D6328F889DC4E7C99114C737A494653CB27A1F55C06F4555E0F160980AF5EAD098ACC195010B2F7'}

The included delegation cert then specifies for the subnet (in addition to it's public key) the canister_ranges D9D9F781824A000000000020000001014A00000000002FFFFF0101 (which is again cbor. Decoded this results in the following ranges:

[(CanisterId(jrlun-jiaaa-aaaab-aaaaa-cai), CanisterId(v2nog-2aaaa-aaaab-p777q-cai))]

Which are the following principals:
Principal(PrincipalInner { len: 10, bytes: [0, 0, 0, 0, 0, 32, 0, 0, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0] }), Principal(PrincipalInner { len: 10, bytes: [0, 0, 0, 0, 0, 47, 255, 255, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0] })

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @frederikrothenberger !

I now realized that the interface spec also provides handy tools for decoding the text representation (which indeed doesn't preserve the lexicographic ordering, due to the use of crc-32).

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the checks now, switched to a hex representation of the principals as that makes the ranges clearly visible

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, thanks for addressing this @oggy-dfin !

const canisterId = Principal.fromText('ryjl3-tyaaa-aaaaa-aaaba-cai');
const cert = new Cert.Certificate(fromHex(SAMPLE_CERT), Promise.resolve(fromHex(IC_ROOT_KEY)));
// This is a bit crufty; verify returns a boolean, but can also indicate verification errors
// by throwing exceptions
try {
const result = await cert.verify(canisterId);
expect(result).toEqual(false);
} catch (e) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

question about this test - it sounds like you want to test that an exception gets thrown. shouldn't you put the expect in the catch block then? and rejig the test so that the call to verify throws an error? one way you can force verify to throw is by using a mock

on the other hand, if all you want to check is that result is a boolean, the try / catch isnt necessary

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem here is that the interface of verify is a bit smelly. It signals some errors by returning false, but others by throwing instead. Here I know that it will throw because I wrote it, but I wanted to cover both ways of reporting errors.

IMO we should clean it up. I think an event better alternative would be to have the constructor take the canisterId as the parameter and internally call verify, which blows up earlier which IMO is a good thing (TM). Thoughts? Should I do that if we're already mucking around with the interfaces and doing backwards incompatible changes?

});

// The only situation in which one can read state of the IC management canister
// is when the user calls provisional_create_canister_with_cycles. In this case,
// we shouldn't check the delegations.
test('delegation check succeeds for the management canister', async () => {
const cert = new Cert.Certificate(fromHex(SAMPLE_CERT), Promise.resolve(fromHex(IC_ROOT_KEY)));
// This is a bit crufty; verify returns a boolean, but can also indicate verification errors
// by throwing exceptions
const result = await cert.verify(Principal.managementCanister());
expect(result).toEqual(true);
});
52 changes: 32 additions & 20 deletions packages/agent/src/certificate.ts
@@ -1,9 +1,9 @@
import { Agent, getDefaultAgent, ReadStateResponse } from './agent';
import * as cbor from './cbor';
import { AgentError } from './errors';
import { hash } from './request_id';
import { blsVerify } from './utils/bls';
import { concat, fromHex, toHex } from './utils/buffer';
import { Principal } from '@dfinity/principal';

/**
* A certificate needs to be verified (using {@link Certificate.prototype.verify})
Expand Down Expand Up @@ -102,20 +102,19 @@ function isBufferEqual(a: ArrayBuffer, b: ArrayBuffer): boolean {
export class Certificate {
private readonly cert: Cert;
private verified = false;
private _rootKey: ArrayBuffer | null = null;

constructor(response: ReadStateResponse, private _agent: Agent = getDefaultAgent()) {
this.cert = cbor.decode(new Uint8Array(response.certificate));
constructor(certificate: ArrayBuffer, private _rootKey: Promise<ArrayBuffer>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

_rootKey should probably be type ArrayBuffer - Promises are typically not passed into constructors

Copy link
Member Author

Choose a reason for hiding this comment

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

If we change the interface to stick the canisterId into the certificate that makes total sense - if we for whatever reason want to call verify separately (I don't know why we would), a Promise would allow to delay blocking until we need it.

this.cert = cbor.decode(new Uint8Array(certificate));
}

public lookup(path: Array<ArrayBuffer | string>): ArrayBuffer | undefined {
this.checkState();
return lookup_path(path, this.cert.tree);
}

public async verify(): Promise<boolean> {
public async verify(canisterId: Principal): Promise<boolean> {
const rootHash = await reconstruct(this.cert.tree);
const derKey = await this._checkDelegation(this.cert.delegation);
const derKey = await this._checkDelegationAndGetKey(canisterId, this.cert.delegation);
const sig = this.cert.signature;
const key = extractDER(derKey);
const msg = concat(domain_sep('ic-state-root'), rootHash);
Expand All @@ -130,28 +129,41 @@ export class Certificate {
}
}

private async _checkDelegation(d?: Delegation): Promise<ArrayBuffer> {
private async _checkDelegationAndGetKey(
canisterId: Principal,
d?: Delegation,
): Promise<ArrayBuffer> {
if (!d) {
if (!this._rootKey) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why removing this logic? fetchRootKey() should only be called in local replica. The root key for the mainnet is hardcoded and doesn't require explicit fetching.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for catching this - my fix had made things worse! However, I do think that this is the wrong place for this logic - there's no real need for Certificate to know anything about the agent IMO (I ended up staring at this piece of code for a while when first looking it, and obviously got it wrong), it just needs a root key.

I changed the code now so that the callers use the root key if available. We can probably extract a convenience method on Agent to do this.

if (this._agent.rootKey) {
this._rootKey = this._agent.rootKey;
return this._rootKey;
}

throw new Error(`Agent does not have a rootKey. Do you need to call 'fetchRootKey'?`);
}
return this._rootKey;
}
const cert: Certificate = new Certificate(d as any, this._agent);
if (!(await cert.verify())) {
const cert: Certificate = new Certificate(d.certificate, this._rootKey);
if (!(await cert.verify(canisterId))) {
throw new Error('fail to verify delegation certificate');
}

const lookup = cert.lookup(['subnet', d.subnet_id, 'public_key']);
if (!lookup) {
if (canisterId.compareTo(Principal.managementCanister()) != 'eq') {
Copy link
Contributor

Choose a reason for hiding this comment

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

javascript is weird, so this should be !== instead of !=. If the latter is used, type coercion is performed. please disregard if type coercion is intentional here.

Copy link
Member Author

Choose a reason for hiding this comment

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

If this post is correct, there'd be no coercion in TS (it'd be a compile-time error instead)

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not rely on TS for correctness. If one side the operand is any type, the check will pass, and we still get coercion when translated to JS.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, changed to !==

const range_lookup = cert.lookup(['subnet', d.subnet_id, 'canister_ranges']);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a nit, but camelCase is used throughout this codebase (and in JS in general). I think we should keep the code style consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for catching this - I changed all the snake_case I introduced to camelCase. However, subnet_id was there before, so I didn't touch this - I'm not sure if there's some magic going on because it's parsed from a subnet_id field in CBOR.

if (!range_lookup) {
throw new Error(`Could not find canister ranges for subnet 0x${toHex(d.subnet_id)}`);
}
const ranges_arr: Array<[Uint8Array, Uint8Array]> = cbor.decode(range_lookup);
Copy link
Contributor

Choose a reason for hiding this comment

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

same nit here re: camelCase

const ranges: Array<[Principal, Principal]> = ranges_arr.map(v => [
Principal.fromUint8Array(v[0]),
Principal.fromUint8Array(v[1]),
]);

const canister_in_range = ranges.some(r => r[0].ltEq(canisterId) && r[1].gtEq(canisterId));
Copy link
Contributor

Choose a reason for hiding this comment

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

same nit here re: camelCase

if (!canister_in_range) {
throw new Error(
`Canister ${canisterId} not in range of delegations for subnet 0x${toHex(d.subnet_id)}`,
);
}
}
const public_key_lookup = cert.lookup(['subnet', d.subnet_id, 'public_key']);
Copy link
Contributor

Choose a reason for hiding this comment

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

same nit here re: camelCase

if (!public_key_lookup) {
throw new Error(`Could not find subnet key for subnet 0x${toHex(d.subnet_id)}`);
}
return lookup;
return public_key_lookup;
}
}

Expand Down
4 changes: 2 additions & 2 deletions packages/agent/src/polling/index.ts
Expand Up @@ -29,8 +29,8 @@ export async function pollForResponse(
): Promise<ArrayBuffer> {
const path = [new TextEncoder().encode('request_status'), requestId];
const state = await agent.readState(canisterId, { paths: [path] });
const cert = new Certificate(state, agent);
const verified = await cert.verify();
const cert = new Certificate(state.certificate, agent.fetchRootKey());
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs an await to pass the actual root key to Certificate

const rootKey = await agent.fetchRootKey();
const cert = new Certificate(state.certificate, rootKey);
const verified = await cert.verify(canisterId);

const verified = await cert.verify(canisterId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a function to compute the effective canister id, especially when the client is calling aaaaa-aa?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have sufficient information at this point in the code to compute this (there's no request); per the comment, the function already expects an effective canister ID. I don't see any special computation of the effective ID, but I may have missed something.

So I think you're right and we should add it, but I don't think this PR is breaking anything beyond what's broken already. In particular, to read the status of calls to the management canister other than provisional_create_canister_with_cycles, you need to compute the effective canister ID regardless of whether you're properly checking the certificate, and then read_state on that ID, otherwise (assuming that the replica actually follows the interface spec) you don't get a response.

if (!verified) {
throw new Error('Fail to verify certificate');
}
Expand Down
17 changes: 17 additions & 0 deletions packages/principal/src/index.test.ts
Expand Up @@ -24,4 +24,21 @@ describe('Principal', () => {
it('errors out on parsing invalid characters', () => {
expect(() => Principal.fromText('Hello world!')).toThrow();
});

it('compares principals correctly', () => {
const anonymous = Principal.anonymous();
const principal1 = Principal.fromText('ryjl3-tyaaa-aaaaa-aaaba-cai');
const principal2 = Principal.fromText('ivg37-qiaaa-aaaab-aaaga-cai');
for (let p of [anonymous, principal1, principal2]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

lint error here: the tests are failing because this is a let instead of const.

expect(p.compareTo(p)).toBe('eq');
expect(p.ltEq(p)).toBe(true);
expect(p.gtEq(p)).toBe(true);
}
expect(principal1.compareTo(principal2)).toBe('lt');
expect(principal1.compareTo(anonymous)).toBe('lt');
expect(principal2.compareTo(principal1)).toBe('gt');
expect(principal2.compareTo(anonymous)).toBe('lt');
expect(anonymous.compareTo(principal1)).toBe('gt');
expect(anonymous.compareTo(principal2)).toBe('gt');
});
});
27 changes: 27 additions & 0 deletions packages/principal/src/index.ts
Expand Up @@ -5,6 +5,8 @@ import { sha224 } from './utils/sha224';
const SELF_AUTHENTICATING_SUFFIX = 2;
const ANONYMOUS_SUFFIX = 4;

const MANAGEMENT_CANISTER_PRINCIPAL_HEX_STR = 'aaaaa-aa';

const fromHexString = (hexString: string) =>
new Uint8Array((hexString.match(/.{1,2}/g) ?? []).map(byte => parseInt(byte, 16)));

Expand All @@ -16,6 +18,10 @@ export class Principal {
return new this(new Uint8Array([ANONYMOUS_SUFFIX]));
}

public static managementCanister(): Principal {
oggy-dfin marked this conversation as resolved.
Show resolved Hide resolved
return this.fromHex(MANAGEMENT_CANISTER_PRINCIPAL_HEX_STR);
}

public static selfAuthenticating(publicKey: Uint8Array): Principal {
const sha = sha224(publicKey);
return new this(new Uint8Array([...sha, SELF_AUTHENTICATING_SUFFIX]));
Expand Down Expand Up @@ -96,4 +102,25 @@ export class Principal {
public toString(): string {
return this.toText();
}

public compareTo(other: Principal): 'lt' | 'eq' | 'gt' {
oggy-dfin marked this conversation as resolved.
Show resolved Hide resolved
for (let i = 0; i < Math.min(this._arr.length, other._arr.length); i++) {
if (this._arr[i] < other._arr[i]) return 'lt';
dfx-json marked this conversation as resolved.
Show resolved Hide resolved
else if (this._arr[i] > other._arr[i]) return 'gt';
dfx-json marked this conversation as resolved.
Show resolved Hide resolved
}
// Here, at least one principal is a prefix of the other principal (they could be the same)
if (this._arr.length < other._arr.length) return 'lt';
dfx-json marked this conversation as resolved.
Show resolved Hide resolved
if (this._arr.length > other._arr.length) return 'gt';
dfx-json marked this conversation as resolved.
Show resolved Hide resolved
return 'eq';
}

public ltEq(other: Principal): boolean {
oggy-dfin marked this conversation as resolved.
Show resolved Hide resolved
const cmp = this.compareTo(other);
return cmp == 'lt' || cmp == 'eq';
}

public gtEq(other: Principal): boolean {
oggy-dfin marked this conversation as resolved.
Show resolved Hide resolved
const cmp = this.compareTo(other);
return cmp == 'gt' || cmp == 'eq';
}
}