feat: expose OpenSSL handles so that other code can create objects

[james-ctc]

Describe your changes

Previously X509HandleOpenSSL and KeyHandleOpenSSL were defined in openssl_supplier.cpp this made it difficult to create Handles from other OpenSSL code and use the methods in evse-security.

Issue ticket number and link

Checklist before requesting a review

• I have performed a self-review of my code
• I have made corresponding changes to the documentation
• I read the contribution documentation and made sure that my changes meet its requirements

[AssemblyJohn]

Hello! I don't see the point in exposing those for all the code. What I'd do is create builder/utility functions takes in a X509 (or a file or whatever) that return the base (X509Handle). If we allow this code, we destroy all the 'not make this dependent on openSSL idea that we had in the first place.

OpenSSL code should not exit the openssl supplier.

[james-ctc]

The file is only in OpenSSL detail and is just like openssl_types.hpp (I'm not clear why those types are allowed to be outside openssl_supplier and not the two handle classes)

Creating a utility function would have to be in open_ssl_supplier since that is where KeyHandleOpenSSL are defined.

That would mean openssl_supplier.hpp would need to have OpenSSL types (X509 ...) rather than just supporting the crypto supplier interface.

I'm happy to add a conversion function to openssl_supplier.hpp but it does expose OpenSSL types to the rest of the code.

[james-ctc]

changed to provide utility function rather than expose class.

[AssemblyJohn]

Ok, but in that case you can leave the 'X509HandleOpenSSL' inside the openssl_supplier.cpp, that was the whole point of creating the utilities. Also, I am not sure, but why would be we need to expose that 'X509HandleOpenSSL' and for what purpose, where exactly?

EDIT: Sorry I did not see the last force push. Again I was not sure, for what purpose do we require this functionality? In case we need to see the internal pointer, maybe we could add a function in 'struct CryptoHandle' (

libevse-security/include/evse_security/crypto/interface/crypto_types.hpp

Line 74 in aa79e39

struct CryptoHandle {

) like void *get_raw_pointer() and cast that if we really know what we are doing?

EDIT2:

static X509Handle X509Handle_from_X509(X509* certificate);
static X509Handle_ptr X509Handle_ptr_from_X509(X509* certificate);

Also add those in the 'AbstractCyptoSupplier' to to make the code dependent on the 'OpenSSLCryptoSupplier' only. I'd also have the parameters as 'void *' instead of 'X509' so that we are not dependant on the raw type, but the supplier can cast internally.

Again if we have a use-case for this it could be more useful. Is this somehow used in some code external to libevse?

[james-ctc]

The specific use case. I'm working on the OpenSSL server code for V2G. That means working with the OCSP data and certificate hashes. I want to be able to calculate hashes on certificates not created by libevse-security. It wouldn't make sense to duplicate the hash generation code.
Since my code is not part of openssl_supplier I was unable to create certificate hashes.
One option was to make the underlying classes available like X509_ptr is done in openssl_types.hpp ...
Or as you prefer a conversion function.

The workflow based on having an OpenSSL X509 pointer:

1. std::make_unique(cert) which gives a X509Handle_ptr
2. create X509Wrapper from the X509Handle_ptr
3. call get_certificate_hash_data()

without any change to libevse-security I can't create the X509Handle_ptr

An alternative is to use X509CertificateHierarchy but I still need the certificates in a X509Wrapper

[james-ctc]

use of void * removes all type checking. Which is why I preferred making X509HandleOpenSSL available to code that needs it.

[AssemblyJohn]

Understood, it's about external code. Now that I know what this is about, maybe the smarter idea (your initial idea) is to move the handles (X509HandleOpenSSL) from the openssl_supplier to the openssl_types.hpp, since, by convention we know that the 'detail' headers are not for external usage. Because there are other methods of making the external code use openssl/mbedtls which are not related to the libevse-usage. In external code it makes sense.

However, there is one more problem, about the certificate hash creation, care has to be taken because the get_certificate_hash_data sometimes only works with hierarchies, since it is dependent on the issuer's key hash. Probably using the 'X509CertificateHierarchy' is the best approach. I don't know how the certificates are loaded, but in that case, maybe it is better use use the already-existing functionality with hierarchies and bundles that take care of properly loading the certificates from a file/path, building the hierarchy etc... instead of relying on manual creation of raw pointers.

[james-ctc]

With OCSP I need to match the OCSP filename to a specific certificate.
The status_request_v2 extension requires that the OCSP responses are in the same order (and only those certificates) that are being used in the OpenSSL handshake. The extension handler has access to X509 pointers not files on a disk.
Unfortunately the hash function is actually hashing the issuer rather than the actual certificate which makes matching certificates to OCSP filename more difficult than it needs to be.

My preference would be for the certificate hashes in get_leaf_certificate_info() to use a different hash function from X509Wrapper that makes it easy to match certificate to OCSP response filename. e.g. just the hash of the certificate public key perhaps get_key_hash()?

The certificate signature (or hash of the signature, or just the hash of tbsCertificate) would be even better. Since that would represent a unique value for a certificate.

Certificate ::= SEQUENCE {
tbsCertificate TBSCertificate,
signatureAlgorithm AlgorithmIdentifier,
signatureValue BIT STRING }

[AssemblyJohn]

Understood, so you need a way to uniquely identify a certificate. I see no problem in either providing an extra function inside the X509Wrapper that hashes the whole certificate, or to use the 'get_key_hash' that hashes the public key that is unique per certificate and can be used to uniquely identify the certificate.

As for the 'get_certificate_hash_data' existing, it made to be compliant with the spec, and you are not enforced to use it inside the handshake or other utility code that might require the unique identification of a certificate. Also the addict of a function 'get_complete_certificate_hash' based on the full certificate, sounds a bit overkill (i'd just use the 'get_key_hash' to internally identify certificates) but is also plausible and usable.

I don't understand the 'get_leaf_certificate_info' part, you should just receive some file paths (that you know that are some certificates) and it is up to you how you load/use them and how you match the data. However, note, that the certificate OCSP data attached in there is matched to the certificates IN ORDER. That is the first certificate in the bundle matches the first OCSP entry in the return function, the second certificate in the bundle matches the second OCSP data in the bundle. That is done on purpose, so that you don't have to do any fancy matching of the OCSP data to the certificate.

How does that sound for you?

[james-ctc]

in pseudo code this is the process

MQTT call_get_leaf_certificate_info()
map<hash, ocsp_filename>
for (ocsp from ocsp optional vector in ):
map[ocsp.hash] = ocsp.ocsp_path

in OpenSSL extension handler
for (cert from certificates in handshake):
hash = hash(cert)
ocsp_response += map[hash]

What I actually need to do is:
MQTT call_get_leaf_certificate_info()
load the certificate chain file from call_get_leaf_certificate_info
certificates = load_from_file()
check number of certificates == number of ocsp
map<signature, ocsp_filename>
for (i = 0 to number of certificates):
map[certificates[i].signature] = ocsp[i].ocsp_path

in OpenSSL extension handler
for (cert from certificates in handshake):
ocsp_response += map[cert.signature]

It is a bit wasteful to have to load the certificate chain file again.
Ideally the MQTT response should be directly usable without having to reload/reparse files etc.
As it stands the hash in the ocsp vector isn't directly usable.

[AssemblyJohn]

Yep, everything is clear. I'd suggest the following:

1. MQTT call_get_leaf_certificate_info()
2. load the certificate chain file from call_get_leaf_certificate_info (certificates = load_from_file())
3. declare map<internal_hash, path> hash_to_ocsp_path;
4. iterate the loaded certificates for(idx = 0; i < certificate.size(); ++idx)
   • assert certificates.size() == number of ocsp
   • internal_hash = X509Wrapper(certificate[idx]).get_key_hash())
   • hash_to_ocsp_path[internal_hash] = ocsp[idx].ocsp_path (since the OCSP data is ordered based on the cert file order)
5. in OpenSSL extension handler for (openssl_handshake_cert from certificates in handshake):
   • internal_hash = X509Wrapper(openssl_handshake_cert).get_key_hash()
   • ocsp_path = hash_to_ocsp_path[internal_hash];
   • ocsp_response += load_file_binary(ocsp_path)

In that way you don't care about the wonky spec 'CertificateHashData' but you just map the OCSP data based on the 'get_key_hash'. Let me know how does this sound!

[james-ctc]

X509HandleOpenSSL and KeyHandleOpenSSL moved to detail/openssl_types.hpp

[AssemblyJohn]

Looks good.