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.

Sign up for GitHub

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

Jump to bottom

New API Proposal: PBKDF2 #98

Merged

Lukasa merged 27 commits into apple:main from admkopec:stagging

Oct 18, 2024

Contributor

admkopec commented Dec 9, 2021 •

edited

Loading

I've proposed an implementation of PBKDF2 algorithm using the CryptoKit style. As per issue #59

Checklist

I've run tests to see all new and existing tests pass
I've followed the code style of the rest of the project
I've read the Contribution Guidelines
I've updated the documentation if necessary

If you've made changes to `gyb` files

I haven't made any changes to gyb files.

New API Proposal: Password based KDF #59

Motivation:

I've added a proposed implementation of PBKDF2, as per proposal in issue #59 to allow easy creation of SymmetricKey from relatively low entropy data sets, such as user provided passwords.

Importance:

This API change is attempting to simplify the creation of SymmetricKeys from user entered passwords.
It uses an ageing algorithm – PBKDF2, but it is still widely supported and popular. A newer algorithm – scrypt is added as a better, more secure alternative.

Modifications:

I've proposed a new API: KDF namespace with Scrypt struct and an Insecure namespace with PBKDF2 struct with associated hash function and a single static method which takes a passphrase and salt, the size of the resulting key in bytes and optionally the number of rounds to use in the PBKDF2 algorithm.
In the case of scrypt algorithm there is no associated hash function due to the nature of the algorithm, the single static method takes passphrase and salt, the size of the resulting key in bytes and optionally: the number of rounds, block size and parallelism factor.
I implemented this functions using both CommonCrypto and BoringSSL where available.
The function names and parameters are heavily influenced by current HKDF implementation.

Additionally, I've added test cases to test the proposed API. I've decided to use test vectors described in RFC6070 for PBKDF2 and test vectors described in RFC7914 for scrypt.

Result:

This will add a new API to _CryptoKitExtras.
It follows the naming conventions from CryptoKit's HKDF implementation.

swift-server-bot commented Dec 9, 2021

Can one of the admins verify this patch?

7 similar comments

swift-server-bot commented Dec 9, 2021

Can one of the admins verify this patch?

swift-server-bot commented Dec 9, 2021

Can one of the admins verify this patch?

swift-server-bot commented Dec 9, 2021

Can one of the admins verify this patch?

swift-server-bot commented Dec 9, 2021

Can one of the admins verify this patch?

swift-server-bot commented Dec 9, 2021

Can one of the admins verify this patch?

swift-server-bot commented Dec 9, 2021

Can one of the admins verify this patch?

swift-server-bot commented Dec 9, 2021

Can one of the admins verify this patch?

Contributor

Lukasa commented Dec 10, 2021

Thanks for filing this! I wanted to let you know that I've seen it, and I'm going to discuss with my colleagues to work out whether PBKDF2 is a good fit here.

Contributor Author

admkopec commented Dec 10, 2021 •

edited

Loading

Possibly Argon2 might be a better choice here, but due to its lack of support in both CommonCrypto and BoringSSL, I decided to implement PBKDF2 instead. Please let me know when you decide, if PBKDF2 should be added here.

admkopec marked this pull request as ready for review

December 18, 2021 21:51

sbhuiyan20 reviewed

View reviewed changes

Sources/Crypto/Key Derivation/BoringSSL/PBKDF2_commoncrypto.swift Outdated

+                  }
+              }
+              #endif

sbhuiyan20 Jan 20, 2022

Suggested change

      
            #endif
          
            #endif

MPLew-is commented Jul 17, 2022

@Lukasa any other updates or thoughts on this? This is the one thing stopping me from switching fully to swift-crypto from other third-party packages.

nerzh commented Sep 22, 2023

any updates ?

Contributor

Lukasa commented Sep 25, 2023

Not at this time, I'm afraid.

Contributor

lovetodream commented Dec 20, 2023

There is a discussion about this going on here. @Lukasa, how do you want to continue? Maybe @admkopec has time to move this to _CryptoExtas? I'm open to help, both in this case and in adding alternatives, if there's anything I can do.

Contributor

Lukasa commented Mar 12, 2024

Sorry for the delay here. Yes, I'm open to taking a PR for PKBDF2 with the following notes:

The API surface should be added in _CryptoExtras.
PBKDF2 should be namespaced under a KDF namespace
PBKDF2 should be further namespaced under an Insecure namespace within KDF, to discourage its use
A version of scrypt should be added as well

Is that acceptable?

MPLew-is commented Mar 12, 2024

Would again advocate for an Argon2 implementation, even if it’s just wrapping the reference C implementation for now.

Contributor Author

admkopec commented Mar 12, 2024

I'll start moving the implementation over to _CryptoExtras and will take a look at scrypt. But as for the better alternative, I'd still vote for Argon2 over scrypt to be honest even if it would mean adding an external dependency to _CryptoExtras.

MPLew-is commented Mar 12, 2024

even if it would mean adding an external dependency to _CryptoExtras

While I'm sure that there are more performant implementations, the reference implementation hasn't been updated in 3 years and the algorithm is final at this point, so you could probably even just copy the source directly into _CryptoExtras and avoid the external dependency. I've been running a project with basically that exact setup for a few years with no issues, running on both Mac and Linux.

Contributor

Lukasa commented Mar 13, 2024

I'm open to us adding Argon2, though I want to have a discussion with my colleagues about whether the reference implementation is the right way to do that. But for now let's focus on PBKDF2 and scrypt: one problem at a time!

nerzh commented Mar 13, 2024

Perfect answer, please implement already, we've been waiting for over 2 years. We are not comfortable including openssl header in our code. Just do it please, we'd rather start writing code on Rust than wait another 2 years for Argon2

Contributor Author

admkopec commented Mar 14, 2024

@Lukasa I've implemented the changes you suggested. Please verify if this is an acceptable solution.

Lukasa reviewed

View reviewed changes

Contributor

Lukasa left a comment

Thanks for this! Some initial notes in the diff. I've mostly restrained my review to the product code for now, I'll review the tests later.

Sources/_CryptoExtras/Key Derivation/PBKDF2/PBKDF2.swift Outdated Show resolved Hide resolved

Sources/_CryptoExtras/Key Derivation/PBKDF2/BoringSSL/PBKDF2_boring.swift Outdated Show resolved Hide resolved

Sources/_CryptoExtras/Key Derivation/PBKDF2/BoringSSL/PBKDF2_boring.swift Outdated Show resolved Hide resolved

Sources/_CryptoExtras/Key Derivation/PBKDF2/BoringSSL/PBKDF2_boring.swift Outdated Show resolved Hide resolved

Sources/_CryptoExtras/Key Derivation/PBKDF2/BoringSSL/PBKDF2_boring.swift Outdated Show resolved Hide resolved

Sources/_CryptoExtras/Key Derivation/PBKDF2/BoringSSL/PBKDF2_commoncrypto.swift Show resolved Hide resolved

Sources/_CryptoExtras/Key Derivation/Scrypt/BoringSSL/Scrypt_boring.swift Outdated Show resolved Hide resolved

Sources/_CryptoExtras/Key Derivation/Scrypt/BoringSSL/Scrypt_boring.swift Outdated Show resolved Hide resolved

Sources/_CryptoExtras/Key Derivation/Scrypt/Scrypt.swift Outdated Show resolved Hide resolved

nerzh commented Apr 1, 2024

Hi @admkopec, can you please tell me if this pullrequest will be developed further ?

Contributor Author

admkopec commented Apr 1, 2024

@nerzh Yes, sorry for the delay, a more urgent thing came up on my side. I'll implement the necessary changes as soon as I get a chance to get back to this PR.

nerzh commented Apr 1, 2024

@admkopec glad to hear you have plans to complete this much needed PR. Thanks for the quick response.

Contributor Author

admkopec commented May 2, 2024

Once again I am very sorry for the delay. I have implemented suggestions from the review. @Lukasa, Please take a look if it is acceptable now.

nerzh commented Jun 11, 2024

@Lukasa ping for this PR 🙂

admkopec force-pushed the stagging branch from 0329183 to 8042b7b Compare

June 11, 2024 11:37

admkopec force-pushed the stagging branch from fcaf437 to add4813 Compare

June 18, 2024 10:18

admkopec added 3 commits

June 18, 2024 12:26


          Dropped the underscore from insecure prefix in HashFunctions struct

c96811a


          Merge branch 'main' into stagging

746fd49


          Merge branch 'main' into stagging

506278d

admkopec requested a review from Lukasa

June 30, 2024 18:41

Contributor

Lukasa commented Jul 1, 2024

Just a note that I’m on vacation right now so I can’t review, but will review when I’m back.


          Merge branch 'main' into stagging

4fea9d5

nerzh commented Aug 3, 2024

Just a note that I’m on vacation right now so I can’t review, but will review when I’m back.

Hello, perhaps you have already rested and can take a closer look at this pull request 🙃


          Merge branch 'main' into stagging

f8b571b

nerzh commented Aug 10, 2024

@admkopec thank you for your work and patience, I hope this pull request will be accepted before the whole world switches to argon2 or scrypt

Lukasa reviewed

View reviewed changes

Sources/_CryptoExtras/Key Derivation/PBKDF2/PBKDF2.swift

+                      ///    - outputByteCount: The length in bytes of resulting symmetric key.
+                      ///    - rounds: The number of rounds which should be used to perform key derivation.
+                      /// - Returns: The derived symmetric key.
+                      public static func deriveKey<Passphrase: DataProtocol, Salt: DataProtocol>(from password: Passphrase, salt: Salt, using hashFunction: HashFunction, outputByteCount: Int, rounds: Int) throws -> SymmetricKey {

Contributor

Lukasa Sep 30, 2024

This API generally looks good, but I'm wondering if we should add a minimum round count to avoid folks naively using the number 1. We should certainly add documentation suggesting a high round count (in the hundreds of thousands), but I can't see any world where we'd want to accept less than 1k.

If we have use-cases that absolutely need lower round counts I'd love to know, but those might be best served the way we serve RSA key sizes: with a second API that is a bit harder to justify typing.

Contributor

lovetodream Sep 30, 2024

I do actually have such a use case. User+password authentication in Oracle typically encrypts the password with 4096 iterations. Hashes it alongside other data using SHA512, does a bit of CBC de- and encryption with it and some more data. And finally encrypts the end result using another 3 rounds of PBKDF2. (https://github.com/lovetodream/oracle-nio/blob/7fee78535f68c9f3a451beeed15483bc6ac2877b/Sources/OracleNIO/Messages/Coding/OracleFrontendMessageEncoder.swift#L926-L974)

It might not be the same for every version of Oracle because this is controlled by the db server, but I confirmed it is the default behaviour with Oracle 23ai (newest release).

Contributor

Lukasa Sep 30, 2024

Ace, that's helpful. So I think that this then suggests we want two APIs, one like the one here and one closer that maybe uses the label unsafeUncheckedRounds (or similar).

My desire is to enable the use-case you have, while strongly discouraging users who pick up PBKDF2 without much thought from putting in extraordinarily low values.

Contributor

lovetodream Sep 30, 2024

Sounds good to me 👍

Contributor Author

admkopec Oct 13, 2024

I added the unsafeUncheckedRounds API method and a guard check in the regular API method to throw an incorrect parameter size error whenever the rounds is less than 1000. Please verify if this is acceptable.

If we now have the unsafe API method, is the 1000 rounds requirement enough though, as it was proposed way back in original RFC with intent to increase number of rounds over time?

Contributor

Lukasa Oct 14, 2024

Probably not. OWASP suggests a minimum round count of 210k for SHA512-based PBKDF2, and so maybe we should consider that the lower bound.

Sources/_CryptoExtras/Key Derivation/PBKDF2/PBKDF2.swift Outdated Show resolved Hide resolved

Sources/_CryptoExtras/Key Derivation/PBKDF2/PBKDF2.swift Outdated Show resolved Hide resolved

Sources/_CryptoExtras/Key Derivation/PBKDF2/PBKDF2.swift Outdated Show resolved Hide resolved

Sources/_CryptoExtras/Key Derivation/KDF.swift Outdated Show resolved Hide resolved

Sources/_CryptoExtras/Key Derivation/KDF.swift Outdated Show resolved Hide resolved

Sources/_CryptoExtras/Key Derivation/Scrypt/Scrypt.swift Outdated Show resolved Hide resolved

Contributor

Lukasa commented Sep 30, 2024

Apologies for the delays getting to this, this is now fairly high up my priority list so we should be able to get it in fairly soon.

Contributor

Lukasa commented Sep 30, 2024

@swift-server-bot add to allowlist

0xTim mentioned this pull request

Add Sensible APIs to support removing BoringSSL from the rest of the ecosystem #263

Open

5 tasks

admkopec added 5 commits

October 12, 2024 16:31


          Added Sendable conformance and explicit Int32 conversion for sysconf …

f5785f4

…function


          Fixed wrong references to hash functions

867eace


          Merge branch 'main' into stagging

589a5a6


          Changed Scrypt's type to enum

ee2b43d


          Added the proposed unsafeUncheckedRounds to PBKDF2 API method and a…

7bd24df

…dded a condition to not accept rounds under 1000 in regular API method

admkopec requested a review from Lukasa

October 13, 2024 18:01

admkopec and others added 2 commits

October 16, 2024 14:34


          Changed the minimum value of rounds parameter in PBKDF2 to 210,000 (a…

00205b2

…s per OWASP recommendations) and iImproved methods' documentation to better describe these requirements.


          Merge branch 'main' into stagging

3932d8a

Lukasa added the 🆕 semver/minor label

Contributor

Lukasa commented Oct 18, 2024

There are a few very minor tweaks to the license headers, as shown in the failing GH workflow. Otherwise this is basically good to go!


          CONTRIBUTORS.md -> CONTRIBUTORS.txt

635ea61

Contributor

Lukasa commented Oct 18, 2024

Actually, let's not bother with that: you've done more than enough to get this patch over the line. I've pushed a fix for those.

Lukasa approved these changes

View reviewed changes

Contributor

Lukasa left a comment

I'm happy with this, thanks so much for your diligent work!

Lukasa enabled auto-merge (squash)

October 18, 2024 17:13

Lukasa linked an issue

that may be closed by this pull request

Password based KDF #59

Closed

Lukasa merged commit 7cb6113 into apple:main

25 checks passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🆕 semver/minor