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

Move X25519/X448 to the default provider #10964

Closed
wants to merge 6 commits into from

Conversation

mattcaswell
Copy link
Member

This doesn't yet implement the required serializers. That will come in a subsequent PR.

@mattcaswell mattcaswell added the branch: master Merge to master branch label Jan 28, 2020
crypto/ec/ecx_meth.c Outdated Show resolved Hide resolved
crypto/ec/ecx_meth.c Outdated Show resolved Hide resolved
crypto/ec/ecx_meth.c Outdated Show resolved Hide resolved
crypto/ec/ecx_meth.c Outdated Show resolved Hide resolved
include/crypto/ecx.h Outdated Show resolved Hide resolved
providers/implementations/exchange/ecx_exch.c Show resolved Hide resolved
providers/implementations/keymgmt/ecx_kmgmt.c Outdated Show resolved Hide resolved
@paulidale
Copy link
Contributor

Travis is also relevant.

crypto/ec/ecx_key.c Outdated Show resolved Hide resolved
Copy link
Member

@slontis slontis left a comment

Choose a reason for hiding this comment

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

Looks pretty good apart from a few small NITS.

@romen
Copy link
Member

romen commented Feb 4, 2020

Just cross-mentioning the related issues: #10353 and #10354

@mattcaswell
Copy link
Member Author

Fixup commits pushed addressing the above comments and the travis failure.

Please take another look.

@mattcaswell
Copy link
Member Author

Oh...and also I realised I missed off the s390 support. Now added.

@p-steuer

Comment on lines 1 to 4
#include "s390x_arch.h"

.text

Copy link
Member

Choose a reason for hiding this comment

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

Where has this file come from?

Copy link
Member

Choose a reason for hiding this comment

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

Its generated by s390xcpuid.pl, @mattcaswell probably added it accidentially.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I did. I'll remove it.

Copy link
Member

@slontis slontis left a comment

Choose a reason for hiding this comment

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

It LGTM..
I would prefer if @p-steuer reviewed the s390 bits though.

@mattcaswell
Copy link
Member Author

Fixed the s390xcpuid.S issue. The travis red cross was due to a bad auto-merge with master during the travis build. I've rebased to resolve that issue.

Please take another look.

@p-steuer
Copy link
Member

p-steuer commented Feb 6, 2020

With moving the s390 code to provider, the corresponding run-time feature detection was lost. My tests of the S390X_EC_ASM build pass on z15 but it should SIGILL on any older machine, since ECC instruction set extension is not available there.

Specifically im talking about the following (non provider) code from crypto/ec/ecx_meth.c:

const EVP_PKEY_METHOD *ecx25519_pkey_method(void)
{
#ifdef S390X_EC_ASM
    if (OPENSSL_s390xcap_P.pcc[1] & S390X_CAPBIT(S390X_SCALAR_MULTIPLY_X25519))
        return &ecx25519_s390x_pkey_meth;
#endif
    return &ecx25519_pkey_meth;
}

const EVP_PKEY_METHOD *ecx448_pkey_method(void)
{
#ifdef S390X_EC_ASM
    if (OPENSSL_s390xcap_P.pcc[1] & S390X_CAPBIT(S390X_SCALAR_MULTIPLY_X448))
        return &ecx448_s390x_pkey_meth;
#endif
    return &ecx448_pkey_meth;
}

This could be easily fixed by checking the above conditions inside provider code's S390X_EC_ASM ifdefs (before every operation).

However, since this check is only required one time at init, i would prefer a solution similar to the non-provider code (to also get rid of a lot of ifdefs and to facilitate integration of future platform specific hw support). E.g. replace member pointing to an OPENSSL_DISPATCH object in OPENSSL_ALGORITHM structure by a member pointing to a function returning such objects. That way, you could define both a platform-spefific and a generic OPENSSL_DISPATCH array, and said function could chose which one to return, based on the hw capabilities detected on init (analogous to what i did to the standard_methods array here: 19bd1fa).

@mattcaswell
Copy link
Member Author

This could be easily fixed by checking the above conditions inside provider code's S390X_EC_ASM ifdefs (before every operation).

Fixed. I have gone with this simple solution for now. I did look at an option for a one time capability check. We actually have a mechanism for that for ciphers. See:

ALGC("AES-128-CBC-HMAC-SHA1", aes128cbc_hmac_sha1_functions,
cipher_capable_aes_cbc_hmac_sha1),
ALGC("AES-256-CBC-HMAC-SHA1", aes256cbc_hmac_sha1_functions,
cipher_capable_aes_cbc_hmac_sha1),
ALGC("AES-128-CBC-HMAC-SHA256", aes128cbc_hmac_sha256_functions,
cipher_capable_aes_cbc_hmac_sha256),
ALGC("AES-256-CBC-HMAC-SHA256", aes256cbc_hmac_sha256_functions,
cipher_capable_aes_cbc_hmac_sha256),

It would not take much to extend that to key exchange. However in this case I think the benefit is very minimal at the cost of quite a lot of extra plumbing. I'm not actually sure you would gain anything very much.

@mattcaswell mattcaswell added the approval: review pending This pull request needs review by a committer label Feb 6, 2020
if (ecxctx->keylen == X25519_KEYLEN) {
#ifdef S390X_EC_ASM
if (OPENSSL_s390xcap_P.pcc[1]
& S390X_CAPBIT(S390X_SCALAR_MULTIPLY_X448)) {
Copy link
Member

Choose a reason for hiding this comment

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

S390X_SCALAR_MULTIPLY_X448 -> S390X_SCALAR_MULTIPLY_X25519

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops. Fixed.

@slontis
Copy link
Member

slontis commented Feb 7, 2020

needs a rebase

@slontis
Copy link
Member

slontis commented Feb 8, 2020

@mattcaswell

@mattcaswell
Copy link
Member Author

needs a rebase

Sigh. Needs a bit more work due to the all the keymgmt changes in master.

Add ref counting and control how we allocate storage for the private key.
We will need this type in following commits where we move the ecx code
to be provider aware.
@mattcaswell
Copy link
Member Author

I rebased this and reworked it to take account of the refactored keymgmt in master.

Please take another look and reconfirm?

@p-steuer p-steuer self-requested a review February 10, 2020 16:45
@mattcaswell
Copy link
Member Author

Fixups pushed addressing both of those issues.

Copy link
Member

@p-steuer p-steuer left a comment

Choose a reason for hiding this comment

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

Looks good to me now.

@mattcaswell mattcaswell 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 Feb 10, 2020
@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 Feb 11, 2020
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request Feb 11, 2020
Add ref counting and control how we allocate storage for the private key.
We will need this type in following commits where we move the ecx code
to be provider aware.

Reviewed-by: Patrick Steuer <patrick.steuer@de.ibm.com>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #10964)
openssl-machine pushed a commit that referenced this pull request Feb 11, 2020
Reviewed-by: Patrick Steuer <patrick.steuer@de.ibm.com>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #10964)
openssl-machine pushed a commit that referenced this pull request Feb 11, 2020
Reviewed-by: Patrick Steuer <patrick.steuer@de.ibm.com>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #10964)
openssl-machine pushed a commit that referenced this pull request Feb 11, 2020
Reviewed-by: Patrick Steuer <patrick.steuer@de.ibm.com>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #10964)
@mattcaswell
Copy link
Member Author

Pushed. Thanks.

@romen romen mentioned this pull request Feb 14, 2020
22 tasks
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.

6 participants