From af4d3f1f29d41aad77a72cab9c7b0c8f8a3c56d6 Mon Sep 17 00:00:00 2001 From: Henry Tsai Date: Fri, 3 Nov 2023 09:32:22 -0700 Subject: [PATCH] Fixed bug on usage of `GeneralJwsVerifier` and improved its usability & performance (#594) * Fixed bug where a new cache is created every time the DWN performs a signature check * Removed the need to instantiate a new instance of GeneralJwsVerifier every time DWN performs a signature check * Improved perf by not doing DID resolution when the signature check result is already cached * Minor error code naming improvements --- src/core/auth.ts | 11 +++---- src/core/dwn-error.ts | 5 ++- src/jose/jws/general/verifier.ts | 54 +++++++++++++++++++++++--------- src/utils/jws.ts | 2 +- tests/jose/jws/general.spec.ts | 33 +++++++++---------- tests/jose/jws/verifier.spec.ts | 8 ----- 6 files changed, 60 insertions(+), 53 deletions(-) delete mode 100644 tests/jose/jws/verifier.spec.ts diff --git a/src/core/auth.ts b/src/core/auth.ts index 1fa678fd8..2d19fc616 100644 --- a/src/core/auth.ts +++ b/src/core/auth.ts @@ -23,7 +23,7 @@ export async function validateMessageSignatureIntegrity( ): Promise<{ descriptorCid: CID, [key: string]: any }> { if (messageSignature.signatures.length !== 1) { - throw new DwnError(DwnErrorCode.AuthenticateMoreThanOneAuthoriation, 'expected no more than 1 signature for authorization purpose'); + throw new DwnError(DwnErrorCode.AuthenticationMoreThanOneSignatureNotSupported, 'expected no more than 1 signature for authorization purpose'); } // validate payload integrity @@ -55,19 +55,16 @@ export async function authenticate(authorizationModel: AuthorizationModel | unde throw new DwnError(DwnErrorCode.AuthenticateJwsMissing, 'Missing JWS.'); } - const signatureVerifier = new GeneralJwsVerifier(authorizationModel.signature); - await signatureVerifier.verify(didResolver); + await GeneralJwsVerifier.verifySignatures(authorizationModel.signature, didResolver); if (authorizationModel.ownerSignature !== undefined) { - const ownerSignatureVerifier = new GeneralJwsVerifier(authorizationModel.ownerSignature); - await ownerSignatureVerifier.verify(didResolver); + await GeneralJwsVerifier.verifySignatures(authorizationModel.ownerSignature, didResolver); } if (authorizationModel.authorDelegatedGrant !== undefined) { // verify the signature of the grantor of the delegated grant const authorDelegatedGrant = await PermissionsGrant.parse(authorizationModel.authorDelegatedGrant); - const grantedBySignatureVerifier = new GeneralJwsVerifier(authorDelegatedGrant.message.authorization.signature); - await grantedBySignatureVerifier.verify(didResolver); + await GeneralJwsVerifier.verifySignatures(authorDelegatedGrant.message.authorization.signature, didResolver); } } diff --git a/src/core/dwn-error.ts b/src/core/dwn-error.ts index 06dc30f26..5886b07ee 100644 --- a/src/core/dwn-error.ts +++ b/src/core/dwn-error.ts @@ -14,8 +14,8 @@ export class DwnError extends Error { */ export enum DwnErrorCode { AuthenticateJwsMissing = 'AuthenticateJwsMissing', - AuthenticateMoreThanOneAuthoriation = 'AuthenticateMoreThanOneAuthoriation', AuthenticateDescriptorCidMismatch = 'AuthenticateDescriptorCidMismatch', + AuthenticationMoreThanOneSignatureNotSupported = 'AuthenticationMoreThanOneSignatureNotSupported', AuthorizationUnknownAuthor = 'AuthorizationUnknownAuthor', AuthorizationNotGrantedToAuthor = 'AuthorizationNotGrantedToAuthor', ComputeCidCodecNotSupported = 'ComputeCidCodecNotSupported', @@ -25,6 +25,7 @@ export enum DwnErrorCode { DidNotValid = 'DidNotValid', DidResolutionFailed = 'DidResolutionFailed', Ed25519InvalidJwk = 'Ed25519InvalidJwk', + GeneralJwsVerifierGetPublicKeyNotFound = 'GeneralJwsVerifierGetPublicKeyNotFound', GeneralJwsVerifierInvalidSignature = 'GeneralJwsVerifierInvalidSignature', GrantAuthorizationGrantExpired = 'GrantAuthorizationGrantExpired', GrantAuthorizationGrantMissing = 'GrantAuthorizationGrantMissing', @@ -76,7 +77,6 @@ export enum DwnErrorCode { ProtocolsQueryUnauthorized = 'ProtocolsQueryUnauthorized', RecordsDecryptNoMatchingKeyEncryptedFound = 'RecordsDecryptNoMatchingKeyEncryptedFound', RecordsDeleteAuthorizationFailed = 'RecordsDeleteAuthorizationFailed', - RecordsGrantAuthorizationConditionPublicationProhibited = 'RecordsGrantAuthorizationConditionPublicationProhibited', RecordsGrantAuthorizationConditionPublicationRequired = 'RecordsGrantAuthorizationConditionPublicationRequired', RecordsGrantAuthorizationScopeContextIdMismatch = 'RecordsGrantAuthorizationScopeContextIdMismatch', @@ -131,5 +131,4 @@ export enum DwnErrorCode { UrlProtocolNotNormalizable = 'UrlProtocolNotNormalizable', UrlSchemaNotNormalized = 'UrlSchemaNotNormalized', UrlSchemaNotNormalizable = 'UrlSchemaNotNormalizable', - VerifierValidPublicKeyNotFound = 'VerifierValidPublicKeyNotFound', }; diff --git a/src/jose/jws/general/verifier.ts b/src/jose/jws/general/verifier.ts index b6acc371c..c0e5d56a1 100644 --- a/src/jose/jws/general/verifier.ts +++ b/src/jose/jws/general/verifier.ts @@ -13,29 +13,53 @@ type VerificationResult = { signers: string[]; }; +/** + * Verifies the signature(s) of a General JWS. + */ export class GeneralJwsVerifier { - jws: GeneralJws; + + private static _singleton: GeneralJwsVerifier; + cache: Cache; - constructor(jws: GeneralJws, cache?: Cache) { - this.jws = jws; + private constructor(cache?: Cache) { this.cache = cache || new MemoryCache(600); } - async verify(didResolver: DidResolver): Promise { + private static get singleton(): GeneralJwsVerifier { + if (GeneralJwsVerifier._singleton === undefined) { + GeneralJwsVerifier._singleton = new GeneralJwsVerifier(); + } + + return GeneralJwsVerifier._singleton; + } + + /** + * Verifies the signatures of the given General JWS. + * @returns the list of signers that have valid signatures. + */ + public static async verifySignatures(jws: GeneralJws, didResolver: DidResolver): Promise { + return await GeneralJwsVerifier.singleton.verifySignatures(jws, didResolver); + } + + /** + * Verifies the signatures of the given General JWS. + * @returns the list of signers that have valid signatures. + */ + public async verifySignatures(jws: GeneralJws, didResolver: DidResolver): Promise { const signers: string[] = []; - for (const signatureEntry of this.jws.signatures) { + for (const signatureEntry of jws.signatures) { let isVerified: boolean; - const cacheKey = `${signatureEntry.protected}.${this.jws.payload}.${signatureEntry.signature}`; const kid = Jws.getKid(signatureEntry); - const publicJwk = await GeneralJwsVerifier.getPublicKey(kid, didResolver); + const cacheKey = `${signatureEntry.protected}.${jws.payload}.${signatureEntry.signature}`; const cachedValue = await this.cache.get(cacheKey); - // explicit strict equality check to avoid potential buggy cache implementation causing incorrect truthy compare e.g. "false" + // explicit `undefined` check to differentiate `false` if (cachedValue === undefined) { - isVerified = await Jws.verifySignature(this.jws.payload, signatureEntry, publicJwk); + const publicJwk = await GeneralJwsVerifier.getPublicKey(kid, didResolver); + isVerified = await Jws.verifySignature(jws.payload, signatureEntry, publicJwk); await this.cache.set(cacheKey, isVerified); } else { isVerified = cachedValue; @@ -54,9 +78,9 @@ export class GeneralJwsVerifier { } /** - * Gets the public key given a fully qualified key ID (`kid`). + * Gets the public key given a fully qualified key ID (`kid`) by resolving the DID to its DID Document. */ - public static async getPublicKey(kid: string, didResolver: DidResolver): Promise { + private static async getPublicKey(kid: string, didResolver: DidResolver): Promise { // `resolve` throws exception if DID is invalid, DID method is not supported, // or resolving DID fails const did = Jws.extractDid(kid); @@ -65,18 +89,18 @@ export class GeneralJwsVerifier { let verificationMethod: VerificationMethod | undefined; - for (const vm of verificationMethods) { + for (const method of verificationMethods) { // consider optimizing using a set for O(1) lookups if needed // key ID in DID Document may or may not be fully qualified. e.g. // `did:ion:alice#key1` or `#key1` - if (kid.endsWith(vm.id)) { - verificationMethod = vm; + if (kid.endsWith(method.id)) { + verificationMethod = method; break; } } if (!verificationMethod) { - throw new DwnError(DwnErrorCode.VerifierValidPublicKeyNotFound, 'public key needed to verify signature not found in DID Document'); + throw new DwnError(DwnErrorCode.GeneralJwsVerifierGetPublicKeyNotFound, 'public key needed to verify signature not found in DID Document'); } validateJsonSchema('JwkVerificationMethod', verificationMethod); diff --git a/src/utils/jws.ts b/src/utils/jws.ts index fc44ed154..0214e8b2f 100644 --- a/src/utils/jws.ts +++ b/src/utils/jws.ts @@ -8,7 +8,7 @@ import isPlainObject from 'lodash/isPlainObject.js'; import { Encoder } from './encoder.js'; import { PrivateKeySigner } from './private-key-signer.js'; import { signatureAlgorithms } from '../jose/algorithms/signing/signature-algorithms.js'; -import { DwnError, DwnErrorCode } from '../index.js'; +import { DwnError, DwnErrorCode } from '../core/dwn-error.js'; /** diff --git a/tests/jose/jws/general.spec.ts b/tests/jose/jws/general.spec.ts index b642e7c24..9c397e33b 100644 --- a/tests/jose/jws/general.spec.ts +++ b/tests/jose/jws/general.spec.ts @@ -46,10 +46,7 @@ describe('General JWS Sign/Verify', () => { resolve: sinon.stub().withArgs('did:jank:alice').resolves(mockResolutionResult) }); - const verifier = new GeneralJwsVerifier(jws); - - const verificationResult = await verifier.verify(resolverStub); - + const verificationResult = await GeneralJwsVerifier.verifySignatures(jws, resolverStub); expect(verificationResult.signers.length).to.equal(1); expect(verificationResult.signers).to.include('did:jank:alice'); }); @@ -80,12 +77,9 @@ describe('General JWS Sign/Verify', () => { resolve: sinon.stub().withArgs('did:jank:alice').resolves(mockResolutionResult) }); - const verifier = new GeneralJwsVerifier(jws); - - const verificatonResult = await verifier.verify(resolverStub); - - expect(verificatonResult.signers.length).to.equal(1); - expect(verificatonResult.signers).to.include('did:jank:alice'); + const verificationResult = await GeneralJwsVerifier.verifySignatures(jws, resolverStub); + expect(verificationResult.signers.length).to.equal(1); + expect(verificationResult.signers).to.include('did:jank:alice'); }); it('should support multiple signatures using different key types', async () => { @@ -148,15 +142,16 @@ describe('General JWS Sign/Verify', () => { resolve: resolveStub }); - const verifier = new GeneralJwsVerifier(jws); - const verificationResult = await verifier.verify(resolverStub); - + const verificationResult = await GeneralJwsVerifier.verifySignatures(jws, resolverStub); expect(verificationResult.signers.length).to.equal(2); expect(verificationResult.signers).to.include(alice.did); expect(verificationResult.signers).to.include(bob.did); }); it('should not verify the same signature more than once', async () => { + // scenario: include two signatures in the JWS, + // repeated calls to verifySignature should only verify each of the signature once, + // resulting total of 2 calls to `Jws.verifySignature()` and 2 calls to cache the results. const { privateJwk: privateJwkEd25519, publicJwk: publicJwkEd25519 } = await Ed25519.generateKeyPair(); const { privateJwk: privateJwkSecp256k1, publicJwk: publicJwkSecp256k1 } = await secp256k1.generateKeyPair(); const payloadBytes = new TextEncoder().encode('anyPayloadValue'); @@ -195,16 +190,16 @@ describe('General JWS Sign/Verify', () => { resolve: sinon.stub().withArgs('did:jank:alice').resolves(mockResolutionResult) }); - const verifier = new GeneralJwsVerifier(jws); - const verifySignatureSpy = sinon.spy(Jws, 'verifySignature'); - const cacheSetSpy = sinon.spy(verifier.cache, 'set'); + const cacheSetSpy = sinon.spy((GeneralJwsVerifier as any).singleton.cache, 'set'); - await verifier.verify(resolverStub); - await verifier.verify(resolverStub); + // intentionally calling verifySignatures() multiple times on the same JWS + await GeneralJwsVerifier.verifySignatures(jws, resolverStub); + await GeneralJwsVerifier.verifySignatures(jws, resolverStub); + await GeneralJwsVerifier.verifySignatures(jws, resolverStub); - sinon.assert.calledTwice(cacheSetSpy); sinon.assert.calledTwice(verifySignatureSpy); + sinon.assert.calledTwice(cacheSetSpy); }); }); diff --git a/tests/jose/jws/verifier.spec.ts b/tests/jose/jws/verifier.spec.ts deleted file mode 100644 index 082610bcd..000000000 --- a/tests/jose/jws/verifier.spec.ts +++ /dev/null @@ -1,8 +0,0 @@ -describe('GeneralJwsVerifier', () => { - describe('getPublicKey', () => { - xit('throws an exception if publicKeyJwk isn\'t present in verificationMethod', () => {}); - xit('throws an exception if DID could not be resolved', () => {}); - xit('throws an exception if appropriate key isnt present in DID Doc', () => {}); - xit('throws an exception if verificationMethod type isn\'t JsonWebKey2020', () => {}); - }); -}); \ No newline at end of file