-
Notifications
You must be signed in to change notification settings - Fork 16
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
ndn_hkdf: does not conform to RFC 5869 #65
Comments
@Zhiyi-Zhang How do you see this HMAC change? Personally I agree with "don't copy the key". But in default AES and ECC, they do copy keys as well. If we change them all, numbers of existing tests and examples would require modification. I wonder is there a balance here: when key length below |
No.
This would be inconsistent behavior and very surprising to users. |
I agree with Junxiao's comments. Let's follow his suggestion on this issue. |
Clear, while still a little question: should a method be reserved for users retrieving the key bytes from the
It was a mistake for my carelessness, sorry for that. I've already changed it back. |
No. In general, secret keys (HMAC, EC, etc) are not extractable. It may as well reside in a hardware TPM (e.g. ECC508 chip). |
5aea33b attempts to fix #56, but it is not a valid fix. It seems that I cannot reopen that issue, so I'm opening a new issue.
RFC5869 does not limit length of IKM. Thus, the implementation should support arbitrary key length.
The underlying TinyCrypt library does not have any limitation on HMAC-SHA256 key length. The limitation comes from a flawed design here:
ndn-lite/security/default-backend/ndn-lite-default-hmac-impl.c
Lines 37 to 39 in 5aea33b
To avoid this problem:
tc_hmac_state_struct
as part ofabstract_hmac_key
.ndn_lite_default_hmac_load_key
, load the key withtc_hmac_set_key
, and don't copy the key.ndn_lite_default_hmac_sha256_init
, memcpy thetc_hmac_state_struct
fromabstract_hmac_key
toabstract_hmac_sha256_state_t
.Also, #54 was accidentally reverted and brought back some warnings.
The text was updated successfully, but these errors were encountered: