Skip to content

KDF: Introduce Argon2i, Argon2d, Argon2id - #12256

Closed
ckalina wants to merge 6 commits into
openssl:masterfrom
ckalina:mr_argon2_v2
Closed

KDF: Introduce Argon2i, Argon2d, Argon2id#12256
ckalina wants to merge 6 commits into
openssl:masterfrom
ckalina:mr_argon2_v2

Conversation

@ckalina

@ckalina ckalina commented Jun 24, 2020

Copy link
Copy Markdown
Contributor

Rework of #9444
Threading has since been singled out into a separate MR here: #12255 and is required for this set to go in.

Currently no RFC is available; IETF draft: https://datatracker.ietf.org/doc/draft-irtf-cfrg-argon2/

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

@ckalina ckalina mentioned this pull request Jun 24, 2020
Closed
1 task done
@guidovranken

guidovranken commented Jun 24, 2020

Copy link
Copy Markdown
Contributor

I tested this again with Cryptofuzz. Verifying against Botan's ARGON2 implementation. So far, output appears to be correct in all cases.

It found a minor flaw:

providers/implementations/kdfs/argon2.c:1128:12: runtime error: null pointer passed as argument 1, which is declared to never be null
/usr/include/string.h:43:28: note: nonnull attribute specified here

You can add return 1 to kdf_argon2_derive if outlen is 0 to circumvent this.

Once this is merged, long term fuzzing at OSS-Fuzz should bring to light any remaining issues.

@ckalina

ckalina commented Jun 24, 2020

Copy link
Copy Markdown
Contributor Author

@guidovranken, nice catch. Fixed (although it should return 0 as Argon2 requires outlen >= 4).

@ckalina

ckalina commented Jun 24, 2020

Copy link
Copy Markdown
Contributor Author

Rebased changes from #12255 (threading support MR), changed a handful of crypto_thread* function calls to conform to changes made therein.

slontis
slontis reviewed Jun 24, 2020
View reviewed changes
Comment thread include/openssl/kdf.h Outdated
slontis
slontis reviewed Jun 24, 2020
View reviewed changes
Comment thread include/crypto/evp.h Outdated
slontis
slontis reviewed Jun 24, 2020
View reviewed changes
Comment thread doc/man7/EVP_KDF-ARGON2.pod Outdated
slontis
slontis reviewed Jun 24, 2020
View reviewed changes
Comment thread include/openssl/evperr.h Outdated
slontis
slontis reviewed Jun 25, 2020
View reviewed changes
Comment thread doc/man7/EVP_KDF-ARGON2.pod Outdated
slontis
slontis reviewed Jun 25, 2020
View reviewed changes
Comment thread doc/man7/EVP_KDF-ARGON2.pod Outdated
slontis
slontis reviewed Jun 25, 2020
View reviewed changes
Comment thread providers/implementations/kdfs/argon2.c Outdated
slontis
slontis reviewed Jun 25, 2020
View reviewed changes
Comment thread test/evp_kdf_test.c Outdated
slontis
slontis reviewed Jun 25, 2020
View reviewed changes
Comment thread test/evp_kdf_test.c Outdated
slontis
slontis reviewed Jun 25, 2020
View reviewed changes
Comment thread test/evp_kdf_test.c Outdated
slontis
slontis reviewed Jun 25, 2020
View reviewed changes
Comment thread providers/implementations/kdfs/argon2.c Outdated
slontis
slontis reviewed Jun 25, 2020
View reviewed changes
Comment thread providers/implementations/kdfs/argon2.c Outdated
slontis
slontis reviewed Jun 25, 2020
View reviewed changes
Comment thread providers/implementations/kdfs/argon2.c Outdated
slontis
slontis reviewed Jun 25, 2020
View reviewed changes
Comment thread providers/implementations/kdfs/argon2.c Outdated
slontis
slontis reviewed Jun 25, 2020
View reviewed changes
Comment thread providers/implementations/kdfs/argon2.c Outdated
slontis
slontis reviewed Jun 25, 2020
View reviewed changes
Comment thread providers/implementations/kdfs/argon2.c Outdated
slontis
slontis reviewed Jun 25, 2020
View reviewed changes
Comment thread providers/implementations/kdfs/argon2.c Outdated
slontis
slontis reviewed Jun 25, 2020
View reviewed changes
Comment thread providers/implementations/kdfs/argon2.c Outdated
slontis
slontis reviewed Jun 25, 2020
View reviewed changes
Comment thread providers/implementations/kdfs/argon2.c Outdated
slontis
slontis reviewed Jun 25, 2020
View reviewed changes
Comment thread providers/implementations/kdfs/argon2.c Outdated
slontis
slontis reviewed Jun 25, 2020
View reviewed changes
Comment thread providers/implementations/kdfs/argon2.c Outdated
slontis
slontis reviewed Jun 25, 2020
View reviewed changes
Comment thread providers/implementations/kdfs/argon2.c Outdated
slontis
slontis reviewed Jun 25, 2020
View reviewed changes
Comment thread providers/implementations/kdfs/argon2.c Outdated
slontis
slontis reviewed Jun 25, 2020
View reviewed changes
Comment thread providers/implementations/kdfs/argon2.c Outdated
@t8m
t8m requested review from paulidale, romen and slontis March 14, 2023 10:29
paulidale
paulidale requested changes Mar 15, 2023
View reviewed changes

@paulidale paulidale left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One possibly typographic issue.

There should be a mention of this in doc/man7/OSSL_PROVIDER-default.pod. It's just this added (with blank lines in the Key Derivation Function section:

=item ARGON2, see L<EVP_KDF-ARGON2(7)>

Approved these are addressed, I'll approve.

A marathon effort, thanks for your persistence.

Comment thread test/evp_test.c
@ckalina
providers: add Argon2 KDF
5472147 
https://datatracker.ietf.org/doc/rfc9106/

Signed-off-by: Čestmír Kalina <ckalina@redhat.com>
@ckalina

ckalina commented Mar 15, 2023

Copy link
Copy Markdown
Contributor Author

Thank you for the reviews! Argon2 entry added into doc/man7/OSSL_PROVIDER-default.pod.

paulidale
paulidale approved these changes Mar 15, 2023
View reviewed changes

@paulidale paulidale left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally!

@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Mar 15, 2023
@paulidale paulidale removed this from the Post 3.0.0 milestone Mar 15, 2023
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Mar 17, 2023
@openssl-machine

openssl-machine commented Mar 17, 2023

Copy link
Copy Markdown
Collaborator

This pull request is ready to merge

@paulidale

paulidale commented Mar 17, 2023

Copy link
Copy Markdown
Contributor

And merged. Thanks for the perseverance on this.

@paulidale paulidale closed this Mar 17, 2023
openssl-machine pushed a commit that referenced this pull request Mar 17, 2023
@ckalina @paulidale
evp_test: add digest xoflen support
c8ebdd6 
Signed-off-by: Čestmír Kalina <ckalina@redhat.com>

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #12256)
openssl-machine pushed a commit that referenced this pull request Mar 17, 2023
@ckalina @paulidale
providers: add XOF support to blake2b
786b9a8 
Signed-off-by: Čestmír Kalina <ckalina@redhat.com>

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #12256)
openssl-machine pushed a commit that referenced this pull request Mar 17, 2023
@ckalina @paulidale
evp_test: add thread support
ae1792e 
Signed-off-by: Čestmír Kalina <ckalina@redhat.com>

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #12256)
openssl-machine pushed a commit that referenced this pull request Mar 17, 2023
@ckalina @paulidale
proverr: add PROV_R_{INVALID_MEMORY_SIZE,INVALID_THREAD_POOL_SIZE}
232dd87 
Signed-off-by: Čestmír Kalina <ckalina@redhat.com>

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #12256)
openssl-machine pushed a commit that referenced this pull request Mar 17, 2023
@ckalina @paulidale
e_os2: add ossl_static_assert_type_eq
0e200d2 
Add a gcc-only static assertion that a variable is of a specified type.

Signed-off-by: Čestmír Kalina <ckalina@redhat.com>

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #12256)
openssl-machine pushed a commit that referenced this pull request Mar 17, 2023
@ckalina @paulidale
providers: add Argon2 KDF
6dfa998 
https://datatracker.ietf.org/doc/rfc9106/

Signed-off-by: Čestmír Kalina <ckalina@redhat.com>

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #12256)
@mbroz

mbroz commented Mar 17, 2023

Copy link
Copy Markdown
Member

Thanks everyone!

For the record how long marathon this was, it started with @ckalina bachelor thesis in 2020 ... Just a few years and we are finally there :-)

@mattcaswell

mattcaswell commented Mar 17, 2023

Copy link
Copy Markdown
Member

Good news!! Thanks @ckalina for all your efforts!

@MarkusTeufelberger MarkusTeufelberger mentioned this pull request May 17, 2023
Closed
@smessmer smessmer mentioned this pull request Jul 13, 2023
Closed
@ckalina
ckalina deleted the mr_argon2_v2 branch August 12, 2023 15:52
@ranisalt ranisalt mentioned this pull request Oct 24, 2023
Merged
@BenMorganMY BenMorganMY mentioned this pull request May 22, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@levitte levitte levitte left review comments
@kroeckx kroeckx kroeckx left review comments
@mattcaswell mattcaswell mattcaswell left review comments
@t8m t8m t8m approved these changes
@paulidale paulidale paulidale approved these changes
@hlandau hlandau Awaiting requested review from hlandau
@slontis slontis Awaiting requested review from slontis
@romen romen Awaiting requested review from romen
+5 more reviewers
@matbech matbech matbech left review comments
@richsalz richsalz richsalz left review comments
@docularxu docularxu docularxu left review comments
@FdaSilvaYY FdaSilvaYY FdaSilvaYY left review comments
@osamahassani05355881581 osamahassani05355881581 osamahassani05355881581 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch severity: fips change The pull request changes FIPS provider sources tests: present The PR has suitable tests present triaged: feature The issue/pr requests/adds a feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.