Add wrapper method for EVP_PKEY_cmp to compare same type keys

[cwjenkins]

No description provided.

[cwjenkins]

Ideally these are added to PKey and overridden in subtypes, but my time is limited and not sure if this approach is desired @rhenium

[rhenium]

Do you have some use cases in mind?

Implementing proper comparison methods on pkey objects can be tricky because the EVP_PKEY is an abstraction interface. The key components are not necessarily accessible from us if the pkey object is not using OpenSSL's default implementation - for example, HSM-backed keys as created by OpenSSL::Engine#load_private_key. It's even more complicated for RSA, DSA, DH, and EC_KEY as they have another layer of abstraction each (which is being deprecated in favor of EVP_PKEY's, but they still exist).

Because of the reason above, it's not possible to properly define equality on all pkeys. Would it be still useful if they can return false even though the two keys are identical?

[cwjenkins]

Yes, in one of our apps we generate certificate signing requests which in turn will have a user upload a X509 certificate in which we wish to validate its RSA public key matches that of the RSA private key it corresponds too.
This is common in other languages FYI.

Golang: https://github.com/golang/go/blob/master/src/crypto/rsa/rsa.go#L62
Rust: https://docs.rs/openssh-keys/0.4.1/src/openssh_keys/lib.rs.html#164
Java: https://github.com/openjdk/jdk/blob/jdk9-b94/jdk/src/java.base/share/classes/sun/security/x509/X509Key.java#L428

I'm not sure I follow. I understand that EVP abstracts away details, but it itself stores knowledge of the alg used via its type attribute which can be fetched via EVP_PKEY_id https://www.openssl.org/docs/man1.1.1/man3/EVP_PKEY_id.html. Using the pkey's type in a switch/case you could then call into the corresponding match.

Example

if (EVP_PKEY_id(self) != EVP_PKEY_id(other)) 
  // ossl_raise(..., "cannot compare different key types");

switch(EVP_PKEY_id(self))  {
  case EVP_PKEY_RSA:
    ossl_rsa_private_or_public_keys_eql(self, other)

One would then need to implement the remaining algs comparison methods to have completeness.

[rhenium]

Yes, in one of our apps we generate certificate signing requests which in turn will have a user upload a X509 certificate in which we wish to validate its RSA public key matches that of the RSA private key it corresponds too.

I think we should add a wrapper for EVP_PKEY_cmp(), which is exactly for this purpose and should take care of the various backends.

I'd like it to have a different from #public_eql? as it resembles Object#eql? - perhaps something like #public_match?. #eql? is expected to never raise an exception, but:

• pkey.public_match?("a-string-is-not-a-pkey") should be TypeError
• pkey.public_match?(another_pkey) should be PKeyError if either doesn't support comparison for some reason

One would then need to implement the remaining algs comparison methods to have completeness.

Just to clarify, the abstraction is not limited to this and allows use of different implementations of cryptographic algorithms, such as an HSM. In that case, OpenSSL will not have access to the actual key components, and RSA_get0_key() will only return a fake value or NULL.

[cwjenkins]

Ah true. https://www.openssl.org/docs/man1.0.2/man3/EVP_PKEY_cmp.html is certainly what I'm looking for for my use case and is the cleaner/correct solution.

Gotcha, the goal was to resemble obj.eql?, but wasn't aware of the convention not to raise. Will change to public_match?.

This would take care of the public_match? case, but not the private. I don't have a need for a private match, but felt like throwing it in there in the event someone else does. As well as providing the 'smarter' comparison func == and eql? to determine if private then compare all attributes else compare only the public portions.

With the knowledge of EVP_PKEY_cmp I'm going to revisit my approach to include it and probably exclude the private_match portion for now.

[cwjenkins]

Just to clarify, the abstraction is not limited to this and allows use of different implementations of cryptographic algorithms, such as an HSM. In that case, OpenSSL will not have access to the actual key components, and RSA_get0_key() will only return a fake value or NULL.

Think we discussed something similar in another PR, is this in reference to where they may provide their own 'engine' ? At which point the engine should implement the comparison methods and openssl EVP_PKEY_cmp should wrap the corresponding engine methods? So if one fails to implement such methods on their engine they shouldn't be surprised to see it not function correctly? If that is the case curious if there is a way to check from openssl if the methods are implemented on the engine else we could raise NotImplemented with a hint to check their engine's implementation.

[cwjenkins]

Alrighty, so after some light reading on the openssl source it would seem EVP_PKEY_cmp returns -1 for type error. If types match then it does a private component comparison if the engine implements params_cmp on the ameth (asn1 method), if not, then it will see if it has pub_cmp, if neither it returns -2 at which point I can raise NotImplemented, 'Check engine implementation'.
The OpenSSL engine, default, does not implement a params_cmp for RSA, but does for DSA, EC, and DH.
Therefore I can implement EVP_PKEY_cmp on the abstract PKEY class for each to inherit.
Given EVP_PKEY_cmp wraps both private/public comparisons I'm thinking to have a single method .match? instead of differing private/public and then override RSA class's match to contain the content of private_eql? func I have in this PR.
Sound good?

[cwjenkins]

@rhenium, updated to use EVP_PKEY_cmp. I did not override match in RSA given I'm not sure why openssl chose not to add a param_cmp, but ideally that functionality is added there instead of here. I'm trying to think of a better method than match? given that resembles regex/pattern matching. Equality is truly what this does so cmp?, compare?, eql?, equal_to?, etc. are all better names IMO.

Lastly I noticed the public_key that is returned on EC is the underlying EC::Point instead of an EVP_PKEY. This leads to different behavior when interacting with EC Keys. Not sure if that was intended or what, but wanted to point it out.

[rhenium]

why openssl chose not to add a param_cmp, but ideally that functionality is added there instead of here.

RSA doesn't have the concept of "parameters". We can add a wrapper for EVP_PKEY_cmp_parameters() too, for other algorithms, but I don't know of any use case. I think we can add later when such a need arises.

Given EVP_PKEY_cmp wraps both private/public comparisons I'm thinking to have a single method .match? instead of differing private/public and then override RSA class's match to contain the content of private_eql? func I have in this PR.

Just to clarify, EVP_PKEY_cmp() compares the public portions of the two given pkeys only. So it would be the same as what public_eql? in the original commit in this PR did. (I assume you already understand this, looking at the test code in the current commit in this PR.)

[rhenium]

I'm trying to think of a better method than match? given that resembles regex/pattern matching. Equality is truly what this does so cmp?, compare?, eql?, equal_to?, etc. are all better names IMO.

That was just an example and we don't have the need to stick to the word "match". However since it compares the public portions only, equal_to? without an modifier (like public_) would be confusing. compare? sounds good to me as well as match?.

Lastly I noticed the public_key that is returned on EC is the underlying EC::Point instead of an EVP_PKEY. This leads to different behavior when interacting with EC Keys. Not sure if that was intended or what, but wanted to point it out.

It's a known issue/inconsistency. #29

[rhenium]

Also a documentation for the method is needed.

[cwjenkins]

why openssl chose not to add a param_cmp, but ideally that functionality is added there instead of here.

RSA doesn't have the concept of "parameters". We can add a wrapper for EVP_PKEY_cmp_parameters() too, for other algorithms, but I don't know of any use case. I think we can add later when such a need arises.

Given EVP_PKEY_cmp wraps both private/public comparisons I'm thinking to have a single method .match? instead of differing private/public and then override RSA class's match to contain the content of private_eql? func I have in this PR.

Just to clarify, EVP_PKEY_cmp() compares the public portions of the two given pkeys only. So it would be the same as what public_eql? in the original commit in this PR did. (I assume you already understand this, looking at the test code in the current commit in this PR.)

The RSA params should be defined in rsa_asn1.c

        ASN1_SIMPLE(RSA, n, BIGNUM),
        ASN1_SIMPLE(RSA, e, BIGNUM),
        ASN1_SIMPLE(RSA, d, CBIGNUM),
        ASN1_SIMPLE(RSA, p, CBIGNUM),
        ASN1_SIMPLE(RSA, q, CBIGNUM),
        ASN1_SIMPLE(RSA, dmp1, CBIGNUM),
        ASN1_SIMPLE(RSA, dmq1, CBIGNUM),
        ASN1_SIMPLE(RSA, iqmp, CBIGNUM),

However, I don't know why they didn't add a param_cmp method to it. Could inquire in openssl, but given I'm not needing it, agree, I'll just leave as is until someone else does.

Hmm, are you sure about that? Checking 1.0.2v I see this in p_lib.c

int EVP_PKEY_cmp(const EVP_PKEY *a, const EVP_PKEY *b)
{
    if (a->type != b->type)
        return -1;

    if (a->ameth) {
        int ret;
        /* Compare parameters if the algorithm has them */
        if (a->ameth->param_cmp) {
            ret = a->ameth->param_cmp(a, b);
            if (ret <= 0)
                return ret;
        }

        if (a->ameth->pub_cmp)
            return a->ameth->pub_cmp(a, b);
    }

    return -2;
}

Notice EVP_PKEY_cmp does a type check first, then sees if param_cmp is avail prior to doing a pub_cmp. param_cmp being the same method used within EVP_PKEY_parameters_cmp.
Is there a version of openssl that differs from the above or am I missing something?
Here is EVP_PKEY_parameters_cmp

int EVP_PKEY_parameters_cmp(const EVP_PKEY *a, const EVP_PKEY *b)
{
    if (a->type != b->type)
        return -1;
    if (a->ameth && a->ameth->param_cmp)
        return a->ameth->param_cmp(a, b);
    return -2;
}

[cwjenkins]

I'm trying to think of a better method than match? given that resembles regex/pattern matching. Equality is truly what this does so cmp?, compare?, eql?, equal_to?, etc. are all better names IMO.

That was just an example and we don't have the need to stick to the word "match". However since it compares the public portions only, equal_to? without an modifier (like public_) would be confusing. compare? sounds good to me as well as match?.

Lastly I noticed the public_key that is returned on EC is the underlying EC::Point instead of an EVP_PKEY. This leads to different behavior when interacting with EC Keys. Not sure if that was intended or what, but wanted to point it out.

It's a known issue/inconsistency. #29

I like compare? given its closer to the underlying C call EVP_PKEY_cmp while still differing from the non-throwing eql? and distinguishes from regex match. Will Update.

Ah gotcha.

[rhenium]

However, I don't know why they didn't add a param_cmp method to it. Could inquire in openssl, but given I'm not needing it, agree, I'll just leave as is until someone else does.

The "parameters" that EVP_PKEY_cmp_parameters() is referring to simply doesn't exist in RSA.

[rhenium]

Check my comment here #383 (comment). At the top EVP_PKEY_cmp does just that.

I checked the docs. EVP_PKEY_cmp(3) says:

The function EVP_PKEY_cmp_parameters() and EVP_PKEY_cmp() return 1 if the keys match, 0 if they don't match, -1 if the key types are different and -2 if the operation is not supported.

So, the -1 is indeed type mismatch for both EVP_PKEY_cmp() and EVP_PKEY_cmp_parameters(). However, EVP_PKEY_ASN1_METHOD(3) allows param_cmp to return an arbitrary negative number.

The pub_cmp() method is called when two public keys are to be compared. It MUST return 1 when the keys are equal, 0 otherwise. It's called by EVP_PKEY_cmp(3).
The param_cmp() method compares the parameters of keys a and b. It MUST return 1 when the keys are equal, 0 when not equal, or a negative number on error. It's called by EVP_PKEY_cmp_parameters(3).

It seems there is some confusion in the docs. At least, it sounds unusual to me that a -1 return means a specific error.

I think we should check the type mismatch explicitly, and check for 1, 0, -2 return and treat everything else as a generic error.

[cwjenkins]

EVP_PKEY_ASN1_METHOD

Ah good eye. Wonder if they meant anything less than -2 for other errors.

I'll add the explicit type check in the event someone implemented the param_cmp with some error for -1.
1 for equal, and 0 for not equal, and anything else as a generic PKeyError.
I thought -2 would work for not implemented, however, as noted above EC pub cmp differs here. The operation is supported however the key lacks information (group, pa, pb).

[cwjenkins]

However, I don't know why they didn't add a param_cmp method to it. Could inquire in openssl, but given I'm not needing it, agree, I'll just leave as is until someone else does.

The "parameters" that EVP_PKEY_cmp_parameters() is referring to simply doesn't exist in RSA.

I figured the params were the ones mentioned above similar to DSA's implementation. It's exactly how I handled it in the original PR.

static int dsa_cmp_parameters(const EVP_PKEY *a, const EVP_PKEY *b)
{
    if (BN_cmp(a->pkey.dsa->p, b->pkey.dsa->p) ||
        BN_cmp(a->pkey.dsa->q, b->pkey.dsa->q) ||
        BN_cmp(a->pkey.dsa->g, b->pkey.dsa->g))
        return 0;
    else
        return 1;
}

In this case they could have created a rsa_cmp_parameters method that checks a/b -> pkey.n,e,p,q,dmp1,dmq1,iqmp that exist on the RSA struct.

[rhenium]

Please squash commits into one, and this is good to merge. Thanks!

[cwjenkins]

Please squash commits into one, and this is good to merge. Thanks!

Thanks for the review/feedback. Done

[rhenium]

Thank you!