Skip to content

Add tests for Import Key Exception #135

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add tests for Import Key Exception #135

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9a57b44
test: add tests for importKeyException
HamdaanAliQuatil Jun 16, 2024
440bcde
test: add validation for importKeyException
HamdaanAliQuatil Jul 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 51 additions & 13 deletions lib/src/testing/utils/testrunner.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ class _TestCase {
// Parameters for key import (always required)
final Map<String, dynamic>? importKeyParams;

// Parameters for import key exception (optional, if there is an exception)
final Map<String, dynamic>? importKeyException;
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this simpler.

Suggested change
// Parameters for import key exception (optional, if there is an exception)
final Map<String, dynamic>? importKeyException;
// If not `null`, then importing private or public key MUST throw
// an exception that contains this string.
final String? importKeyException;

Instead of making a test case that contains different strings for pkcs8Exception and jwkException, we just make two test cases. One that tests the exception when importing a PKCS8 key and on that tests it when importing a JWK key.

Notice that when specifying a test case, you don't have to specify both privatePkcs8KeyData and privateJsonWebKeyData, it's enough to specify one of them. So if we want to test different error messages (from different formats), we really should just make two test cases.

That way, we also avoid having an absolute explosion of properties on the test case objects.

Copy link
Member

Choose a reason for hiding this comment

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

We also need to update _validateTestCase

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated with one string importKeyException. Made necessary changes to _validateTestCase


// Parameters for sign/verify (required, if there is a signature)
final Map<String, dynamic>? signVerifyParams;

Expand All @@ -72,6 +75,20 @@ class _TestCase {
// Parameters for deriveBits (required, if there is a derivedBits)
final Map<String, dynamic>? deriveParams;

String? get pkcs8ImportKeyException {
if (importKeyException != null) {
return importKeyException?['pkcs8Exception'];
}
return null;
}

String? get jwkImportKeyException {
if (importKeyException != null) {
return importKeyException?['jwkException'];
}
return null;
}

_TestCase(
this.name, {
this.generateKeyParams,
Expand All @@ -90,6 +107,7 @@ class _TestCase {
this.signVerifyParams,
this.encryptDecryptParams,
this.deriveParams,
this.importKeyException,
});

factory _TestCase.fromJson(Map json) {
Expand All @@ -110,6 +128,7 @@ class _TestCase {
derivedBits: _optionalBase64Decode(json['derivedBits']),
derivedLength: json['derivedLength'] as int?,
importKeyParams: _optionalStringMapDecode(json['importKeyParams']),
importKeyException: _optionalStringMapDecode(json['importKeyException']),
signVerifyParams: _optionalStringMapDecode(json['signVerifyParams']),
encryptDecryptParams:
_optionalStringMapDecode(json['encryptDecryptParams']),
Expand Down Expand Up @@ -746,6 +765,23 @@ void _runTests<PrivateKey, PublicKey>(
publicKey = pair.publicKey;
privateKey = pair.privateKey;
});
} else if (c.importKeyException != null) {
test('pkcs8 import exception', () async {
try {
Copy link
Member

Choose a reason for hiding this comment

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

formatting of code is weird here. Please use dart format you can enable it in vscode with the Dart extension. I think it's even on by default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tried dart format, but nothing seemed to change on this line. A few other parts were formatted though.

await r._importPrivatePkcs8Key!(c.privatePkcs8KeyData!, {'curve': 'p-521'});
check(false, 'Expected an exception for P-512 import');
} catch (e) {
check(e.toString().contains(c.pkcs8ImportKeyException!));
}
});
test('jwk import exception', () async {
try {
await r._importPrivateJsonWebKey!(c.privateJsonWebKeyData!, {'curve': 'p-521'});
check(false, 'Expected an exception for P-512 import');
} catch (e) {
check(e.toString().contains(c.jwkImportKeyException!));
}
});
Copy link
Member

Choose a reason for hiding this comment

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

If importKeyException is not null, then we can test that importing public and private keys fail, but all the other tests we can't do, right?

So maybe it's fine to just do a return; here? That way we don't have to add && c.pkcs8ImportKeyException == null to all the other test cases.

Copy link
Member

Choose a reason for hiding this comment

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

We should also when doing this, make sure that in _validateTestCase we validate that the test case doesn't have values that won't be used.

In particular we need to validate that:

  • if generateKeyParams is not null, then importKeyException must be null!
  • if importKeyException is not null, then
    • All the other properties that won't be used must be null.
      • This includes plaintext, signature, ciphertext, derivedBits, derivedLength and more.
        (If the tests don't use the values, they MUST not be specified, otherwise the test case will be hard to read and understand)
      • Probably also includes signVerifyParams, etc. since again, we can't use them for anything, so they shouldn't be specified.
    • At-least one of:
      • publicRawKeyData, publicSpkiKeyData, publicJsonWebKeyData, privateRawKeyData, privatePkcs8KeyData, or privateJsonWebKeyData must be specified.
      • Note: that we probably don't need all of them to be specified.
      • Note: it's probably okay if there is no private key or no public key, just so long as there is at-least ONE of these properties provided. Normally, when importKeyException == null, this is ofcourse not allowed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a return; in this section. Removed recurring && c.pkcs8ImportKeyException == null
Updated _validateTestCase with the above checks

} else {
test('import key-pair', () async {
// Get a privateKey
Expand Down Expand Up @@ -876,7 +912,8 @@ void _runTests<PrivateKey, PublicKey>(
});
}

test('validated derivedBits', () async {
if (c.pkcs8ImportKeyException == null){
test('validated derivedBits', () async {
final derived = await r._deriveBits(
_KeyPair(
privateKey: privateKey as PrivateKey,
Expand All @@ -889,6 +926,7 @@ void _runTests<PrivateKey, PublicKey>(
'failed to derivedBits are not consistent',
);
});
}
}

//------------------------------ Utilities for testing
Expand Down Expand Up @@ -1021,7 +1059,7 @@ void _runTests<PrivateKey, PublicKey>(

//------------------------------ Test import public key

if (c.publicRawKeyData != null) {
if (c.publicRawKeyData != null && c.pkcs8ImportKeyException == null) {
assert(!r._isSymmetric && r._importPublicRawKey != null);

test('importPublicRawKey()', () async {
Expand All @@ -1033,7 +1071,7 @@ void _runTests<PrivateKey, PublicKey>(
});
}

if (c.publicSpkiKeyData != null) {
if (c.publicSpkiKeyData != null && c.pkcs8ImportKeyException == null) {
assert(!r._isSymmetric && r._importPublicSpkiKey != null);

test('importPublicSpkiKey()', () async {
Expand All @@ -1045,7 +1083,7 @@ void _runTests<PrivateKey, PublicKey>(
});
}

if (c.publicJsonWebKeyData != null) {
if (c.publicJsonWebKeyData != null && c.jwkImportKeyException == null) {
assert(!r._isSymmetric && r._importPublicJsonWebKey != null);

test('importPublicJsonWebKey()', () async {
Expand All @@ -1069,7 +1107,7 @@ void _runTests<PrivateKey, PublicKey>(
});
}

if (c.privatePkcs8KeyData != null) {
if (c.privatePkcs8KeyData != null && c.pkcs8ImportKeyException == null) {
test('importPrivatePkcs8Key()', () async {
final key = await r._importPrivatePkcs8Key!(
c.privatePkcs8KeyData!,
Expand All @@ -1079,7 +1117,7 @@ void _runTests<PrivateKey, PublicKey>(
});
}

if (c.privateJsonWebKeyData != null) {
if (c.privateJsonWebKeyData != null && c.jwkImportKeyException == null) {
test('importPrivateJsonWebKey()', () async {
final key = await r._importPrivateJsonWebKey!(
c.privateJsonWebKeyData!,
Expand Down Expand Up @@ -1360,7 +1398,7 @@ void _runTests<PrivateKey, PublicKey>(
}

//------------------------------ Test derivedBits
if (r._deriveBits != null) {
if (r._deriveBits != null && (c.pkcs8ImportKeyException == null || c.jwkImportKeyException == null)) {
test('deriveBits', () async {
final derived = await r._deriveBits(
_KeyPair(
Expand All @@ -1374,7 +1412,7 @@ void _runTests<PrivateKey, PublicKey>(
}

//------------------------------ export/import private key
if (r._exportPrivateRawKey != null) {
if (r._exportPrivateRawKey != null && (c.pkcs8ImportKeyException == null || c.jwkImportKeyException == null)) {
test('export/import raw private key', () async {
final keyData = await r._exportPrivateRawKey(privateKey as PrivateKey);
check(keyData.isNotEmpty, 'exported key is empty');
Expand All @@ -1384,7 +1422,7 @@ void _runTests<PrivateKey, PublicKey>(
});
}

if (r._exportPrivatePkcs8Key != null) {
if (r._exportPrivatePkcs8Key != null && c.pkcs8ImportKeyException == null) {
test('export/import pkcs8 private key', () async {
final keyData = await r._exportPrivatePkcs8Key(privateKey as PrivateKey);
check(keyData.isNotEmpty, 'exported key is empty');
Expand All @@ -1394,7 +1432,7 @@ void _runTests<PrivateKey, PublicKey>(
});
}

if (r._exportPrivateJsonWebKey != null) {
if (r._exportPrivateJsonWebKey != null && c.jwkImportKeyException == null) {
test('export/import jwk private key', () async {
final jwk = await r._exportPrivateJsonWebKey(privateKey as PrivateKey);
check(jwk.isNotEmpty, 'exported key is empty');
Expand All @@ -1406,7 +1444,7 @@ void _runTests<PrivateKey, PublicKey>(

//------------------------------ export/import public key

if (r._exportPublicRawKey != null) {
if (r._exportPublicRawKey != null && (c.pkcs8ImportKeyException == null || c.jwkImportKeyException == null)) {
assert(!r._isSymmetric && r._importPublicRawKey != null);

test('export/import raw public key', () async {
Expand All @@ -1418,7 +1456,7 @@ void _runTests<PrivateKey, PublicKey>(
});
}

if (r._exportPublicSpkiKey != null) {
if (r._exportPublicSpkiKey != null && c.pkcs8ImportKeyException == null) {
assert(!r._isSymmetric && r._importPublicSpkiKey != null);

test('export/import pkcs8 public key', () async {
Expand All @@ -1430,7 +1468,7 @@ void _runTests<PrivateKey, PublicKey>(
});
}

if (r._exportPublicJsonWebKey != null) {
if (r._exportPublicJsonWebKey != null && c.jwkImportKeyException == null) {
assert(!r._isSymmetric && r._importPublicJsonWebKey != null);

test('export/import jwk public key', () async {
Expand Down
30 changes: 30 additions & 0 deletions lib/src/testing/webcrypto/ecdh.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,36 @@ final _testData = [
"importKeyParams": {"curve": "p-256"},
"deriveParams": {}
},
{
"name": "generated on boringssl/linux (import key exception) at 2020-01-22T23:24:34",
"privatePkcs8KeyData":
"MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQg3aTiZ7odKAODYk4BpZlzulBCB/BptmxjtvrzyXI71UyhRANCAATl0GVa8O1sXXf2NV5qGJ/9/Vq8PVWCZuezADa1F0Vr2TaB8BseZIW+rhmEmLC2FfCdxj9NmLp00SilRTm40Hxm",
"privateJsonWebKeyData": {
"kty": "EC",
"crv": "P-256",
"x": "5dBlWvDtbF139jVeahif_f1avD1VgmbnswA2tRdFa9k",
"y": "NoHwGx5khb6uGYSYsLYV8J3GP02YunTRKKVFObjQfGY",
"d": "3aTiZ7odKAODYk4BpZlzulBCB_BptmxjtvrzyXI71Uw"
},
"publicRawKeyData":
"BHiIXxrwhM92v4ueDrj3x1JJY4uS+II/IJPjqMvaKj/QfoOllnEkrnaOW1owBYRBMnP0pPouPkqbVfPACMUsfKs=",
"publicSpkiKeyData":
"MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEeIhfGvCEz3a/i54OuPfHUklji5L4gj8gk+Ooy9oqP9B+g6WWcSSudo5bWjAFhEEyc/Sk+i4+SptV88AIxSx8qw==",
"publicJsonWebKeyData": {
"kty": "EC",
"crv": "P-256",
Copy link
Member

Choose a reason for hiding this comment

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

This curve doesn't look wrong?

"x": "eIhfGvCEz3a_i54OuPfHUklji5L4gj8gk-Ooy9oqP9A",
"y": "foOllnEkrnaOW1owBYRBMnP0pPouPkqbVfPACMUsfKs"
},
"derivedBits": "WA==",
"derivedLength": 7,
"importKeyParams": {"curve": "p-256"},
"importKeyException": {
"pkcs8Exception": "FormatException: incorrect elliptic curve",
"jwkException": "JWK property \"crv\" is not"
},
"deriveParams": {}
},
{
"name": "generated on chrome/linux at 2020-01-22T23:24:39",
"privatePkcs8KeyData":
Expand Down