Test the AES-CCM test vectors from the NIST Known Answer Tests.

[jhb]

Summary

The CCM test vectors use a slightly different file format in that
there are global key-value pairs as well as section key-value
pairs that need to be used in each test. In addition, the sections
can set multiple key-value pairs in the section name. The CCM KAT
parser class is an iterator that returns a dictionary once per test
where the dictionary contains all of the relevant key-value pairs
for a given test (global, section name, section, test-specific).

Note that all of the CCM decrypt tests use nonce and tag lengths
that are not supported by OCF (OCF only supports a 12 byte nonce
and 16 byte tag), so none of the decryption vectors are actually
tested.

Test Plan

• ran against ccr0, cryptosoft0, and aesni0. No failures found. Did confirm via stats on ccr0 that it did do ccm encrypt operations, but no decrypts (this is what led to figuring out why none of the decryption vectors were run).

[cem]

Hm, why isn't maclen of 16 an issue for encrypt?

And can we pad tags with zeros out to 16 bytes to verify decrypt, or does the taglen get incorporated into the MAC (I forget)? (Edit: nevermind, I guess even if the tag is truncated we have no way to tell OCF to only verify so-many bytes of it, and the zeros won't match. Ugh.)

I'd encourage adding the note (that none of the NIST KAT decryption vectors are supported by OCF CCM, so runCCMDecrypt is a big no-op for now) to the code itself as well.

[ngie]

Requesting changes because there are a handful of non-style nits that should be resolved before this is committed to ^/head .

tests/sys/opencrypto/cryptodev.py

390 ↗

(On Diff #56412)

A general comment: it's better to allocate this later on to avoid leaks and make cleanup at __del__(..) time unnecessary (that's what you'll have to do in this case).

430 ↗

(On Diff #56412)

style nits:

Use line.startswith("#") instead of line[0] == "#".

Also, if you strip line, then you can condense the if not line and if not line.strip() into a single if not line check, e.g.,

line = self.fp.readline()
if line.startswith("#"):
    continue
line = line.strip()
# Will catch a blank line or a line that consists only of space characters.
if not line:
    continue

437 ↗

(On Diff #56412)

This could cause problems if the line has more than one = in the string. You might want to use f, v = line.split(" =", 1) to avoid the unnecessary exception.

438 ↗

(On Diff #56412)

Bare excepts are generally an anti-pattern because it will catch other exceptions, like sys.exit, KeyboardInterrupt, etc. You probably should just try capturing ValueError, instead as that's the error that you likely care about the most:

>>> f, v = "bar".split(" =")                                                                                                                                                                               
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: not enough values to unpack (expected 2, got 1)

462 ↗

(On Diff #56412)

Same comment as above.

465–468 ↗

(On Diff #56412)

A regular expression that was pre-compiled and used for grabbing matches would likely be better, but this is potentially more performant in the right circumstances.

The only thing I'd do is use line.startswith("[") and be a bit more defensive with the split, as it will give you false positives, as seen below:

>>> "bar".split("]", 1)[0]
'bar'

tests/sys/opencrypto/cryptotest.py

260–262 ↗

(On Diff #56412)

It's better to format this string like so (it won't trace back if data["Count"] cannot be concatenated to "" and is a bit more legible/greppable):

self.assertEqual(out, ct,
    "Count: %s. Actual: %r. Expected: %r on cname" % (
        data["Count"], out.encode("hex"), data, cname
    )
)

291 ↗

(On Diff #56412)

Please use EnvironmentError as e for py3 compat <3.

297–298 ↗

(On Diff #56412)

This might be a bit easier to follow (and plus, you can test the exception properties afterwards):

with self.assertRaises(IOError):
    c.decrypt(payload, nonce, aad, tag)

306–310 ↗

(On Diff #56412)

Same comment about self.assertEqual above.

[jhb]

In general, many of the style issues are present in the rest of the file. I think for those sorts of things it is better to fix all of them at once (e.g. in followup changes) rather than having the CCM tests be very different from the other tests.

tests/sys/opencrypto/cryptodev.py

390 ↗

(On Diff #56412)

I was just matching the existing code KATParser class in that regard.

430 ↗

(On Diff #56412)

The proposed version misses the test for EOF. Also, the existing changes match the existing style in KATParser (for better or worse).

437 ↗

(On Diff #56412)

The file format is that you have lines with a single 'key = value' pair. A line not match that should be treated as corrupt rather than trying to handle multiple pairs, etc.

438 ↗

(On Diff #56412)

That may very well be, this was also copied from the KATParser.

tests/sys/opencrypto/cryptotest.py

260–262 ↗

(On Diff #56412)

This is the current pattern used throughout the rest of this file. I do think that using a format string instead of concat probably is better though and would be fine doing a sweep to fix all of them in a followup after you reindent (since the indentation also makes these lines nearly unreadable).

297–298 ↗

(On Diff #56412)

I did look at using that, but chose to match the existing use of self.assertRaises in the GCM tests.

[ngie]

Ok, let's proceed with this change. I'll work on fixing the style issues on my branch.

[ngie]

tests/sys/opencrypto/cryptodev.py

438 ↗

(On Diff #56412)

Ugnnnnnnnnhhhhhh....

[jhb]

In D19978#429425, @cem wrote:

Hm, why isn't maclen of 16 an issue for encrypt?

For encrypt we just do the truncation in the python code of the 16 byte result from OCF.

And can we pad tags with zeros out to 16 bytes to verify decrypt, or does the taglen get incorporated into the MAC (I forget)? (Edit: nevermind, I guess even if the tag is truncated we have no way to tell OCF to only verify so-many bytes of it, and the zeros won't match. Ugh.)

Correct, since the verification of the tag on decrypt happens in the kernel, we need OCF itself to understand the tag length. In theory the 'mlen' field in the crypto session should let us do this, but I think the current /dev/crypto CIOGSESSION ioctls don't expose that, and the current GCM and CCM software bits in the kernel don't honor/support truncated tags.

I'd encourage adding the note (that none of the NIST KAT decryption vectors are supported by OCF CCM, so runCCMDecrypt is a big no-op for now) to the code itself as well.

Will do in the commit.