Dmitry Eremin-Solenikov(lumag) replied on github web page:
test/performance/odp_crypto.c
line 35
@@ -226,6 +241,18 @@ static crypto_alg_config_t algs_config[] = {
.auth_digest_len = 12,
},
},
+ {
+ .name = "null-hmac-md5-96",
+ .session = {
+ .cipher_alg = ODP_CIPHER_ALG_NULL,
+ .auth_alg = ODP_AUTH_ALG_MD5_HMAC,
+ .auth_key = {
+ .data = test_key64,
+ .length = sizeof(test_key64)
+ },
+ .auth_digest_len = 16,
Comment:
Name of the test suggests, that test is using truncated hmac, but then you
specify full auth_digest_len.
> Dmitry Eremin-Solenikov(lumag) wrote:
> and this
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> Will it work w/o crypto_null driver?
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> `res` does not quite follow `crypto_chain_order`, does it? Please rename to
>>> 'order' or smth. like that.
>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>> Push this logic to `set_chain_order`
>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>> Make it return `crypto_chain_order`.
>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>> No need to keep this under `if (iv_length)`
>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>> Liked this typo. It shows that the code never worked as expected.
>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>> same issue here.
>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>> Same issue here.
>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>> `auth.digest_length` should be set for MD5_96 here, but not for
>>>>>>>>>> MD5_HMAC.
>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>> It's really not a problem at this moment, this can be fixed later.
>>>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>>>> oke I see. I will make the update in the next version.
>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>> both = Encrypt-then-MAC and MAC-cleartext
>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>> Could you please add tests cases rather than modify existing
>>>>>>>>>>>>>> ones?
>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>> Implementation should not set digest len for cases other than
>>>>>>>>>>>>>>> several deprecated auth algos.
>>>>>>>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>>>>>>>> setting digest_length is moved to get_crypto_dev(), where it
>>>>>>>>>>>>>>>> reads from PMD and set the min digest length if application
>>>>>>>>>>>>>>>> doesn't set it. Reagrding the QAT, I need to look at it and
>>>>>>>>>>>>>>>> understand. may be as a separate PR.
>>>>>>>>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>>>>>>>>> I have updated the tests so that it works with both the
>>>>>>>>>>>>>>>>> platforms linux-generci & linux-dpdk.
>>>>>>>>>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>>>>>>>>>> what do you mean by testing both combinations ?. We are
>>>>>>>>>>>>>>>>>> testing both the combinations ENCODE & DECODE. It's the
>>>>>>>>>>>>>>>>>> default value we are changing it.
>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>> Checkpatch flags this line:
>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>> WARNING: line over 80 characters
>>>>>>>>>>>>>>>>>>> #363: FILE: platform/linux-dpdk/test/wrapper-script.sh:3:
>>>>>>>>>>>>>>>>>>> +export ODP_PLATFORM_PARAMS=${ODP_PLATFORM_PARAMS:--n 4
>>>>>>>>>>>>>>>>>>> --vdev "crypto_openssl" --vdev "crypto_null"}
>>>>>>>>>>>>>>>>>>> total: 0 errors, 1 warnings, 0 checks, 530 lines checked
>>>>>>>>>>>>>>>>>>> NOTE: Ignored message types: AVOID_EXTERNS BIT_MACRO
>>>>>>>>>>>>>>>>>>> COMPARISON_TO_NULL DEPRECATED_VARIABLE NEW_TYPEDEFS
>>>>>>>>>>>>>>>>>>> PREFER_PRINTF PREFER_SCANF SPLIT_STRING SSCANF_TO_KSTRTO
>>>>>>>>>>>>>>>>>>> VOLATILE
>>>>>>>>>>>>>>>>>>> 0001-linux-dpdk-crypto-support-for-cipher-auth-only-featu.patch
>>>>>>>>>>>>>>>>>>> has style problems, please review.
>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>> I suggest to factor out cipher and auth checks as separate
>>>>>>>>>>>>>>>>>>>> functions and apply them only if corresponding algorithm
>>>>>>>>>>>>>>>>>>>> is not NULL. Code would be simpler.
>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>> For deprecated MD5_96 (and SHA256_128/AES128_GCM)
>>>>>>>>>>>>>>>>>>>>> application won't set `auth.digest_len`, so you have to
>>>>>>>>>>>>>>>>>>>>> enforce 12 and 16 bytes digest len. For
>>>>>>>>>>>>>>>>>>>>> MD5_HMAC/SHA256_HMAC (and others) application will set
>>>>>>>>>>>>>>>>>>>>> `auth.digest_len`.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> IIRC QAT driver should support truncated digests
>>>>>>>>>>>>>>>>>>>>> out-of-box. Also you can try expanding pmd_openssl to
>>>>>>>>>>>>>>>>>>>>> support truncated lengths, it should not be very
>>>>>>>>>>>>>>>>>>>>> complicated. Also note, that full-length HMAC won't be
>>>>>>>>>>>>>>>>>>>>> useful for IPsec.
>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>> "Do not change tests". See my previous comment.
>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>> Theoretically we'd better be testing both combinations.
>>>>>>>>>>>>>>>>>>>>>>> The problem with test changes in your commit is that
>>>>>>>>>>>>>>>>>>>>>>> you change tests rather than expand them. Can you
>>>>>>>>>>>>>>>>>>>>>>> import tests from #379?
>>>>>>>>>>>>>>>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>>>>>>>>>>>>>>>> not sure what do u mean here.
>>>>>>>>>>>>>>>>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>>>>>>>>>>>>>>>>> Not sure if I have fallowed. I introduced Chain order
>>>>>>>>>>>>>>>>>>>>>>>>> because dpdk PMD's doesn't have support for
>>>>>>>>>>>>>>>>>>>>>>>>> combination of NULL + Valid algo. So linux-dpdk
>>>>>>>>>>>>>>>>>>>>>>>>> implementation converts the NULL also to chain order
>>>>>>>>>>>>>>>>>>>>>>>>> as if it cipher only & auth only.
>>>>>>>>>>>>>>>>>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>>>>>>>>>>>>>>>>>> my understanding of auth_cipher_text = true means do
>>>>>>>>>>>>>>>>>>>>>>>>>> encode then authenticate. and when it is applied
>>>>>>>>>>>>>>>>>>>>>>>>>> together with if (ODP_CRYPTO_OP_ENCODE == param->op)
>>>>>>>>>>>>>>>>>>>>>>>>>> sets the "entry->do_cipher_first" to true by
>>>>>>>>>>>>>>>>>>>>>>>>>> default and if the param->op == DECODE it sets the
>>>>>>>>>>>>>>>>>>>>>>>>>> !auth_cipher_text. I felt this is more easier for
>>>>>>>>>>>>>>>>>>>>>>>>>> understanding.
>>>>>>>>>>>>>>>>>>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>>>>>>>>>>>>>>>>>>> openssl PMD in 17.02 supports the digest length to
>>>>>>>>>>>>>>>>>>>>>>>>>>> be min 16 bytes. But I have now removed the setting
>>>>>>>>>>>>>>>>>>>>>>>>>>> digest_lengths from here as it sets it from the
>>>>>>>>>>>>>>>>>>>>>>>>>>> application.
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>> why?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `non tangere testos meos`
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I'd suggest to use ALG_NULL checks directly,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> rather than introduce chain order.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> This is wrong. MD5_96 should use 12-byte digest
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> length. MD5_HMAC should use digest length
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> specified by application.
https://github.com/Linaro/odp/pull/385#discussion_r161768552
updated_at 2018-01-16 14:21:10
