Add a CreatePSS method to SignatureCreator to permit the generation of PSS signatures.

Add a CreatePSS method to SignatureCreator to permit the generation of PSS signatures.

BUG=

[Ryan Hamilton, 2015-08-21 18:44:12]

rch@chromium.org changed reviewers:
+ davidben@chromium.org

[Ryan Hamilton, 2015-08-21 18:44:12]

This is probably the wrong approach for making PSS signatures available for
QUIC, but it's a first pass. I'm happy to do something different if you prefer,
just let me know what you think. Also, I have no idea how to do PSS with NSS, so
any guidance you have would be great.

[davidben, 2015-08-21 21:52:22]

davidben@chromium.org changed reviewers:
+ rsleevi@chromium.org

[davidben, 2015-08-21 21:52:23]

+rsleevi who probably has stronger opinions than me as to where we want //crypto
to go.

My view is that there's not a whole lot of sense in having these wrappers in a
post-BoringSSL world. After adding a few scopers, the BoringSSL API isn't
terribly messier than the //crypto ones. Some of our scopers probably could do
with some tidying (I think the scoped contexts should work more like [1]), but
whatever.

Of course, up until very recently, we weren't in a post-BoringSSL world. And we
still aren't quiiiiite in one, but we mostly are. Just iOS is left, and I dunno
how much QUIC server code would be relevant there. The server stuff would mostly
be for WebRTC P2P type things, right? WebRTC doesn't exist in Chrome for iOS and
WebRTC standalone for iOS is BoringSSL. (Also WebRTC has its own internal
private key + cert abstraction.)

tl;dr: rsleevi generally talks louder than I do, but I currently lean towards
just using EVP_PKEY directly based on what you've told me about what this is
for.

[1]
https://boringssl.googlesource.com/boringssl/+/master/crypto/test/scoped_types.h

https://codereview.chromium.org/1305183005/diff/1/crypto/signature_creator_ns...
File crypto/signature_creator_nss.cc (right):

https://codereview.chromium.org/1305183005/diff/1/crypto/signature_creator_ns...
crypto/signature_creator_nss.cc:50: HashAlgorithm hash_algm) {
hash_alg, to match the others?

https://codereview.chromium.org/1305183005/diff/1/crypto/signature_creator_ns...
crypto/signature_creator_nss.cc:66: return NULL;
Pfft. Judging by crypto/signature_verifier_nss.cc, it seems to be a mess. Though
perhaps now that we can assume the bundled NSS for this file, this can be
simpler. I'll defer that to Ryan.

https://codereview.chromium.org/1305183005/diff/1/crypto/signature_creator_op...
File crypto/signature_creator_openssl.cc (right):

https://codereview.chromium.org/1305183005/diff/1/crypto/signature_creator_op...
crypto/signature_creator_openssl.cc:44: HashAlgorithm hash_algm) {
Nit: hash_alg, to be consistent?

https://codereview.chromium.org/1305183005/diff/1/crypto/signature_creator_op...
crypto/signature_creator_openssl.cc:70: if (1 !=
EVP_PKEY_CTX_set_rsa_padding(pkey_ctx, RSA_PKCS1_PSS_PADDING)) {
You can just use ! now. BoringSSL narrowed both of these functions to return 0
or 1 like civilized people do.

[davidben, 2015-08-21 21:53:18]

> rsleevi generally talks louder than I do

Er, to clarify, that was a "generally talks louder than I do, so defer to him if
he disagrees with me" not a "generally talks louder than I do" in any negative
sense. :-)

[Ryan Hamilton, 2015-08-24 17:24:52]

On 2015/08/21 21:52:23, David Benjamin wrote:
> +rsleevi who probably has stronger opinions than me as to where we want
//crypto
> to go.
> 
> My view is that there's not a whole lot of sense in having these wrappers in a
> post-BoringSSL world. After adding a few scopers, the BoringSSL API isn't
> terribly messier than the //crypto ones. Some of our scopers probably could do
> with some tidying (I think the scoped contexts should work more like [1]), but
> whatever.
> 
> Of course, up until very recently, we weren't in a post-BoringSSL world. And
we
> still aren't quiiiiite in one, but we mostly are. Just iOS is left, and I
dunno
> how much QUIC server code would be relevant there. The server stuff would
mostly
> be for WebRTC P2P type things, right? WebRTC doesn't exist in Chrome for iOS
and
> WebRTC standalone for iOS is BoringSSL. (Also WebRTC has its own internal
> private key + cert abstraction.)
> 
> tl;dr: rsleevi generally talks louder than I do, but I currently lean towards
> just using EVP_PKEY directly based on what you've told me about what this is
> for.

Fair enough. I'll give that a whirl. One of the reasons I was inclined to go the

SSL-library-agnostic route is that we have tests which attempt to use a real
ProofSource
and ProofVerifier to ensure that they are able to talk to each other. It'd be
great if
these tests could run on iOS. But if thats more trouble than it's worth, so be
it :>

Thanks!

[Ryan Sleevi, 2015-08-25 20:01:38]

Personally, I prefer that we have wrappers.

While the BoringSSL cleanup is good, it does seem like there's still a lot of
sharp edges and footguns here. For example, you could easily forget to set a
parameter on the EVP CTX. Checking return values and having them consistently
return sane things is good (are we actually there yet?), but we can't do things
like WARN_UNUSED_RESULT or audit callers (which I do about once a quarter).

I have also found that having to have //crypto OWNERS approve the DEPS additions
(which would not be a pre-requisite for directly depending on BoringSSL) have
helped catch a number of issues.

So I would prefer the direct use of BoringSSL be carefully evaluated; we should
abstract away common tasks (which I suspect this will be) to ensure consistency,
but still allow low-level usage when appropriate (e.g. WebRTC)

https://codereview.chromium.org/1305183005/diff/1/crypto/signature_creator.h
File crypto/signature_creator.h (right):

https://codereview.chromium.org/1305183005/diff/1/crypto/signature_creator.h#...
crypto/signature_creator.h:50: // specified in PKCS #1 v1.5.
This of course would be inconsistent with your proposed change :)

https://codereview.chromium.org/1305183005/diff/1/crypto/signature_creator.h#...
crypto/signature_creator.h:64: static SignatureCreator*
CreateImpl(RSAPrivateKey* key,
This doesn't actually need to be static, does it?

[davidben, 2015-08-25 20:10:56]

On 2015/08/25 20:01:38, Ryan Sleevi wrote:
> Personally, I prefer that we have wrappers.
> 
> While the BoringSSL cleanup is good, it does seem like there's still a lot of
> sharp edges and footguns here. For example, you could easily forget to set a
> parameter on the EVP CTX. Checking return values and having them consistently
> return sane things is good (are we actually there yet?), but we can't do
things
> like WARN_UNUSED_RESULT or audit callers (which I do about once a quarter).

Yes, all boolean return values for BoringSSL's crypto routines are sane now.
They have been for quite some time. (Plus, strictly speaking, none of them can
actually fail unless we allow malloc failures.)

Honestly, I feel like Chromium's //crypto footguns are worse right now. We don't
have much of an AEAD interface and instead let people do CBC and CTR mode
without authentication. With BoringSSL, we can push everyone towards EVP_AEAD
which is the only sort of API that's actually reasonable for symmetric crypto.

> I have also found that having to have //crypto OWNERS approve the DEPS
additions
> (which would not be a pre-requisite for directly depending on BoringSSL) have
> helped catch a number of issues.

That's part of what this thing is for. :-)
https://codereview.chromium.org/1312203002/

> So I would prefer the direct use of BoringSSL be carefully evaluated; we
should
> abstract away common tasks (which I suspect this will be) to ensure
consistency,
> but still allow low-level usage when appropriate (e.g. WebRTC)
> 
> https://codereview.chromium.org/1305183005/diff/1/crypto/signature_creator.h
> File crypto/signature_creator.h (right):
> 
>
https://codereview.chromium.org/1305183005/diff/1/crypto/signature_creator.h#...
> crypto/signature_creator.h:50: // specified in PKCS #1 v1.5.
> This of course would be inconsistent with your proposed change :)
> 
>
https://codereview.chromium.org/1305183005/diff/1/crypto/signature_creator.h#...
> crypto/signature_creator.h:64: static SignatureCreator*
> CreateImpl(RSAPrivateKey* key,
> This doesn't actually need to be static, does it?