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

WIP: Add EC_FLAG_EXT_PKEY #2243

Closed
wants to merge 1 commit into from
Closed

Conversation

tmshort
Copy link
Contributor

@tmshort tmshort commented Jan 17, 2017

RSA has the RSA_FLAG_EXT_PKEY and RSA_METHOD_FLAG_NO_CHECK that
are used to indicate whether a key should be checked because it
is stored externally. These flags are somewhat redundant in the
case of RSA, but are missing documentation, and are missing from
the EC code. So, add EC_FLAG_EXT_PKEY to allow an ENGINE
implementation of EC to store private EC keys external to OpenSSL.

This also adds some missing RSA functionality (i.e. flag retreival).

Checklist
  • documentation is added or updated
  • tests are added or updated
  • CLA is signed
Description of change

RSA has the RSA_FLAG_EXT_PKEY and RSA_METHOD_FLAG_NO_CHECK that
are used to indicate whether a key should be checked because it
is stored externally. These flags are somewhat redundant in the
case of RSA, but are missing documentation, and are missing from
the EC code. So, add EC_FLAG_EXT_PKEY to allow an ENGINE
implementation of EC to store private EC keys external to OpenSSL.

This also adds some missing RSA functionality (i.e. flag retreival).
@t8m
Copy link
Member

t8m commented Jan 18, 2017

There is already RSA_test_flags() which can be used instead of RSA_get_flags().
However for better regularity of the API I believe either this call or EC_KEY_test_flags() call should be added.

@snhenson
Copy link
Contributor

There shouldn't be any need for this if the public key components are included in the private key. Some functionality requires it (for example generating a certificate request) so having an EVP_PKEY which does not contain public key component will cause at least some things to misbehave.

If the intention of this flag is to cover the case where its is not possible to include public key components in an EVP_PKEY then (with the above caveats) this could work. However I think that should be done in an algorithm neutral way and not hard coding algorithm specific behaviour in EVP_PKEY functions.

@tmshort
Copy link
Contributor Author

tmshort commented Jan 18, 2017

There is already RSA_test_flags() which can be used instead of RSA_get_flags().
However for better regularity of the API I believe either this call or EC_KEY_test_flags() call should be added.

I am not a fan of overloading the use of RSA_test_flags() (or any other test-type-function) to get flags, since it requires a magic value to be used to get all the flags. In addition, there is a big comment about how RSA_flags() gets the RSA-method flags not the RSA-object flags, so having an RSA_get_flags() function that gets the RSA-object flags is used to balance that.

@tmshort
Copy link
Contributor Author

tmshort commented Jan 18, 2017

There shouldn't be any need for this if the public key components are included in the private key. Some functionality requires it (for example generating a certificate request) so having an EVP_PKEY which does not contain public key component will cause at least some things to misbehave.

I completely agree with you on that, and in fact my PR #1130, takes full advantage of that behavior.

If the intention of this flag is to cover the case where its is not possible to include public key components in an EVP_PKEY then (with the above caveats) this could work. However I think that should be done in an algorithm neutral way and not hard coding algorithm specific behaviour in EVP_PKEY functions.

Understood, this was intended to match the behavior of RSA_FLAG_EXT_KEY and RSA_METHOD_FLAG_NO_CHECK for EC_KEYs.

@petrovr
Copy link

petrovr commented Jan 21, 2017

What is reason to add new function that return if key is external?
Implementation of key method (RSA,DSA,ECDSA,EC) that manage operations with externally stored keys does not require flags to be set. For instance you could check code of CAPI engine - private key material is external and engines does not set flags.

@tmshort
Copy link
Contributor Author

tmshort commented Jan 31, 2017

Closing this PR, it's not that useful.

@tmshort tmshort closed this Jan 31, 2017
@anirudhvr
Copy link

I found this PR when attempting to do something similar (add EC_KEY_EXT_PKEY). The use-case I have is to set a "dummy" private key - whether RSA or EC - in an SSL_CTX; this is required when you want to keep the private key external to the program handling the TLS handshake and still want the SSL_CTX_use_PrivateKey to succeed. (Note - I do not want the overhead of calling out to the engine/system that holds the private key until an actual handshake happens.)

I'm able to do this for RSA keys using RSA_FLAG_EXT_PKEY and RSA_METHOD_FLAG_NO_CHECK, but the EC key verification fails without a patch like this.

@tmshort @snhenson would this be reasonable to reopen?

@anirudhvr
Copy link

@openssl or @tmshort - any comments / can i re-open + re-submit a similar patch?

@t-j-h
Copy link
Member

t-j-h commented Nov 21, 2017

This could be implemented as a simple addition on an EC-specific flag and a repeat of the two call usages as per the RSA content rather than adding new functions etc - i.e. the addition of the new functions was unnecessary in my view for this specific context (they are not used widely enough in the code).

@anirudhvr
Copy link

Yes, that sounds fine as well. I will submit another PR. Thanks!

@tmshort
Copy link
Contributor Author

tmshort commented Nov 22, 2017

Where this originally came from, I believe we decided upon an alternative mechanism to do this. However, I'd be interested in reviewing it.

@anirudhvr
Copy link

Sorry for the delay; opened #4850.

@tmshort tmshort deleted the tshort-ec_no_check branch July 15, 2020 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants