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

Rename EVP_PKEY_cmp() to EVP_PKEY_eq() and EVP_PKEY_cmp_parameters() to EVP_PKEY_parameters_eq() #11953

Closed
wants to merge 1 commit into from

Conversation

@DDvO
Copy link
Contributor

@DDvO DDvO commented May 25, 2020

As discussed in #11870

  • documentation is added or updated
  • tests are added or updated
@DDvO DDvO requested a review from t8m May 25, 2020
Copy link
Member

@t8m t8m left a comment

I am not sure the old functions should completely disappear from the documentation. IMO they should stay in the manual page header and synopsis. And the history notice should be amended to mention that the old functions are identical but deprecated. Maybe also mentioning the confusing equality return value would make sense.

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented May 25, 2020

All right, done.

@romen
Copy link
Member

@romen romen commented May 25, 2020

Should these changes be reflected also on the provider interface?

@levitte
Copy link
Member

@levitte levitte commented May 25, 2020

Should these changes be reflected also on the provider interface?

The name in that interface is OP_keymgmt_match(). Semantically speaking, I don't see that it needs to be changed.

@vt-alt
Copy link

@vt-alt vt-alt commented May 25, 2020

Is it correct just to replace EVP_PKEY_cmp with EVP_PKEY_eq? I thought it's decided they should return different values.

-        ret = EVP_PKEY_cmp(xk, k);
+        ret = EVP_PKEY_eq(xk, k);
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented May 25, 2020

@vt-alt, no, it would be very hard to change the semantics of EVP_PKEY_cmp() to properly do an order-like comparison. Instead we make clear that it basically just checks for equality.
OTOH, the name and return values of X509_PUBKEY_cmp(), now X509_PUBKEY_eq(), will change due to #11894, and will be consistent with EVP_PKEY_eq().

@vt-alt
Copy link

@vt-alt vt-alt commented May 25, 2020

I don't say to change EVP_PKEY_cmp, but isn't EVP_PKEY_eq should return 1 for a match? Now it will return 0, isn't it?

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented May 25, 2020

On equal input, EVP_PKEY_eq(), which is the same as the former EVP_PKEY_cmp(), continues to return 1, and X509_PUBKEY_eq(), which is the fixed X509_PUBKEY_cmp(), will return also 1 (making direct internal use of EVP_PKEY_eq/cmp(), which is now correct, see https://github.com/openssl/openssl/pull/11894/files#diff-a4d3e5a924de45dc8e27b1f12c1907ad).

@vt-alt
Copy link

@vt-alt vt-alt commented May 25, 2020

@DDvO Ah, yes! Thanks.

@t8m
t8m approved these changes May 26, 2020
Copy link
Member

@t8m t8m left a comment

LGTM now! Good work!

@t8m
Copy link
Member

@t8m t8m commented May 26, 2020

Of course this will require update after #11894 is merged.

@t8m t8m removed the approval: done label May 26, 2020
@levitte
Copy link
Member

@levitte levitte commented May 26, 2020

Sorry for throwing this in so late, but I'm not sure eq is much better than cmp. What does "equality" mean, precisely? For example, EVP_PKEY_cmp() compares key parameters and the public key, does that make two EVP_PKEYs equal? One could contain the private half while the other doesn't, are they equal?

How about "match" instead of "eq"?

(I'll totally blame @romen for reminding me of the name used in the provider interface 😉)

@DDvO DDvO force-pushed the siemens:rename_EVP_PKEY_cmp branch May 26, 2020
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented May 26, 2020

Of course this will require update after #11894 is merged.

Right - done.

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented May 26, 2020

Sorry for throwing this in so late, but I'm not sure eq is much better than cmp. What does "equality" mean, precisely? For example, EVP_PKEY_cmp() compares key parameters and the public key, does that make two EVP_PKEYs equal? One could contain the private half while the other doesn't, are they equal?

How about "match" instead of "eq"?

I agree that "match" typically has a broader meaning than "eq" (equality). Both should be instances of an equivalence relation, mathematically speaking.

Both "match" and "eq" are certainly better here than "cmp" (compare), which typically involves an ordering relation (such as "<" or ">="), not an equivalence relation.

Also the meaning of equality is generally not as narrow as one might naively expect, not only in mathematics/logics (where AFAICS any equivalence relation would do), but think also of user-defined equality operators that many OO programming languages offer.

In our concrete case here in my view "eq" would be broad enough to cover also the case that a private key is not present while the public keys are the same. In particular the difference cannot be large because if the public keys are equal the corresponding private keys, being included or not, (most likely) are equal, too.

Moreover, the name "eq" is shorter and easier to read/recognize than "match" 😉

@levitte
Copy link
Member

@levitte levitte commented May 26, 2020

I see you're not much acquainted with lisp 😉

But yeahok, I can accept that argument

@t8m
Copy link
Member

@t8m t8m commented May 26, 2020

You should also rename the EVP_PKEY_cmp call from X509_PUBKEY_eq().

…to EVP_PKEY_parameters_eq()
@DDvO DDvO force-pushed the siemens:rename_EVP_PKEY_cmp branch to fc81bb5 May 26, 2020
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented May 26, 2020

You should also rename the EVP_PKEY_cmp call from X509_PUBKEY_eq().

Ah, I thought I had already done this, but my --amend missed that x_pubkey.c file.
Now it's really pushed.

@t8m
t8m approved these changes May 26, 2020
Copy link
Member

@t8m t8m left a comment

Re-approved.

@openssl-machine
Copy link

@openssl-machine openssl-machine commented May 27, 2020

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request May 27, 2020
…to EVP_PKEY_parameters_eq()

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #11953)
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented May 27, 2020

Merged after sufficient discussion :)

@DDvO DDvO closed this May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants