Support hashing directories of certificates #852
Conversation
jannejy
requested review from
Pietfried,
hikinggrass,
marcemmers and
maaikez
as code owners
jannejy
force-pushed
the
feature/support_folder_certificates
branch
from
d527a9a to
0d5a28a
Compare
3rd_party/cert_rehash/c_rehash.hpp
Outdated
| @@ -0,0 +1,310 @@ | |||
| /* c_rehash.c - Create hash symlinks for certificates | |||
There was a problem hiding this comment.
This file was taken from: https://gitlab.alpinelinux.org/alpine/ca-certificates/-/blob/master/c_rehash.c?ref_type=heads
Done some changes:
- only the
hash_dirand dependent methods are left - removed all the parameters of command line that are were used in main.
- changed logging to Everest (can be an issue), maybe needed help to overcome static analysis issues
- added explicit C-conversion to compile
- wrapped in the ocpp namespace
- some initialization is moved to
hash_dirto work properly
There was a problem hiding this comment.
Also, it's worth looking together with the PR in evse-security: EVerest/libevse-security#96
jannejy
force-pushed
the
feature/support_folder_certificates
branch
2 times, most recently
from
6a20562 to
991e73f
Compare
|
In general it looks good to me. I like the fact that there's a conditional to check if we're using a directory or a file, however see my comment in the evse-security PR: EVerest/libevse-security#96 (comment) If it handles that case in security code too, I'll have another look and approve. |
| @@ -370,11 +371,16 @@ bool WebsocketLibwebsockets::tls_init(SSL_CTX* ctx, const std::string& path_chai | |||
| } | |||
|
|
|||
| if (this->evse_security->is_ca_certificate_installed(ocpp::CaCertificateType::CSMS)) { | |||
| std::string ca_csms = this->evse_security->get_verify_file(ocpp::CaCertificateType::CSMS); | |||
| std::string ca_csms = this->evse_security->get_verify_location(ocpp::CaCertificateType::CSMS); | |||
There was a problem hiding this comment.
I see here that the get_verify_location is used, if based on the config this is a file, the file should be used, else the directory, extra checks have to be placed here to use the proper location/file based on the configuration.
There was a problem hiding this comment.
There is below the check if this is a directory or file. What do you mean?
There was a problem hiding this comment.
The problem with this approach is that it uses a fundamentally different approach than get_verify_file. The get_verify_file searches inside the whole directory, for a single root certificate that is valid. For example if we are using a folder 'certificates/ca/mf' and in it we have MF_ROOT1.pem and MF_ROOT2.pem, the get_verify_file will return the certificates/ca/mf/MF_ROOT2.pem presuming that it will be newer. On the other hand if we use get_verify_location it will return either certificates/ca/mf/MF_ROOT1.pem or 'certificates/ca/mf' based on the configuration if we use a directory.
The behavior is very different.
What we need here is some flag and/or config value (I am can't remember if one already exists) that either uses the old get_verify_file with it's behavior or the new get_verify_location with it's particular behavior.
There was a problem hiding this comment.
For the file, the behavior is not changed at all, as both methods are returning a path to a file.
If we are using a directory, we just let the openssl library choose the proper one, not based on the assumptions, which certificate is newer. For example, we have one certificate for one server, it is working, ok. We added another one for another server. If we want to connect to the first server, we won't, as this newest certificate is for another server. This way, we burden the end-user to do extra actions, such as deleting the second certificate.
Why don't we want to pass to the openssl library the choice which certificate we are using for the particular server?
There was a problem hiding this comment.
The point is valid, however I do need to double check if there can be issues when using c_rehash on some environments, or other workarounds that have been applied in places. If I can get a confirmation that those are not likely to cause issues, I'll approve, since your approach seems better.
There was a problem hiding this comment.
Based on latest feedback, the new approach that can automatically smart select the certificate based even on multiple CSMS roots is better. However, based on internal discussions, it would be better to move the c_rehash functionality inside libevse-security since we do not want to mix too much security in libocpp.
Edit: the function c_rehash can be called inside get_verify_location, with the note that in the function's documentation that should be stated, that at each invocation the c_rehash is invoked.
jannejy
force-pushed
the
feature/support_folder_certificates
branch
from
afe4c3f to
1abc61f
Compare
Signed-off-by: Ivan Rogach <[email protected]>
Signed-off-by: Ivan Rogach <[email protected]>
Signed-off-by: Ivan Rogach <[email protected]>
Signed-off-by: Ivan Rogach <[email protected]>
jannejy
force-pushed
the
feature/support_folder_certificates
branch
from
d186227 to
d643278
Compare
Describe your changes
Issue ticket number and link
Checklist before requesting a review