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

Add EVP/KDF API #6674

Closed
wants to merge 1 commit into
base: master
from

Conversation

@davidmakepeace
Copy link
Contributor

davidmakepeace commented Jul 9, 2018

This PR adds a new EVP/KDF API that collects all KDFs and PRFs under the one API.

#include <openssl/kdf.h>

Currently some KDFs are available directly (PBKDF2 and scrypt) while others are available through the PKEY API (scrypt, TLS1 PRF and HKDF) even though the PKEY API was originally intended for Public Key algorithms. Note that the low level KDF functions (PKCS5_PBKDF2_HMAC and EVP_PBE_scrypt) are defined in openssl/evp.h even though EVP is the high level interface.

The new API is modeled after the current KDF support under the PKEY API in order to minimize the work required to update applications to use the new API. Support is included for PBKDF2, scrypt, TLS1 PRF and HKDF. PBKDF1 and PKCS12 KDF are not implemented.

The low level KDF functions and PKEY KDF support have been changed to wrap the new API. Other parts of OpenSSL have not yet been updated to call the new API.

Documentation to follow.

  • documentation is added or updated
  • tests are added or updated

@davidmakepeace davidmakepeace changed the title WIP Add EVP/KDF API WIP: Add EVP/KDF API Jul 9, 2018

@davidmakepeace davidmakepeace force-pushed the davidmakepeace:add-evp-kdf-api branch 2 times, most recently from a4b5940 to 1615c66 Jul 9, 2018

Show resolved Hide resolved include/openssl/evp_kdf.h Outdated

@davidmakepeace davidmakepeace force-pushed the davidmakepeace:add-evp-kdf-api branch 4 times, most recently from 868ad00 to 2a1efeb Jul 11, 2018

@mattcaswell mattcaswell added this to the Post 1.1.1 milestone Jul 13, 2018

@paulidale

This comment has been minimized.

Copy link
Contributor

paulidale commented Jul 24, 2018

We've got a single step KDF (SP 800-56) implemented and passing test vectors but it will need some cleanup to fit into this framework. The intention is to submit this it after this PR has been accepted or at least after the final form of these changes are determined.

@paulidale

This comment has been minimized.

Copy link
Contributor

paulidale commented Jul 26, 2018

Thought from today: this interface should deal with NULL result buffers better. Some of the algorithms use this to request the size of the result. The rest are variably sized and don't support this functionality. Uniformity would be better here.

My suggestion is that a new API is introduced that requests the size of the result (which matches digests) and that this returns 0 or -1 for variable size outcomes (I favour zero). Then, the NULL checking can be removed from the new APIs and handed off to the legacy wrappers.

Thoughts?

@t-j-h

This comment has been minimized.

Copy link
Member

t-j-h commented Aug 27, 2018

Do we really need the uint64 in the scrypt interface?

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Aug 27, 2018

The scrypt page on tarsnap seems to indicate that at least N can be a large enough integer to warrant 64 bits...

@t-j-h

This comment has been minimized.

Copy link
Member

t-j-h commented Aug 27, 2018

Sure it can technically go that high - but pragmatically would anyone ever exceed 2^32 - that I seriously doubt is realistic in any context

@paulidale paulidale referenced this pull request Sep 20, 2018

Closed

Initial KDF contribution #368

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 20, 2018

@paulidale, it would be great if you could rebase and fix the conflicts.
(re crypto/err/openssl.txt, crypto/evp/evp_err.c, include/openssl/evperr.h and util/libcrypto.num, I would just toss them and do make update again)

Show resolved Hide resolved crypto/evp/evp_kdf.c Outdated
Show resolved Hide resolved include/openssl/evp_kdf.h Outdated
@paulidale

This comment has been minimized.

Copy link
Contributor

paulidale commented Sep 20, 2018

I'll ask @davidmakepeace to update tomorrow.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 20, 2018

Oh oops, I mixed you guys up... sorry :-/

@paulidale

This comment has been minimized.

Copy link
Contributor

paulidale commented Sep 20, 2018

I'm fatter and uglier. I figured it would have been hard to mix us up :)

Show resolved Hide resolved crypto/evp/pkey_kdf.c Outdated
@simo5

This comment has been minimized.

Copy link
Contributor

simo5 commented Sep 20, 2018

@davidmakepeace any chance you will rebase this PR soonish, it's been indicated as a base to be used to implement SSHKDF and it'd be nice to start on a fresh rebase that does not conflict with master

@davidmakepeace davidmakepeace force-pushed the davidmakepeace:add-evp-kdf-api branch 2 times, most recently from cd71808 to bfa7eaa Sep 20, 2018

Show resolved Hide resolved include/openssl/evp_kdf.h Outdated

@simo5 simo5 referenced this pull request Sep 22, 2018

Closed

Implement SSH KDF per RFC 4253 Section 7.2 #7290

2 of 2 tasks complete

@davidmakepeace davidmakepeace force-pushed the davidmakepeace:add-evp-kdf-api branch from 1cbe3c6 to bafba46 Sep 22, 2018

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Feb 11, 2019

Another thing I notice that PKCS5_PBKDF2_HMAC does in master is to assign an empty string to the passed password if that happens to be NULL...

@davidmakepeace

This comment has been minimized.

Copy link
Contributor Author

davidmakepeace commented Feb 12, 2019

Another thing I notice that PKCS5_PBKDF2_HMAC does in master is to assign an empty string to the passed password if that happens to be NULL...

Yes, and that case is now handled in PKCS5_PBKDF2_HMAC() at crypto/evp/p5_crpt2.c:36 for compatibility.
I will add an additional check in PKCS5_PBKDF2_HMAC() for salt being NULL and saltlen being zero to ensure that the documented semantics are maintained.

@davidmakepeace davidmakepeace force-pushed the davidmakepeace:add-evp-kdf-api branch from c767d9e to 35efab3 Feb 12, 2019

@davidmakepeace

This comment has been minimized.

Copy link
Contributor Author

davidmakepeace commented Feb 12, 2019

I added a fix for the PKCS5_PBKDF2_HMAC() salt == NULL issue and rebased to master.

@paulidale

This comment has been minimized.

Copy link
Contributor

paulidale commented Feb 12, 2019

It might be worthwhile to squash these down to a single commit with an all encompassing log message.

@davidmakepeace davidmakepeace force-pushed the davidmakepeace:add-evp-kdf-api branch from 35efab3 to 0178921 Feb 12, 2019

@davidmakepeace

This comment has been minimized.

Copy link
Contributor Author

davidmakepeace commented Feb 12, 2019

It might be worthwhile to squash these down to a single commit with an all encompassing log message.

Okay. I have just done that.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Feb 12, 2019

Ok, I'm happy. 2nd reviewer needed.

Show resolved Hide resolved crypto/kdf/scrypt.c
Show resolved Hide resolved crypto/kdf/scrypt.c
@paulidale
Copy link
Contributor

paulidale left a comment

Approved once the two nits are addressed (both should be non-code impacting) and the merge conflict resolved (ditto, it is the changes file).

@davidmakepeace davidmakepeace force-pushed the davidmakepeace:add-evp-kdf-api branch from 0178921 to 1c18331 Feb 12, 2019

@paulidale

This comment has been minimized.

Copy link
Contributor

paulidale commented Feb 12, 2019

The build failures are unrelated to this.

@paulidale
Copy link
Contributor

paulidale left a comment

Reaffirmed after nit fixes and rebase.

Added new EVP/KDF API.
Changed PKEY/KDF API to call the new API.
Added wrappers for PKCS5_PBKDF2_HMAC() and EVP_PBE_scrypt() to call the new EVP KDF APIs.
Documentation updated.

@davidmakepeace davidmakepeace force-pushed the davidmakepeace:add-evp-kdf-api branch from 1c18331 to 7a9cf03 Feb 13, 2019

@davidmakepeace

This comment has been minimized.

Copy link
Contributor Author

davidmakepeace commented Feb 13, 2019

I just rebased, now that master has been fixed.
We should get a clean build now.

@levitte
Copy link
Member

levitte left a comment

Reaffirmed

@levitte levitte added ready and removed pending 2nd review labels Feb 13, 2019

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Feb 13, 2019

Merged.

5a285ad Added new EVP/KDF API. Changed PKEY/KDF API to call the new API. Added wrappers for PKCS5_PBKDF2_HMAC() and EVP_PBE_scrypt() to call the new EVP KDF APIs. Documentation updated.

@levitte levitte closed this Feb 13, 2019

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Feb 13, 2019

This was great work, @davidmakepeace

levitte added a commit that referenced this pull request Feb 13, 2019

Added new EVP/KDF API.
Changed PKEY/KDF API to call the new API.
Added wrappers for PKCS5_PBKDF2_HMAC() and EVP_PBE_scrypt() to call the new EVP KDF APIs.
Documentation updated.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #6674)
@paulidale

This comment has been minimized.

Copy link
Contributor

paulidale commented Feb 13, 2019

219 days in coming. A great effort and a very worthwhile contribution. Thanks to all who assisted and contributed.

The various follow on KDFs can now be tapped onto this work. @slontis has single step KDF, @romen has one too I believe. No doubt there are others. Let the floodgates open to the pressure of the 617th.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Feb 13, 2019

This search shows all the KDF related PRs. I'll have a quick run through, as I'm sure some can just be closed by now

@levitte levitte referenced this pull request Feb 13, 2019

Closed

Add support for HKDF context initialization #4968

0 of 1 task complete

@davidmakepeace davidmakepeace deleted the davidmakepeace:add-evp-kdf-api branch Feb 18, 2019

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.