Add support for NETSCAPE_SPKI_print #1624
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1624 +/- ##
=======================================
Coverage 78.15% 78.16%
=======================================
Files 562 562
Lines 94638 94675 +37
Branches 13573 13576 +3
=======================================
+ Hits 73969 74006 +37
Misses 20075 20075
Partials 594 594 ☔ View full report in Codecov by Sentry. |
We could consider it, but it did seem harmless to incorporate given that we have support for all the surrounding functions within the module: https://github.com/ruby/ruby/blob/ruby_2_7/ext/openssl/ossl_ns_spki.c#L378-L405 Great question on potential impact, I did some more digging. The only concern would be us failing this Ruby test which asserts that the inputted BIO has at least some contents, but we can pass a short string into BIO to prevent that from happening. My preference would be supporting the entire module with no caveats, but I'm open to any other concerns with us supporting this. |
| if (pkey == NULL) { | ||
| BIO_printf(out, " Unable to load public key\n"); | ||
| } else { | ||
| EVP_PKEY_print_public(out, pkey, 4, NULL); |
There was a problem hiding this comment.
what's the significance of 4 here?
There was a problem hiding this comment.
The 4 appears to be the indentation. ¯\_(ツ)_/¯
There was a problem hiding this comment.
Yes the seemingly arbitrary numbers throughout this code are all indentations
Jokes on me, I thought I had thought the 7 below was an indentation as well haha
| : OBJ_nid2ln(OBJ_obj2nid(spkioid))); | ||
| EVP_PKEY *pkey = X509_PUBKEY_get0(spki->spkac->pubkey); | ||
| if (pkey == NULL) { | ||
| BIO_printf(out, " Unable to load public key\n"); |
There was a problem hiding this comment.
In this case, the function would still return success?
There was a problem hiding this comment.
Yes this function is just printing out the contents of the struct. The absence of a public key doesn't indicate that the print is unsuccessful, it just means that there isn't one set.
We could make the argument that the wording could be better though, but I didn't want any unnecessary misalignments from upstream behavior.
| if (pkey == NULL) { | ||
| BIO_printf(out, " Unable to load public key\n"); | ||
| } else { | ||
| EVP_PKEY_print_public(out, pkey, 4, NULL); |
There was a problem hiding this comment.
The 4 appears to be the indentation. ¯\_(ツ)_/¯
samuel40791765
force-pushed
the
netscape-print
branch
from
a28de13 to
a242859
Compare
samuel40791765
force-pushed
the
netscape-print
branch
from
a242859 to
261a5d9
Compare
samuel40791765
force-pushed
the
netscape-print
branch
2 times, most recently
from
f8cb242 to
fc40fe4
Compare
samuel40791765
force-pushed
the
netscape-print
branch
from
fc40fe4 to
d3bcc10
Compare
Issues:
Resolves
CryptoAlg-1717Description of changes:
Ruby consumes NETSCAPE_SPKI_print for debugging purposes. This adds support for the symbol for easier integration.
Call-outs:
N/A
Testing:
Expected output test
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.