ExplicitKeyTrustEngine doesn't handle EC in the OpenSSL case
Description
Environment
Activity
Scott Cantor January 12, 2018 at 8:38 PM
I did actually test this recently with the IdP.
The guards all look correct to me, but we'll flush that out if anything comes up on older platforms.
Scott Cantor April 14, 2017 at 9:16 PM
3f9febcc15e5036b55a610d4d244b2e550790a03
The WIndows build for the existing VC10/1.6 branch failed, which I traced to a missing helper method when it's still using OpenSSL 1.0.2. Also any EC usage has to be guarded or it will start breaking on RH5 and other platforms where even older versions are used, or EC has been compiled out.
I believe the fix is correct, but I still need to really review these changes.
Rod Widdowson February 13, 2017 at 4:33 PM
fixed by 65cbff5.
The tests all pass (including the new one to test exactly this), but I think I'd be happier if Scott eyeballed this (any mistake here would be embarrassing)
Rod Widdowson February 13, 2017 at 2:20 PM
Musing out loud here...
I'm still trying to work out the best structure for this new class.
At the bottom the obvious methods are
matchesRSAPublic(RSA *r1, RSA *r2);
matchesRSAPrivate(RSA *r1, RSA *r2);
matchesDSAPublic(DSA *d1, DSA *d2);
matchesDSAPrivate(DSA *d1, DSA *d2);
matchesECPublic(EC *ec1, EC *ec2);
matchesECPrivate(EC *ec1, EC *ec2);But the next stage up gets harder. The commonality between ExplicitKeyTrustEngine and SecurityHelper is that one of the parameters is a XSECCryptoKey*. But the difference is that the second parameter is another XSECCryptoKey* or a EVP_PKEY* (from an X509* )
However we currently use the (first) XSECCryptoKey* to drive what sort of comparison to make and I am not (yet) confident that if we used the other parameter (so using the type of the EVP_PKEY* we would always get the same answer when the key material types mismatches (surely the answer just needs to be "no"?).
When I can convince myself of that I can see a second level of methods:
matchesRSAPublic(XSECCryptoKey *r1, RSA *r2);
matchesRSAPrivate(XSECCryptoKey* r1, RSA *r2);
matchesDSAPublic(XSECCryptoKey *d1, DSA *d2);
matchesDSAPrivate(XSECCryptoKey *d1, DSA *d2);
matchesECPublic(XSECCryptoKey *ec1, EC *ec2);
matchesECPrivate(XSECCryptoKey *ec1, EC *ec2);Which could in fact replace the first layer.
Then we can recast SecurityHelper::matches in terms of them and trivially do a
matches(XSECCryptoKey *ec1, EVP_PKEY)for the validate case.
Scott Cantor November 21, 2016 at 4:30 PM
Given the extra object creation, I'd rather just create an OpenSSLSecurityHelper class with extra methods for comparing things, and probably just reroute SecurityHelper and the trust engine to call that version of the matches method.
There are already headers with OpenSSL dependencies in them, so not really breaking any barriers with that.
I have no idea whether this matters, it just emerged while I was writing tests.
The
ExplicitKeyTrustEnginecode which implementsvirtual bool validate( X509* certEE, STACK_OF(X509)* certChain, const CredentialResolver& credResolver, CredentialCriteria* criteria=nullptr ) const=0;(lines 214 to 294) doesn't have a case to handle
XSECCryptoKey::KEY_EC_PUBLIC(it has the RSA and the DSA case).As an extra aside it won't handle the cases when it gets given a key par rather than just a public key, but that may not matter).
If this matters I could try to put together the fix, although IIRC it is hard to compare the openSSL EC case (it may even be impossible which is why this hasn't been implemented)