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

mac: deprecate low level calls to CMAC and HMAC. #10836

Closed
wants to merge 5 commits into from

Conversation

paulidale
Copy link
Contributor

Use of the low level CMAC and HMAC functions has been informally discouraged for a long time. We now formally deprecate them.

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

@paulidale paulidale added the branch: master Merge to master branch label Jan 14, 2020
@paulidale
Copy link
Contributor Author

CMAC is complete. HMAC is more problematic.

include/openssl/hmac.h Outdated Show resolved Hide resolved
@paulidale paulidale marked this pull request as ready for review January 24, 2020 02:46
@paulidale paulidale changed the title WIP: mac: deprecate low level calls to CMAC and HMAC. mac: deprecate low level calls to CMAC and HMAC. Jan 24, 2020
@paulidale
Copy link
Contributor Author

Bringing this out of WiP for review.

The Travis failure isn't relevant.

apps/lib/s_cb.c Outdated Show resolved Hide resolved
apps/lib/s_cb.c Outdated Show resolved Hide resolved
apps/lib/s_cb.c Show resolved Hide resolved
CHANGES Show resolved Hide resolved
doc/man3/SSL_CTX_set_tlsext_ticket_key_cb.pod Show resolved Hide resolved
ssl/statem/statem_srvr.c Outdated Show resolved Hide resolved
ssl/t1_lib.c Outdated Show resolved Hide resolved
ssl/t1_lib.c Outdated Show resolved Hide resolved
test/sslapitest.c Outdated Show resolved Hide resolved
* Test 10: TLSv1.2, ticket key callback, ticket, renewal
* Test 11: TLSv1.3, ticket key callback, ticket, renewal
*/
static int test_ticket_callbacks(int tst)
Copy link
Member

Choose a reason for hiding this comment

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

Surely test_ticket_callbacks and test_old_ticket_callbacks can be combined in some way. Virtually of the code is identical. Tests 0-7 are identical aren't they (no ticket key callback)? Probably we should have just one function with tests 12-15 added which use the new callback instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this but got worried about the #ifdefs. I'll revisit the idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This ended up pretty clean.

@paulidale
Copy link
Contributor Author

I think that's all the comments addressed.

test/bad_dtls_test.c Outdated Show resolved Hide resolved
test/bad_dtls_test.c Outdated Show resolved Hide resolved
@paulidale
Copy link
Contributor Author

Ping?

@levitte
Copy link
Member

levitte commented Jan 27, 2020

Ping?

This does look good enough to me, but I would like to see @mattcaswell pitch in one last time as well before approval.

Use of the low level CMAC functions has been informally discouraged for a
long time.  We now formally deprecate them.

Applications should instead use EVP_MAC_CTX_new(3), EVP_MAC_CTX_free(3),
EVP_MAC_init(3), EVP_MAC_update(3) and EVP_MAC_final(3).
Use of the low level HMAC functions has been informally discouraged for a
long time.  We now formally deprecate them.

Applications should instead use EVP_MAC_CTX_new(3), EVP_MAC_CTX_free(3),
EVP_MAC_init(3), EVP_MAC_update(3) and EVP_MAC_final(3).
Backwards compatibility with the old ticket key call back is maintained.
This will be removed when the low level HMAC APIs are finally removed.
Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

LGTM

@mattcaswell mattcaswell added the approval: done This pull request has the required number of approvals label Jan 28, 2020
@paulidale paulidale 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 Jan 29, 2020
@paulidale
Copy link
Contributor Author

Merged to master. Thanks for the positive input.

@paulidale paulidale closed this Jan 29, 2020
@paulidale paulidale deleted the mac branch January 29, 2020 09:51
openssl-machine pushed a commit that referenced this pull request Jan 29, 2020
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10836)
openssl-machine pushed a commit that referenced this pull request Jan 29, 2020
Use of the low level CMAC functions has been informally discouraged for a
long time.  We now formally deprecate them.

Applications should instead use EVP_MAC_CTX_new(3), EVP_MAC_CTX_free(3),
EVP_MAC_init(3), EVP_MAC_update(3) and EVP_MAC_final(3).

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10836)
openssl-machine pushed a commit that referenced this pull request Jan 29, 2020
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10836)
openssl-machine pushed a commit that referenced this pull request Jan 29, 2020
Use of the low level HMAC functions has been informally discouraged for a
long time.  We now formally deprecate them.

Applications should instead use EVP_MAC_CTX_new(3), EVP_MAC_CTX_free(3),
EVP_MAC_init(3), EVP_MAC_update(3) and EVP_MAC_final(3).

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10836)
openssl-machine pushed a commit that referenced this pull request Jan 29, 2020
Backwards compatibility with the old ticket key call back is maintained.
This will be removed when the low level HMAC APIs are finally removed.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10836)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants