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 all commits
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
66 changes: 62 additions & 4 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,10 @@ class _TestCase {
// Parameters for key import (always required)
final Map<String, dynamic>? importKeyParams;

// If not `null`, then importing private or public key MUST throw
// an exception that contains this string.
final String? importKeyException;

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

Expand Down Expand Up @@ -90,6 +94,7 @@ class _TestCase {
this.signVerifyParams,
this.encryptDecryptParams,
this.deriveParams,
this.importKeyException,
});

factory _TestCase.fromJson(Map json) {
Expand All @@ -110,6 +115,7 @@ class _TestCase {
derivedBits: _optionalBase64Decode(json['derivedBits']),
derivedLength: json['derivedLength'] as int?,
importKeyParams: _optionalStringMapDecode(json['importKeyParams']),
importKeyException: json['importKeyException'] as String?,
signVerifyParams: _optionalStringMapDecode(json['signVerifyParams']),
encryptDecryptParams:
_optionalStringMapDecode(json['encryptDecryptParams']),
Expand Down Expand Up @@ -691,7 +697,14 @@ void _validateTestCase<PrivateKey, PublicKey>(
check(c.importKeyParams != null);
check((c.signVerifyParams != null) == (r._signBytes != null));
check((c.encryptDecryptParams != null) == (r._encryptBytes != null));
check((c.deriveParams != null) == (r._deriveBits != null));

if (c.deriveParams != null) {
check((c.deriveParams != null) == (r._deriveBits != null));

if (r._deriveBits != null) {
check(c.derivedLength != null);
}
}
Comment on lines -694 to +707
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 unrelated to this change, if so let's make it a different PR

if (c.signature != null) {
check(r._signBytes != null);
}
Expand All @@ -701,9 +714,6 @@ void _validateTestCase<PrivateKey, PublicKey>(
if (c.derivedBits != null) {
check(r._deriveBits != null);
}
if (r._deriveBits != null) {
check(c.derivedLength != null);
}

// Check that data matches the methods we have in the runner.
check(r._importPrivateRawKey != null || c.privateRawKeyData == null);
Expand All @@ -712,6 +722,30 @@ void _validateTestCase<PrivateKey, PublicKey>(
check(r._importPublicRawKey != null || c.publicRawKeyData == null);
check(r._importPublicSpkiKey != null || c.publicSpkiKeyData == null);
check(r._importPublicJsonWebKey != null || c.publicJsonWebKeyData == null);

if (c.generateKeyParams != null) {
check(c.importKeyException == null,
'importKeyException must be null when generateKeyParams is provided');
}

if (c.importKeyException != null) {
check(c.plaintext == null,
'plaintext must be null when importKeyException is provided');
check(c.signature == null,
'signature must be null when importKeyException is provided');
check(c.ciphertext == null,
'ciphertext must be null when importKeyException is provided');
check(c.derivedBits == null,
'derivedBits must be null when importKeyException is provided');
check(c.derivedLength == null,
'derivedLength must be null when importKeyException is provided');
check(c.signVerifyParams == null,
'signVerifyParams must be null when importKeyException is provided');
check(c.encryptDecryptParams == null,
'encryptDecryptParams must be null when importKeyException is provided');
check(c.deriveParams == null,
'deriveParams must be null when importKeyException is provided');
}
}

void _runTests<PrivateKey, PublicKey>(
Expand Down Expand Up @@ -746,6 +780,30 @@ void _runTests<PrivateKey, PublicKey>(
publicKey = pair.publicKey;
privateKey = pair.privateKey;
});
} else if (c.importKeyException != null) {
if (c.privatePkcs8KeyData != null) {
test('pkcs8 import exception', () async {
try {
await r._importPrivatePkcs8Key!(
c.privatePkcs8KeyData!, {'curve': 'p-521'});
check(false, 'Expected an exception for P-512 import');
} catch (e) {
check(e.toString().contains(c.importKeyException!));
}
});
}
if (c.privateJsonWebKeyData != null) {
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.importKeyException!));
}
});
}
Comment on lines +784 to +805
Copy link
Member

Choose a reason for hiding this comment

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

We need a test case for each kind of key possible:

  final List<int>? privateRawKeyData;
  final List<int>? privatePkcs8KeyData;
  final Map<String, dynamic>? privateJsonWebKeyData;
  final List<int>? publicRawKeyData;
  final List<int>? publicSpkiKeyData;
  final Map<String, dynamic>? publicJsonWebKeyData;

Also I'd suggest naming the test cases like:

if (c.publicRawKeyData != null) {
  test('importPublicRawKey() throws', () async {
    ...
  });
}
if (c.publicJsonWebKeyData != null) {
  test('importPublicJsonWebKey() throws', () async {
    ...
  });
}
...

// This test case is testing failure to import a key
// further tests will not be conducted, since they don't
// make sense after failing to import a key.
return;

return;
} else {
test('import key-pair', () async {
// Get a privateKey
Expand Down
33 changes: 33 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,39 @@ final _testData = [
"importKeyParams": {"curve": "p-256"},
"deriveParams": {}
},
{
"name": "generated on boringssl/linux (pkcs8 import key exception) at 2020-01-22T23:24:34",
Copy link
Member

Choose a reason for hiding this comment

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

I think we can do a better "name" here.

For exceptions, we shouldn't really care where the test case was generated.
Because it's NOT valid, you probably did something to it to make it invalid! 🤣

So better make the test case name say: "import incorrect curve" or something like that.

"privatePkcs8KeyData":
"MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQg3aTiZ7odKAODYk4BpZlzulBCB/BptmxjtvrzyXI71UyhRANCAATl0GVa8O1sXXf2NV5qGJ/9/Vq8PVWCZuezADa1F0Vr2TaB8BseZIW+rhmEmLC2FfCdxj9NmLp00SilRTm40Hxm",
"publicRawKeyData":
"BHiIXxrwhM92v4ueDrj3x1JJY4uS+II/IJPjqMvaKj/QfoOllnEkrnaOW1owBYRBMnP0pPouPkqbVfPACMUsfKs=",
"publicSpkiKeyData":
"MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEeIhfGvCEz3a/i54OuPfHUklji5L4gj8gk+Ooy9oqP9B+g6WWcSSudo5bWjAFhEEyc/Sk+i4+SptV88AIxSx8qw==",
Comment on lines +116 to +121
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 change the _validateTestCase further.

Right now a test case is require to have one of:

  • import key parameters,
  • if symmetric has private key (and not public key)
  • if not symmetric has both:
    • a private key, and,
    • a public key.

We should want that if importKeyException then it is not necessary to have both private and public key.

Because, in some cases you may want to check an exception that can only be produced when trying to import a private key (or trying to import a public key).

Example: trying to import a public key will never complain about d being missing from the JWT.

"importKeyParams": {"curve": "p-256"},
"importKeyException": "FormatException: incorrect elliptic curve"
},
{
"name": "generated on boringssl/linux (jwk import key exception) at 2020-01-22T23:24:34",
"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"
},
"importKeyParams": {"curve": "p-256"},
"importKeyException": "JWK property \"crv\" is not"
},
{
"name": "generated on chrome/linux at 2020-01-22T23:24:39",
"privatePkcs8KeyData":
Expand Down
Loading