Skip to content

Commit

Permalink
Fixed bug on usage of GeneralJwsVerifier and improved its usability…
Browse files Browse the repository at this point in the history 
… & 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
  • Loading branch information
@thehenrytsai
thehenrytsai authored Nov 3, 2023
1 parent fcf1af4 commit af4d3f1
Show file tree
Hide file tree
Showing 6 changed files with 60 additions and 53 deletions.
11 changes: 4 additions & 7 deletions src/core/auth.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
}

Expand Down
5 changes: 2 additions & 3 deletions src/core/dwn-error.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -25,6 +25,7 @@ export enum DwnErrorCode {
DidNotValid = 'DidNotValid',
DidResolutionFailed = 'DidResolutionFailed',
Ed25519InvalidJwk = 'Ed25519InvalidJwk',
GeneralJwsVerifierGetPublicKeyNotFound = 'GeneralJwsVerifierGetPublicKeyNotFound',
GeneralJwsVerifierInvalidSignature = 'GeneralJwsVerifierInvalidSignature',
GrantAuthorizationGrantExpired = 'GrantAuthorizationGrantExpired',
GrantAuthorizationGrantMissing = 'GrantAuthorizationGrantMissing',
Expand Down Expand Up @@ -76,7 +77,6 @@ export enum DwnErrorCode {
ProtocolsQueryUnauthorized = 'ProtocolsQueryUnauthorized',
RecordsDecryptNoMatchingKeyEncryptedFound = 'RecordsDecryptNoMatchingKeyEncryptedFound',
RecordsDeleteAuthorizationFailed = 'RecordsDeleteAuthorizationFailed',

RecordsGrantAuthorizationConditionPublicationProhibited = 'RecordsGrantAuthorizationConditionPublicationProhibited',
RecordsGrantAuthorizationConditionPublicationRequired = 'RecordsGrantAuthorizationConditionPublicationRequired',
RecordsGrantAuthorizationScopeContextIdMismatch = 'RecordsGrantAuthorizationScopeContextIdMismatch',
Expand Down Expand Up @@ -131,5 +131,4 @@ export enum DwnErrorCode {
UrlProtocolNotNormalizable = 'UrlProtocolNotNormalizable',
UrlSchemaNotNormalized = 'UrlSchemaNotNormalized',
UrlSchemaNotNormalizable = 'UrlSchemaNotNormalizable',
VerifierValidPublicKeyNotFound = 'VerifierValidPublicKeyNotFound',
};
54 changes: 39 additions & 15 deletions src/jose/jws/general/verifier.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<VerificationResult> {
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<VerificationResult> {
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<VerificationResult> {
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;
Expand All @@ -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<PublicJwk> {
private static async getPublicKey(kid: string, didResolver: DidResolver): Promise<PublicJwk> {
// `resolve` throws exception if DID is invalid, DID method is not supported,
// or resolving DID fails
const did = Jws.extractDid(kid);
Expand All @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/utils/jws.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';


/**
Expand Down
33 changes: 14 additions & 19 deletions tests/jose/jws/general.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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);
});

});
8 changes: 0 additions & 8 deletions tests/jose/jws/verifier.spec.ts
View file

This file was deleted.

0 comments on commit af4d3f1

Please sign in to comment.