Crypto signature validation, Test fixes

[inmyth]

Added digital signature validation based on tcn-node exposed validation method. (covidwatchorg/tcn-node#5)

• added the implementation and its unit test
• adjusted response to allow custom error message (previously pretty error came from spakson lib)
• fixed Firebase rules that gave non-auth user write access The test allows anyone to write into database #5

[madhavajay]

This is really awesome! Thanks for all the hard work here.
Regarding the tests, I use the firestore.rules.insecure so that its easy to load those open rules write data for mocks during tests and then re-enable the real security. Did you find a different way to do this during tests?

Secondly, the firestore rules are currently wrong for a few reasons so writing tests against the wrong rules isnt a good idea.

Currently they are open because we didn't have a REST endpoint before but now that we have one were going to deprecate open writes. This means we need to add some rules which will look like this:

function isAdmin() {
    return get(/databases/$(database)/documents/users/$(request.auth.uid)).data.isAdmin;
}    

match /{document=**} {
    allow read, write: if isAdmin();
}

match /signed_reports/{document=**} {
    allow read: if true;
    allow write: if isAdmin();
}

If you are able to make these changes I can merge otherwise it'll have to wait a bit before I get a chance to jump in here and fix these small issues.

[inmyth]

Ok I see better now. The isAdmin() clarifies it.
So I'm looking at this article which resembles the previous setup. If I interpret it, our app doesn't have user personal pages or allow non-admin to write, we might as well write mock data as admin as it reflects production. That's why I deleted the insecure rules.
I could expand the mock test to accommodate admin access. I can try something today, we can look into it later if it's not up to spec I can revert it no problem.

EDIT:
Wait I think I know why we need the .insecure.rules. It's because we first need to populate the db with admin user.

[inmyth]

I updated the tests, firebase.rules, brought back the insecure.rules. The current rules would be to allow admin full read and write access on all docs (minus deletion), and to allow non-admin only read access on signed_reports.

[miridius]

Wouldn't it be easier to just do a Map lookup instead of making a switch statement?

Also I think unknown values should error. Setting them to Reserved (0xff) would mean we are changing the memo type value which means the signature would fail but that would be quite confusing. It's better to fail early.

Suggested change

	function enumMemoTypeOf(memoType: number): string {
	let res: string;
	switch(memoType) {
	case 0: {
	res = 'CoEpiV1';
	break;
	}
	case 1: {
	res = 'CovidWatchV1';
	break;
	}
	default: {
	res = 'Reserved';
	break;
	}
	};
	return res;
	}
	const MEMO_TYPES = new Map([
	[0, "CoEpiV1"],
	[1, "CovidWatchV1"],
	[0xff, "Reserved"],
	]);
	function enumMemoTypeOf(memoType: number): string {
	const res = MEMO_TYPES.get(memoType);
	if (res === undefined) {
	throw new Error(
	`Unknown memo type: ${memoType}. Valid values: ${[...MEMO_TYPES.keys()]}`
	);
	}
	return res;
	}

[inmyth]

The thing is all values from 1 to 255 are valid but currently unassigned. What do you think @madhavajay ? If we stick to TCN implementation we will strictly reject any values outside 0,1 and 255 and we can do this suggestion. Or should we do whichever for the current version and later update it ?

Btw, regardless of which we still should expose the enum from tcn-node.

[miridius]

Currently the Rust TCN implementation will not accept any values other than the ones in its Enum (0, 1, or 0xff) so we can't support other values anyway. But I will extend the Rust code to allow an "Other" option containing any u8 value (0 - 255) to make it future proof.

Yes I agree we should expose the enum from tcn-node, that will be done in 0.4.0. I'm not sure if you want to wait for that release before finishing this PR though as it will also change the API.

[madhavajay]

Yeah I had this exact issue in another project, thats the annoying thing about ints they dont really have good sentinels for invalid and the classic solution has been -1 but then thats a signed int so yeah... nightmare. I think all 255 enums should use the final position as BAD by default so 255 would be incorrect. Anyway, for now we can just block anything but 1 & 2 if the tcn lib doesnt allow it.

[madhavajay]

Great work @inmyth, i'm going to merge this in because I have another v2 branch that I want to rebase on this ASAP. Ill add you both to the repo so you can make changes easily in future.