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

Implement HKDF algorithm (RFC 5869) #355

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
2 participants
@ghedo
Copy link
Contributor

ghedo commented Aug 3, 2015

This PR implements the HMAC-based Extract-and-Expand Key Derivation Function (HKDF) as defined in RFC 5869.

It is required to implement the QUIC and TLS 1.3 protocols (among others).

@ghedo

This comment has been minimized.

Copy link
Contributor Author

ghedo commented Aug 3, 2015

Fixed merge conflict due to 7e729bb.

@ghedo

This comment has been minimized.

Copy link
Contributor Author

ghedo commented Sep 3, 2015

Just FYI, I just rebased the patches on master to fix a merge conflict caused by 3a3cb62.

Also, I kept the actual implementation and the tests on two separate commits for easier review, but if you prefer I can squash them together.

@ghedo

This comment has been minimized.

Copy link
Contributor Author

ghedo commented Sep 3, 2015

Heh, incidentally Travis just caught the fact that there still was an unresolved merge conflict, causing the build to fail. Fixed now :)

@ghedo

This comment has been minimized.

Copy link
Contributor Author

ghedo commented Sep 6, 2015

Rebased again after the nptest thing (waiting for travis to finish, but everything seems ok).

@ghedo

This comment has been minimized.

Copy link
Contributor Author

ghedo commented Jan 12, 2016

@richsalz is there any chance of this getting merged? It's been open for a while now. I think this should go into 1.1 so that we are going to be somewhat readier for the TLS 1.3 work afterwards. If it's a problem of "who's gonna maintain this?" than I'm of course available if that has any value. The code is also pretty simple (the algorithm itself is simple), it has documentation and tests as well (the vectors come from the RFC), but if I somehow can make things better to get this merged let me know.

@ghedo

This comment has been minimized.

Copy link
Contributor Author

ghedo commented Jan 12, 2016

I just realised the patch needed to be updated to the new HMAC_CTX API, done now.

@ghedo

This comment has been minimized.

Copy link
Contributor Author

ghedo commented Feb 17, 2016

I rebased this yet again, and adapted to yet another build system :)

I'd really like to know if there's any interest in this at all though, so I can put my soul to rest and just stop rebasing this. Since at some point this will need to be implemented for TLS 1.3 anyway, I'd say better merge it now than have to re-implement it later.

@richsalz

This comment has been minimized.

Copy link
Contributor

richsalz commented Feb 17, 2016

I am interested in this. Don't know if that puts your soul to rest or adds more unease.

@ghedo

This comment has been minimized.

Copy link
Contributor Author

ghedo commented Feb 17, 2016

At least now I know I'm not doing this for nothing :)

@richsalz

This comment has been minimized.

Copy link
Contributor

richsalz commented Feb 24, 2016

Can we expose a "simpler" EVP interface? Or not?

@ghedo

This comment has been minimized.

Copy link
Contributor Author

ghedo commented Feb 24, 2016

We don't currently have an EVP interface that exposes this kind of functionality AFAICT, so it would need to be added.

Note that for TLS 1.3 we'll probably want to call HKDF_Extract and HKDF_Expand directly, as a matter of performance and to follow the spec more closely, so even if we had a simpler EVP interface we'd still need to have a more specific HKDF API as well.

@richsalz richsalz self-assigned this Feb 24, 2016

@richsalz richsalz added the reviewed label Feb 24, 2016

@richsalz

This comment has been minimized.

Copy link
Contributor

richsalz commented Feb 24, 2016

Not even faking it out with EVP ctrl options? Probably not.
+1 I'll merge when someone else reviews.
nice job on the docs!

@ghedo

This comment has been minimized.

Copy link
Contributor Author

ghedo commented Feb 24, 2016

Not even faking it out with EVP ctrl options?

Well, that would probably make it more complicated instead of simpler :)

I could see adding a generic KDF API, but e.g. PBKDF2 has very different use cases than HKDF and a different API as well (it has an iterations count argument), so it would still be quite awkward I think.

@richsalz

This comment has been minimized.

Copy link
Contributor

richsalz commented Feb 24, 2016

yeah. let me see what Steve thinks.

@ghedo

This comment has been minimized.

Copy link
Contributor Author

ghedo commented Feb 29, 2016

@richsalz @snhenson I've updated this to expose the EVP_PKEY interface as requested. I'm not 100% sure about the API though, so I removed the documentation for now. Please review and let me know what you think.

FYI, I removed the crypto/hkdf directory and merged everything into crypto/kdf/hkdf.c because I was having some troubles with the build system. This should be slightly cleaner though, so it's probably better anyway.

@ghedo

This comment has been minimized.

Copy link
Contributor Author

ghedo commented Feb 29, 2016

Also, this doesn't expose the "extract and "expand" phases anymore, so there could be "loss of efficiency" in some cases, as the TLS 1.3 spec puts it, but it's probably very small. In any case it shouldn't be hard to expose my original API (HKDF_Extract, HKDF_Expand, HKDF) if needed.

@richsalz

This comment has been minimized.

Copy link
Contributor

richsalz commented Feb 29, 2016

this looks good. you can add the docs now, or perhaps wait to see if others on the team have feedback for you.

thanks!

@ghedo

This comment has been minimized.

Copy link
Contributor Author

ghedo commented Feb 29, 2016

Re-added documentation.

EVP_PKEY_CTX *pctx;
unsigned char out[10];
size_t outlen = sizeof(out);
pctx = EVP_PKEY_CTX_new_id(EVP_PKEY_HKDF, NULL);

This comment has been minimized.

Copy link
@richsalz

richsalz Feb 29, 2016

Contributor

blank line here :)

This comment has been minimized.

Copy link
@ghedo

ghedo Feb 29, 2016

Author Contributor

Ok, fixed. FWIW most of the documentation page was copied from EVP_PKEY_TLS1_PRF.pod and adapted to HKDF.

@richsalz

This comment has been minimized.

Copy link
Contributor

richsalz commented Feb 29, 2016

+1 Nice job!

@ghedo

This comment has been minimized.

Copy link
Contributor Author

ghedo commented Mar 2, 2016

@richsalz aaaaaaand done. Note that I had to make a few small changes to adjust to the new test suite, so a quick re-review may be needed.

@richsalz

This comment has been minimized.

Copy link
Contributor

richsalz commented Mar 2, 2016

:) looks good. pushing for another review soon.

Implement HMAC-based extract-and-expand key derivation function
This patch implements the HMAC-based Extract-and-Expand Key Derivation
Function (HKDF) as defined in RFC 5869.

It is required to implement the QUIC and TLS 1.3 protocols (among others).
@richsalz

This comment has been minimized.

Copy link
Contributor

richsalz commented Mar 3, 2016

done! thanks.

@richsalz

This comment has been minimized.

Copy link
Contributor

richsalz commented Mar 3, 2016

and ... @aacfb13 ... done!

@richsalz richsalz closed this Mar 3, 2016

@ghedo ghedo deleted the ghedo:hkdf branch Mar 8, 2016

@daurnimator daurnimator referenced this pull request Jul 25, 2017

Closed

Bind HKDF functions #95

@bdewater bdewater referenced this pull request Nov 20, 2017

Closed

HKDF support #172

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.