Document EVP_PKEY_ASN1_METHOD and associated functions #4596

levitte · 2017-10-26T22:12:21Z

No description provided.

levitte · 2017-10-26T22:13:14Z

@mattcaswell, you might be interested of this...

levitte · 2017-10-27T10:04:19Z

Ok, this is now done as far as I can see. Time for a review.

bernd-edlinger · 2017-10-27T10:48:36Z

So supposed I want to implement a better RSAPSS algorithm,
then I can override the standard RSAPSS method, apparently.
But what if openssl is used by more than one software package in the same
address space how can I make sure they wont overwrite each other's RSAPSS
algorihm?

levitte · 2017-10-27T11:00:51Z

Well, an override is made by the application, or an engine if you load one (possibly through a config file), dynamically. Ultimately, that's controlled by the order in which the application does things, doesn't it?

bernd-edlinger · 2017-10-27T11:11:10Z

doc/man3/EVP_PKEY_asn1_get_count.pod

+EVP_PKEY_asn1_find_str() looks up the B<EVP_PKEY_ASN1_METHOD> with PEM
+type string B<str>.
+Just like EVP_PKEY_asn1_find(), it searches through the methods added
+by the application first, and then through the standard methods.


Have a look for yourself ;-)

to me this looks like the standard methods win:

for (i = 0; i < EVP_PKEY_asn1_get_count(); i++) { ameth = EVP_PKEY_asn1_get0(i); if (ameth->pkey_flags & ASN1_PKEY_ALIAS) continue; if (((int)strlen(ameth->pem_str) == len) && (strncasecmp(ameth->pem_str, str, len) == 0))

FYI: The comment refers to EVP_PKEY_asn1_find_str

Ah, you are quite correct! That's... not very consistent. I guess the idea was that everyone would play nice with each other and not try to compete over the same algorithms, but rather that application would handle their own different algo.

When I look at EVP_PKEY_asn1_add0 and the way it sorts the stack every time a new method is added, it seems clear to me that the original line of thought is one of no competition, and to make sure that is always true, EVP_PKEY_asn1_add0 should refuse to add a method for an algo that already has one. In that case, the exact order in which these methods are found don't matter at all.

Yes, that would be a significant improvement.

Should the check in the add method also look for duplicate strings?

bernd-edlinger · 2017-10-27T11:18:53Z

doc/man3/EVP_PKEY_asn1_get_count.pod

+EVP_PKEY_asn1_find() looks up the B<EVP_PKEY_ASN1_METHOD> with NID
+B<type>.
+It searches through the methods added by the application first, and
+then through the standard methods.


and what happens if pe != NULL ?

that looks like the engine always wins ?

in EVP_PKEY_asn1_find the first loop changes "type" if ASN1_PKEY_ALIAS
is in app_methods, and then if pe != NULL
ENGINE_get_pkey_asn1_meth_engine can look up something,
otherwise "t" from the frst loop is returned.
Weird...

If I understand you correct, then it should not matter where we search first.
In that case this sentence can be removed.
However I wonder why the engine search uses the aliases at all, maybe the
whole "if (pe)" block should move up?

previous comment refers to EVP_PKEY_asn1_find

The loop at the start is there to follow aliases down to the root identity, which means that the methods provided by engines can't be aliases. An engine can of course still provide an alias by adding them directly with EVP_PKEY_asn1_add_alias, but that will end up in the app_methods stack anyway... So the loop to resolve aliases should come first, as it does.

Maybe the documentation should reflect that ?

Dunno how it would here. EVP_PKEY_asn1_add_alias is the only way to add an alias, and it's not ENGINE specific, so...

The ENGINE API documentation needs to be refreshed for sure, though. That's for a different PR...

More or less make one sentence out of your previous two comments.
I believe the rationale is not obvious.

bernd-edlinger · 2017-10-27T11:40:07Z

I would have liked to have a way to store the app_methods in a context structure,
not in a static.

levitte · 2017-10-27T11:47:36Z

In what context?

bernd-edlinger · 2017-10-27T12:37:05Z

Maybe it is a silly idea or just infeasible...

But I mean as an extra parameter maybe similar to an EVP object, which
contains a lookup for algorithms I want to use in the requested operation.

levitte · 2017-10-27T20:22:59Z

I think it's an infeasible idea in this case, sorry...

levitte · 2017-10-27T20:53:35Z

@bernd-edlinger, I hope the new commits I just pushed answer your concerns.

bernd-edlinger · 2017-10-28T06:35:29Z

doc/man3/EVP_PKEY_asn1_get_count.pod

+EVP_PKEY_asn1_get0() return a public key method or B<NULL> if B<idx> is
+out of range.
+
+EVP_PKEY_asn1_get0_info() returns 0 on failure, 1 on success.


maybe it should be noted that these APIs are not thread-safe?

bernd-edlinger · 2017-10-28T06:37:40Z

doc/man3/EVP_PKEY_ASN1_METHOD.pod

+B<EVP_PKEY_ASN1_METHOD> object otherwise.
+
+EVP_PKEY_asn1_add0() and EVP_PKEY_asn1_add_alias() return 0 on error,
+or 1 on success.


Also these APIs are not thread-safe, either make the thread-safe or document that?

bernd-edlinger · 2017-10-28T08:09:34Z

doc/man3/EVP_PKEY_ASN1_METHOD.pod

+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 L<EVP_PKEY_cmp(3)>.


new sentence, start with upper case.

bernd-edlinger · 2017-10-28T08:10:04Z

doc/man3/EVP_PKEY_ASN1_METHOD.pod

+The pub_print() method is called to print a public key in humanly
+readable text to B<out>, indented B<indent> spaces.
+It MUST return 0 on error, 1 on success.
+it's called by L<EVP_PKEY_print_public(3)>.


bernd-edlinger · 2017-10-28T08:10:19Z

doc/man3/EVP_PKEY_ASN1_METHOD.pod

+The priv_decode() and priv_encode() methods are called to decode /
+encode B<PKCS8_PRIV_KEY_INFO> form private key to / from B<pk>.
+They MUST return 0 on error, 1 on success.
+they're called by L<EVP_PKCS82PKEY(3)> and L<EVP_PKEY2PKCS8(3)>.


levitte · 2017-10-30T12:11:38Z

Because the thread safety question isn't quite resolved yet and I don't want this PR to wait endlessly, I just added a commit with thread safety information. Looks good?

bernd-edlinger

Yes, looks good, the other PR needs to be merged first.

levitte · 2017-10-30T17:20:27Z

Hmmm, most of this should also be backported to 1.1.0. I'll prepare a separate PR for it, considering master has added functionality...

bernd-edlinger · 2017-10-30T17:22:14Z

Is the red cross from travis a doc-nits ?

[skip ci] Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #4596)

No two public key ASN.1 methods with the same pkey_id can be registered at the same time. Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #4596)

levitte · 2017-10-30T17:26:47Z

Good catch! Added commit should resolved the matter

levitte · 2017-10-30T17:30:11Z

Unfortunately, I already merged the commits before the make update one... 751148e and d85722d

bernd-edlinger · 2017-10-30T17:31:54Z

Sorry, for not checking earlier.

levitte · 2017-10-30T17:35:31Z

It's alright, but I need you to approve the added commit

[skip ci] Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from openssl#4596)

No two public key ASN.1 methods with the same pkey_id can be registered at the same time. Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from openssl#4596)

levitte · 2017-10-30T17:45:50Z

Merged the last commit as well. 79204b9

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #4596)

levitte added the branch: master Merge to master branch label Oct 26, 2017

levitte force-pushed the EVP_PKEY_ASN1_METHOD_docs branch from 3dad8bc to 823b6aa Compare October 26, 2017 22:12

levitte changed the title ~~WIP: Document EVP_PKEY_ASN1_METHOD and associated functions~~ Document EVP_PKEY_ASN1_METHOD and associated functions Oct 27, 2017

levitte requested review from mattcaswell and bernd-edlinger October 27, 2017 10:04

bernd-edlinger reviewed Oct 27, 2017

View reviewed changes

bernd-edlinger reviewed Oct 28, 2017

View reviewed changes

levitte mentioned this pull request Oct 28, 2017

EVP_PKEY_ASN1_METHOD: add functions to set siginf_set and pkey_check methods #4589

Closed

bernd-edlinger approved these changes Oct 30, 2017

View reviewed changes

levitte added a commit that referenced this pull request Oct 30, 2017

Document EVP_PKEY_ASN1_METHOD and associated functions

751148e

[skip ci] Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #4596)

levitte force-pushed the EVP_PKEY_ASN1_METHOD_docs branch 2 times, most recently from 29ea155 to d8586bb Compare October 30, 2017 17:26

make update

775866a

levitte force-pushed the EVP_PKEY_ASN1_METHOD_docs branch from d8586bb to 775866a Compare October 30, 2017 17:28

bernd-edlinger approved these changes Oct 30, 2017

View reviewed changes

levitte added a commit to levitte/openssl that referenced this pull request Oct 30, 2017

Document EVP_PKEY_ASN1_METHOD and associated functions

8a69568

[skip ci] Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from openssl#4596)

levitte mentioned this pull request Oct 30, 2017

Document EVP_PKEY_ASN1_METHOD and associated functions [1.1.0] #4620

Closed

levitte closed this Oct 30, 2017

levitte added a commit that referenced this pull request Oct 30, 2017

make update

79204b9

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #4596)

Document EVP_PKEY_ASN1_METHOD and associated functions #4596

Document EVP_PKEY_ASN1_METHOD and associated functions #4596

Conversation

levitte commented Oct 26, 2017

levitte commented Oct 26, 2017

levitte commented Oct 27, 2017

bernd-edlinger commented Oct 27, 2017

levitte commented Oct 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernd-edlinger commented Oct 27, 2017

levitte commented Oct 27, 2017

bernd-edlinger commented Oct 27, 2017

levitte commented Oct 27, 2017

levitte commented Oct 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levitte commented Oct 30, 2017

bernd-edlinger left a comment

Choose a reason for hiding this comment

levitte commented Oct 30, 2017

bernd-edlinger commented Oct 30, 2017

levitte commented Oct 30, 2017

levitte commented Oct 30, 2017

bernd-edlinger commented Oct 30, 2017

levitte commented Oct 30, 2017

levitte commented Oct 30, 2017