Skip to content
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.

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

Fix test case descriptions #270

Conversation

gilles-peskine-arm
Copy link
Collaborator

Remove ; from test case descriptions and give a unique description to each test case within each test suite. This is necessary for Mbed-TLS/mbedtls#2843. Fix #262.

To give unique descriptions, I investigated the places where multiple test cases had exactly the same description and test data:

for x in tests/suites/*.data; do perl -00 -ne 'warn "$ARGV: $. = $seen{$_}\n" if $seen{$_}; $seen{$_}=$.' $x; done

I did not investigate the ~1400 instances of test cases with the same description but different test data, I just gave them a unique suffix [#1], [#2], etc. Manual investigation is out of scope of this PR. We'll be able to find these mechanically-added suffixes later by searching for [# which was not present before.

Needs to be backported to mbedtls LTS branches if we backport Mbed-TLS/mbedtls#2843. If we don't, it may still be a good idea to backport at least the applicable fixes to duplicate test data.

Don't use semicolons in test case descriptions. The test outcome file
is a semicolon-separated CSV file without quotes to keep things
simple, so fields in that file may not contain semicolons.
@gilles-peskine-arm gilles-peskine-arm added bug Something isn't working needs: review The pull request is ready for review. This generally means that it has no known issues. labels Sep 19, 2019
@AndrzejKurek AndrzejKurek self-requested a review September 20, 2019 06:22
@@ -13,16 +13,16 @@ camellia_encrypt_ecb:"0123456789abcdeffedcba98765432100011223344556677":"0123456
Camellia-256-ECB Encrypt RFC3713 #1
camellia_encrypt_ecb:"0123456789abcdeffedcba987654321000112233445566778899aabbccddeeff":"0123456789abcdeffedcba9876543210":"9acc237dff16d76c20ef7c919e3a7509":0

Camellia-128-ECB Encrypt Perl EVP #1
Camellia-128-ECB Encrypt Perl EVP #1 [#1]
Copy link
Contributor

Choose a reason for hiding this comment

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

These seem to be a bit unnecessary. Please unify them so that all use the same [#n] format.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rejected as out of scope. The existing numbers are sometimes significant because they match numbers in a reference document. There are ~1400 duplicates and I don't have time to manually review them all.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can understand that we need unique descriptions, but having, for example:

when it should rather:

  • ARC4 Encrypt and decrypt 22 bytes in multiple parts
  • ARC4 Encrypt and decrypt 23 bytes in multiple parts

makes a big difference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I fully agree that this is a bug, and thank you for doing the analysis. But it is out of scope of the current pull request, which solely aims to make test descriptions unique. I've filed #271 to make these test descriptions correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

solely aims to make test descriptions unique.

You did fix a few things on accident. Collateral damage. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I decided that when two test cases had the same test data and the same test description, that absolutely had to be a mistake, which could either be that the same paragraph was copy-pasted more than needed or that the paragraph was copy-pasted but not edited when it should have been, meaning that the test data is not what was intended and we have missing coverage. The number of such cases was manageable so I went through them. The other cases are identical descriptions for differing test data, and while the test data may be wrong sometimes, in most cases I think it's only the description that's wrong.

depends_on:MBEDTLS_ARC4_C
enc_dec_buf:MBEDTLS_CIPHER_ARC4_128:"ARC4-128":128:32:-1

ARC4 Encrypt and decrypt 32 bytes
ARC4 Encrypt and decrypt 32 bytes [#2]
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong description - 33 bytes. Following - please remove the [#1] from the test above and [#2] from this test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rejected as out of scope.

depends_on:MBEDTLS_ARC4_C
enc_dec_buf_multipart:MBEDTLS_CIPHER_ARC4_128:128:15:7:-1:15:7:15:7

ARC4 Encrypt and decrypt 22 bytes in multiple parts 1
ARC4 Encrypt and decrypt 22 bytes in multiple parts 1 [#2]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the number at the end of these test three test cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, otherwise the descriptions would not be unique.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I mean is - there are a couple of test cases that end with parts 1 [#n] in succesion. It should be either parts 1, parts 2, etc. , or parts [#1], parts [#2].

@@ -2,63 +2,63 @@ BLOWFISH CBC Decrypt empty buffer
depends_on:MBEDTLS_BLOWFISH_C:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_CIPHER_PADDING_PKCS7
dec_empty_buf:MBEDTLS_CIPHER_BLOWFISH_CBC:0:0

BLOWFISH Encrypt and decrypt 0 bytes
BLOWFISH Encrypt and decrypt 0 bytes [#1]
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather opt for having precise test names rather than numbers, so:

  • BLOWFISH-CBC Encrypt [...];
  • BLOWFISH-CFB6 Encrypt [...];
  • BLOWFISH-CTR Encrypt [...];

Copy link
Contributor

Choose a reason for hiding this comment

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

The same applies to CAMELLIA tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rejected as out of scope.

depends_on:MBEDTLS_BLOWFISH_C:MBEDTLS_CIPHER_MODE_CFB
enc_dec_buf:MBEDTLS_CIPHER_BLOWFISH_CFB64:"BLOWFISH-CFB64":128:32:-1

BLOWFISH Encrypt and decrypt 32 bytes
BLOWFISH Encrypt and decrypt 32 bytes [#4]
Copy link
Contributor

Choose a reason for hiding this comment

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

33 bytes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rejected as out of scope.

depends_on:MBEDTLS_BLOWFISH_C:MBEDTLS_CIPHER_MODE_CFB
enc_dec_buf_multipart:MBEDTLS_CIPHER_BLOWFISH_CFB64:128:15:7:-1:15:7:15:7

BLOWFISH Encrypt and decrypt 22 bytes in multiple parts 1
BLOWFISH Encrypt and decrypt 22 bytes in multiple parts 1 [#5]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the number at the end of this test case and others like this and have the unified [#n] format instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rejected as out of scope.

depends_on:MBEDTLS_BLOWFISH_C:MBEDTLS_CIPHER_MODE_CTR
enc_dec_buf:MBEDTLS_CIPHER_BLOWFISH_CTR:"BLOWFISH-CTR":128:32:-1

BLOWFISH Encrypt and decrypt 32 bytes
BLOWFISH Encrypt and decrypt 32 bytes [#6]
Copy link
Contributor

Choose a reason for hiding this comment

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

33 bytes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rejected as out of scope.

depends_on:MBEDTLS_CAMELLIA_C:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_CIPHER_PADDING_PKCS7
enc_dec_buf:MBEDTLS_CIPHER_CAMELLIA_128_CBC:"CAMELLIA-128-CBC":128:32:-1

CAMELLIA Encrypt and decrypt 32 bytes
CAMELLIA Encrypt and decrypt 32 bytes [#2]
Copy link
Contributor

Choose a reason for hiding this comment

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

33 bytes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rejected as out of scope.

depends_on:MBEDTLS_CAMELLIA_C:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_CIPHER_PADDING_PKCS7
enc_dec_buf:MBEDTLS_CIPHER_CAMELLIA_128_CBC:"CAMELLIA-128-CBC":128:32:MBEDTLS_PADDING_ZEROS_AND_LEN

CAMELLIA Encrypt and decrypt 32 bytes with zeros and len padding
CAMELLIA Encrypt and decrypt 32 bytes with zeros and len padding [#2]
Copy link
Contributor

Choose a reason for hiding this comment

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

33 bytes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rejected as out of scope.

depends_on:MBEDTLS_GCM_C:MBEDTLS_AES_C
auth_crypt_tv:MBEDTLS_CIPHER_AES_128_GCM:"241067a0301edf0f825d793e03383ea1":"a30994261f48a66bb6c1fc3d69659228":"":"6984bb9830843529fad7f5e7760db89c778d62c764fcd2136ffb35d7d869f62f61d7fef64f65b7136398c1b5a792844528a18a13fba40b186ae08d1153b538007fc460684e2add8a9ed8dd82acbb8d357240daaa0c4deb979e54715545db03fe22e6d3906e89bdc81d535dae53075a58f65099434bfeed943dbc6024a92aa06a":"36c3b4a732ba75ae":"FAIL":"":0

AES-GCM NIST Validation (AES-128,128,1024,0,32) #0
AES-GCM NIST Validation (AES-128,128,1024,0,32) #0 [#1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicated numeration of test cases in this file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rejected as out of scope.

@@ -78,15 +78,15 @@ NULL Encrypt and decrypt 16 bytes in multiple parts 4
depends_on:MBEDTLS_CIPHER_NULL_CIPHER
enc_dec_buf_multipart:MBEDTLS_CIPHER_NULL:0:15:1:-1:15:1:15:1

NULL Encrypt and decrypt 22 bytes in multiple parts 1
NULL Encrypt and decrypt 22 bytes in multiple parts 1 [#1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the previous numeration of tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rejected as out of scope.

depends_on:!MBEDTLS_CTR_DRBG_USE_128_BIT_KEY
ctr_drbg_validate_reseed_between:"4a2a7dcbde58b8b3c3f4697beb67bba2":"58364ceefad37581c518b7d42ac4f9aae22befd84cbc986c08d1fb20d3bd2400a899bafd470278fad8f0a50f8490af29f938471b4075654fda577dad20fa01ca":"":"":"":"20c5117a8aca72ee5ab91468daf44f29"

CTR_DRBG NIST Validation (AES-256 use df,False,256,128,0,0) #3
CTR_DRBG NIST Validation (AES-256 use df,False,256,128,0,0) #3 [#1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Numeration got messed up here too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rejected as out of scope.

@@ -793,11 +793,11 @@ generic multi step HMAC-SHA-384 Test Vector NIST CAVS #4
depends_on:MBEDTLS_SHA512_C
md_hmac_multi:"SHA384":48:"01ac59f42f8bb91d1bd10fe6990d7a87":"3caf18c476edd5615f343ac7b7d3a9da9efade755672d5ba4b8ae8a7505539ea2c124ff755ec0457fbe49e43480b3c71e7f4742ec3693aad115d039f90222b030fdc9440313691716d5302005808c07627483b916fdf61983063c2eb1268f2deeef42fc790334456bc6bad256e31fc9066de7cc7e43d1321b1866db45e905622":"1985fa2163a5943fc5d92f1fe8831215e7e91f0bff5332bc713a072bdb3a8f9e5c5157463a3bfeb36231416e65973e64"

generic multi step HMAC-SHA-384 Test Vector NIST CAVS #5
generic multi step HMAC-SHA-384 Test Vector NIST CAVS #5 [#1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Double test numeration

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rejected as out of scope.

@@ -406,10 +406,10 @@ mbedtls_mpi_shift_r:10:"128":1:10:"64"
Test mbedtls_mpi_shift_r #2
mbedtls_mpi_shift_r:10:"120815570979701484704906977000760567182871429114712069861589084706550626575967516787438008593490722779337547394120718248995900363209947025063336882559539208430319216688889117222633155838468458047056355241515415159736436403445579777425189969":45:10:"3433785053053426415343295076376096153094051405637175942660777670498379921354157795219578264137985649407981651226029903483433269093721578004287291678324982297860947730012217028349628999378309630601971640587504883789518896817457"

Test mbedtls_mpi_shift_r #4
Test mbedtls_mpi_shift_r #4 [#1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Numbers of test cases here are wild. 1, 2, 4, 4, 6

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rejected as out of scope.

@@ -1208,23 +1208,23 @@ PSA symmetric decryption multipart: AES-CBC-nopad, 20+12 bytes
depends_on:MBEDTLS_AES_C:MBEDTLS_CIPHER_MODE_CBC
cipher_decrypt_multipart:PSA_ALG_CBC_NO_PADDING:PSA_KEY_TYPE_AES:"2b7e151628aed2a6abf7158809cf4f3c":"2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a":"a076ec9dfbe47d52afc357336f20743b89906f2f9207ac02aa658cb4ef19c61f":20:16:16:"6bc1bee22e409f96e93d7e117393172a5434f378a597bcef1389318c7fc865ef"

PSA symmetric encryption multipart: AES-CTR, 11+5 bytes
PSA symmetric encryption multipart: AES-CTR, 11+5 bytes [#2]
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy paste errors here and below: should be decryption

depends_on:MBEDTLS_DSA_C:MBEDTLS_DSA_DETERMINISTIC
asymmetric_signature_wildcard:PSA_ALG_DETERMINISTIC_DSA( PSA_ALG_ANY_HASH ):ALG_IS_DSA | ALG_IS_DETERMINISTIC_DSA | ALG_DSA_IS_DETERMINISTIC

Asymmetric signature: randomized ECDSA with wildcard hash
depends_on:MBEDTLS_ECDSA_C
asymmetric_signature_wildcard:PSA_ALG_ECDSA( PSA_ALG_ANY_HASH ):ALG_IS_ECDSA | ALG_IS_RANDOMIZED_ECDSA

Asymmetric signature: deterministic DSA with wildcard hash
Asymmetric signature: deterministic DSA with wildcard hash [#2]
Copy link
Contributor

Choose a reason for hiding this comment

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

W would rather name it "ECDSA"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rejected as out of scope.

@@ -98,6 +98,6 @@ import/export persistent key RSA keypair file not exist with restart: 1024-bit
depends_on:MBEDTLS_PK_C:MBEDTLS_PK_PARSE_C:MBEDTLS_RSA_C
import_export_persistent_key:"3082025e02010002818100af057d396ee84fb75fdbb5c2b13c7fe5a654aa8aa2470b541ee1feb0b12d25c79711531249e1129628042dbbb6c120d1443524ef4c0e6e1d8956eeb2077af12349ddeee54483bc06c2c61948cd02b202e796aebd94d3a7cbf859c2c1819c324cb82b9cd34ede263a2abffe4733f077869e8660f7d6834da53d690ef7985f6bc3020301000102818100874bf0ffc2f2a71d14671ddd0171c954d7fdbf50281e4f6d99ea0e1ebcf82faa58e7b595ffb293d1abe17f110b37c48cc0f36c37e84d876621d327f64bbe08457d3ec4098ba2fa0a319fba411c2841ed7be83196a8cdf9daa5d00694bc335fc4c32217fe0488bce9cb7202e59468b1ead119000477db2ca797fac19eda3f58c1024100e2ab760841bb9d30a81d222de1eb7381d82214407f1b975cbbfe4e1a9467fd98adbd78f607836ca5be1928b9d160d97fd45c12d6b52e2c9871a174c66b488113024100c5ab27602159ae7d6f20c3c2ee851e46dc112e689e28d5fcbbf990a99ef8a90b8bb44fd36467e7fc1789ceb663abda338652c3c73f111774902e840565927091024100b6cdbd354f7df579a63b48b3643e353b84898777b48b15f94e0bfc0567a6ae5911d57ad6409cf7647bf96264e9bd87eb95e263b7110b9a1f9f94acced0fafa4d024071195eec37e8d257decfc672b07ae639f10cbb9b0c739d0c809968d644a94e3fd6ed9287077a14583f379058f76a8aecd43c62dc8c0f41766650d725275ac4a1024100bb32d133edc2e048d463388b7be9cb4be29f4b6250be603e70e3647501c97ddde20a4e71be95fd5e71784e25aca4baf25be5738aae59bbfe1c997781447a2b24":PSA_KEY_TYPE_RSA_KEY_PAIR:1024:1:1

PSA import/export-persistent symmetric key: 16 bytes
PSA import/export-persistent symmetric key: 16 bytes [#2]
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not obvious how this test differs from case 1 - maybe the description should mention it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rejected as out of scope.

@AndrzejKurek
Copy link
Contributor

Test suites fail on the merge PR job.

No data seems to be missing, just some duplicated cases, perhaps due
to naming inconsistencies "ECB_Xxcrypt" vs "ECB Xxcrypt" which I also
fixed.
Test vector ARMmbed#15 was encrypted twice. Decrypt it the second time.
There are two test vectors in RFC 5649. There was only one in our test
suite, run twice. Put the second test vector instead of repeating the
first.
There should have been a good-saltlen test case and a bad-saltlen test
case for both sizes 522 and 528, but the 522-bad-saltlen test case was
missing and the 528-good-saltlen test case was repeated. Fix this.
Before: say CCM twice, do GCM twice.
After: say CCM and do CCM, then say GCM and do GCM.
Nothing seems to be missing in its stead.
There's a SHA256 test without a label and one with a label, so do the
same for SHA384.
A test case for 32+0 was present three times, evidently overeager
copy-paste. Replace the duplicates by test cases that read more than
32 bytes, which exercises HKDF a little more (32 bytes is significant
because HKDF-SHA-256 produces output in blocks of 32 bytes).

I obtained the test data by running our implementation, because we're
confident in our implementation now thanks to other test cases: this
data is useful as a non-regression test.
Make check-test-cases.py pass.

Prior to this commit, there were many repeated test descriptions, but
none with the same test data and dependencies and comments, as checked
with the following command:

    for x in tests/suites/*.data; do perl -00 -ne 'warn "$ARGV: $. = $seen{$_}\n" if $seen{$_}; $seen{$_}=$.' $x; done

Wherever a test suite contains multiple test cases with the exact same
description, add " [ARMmbed#1]", " [ARMmbed#2]", etc. to make the descriptions
unique. We don't currently use this particular arrangement of
punctuation, so all occurrences of " [#" were added by this script.

I used the following ad hoc code:

import sys

def fix_test_suite(data_file_name):
    in_paragraph = False
    total = {}
    index = {}
    lines = None
    with open(data_file_name) as data_file:
        lines = list(data_file.readlines())
        for line in lines:
            if line == '\n':
                in_paragraph = False
                continue
            if line.startswith('#'):
                continue
            if not in_paragraph:
                # This is a test case description line.
                total[line] = total.get(line, 0) + 1
                index[line] = 0
            in_paragraph = True
    with open(data_file_name, 'w') as data_file:
        for line in lines:
            if line in total and total[line] > 1:
                index[line] += 1
                line = '%s [#%d]\n' % (line[:-1], index[line])
            data_file.write(line)

for data_file_name in sys.argv[1:]:
    fix_test_suite(data_file_name)
@gilles-peskine-arm gilles-peskine-arm force-pushed the test_outcome_file-crypto-fix branch from 9a6fb35 to efa2ac8 Compare September 20, 2019 13:59
@gilles-peskine-arm
Copy link
Collaborator Author

I force-pushed a new history that just changes the commit for the test case in test_suite_cipher.nist_kw.data. I'd made a mistake in entering the test data.

The current test generator code accepts multiple colons as a
separator, but this is just happenstance due to how the code, it isn't
robust. Replace "::" by ":", which is more future-proof and allows
simple separator-based navigation.
@AndrzejKurek
Copy link
Contributor

The export keys test is failing, is it related to this PR?

@gilles-peskine-arm
Copy link
Collaborator Author

The failure of the export keys test is due to an interface change in Mbed TLS. I haven't followed the exact history but AFAIK it should be resolved once all branches and submodules are up to date. In any case, it's unrelated to this PR.

Copy link
Contributor

@Patater Patater left a comment

Choose a reason for hiding this comment

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

LGTM

@gilles-peskine-arm gilles-peskine-arm removed the needs: review The pull request is ready for review. This generally means that it has no known issues. label Sep 24, 2019
@gilles-peskine-arm
Copy link
Collaborator Author

CI passed except for "export keys functionality" in TLS which is a known issue unrelated to this PR.

@gilles-peskine-arm gilles-peskine-arm merged commit 0a048b2 into ARMmbed:development Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate test case descriptions
3 participants