- Rename 'blkcipher' to 'cipher'. Some of the ciphers being tested are stream ciphers.
- Rename 'authenc' to 'eta' as it is only testing ETA chained operations and not other combination modes.
- Add a notion of an OCF session and some helper routines to try to reduce duplicated code. This also uses a single session for both encrypt and decrypt operations during a single test.
- Add tests to ensure that AEAD algorithms fail decryption with EBADMSG when given a corrupted tag.
- Remove the transitional hack for COP_F_CIPHER_FIRST.
- Update block comment to mention plain hashes.
Details
Details
- Reviewers
cem - Commits
- rS356457: Various cleanups to cryptocheck.
- tested against ccr0 and cryptosoft
Diff Detail
Diff Detail
- Lint
No Lint Coverage - Unit
No Test Coverage - Build Status
Buildable 28390 Build 26473: arc lint + arc unit
Event Timeline
| tools/tools/crypto/cryptocheck.c | ||
|---|---|---|
| 85 | Is the tool missing sha2-224? And I guess the 512/256 and similar primitives? I know OCF *has* the former. | |
| 88–89 | Hm. Sure, I think this matches how blake2 is exercised by this program. And while it is a keyed MAC, it does not use the HMAC construction, so hmac was somewhat misleading. I think the only reason I didn't test keyed MAC mode was that OpenSSL lacked support for that mode at the time? I might be misremembering. Maybe the HMAC section should be renamed MAC instead? I know there is some code in cryptocheck that assumes a MAC is HMAC, which would need generalizing to actually test blake as a MAC for E-T-A. | |
| 108 | Internally chacha is a 512-bit block cipher in CTR mode :-). It just doesn't expose that through the canonical API. | |
| 111 | s/hmac/mac/? | |
| 426 | Is crid a global? It seems like it'd be good to make it an explicit parameter. Ditto below | |
| 430–450 | Maybe init and deinit? These routines do not allocate or free memory, so especially free seems like the wrong verb to me. Subjective. | |
| 462–469 | const struct ocf_session *? | |
| 702–703 | _create_session? const struct alg *? | |
| 714 | const struct alg * | |
| 715 | If we're going to use a boolean in new code, I'd prefer to spell it bool and true/false instead of int and 0/1. That said, I find booleans in APIs kind of unhelpful; maybe instead of enc we could take op of COP_ENCRYPT or COP_DECRYPT directly? This helps make invocations self-documenting. | |
| 737 | Can this be const struct alg *? I'm not sure if some of the downstream APIs like openssl_cipher or generate_iv use const properly on that pointer parameter, but afaik nothing has any reason to be modifying the alg. | |
| 820 | style nit: space missing after comma | |
| 838 | ditto enc bool / COP_foo suggestion from above. (And const alg.) | |
| 1034–1035 | This is kind of a weird thing to put into this API instead of just returning an errno value and performing that behavior in the caller. The rest of the similar routines return booleans, but they could instead return errno values. | |
| 1081–1083 | test_tag is already equal to control_tag at this point, and it should be sufficient to just flip a single bit: test_tag[0] ^= 0x1; | |
| 1387 | The memory management around build_eta etc is pretty gross. I realize this is inherited, fine to leave alone. | |
| 1475–1481 | Are there any CI tests that will need to be updated to run cryptocheck with the new parameters? Or do we only run the Python cryptocheck in CI? | |
| tools/tools/crypto/cryptocheck.c | ||
|---|---|---|
| 85 | Looks like only the comment wasn't updated when 224 was added. I'll fix that part. OCF doesn't have support any of the 512/N primitives directly. | |
| 88–89 | In the ocf_rework branch I already rename this to "Keyed-hashes" because it adds GMAC, but I think I left the command line verb as "hmac" still. I'm not familiar enough with the blake bits in openssl so probably won't try to add keyed tests of blake anytime soon. I do think "MAC" might be a better name than "hmac" though, even once I add GMAC in the future. | |
| 426 | It is a global parsed from getopt(). Passing it in explicitly would mean passing it down through about 4-5 layers of APIs. (run_*_tests -> run_test -> test_foo, etc.) I think it would mostly be harder to read to do that. One change I did not make was to just do a single crget() instead of separate fds, though if we wanted to do that, that would now only change a few places. | |
| 430–450 | init/destroy is a pair we use often in other places, so maybe that. Agreed that free is perhaps not the best choice. | |
| 702–703 | ocf_create_cipher_session felt verbose, I guess it's not too bad with init instead. Will add lots of const poison for alg. | |
| 715 | I almost did the COP_ thing, and I guess I will for the same reasons. I had used 0/1 because that's what openssl uses (ugh) | |
| 1034–1035 | I guess that's true. It does mean some duplication of the code to emit the warning perhaps, but that is ok. | |
| 1081–1083 | True, I guess I went for a sledge hammer, but a single bit is simpler. | |
| 1387 | I went ahead and added a free_eta wrapper. | |
| 1475–1481 | We only run the python bits in CI currently. | |
Comment Actions
- Various fixes from Conrad.
- Merge run_gcm_test and run_ccm_test.
- Axe spurious return.
| tools/tools/crypto/cryptocheck.c | ||
|---|---|---|
| 475–487 | Ah, I had started converting all the ocf_foo functions to return an error, but decided to only do it for the ones that want to support checking for EBADMSG (so ocf_aead for now, but also ocf_eta in my rework branch). Forgot to put the return type back to bool here. | |