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

Nss rsa #259

Merged
merged 42 commits into from Sep 5, 2018
Merged

Nss rsa #259

merged 42 commits into from Sep 5, 2018

Conversation

Labels
None yet
Projects
None yet
4 participants
@nmathewson
Copy link
Contributor

@nmathewson nmathewson commented Aug 1, 2018

No description provided.

nmathewson added 30 commits Jul 11, 2018
When it is set, include the NSS headers and libraries as
appropriate.  Doesn't actually use them yet, though.
These are now part of crypto_init.c.  The openssl-only parts now
live in crypto_openssl_mgt.c.

I recommend reviewing this patch with -b and --color-moved.
This is largely conjectural, based on online documentation for NSS
and NSPR.
We need this in our unit tests, since otherwise NSS will notice
we've forked and start cussing us out.

I suspect we'll need a different hack for daemonizing, but this
should be enough for tinytest to work.
This was a fairly straightforward port, once I realized which layer
I should be calling into.
This is comparatively straightforward too, except for a couple of
twists:

   * For as long as we're building with two crypto libraries, we
     want to seed _both_ their RNGs, and use _both_ their RNGs to
     improve the output of crypto_strongest_rand()

   * The NSS prng will sometimes refuse to generate huge outputs.
     When it does, we stretch the output with SHAKE.  We only need
     this for the tests.
We only ever need this to get us a DH ephemeral key object,
so make a function that does just that.
Notably, there's a test to make sure that it round-trips with
OpenSSL, if OpenSSL is enabled.
(We do this unconditionally, since we still need it for tortls.c)
@nmathewson nmathewson force-pushed the nss_rsa branch 2 times, most recently from b7fef99 to 857aed8 Aug 11, 2018
@coveralls
Copy link

@coveralls coveralls commented Aug 11, 2018

Coverage Status

Coverage increased (+0.06%) to 59.609% when pulling 3ccb94d on nmathewson:nss_rsa into 9bb0ac4 on torproject:master.

nmathewson added 11 commits Aug 14, 2018
It is not nice to expose a private key's contents without having the
function name advertise the fact.  Fortunately, we weren't misusing
these yet.
These functions exist only to expose RSA keys to other places in Tor
that use OpenSSL; let's be specific about their purpose.
This cleans up a lot of junk from crypto_rsa_openssl, and will
save us duplicated code in crypto_rsa_nss (when it exists).

(Actually, it already exists, but I am going to use git rebase so
that this commit precedes the creation of crypto_rsa_nss.)
src/lib/crypt_ops/crypto_rsa.c Show resolved Hide resolved
src/lib/crypt_ops/crypto_rsa.c Show resolved Hide resolved
src/lib/crypt_ops/crypto_rsa.c Show resolved Hide resolved
crypto_pk_base64_decode_private(const char *str, size_t len)
{
crypto_pk_t *pk = NULL;

Copy link
Member

@asn-d6 asn-d6 Aug 29, 2018

It's not documented in this function what len is here. It's the siez of str and also the size of der? Shouldn't base64_decode() expand the data such that it doesn't fit?

src/lib/crypt_ops/crypto_rsa_nss.c Show resolved Hide resolved
crypto_pk_asn1_encode(const crypto_pk_t *pk, char *dest, size_t dest_len)
{
tor_assert(pk);
if (pk->pubkey == NULL)
Copy link
Member

@asn-d6 asn-d6 Aug 29, 2018

Should we use check_key?

Copy link
Member

@asn-d6 asn-d6 left a comment

review

Copy link
Member

@asn-d6 asn-d6 left a comment

review

@torproject-pusher torproject-pusher merged commit 3ccb94d into torproject:master Sep 5, 2018
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment