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

Prevent perpetual use of expired auth signature; remove superfluous API functions #567

Merged
merged 8 commits into from
Aug 27, 2024
13 changes: 0 additions & 13 deletions examples/taco/nodejs/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {
encrypt,
fromBytes,
initialize,
isAuthorized,
toBytes,
toHexString,
} from '@nucypher/taco';
Expand Down Expand Up @@ -81,18 +80,6 @@ const encryptToBytes = async (messageString: string) => {
encryptorSigner,
);

Comment on lines 81 to 82
Copy link
Member

Choose a reason for hiding this comment

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

Are we still "checking contract compatibility" even when removing this from the Testnet examples that are run every hour?

https://github.com/nucypher/taco-web/actions/workflows/lynx.yml

Copy link
Member Author

@derekpierre derekpierre Aug 21, 2024

Choose a reason for hiding this comment

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

At the moment, I don't think isAuthorized is useful as part of the exposed taco API, hence the removal here. If others feel that it is useful I can put it back.

It's unclear to me how comprehensive this check is as it pertains to "checking compatibility" based on one function. Do you have any additional insight there, perhaps I'm missing something? Perhaps testing the DkgCoordinatorAgent with lynx Coordinator contract via a separate workflow could be a better solution to CI checking compatibility...maybe - not sure what that entails or if it is feasible?

Additionally, Coordinator.isEncryptionAuthorized() which is called by isAuthorized(), is actually deprecated in the latest Coordinator contract, and will be removed down the road. Instead, the underlying acccessController for the ritual should be called directly for that check.

Copy link
Member

Choose a reason for hiding this comment

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

No, I don't have a clear idea about this check beyond that we can check that this specific method on the Coordinator contract has not been modified.

So yes, I agree we can delete this.

My concern came after I read the commented lines in the code talking about "checking contract compatibility": I was wondering if I'm missing something here.

Copy link
Member Author

@derekpierre derekpierre Aug 21, 2024

Choose a reason for hiding this comment

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

My concern came after I read the commented lines in the code talking about "checking contract compatibility": I was wondering if I'm missing something here.

Totally! I was hoping someone had a better picture of it than I did. Definitely a good conversation to have.

It was added as part of #543 (comment), 😅 , but I don't remember why that is - maybe a side discussion with Piotr. But the problem it is trying to solve seems like a stretch.

Perhaps others may weigh in.

Copy link
Member

Choose a reason for hiding this comment

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

the problem it is trying to solve seems like a stretch.

Agreed. Including a compatibility check as part of an example is not a good idea (although I understand that it stems from the example doubling as the CI smoke test). But the original question still stands, and I'm afraid I can't help. I don't understand why it was added originally.

// Note: Not actually needed but used by CI for checking contract compatibility.
// Calling it after the encryption because we need material from messageKit.
const isEncryptorAuthenticated = await isAuthorized(
provider,
domain,
messageKit,
ritualId,
);
if (!isEncryptorAuthenticated) {
throw new Error('Not authorized');
}

return messageKit.toBytes();
};

Expand Down
6 changes: 3 additions & 3 deletions packages/pre/test/acceptance/alice-grants.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {
bytesEqual,
fakePorterUri,
fakeProvider,
fakeSigner,
fakeUrsulas,
fromBytes,
mockGetUrsulas,
Expand Down Expand Up @@ -70,9 +69,10 @@ describe('story: alice shares message with bob through policy', () => {
startDate,
endDate,
};
const provider = fakeProvider();
policy = await alice.grant(
fakeProvider(),
fakeSigner(),
provider,
provider.getSigner(),
domains.DEVNET,
fakePorterUri,
policyParams,
Expand Down
3 changes: 1 addition & 2 deletions packages/pre/test/acceptance/delay-enact.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import {
bytesEqual,
fakePorterUri,
fakeProvider,
fakeSigner,
fakeUrsulas,
mockGetUrsulas,
} from '@nucypher/test-utils';
Expand Down Expand Up @@ -61,7 +60,7 @@ describe('story: alice creates a policy but someone else enacts it', () => {

const enacted = await preEnactedPolicy.enact(
provider,
fakeSigner(),
provider.getSigner(),
domains.DEVNET,
);
expect(enacted.txHash).toBeDefined();
Expand Down
21 changes: 20 additions & 1 deletion packages/taco-auth/src/providers/eip4361/eip4361.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,13 @@ export class EIP4361AuthProvider {
// If we have a signature in localStorage, return it
const maybeSignature = this.storage.getAuthSignature(storageKey);
if (maybeSignature) {
return maybeSignature;
// check whether older than node freshness requirement
if (this.isMessageExpired(maybeSignature.typedData)) {
// clear signature so that it will be recreated and stored
this.storage.clear(storageKey);
} else {
return maybeSignature;
}
}

// If at this point we didn't return, we need to create a new message
Expand All @@ -61,6 +67,19 @@ export class EIP4361AuthProvider {
return authMessage;
}

private isMessageExpired(message: string): boolean {
const siweMessage = new SiweMessage(message);
if (!siweMessage.issuedAt) {
// don't expect to ever happen; but just in case
return false;
}
Comment on lines +72 to +75
Copy link
Member

Choose a reason for hiding this comment

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

This is a sentinel against undefined?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

From the EIP4361 spec,issued-at is a required value - https://docs.login.xyz/general-information/siwe-overview/eip-4361#specification.

However, in the siwe typescript library implementation, it is an optional value in the constructor - https://github.com/spruceid/siwe/blob/main/packages/siwe/lib/client.ts#L49. As a result, Typescript complains if we don't check whether it is defined first before using it.

Copy link
Member

Choose a reason for hiding this comment

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

Is this a divergence of siwe wrt to the spec then?

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I can tell, yes, it is a divergence.

Copy link
Member Author

Choose a reason for hiding this comment

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

I take that back. If issued at is not provided, the current date is used i.e. either the creator can specify the date or the current date is used.

Copy link
Member Author

@derekpierre derekpierre Aug 27, 2024

Choose a reason for hiding this comment

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

Well, it's not the constructor but the SiweMessage struct - so still a deviation - https://github.com/spruceid/siwe/blob/main/packages/siwe/lib/client.ts#L49

Without the check, the compiler returns the following:

packages/taco-auth/src/providers/eip4361/eip4361.ts:72:36 - error TS2769: No overload matches this call.
  Overload 1 of 4, '(value: string | number | Date): Date', gave the following error.
    Argument of type 'string | undefined' is not assignable to parameter of type 'string | number | Date'.
      Type 'undefined' is not assignable to type 'string | number | Date'.
  Overload 2 of 4, '(value: string | number): Date', gave the following error.
    Argument of type 'string | undefined' is not assignable to parameter of type 'string | number'.
      Type 'undefined' is not assignable to type 'string | number'.

I'll file an issue for the siwe library, and perhaps they can either clarify or fix.

(Update: filed spruceid/siwe#211)


const twoHourWindow = new Date(siweMessage.issuedAt);
twoHourWindow.setHours(twoHourWindow.getHours() + 2);
const now = new Date();
return twoHourWindow < now;
}

private async createSIWEAuthMessage(): Promise<AuthSignature> {
const address = await this.signer.getAddress();
const { domain, uri } = this.providerParams;
Expand Down
14 changes: 14 additions & 0 deletions packages/taco-auth/src/storage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ interface IStorage {
getItem(key: string): string | null;

setItem(key: string, value: string): void;

removeItem(key: string): void;
}

class BrowserStorage implements IStorage {
Expand All @@ -14,6 +16,10 @@ class BrowserStorage implements IStorage {
public setItem(key: string, value: string): void {
localStorage.setItem(key, value);
}

public removeItem(key: string): void {
localStorage.removeItem(key);
}
}

class NodeStorage implements IStorage {
Expand All @@ -26,6 +32,10 @@ class NodeStorage implements IStorage {
public setItem(key: string, value: string): void {
this.storage[key] = value;
}

public removeItem(key: string) {
delete this.storage[key];
}
}

export class LocalStorage {
Expand All @@ -50,4 +60,8 @@ export class LocalStorage {
const asJson = JSON.stringify(authSignature);
this.storage.setItem(key, asJson);
}

public clear(key: string): void {
this.storage.removeItem(key);
}
}
88 changes: 84 additions & 4 deletions packages/taco-auth/test/auth-provider.test.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,20 @@
import {
bobSecretKeyBytes,
fakeProvider,
fakeSigner,
TEST_SIWE_PARAMS,
} from '@nucypher/test-utils';
import { SiweMessage } from 'siwe';
import { describe, expect, it } from 'vitest';
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';

import { EIP4361AuthProvider } from '../src/providers';
import {
EIP4361AuthProvider,
SingleSignOnEIP4361AuthProvider,
} from '../src/providers';
import { EIP4361TypedDataSchema } from '../src/providers/eip4361/common';

describe('auth provider', () => {
const provider = fakeProvider(bobSecretKeyBytes);
const signer = fakeSigner(bobSecretKeyBytes);
const signer = provider.getSigner();
const eip4361Provider = new EIP4361AuthProvider(
provider,
signer,
Expand Down Expand Up @@ -52,3 +54,81 @@ describe('auth provider', () => {
).toThrow();
});
});

describe('auth provider caching', () => {
beforeEach(() => {
// tell vitest we use mocked time
vi.useFakeTimers();
});

afterEach(() => {
// restoring date after each test run
vi.useRealTimers();
});

const provider = fakeProvider(bobSecretKeyBytes);
const signer = provider.getSigner();
const eip4361Provider = new EIP4361AuthProvider(
provider,
signer,
TEST_SIWE_PARAMS,
);

it('caches auth signature, but regenerates when expired', async () => {
const createAuthSignatureSpy = vi.spyOn(
eip4361Provider,
'createSIWEAuthMessage',
);

const typedSignature = await eip4361Provider.getOrCreateAuthSignature();
expect(createAuthSignatureSpy).toHaveBeenCalledTimes(1);

const typedSignatureSecondCall =
await eip4361Provider.getOrCreateAuthSignature();
// auth signature not expired, so spy is not called a 2nd time
expect(createAuthSignatureSpy).toHaveBeenCalledTimes(1);
expect(typedSignatureSecondCall).toEqual(typedSignature);

// time travel to 2h:1m in the future; auth signature is now expired
const now = new Date();
now.setHours(now.getHours() + 2, now.getMinutes() + 1);
vi.setSystemTime(now);

const typedSignatureThirdCall =
await eip4361Provider.getOrCreateAuthSignature();
// auth signature is now expired, so spy is called a 2nd time
expect(createAuthSignatureSpy).toHaveBeenCalledTimes(2);
expect(typedSignatureThirdCall).not.toEqual(typedSignature);
});
});

describe('single sign-on auth provider', async () => {
const provider = fakeProvider(bobSecretKeyBytes);
const signer = provider.getSigner();

const eip4361Provider = new EIP4361AuthProvider(
provider,
signer,
TEST_SIWE_PARAMS,
);
const originalTypedSignature =
await eip4361Provider.getOrCreateAuthSignature();

it('use existing SIWE message', async () => {
const originalSiweMessage = originalTypedSignature.typedData;
const originalSiweSignature = originalTypedSignature.signature;

const singleSignOnProvider =
await SingleSignOnEIP4361AuthProvider.fromExistingSiweInfo(
originalSiweMessage,
originalSiweSignature,
);

const typedSignature =
await singleSignOnProvider.getOrCreateAuthSignature();
expect(typedSignature.typedData).toEqual(originalSiweMessage);
expect(typedSignature.signature).toEqual(originalSiweSignature);
expect(typedSignature.address).toEqual(await signer.getAddress());
expect(typedSignature.scheme).toEqual('EIP4361');
});
});
4 changes: 2 additions & 2 deletions packages/taco/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ export {
} from '@nucypher/shared';

export * as conditions from './conditions';
// TODO(#324): Expose registerEncrypters from taco API
export { decrypt, encrypt, encryptWithPublicKey, isAuthorized } from './taco';

export { decrypt, encrypt, encryptWithPublicKey } from './taco';

// TODO: Remove this re-export once `@nucypher/taco-auth` is mature and published
export {
Expand Down
45 changes: 0 additions & 45 deletions packages/taco/src/taco.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,10 @@ import {
ThresholdMessageKit,
} from '@nucypher/nucypher-core';
import {
ChecksumAddress,
DkgCoordinatorAgent,
Domain,
fromHexString,
getPorterUris,
GlobalAllowListAgent,
PorterClient,
toBytes,
} from '@nucypher/shared';
Expand Down Expand Up @@ -160,46 +158,3 @@ export const decrypt = async (
context,
);
};

/**
* Checks if the encryption from the provided messageKit is authorized for the specified ritual.
*
* @export
* @param {ethers.providers.Provider} provider - Instance of ethers provider which is used to interact with
* your selected network.
* @param {Domain} domain - The domain which was used to encrypt the network. Must match the `ritualId`.
* @param {ThresholdMessageKit} messageKit - The encrypted message kit to be checked.
* @param {number} ritualId - The ID of the DKG Ritual under which the messageKit was supposedly encrypted.
*
* @returns {Promise<boolean>} Returns a Promise that resolves with the authorization status.
* True if authorized, false otherwise
*/
export const isAuthorized = async (
provider: ethers.providers.Provider,
domain: Domain,
messageKit: ThresholdMessageKit,
ritualId: number,
): Promise<boolean> =>
DkgCoordinatorAgent.isEncryptionAuthorized(
provider,
domain,
ritualId,
messageKit,
);

// TODO is this still valid and actually needed? should we remove this?
export const registerEncrypters = async (
provider: ethers.providers.Provider,
signer: ethers.Signer,
domain: Domain,
ritualId: number,
encrypters: ChecksumAddress[],
): Promise<void> => {
await GlobalAllowListAgent.registerEncrypters(
provider,
signer,
domain,
ritualId,
encrypters,
);
};
14 changes: 5 additions & 9 deletions packages/taco/test/conditions/context.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,7 @@ import {
USER_ADDRESS_PARAM_DEFAULT,
USER_ADDRESS_PARAM_EXTERNAL_EIP4361,
} from '@nucypher/taco-auth';
import {
fakeAuthProviders,
fakeProvider,
fakeSigner,
} from '@nucypher/test-utils';
import { fakeAuthProviders, fakeProvider } from '@nucypher/test-utils';
import { ethers } from 'ethers';
import { beforeAll, describe, expect, it, vi } from 'vitest';

Expand Down Expand Up @@ -308,8 +304,8 @@ describe('context', () => {

// TODO: Move to a separate file
describe('No authentication provider', () => {
let provider: ethers.providers.Provider;
let signer: ethers.Signer;
let provider: ethers.providers.Web3Provider;
let signer: ethers.providers.JsonRpcSigner;
let authProviders: Record<string, AuthProvider>;

async function testEIP4361AuthSignature(
Expand Down Expand Up @@ -337,8 +333,8 @@ describe('No authentication provider', () => {
beforeAll(async () => {
await initialize();
provider = fakeProvider();
signer = fakeSigner();
authProviders = await fakeAuthProviders();
signer = provider.getSigner();
authProviders = await fakeAuthProviders(signer);
});

it('throws an error if there is no auth provider', () => {
Expand Down
5 changes: 2 additions & 3 deletions packages/taco/test/taco.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {
fakeDkgFlow,
fakePorterUri,
fakeProvider,
fakeSigner,
fakeTDecFlow,
mockGetRitualIdFromPublicKey,
mockTacoDecrypt,
Expand Down Expand Up @@ -48,7 +47,7 @@ describe('taco', () => {
const mockedDkg = fakeDkgFlow(FerveoVariant.precomputed, 0, 4, 4);
const mockedDkgRitual = fakeDkgRitual(mockedDkg);
const provider = fakeProvider(aliceSecretKeyBytes);
const signer = fakeSigner(aliceSecretKeyBytes);
const signer = provider.getSigner();
const getFinalizedRitualSpy = mockGetActiveRitual(mockedDkgRitual);

const messageKit = await taco.encrypt(
Expand Down Expand Up @@ -110,7 +109,7 @@ describe('taco', () => {
const mockedDkg = fakeDkgFlow(FerveoVariant.precomputed, 0, 4, 4);
const mockedDkgRitual = fakeDkgRitual(mockedDkg);
const provider = fakeProvider(aliceSecretKeyBytes);
const signer = fakeSigner(aliceSecretKeyBytes);
const signer = provider.getSigner();
const getFinalizedRitualSpy = mockGetActiveRitual(mockedDkgRitual);

const customParamKey = ':nftId';
Expand Down
Loading