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.

Sign up for GitHub

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

Support hashing directories of certificates #852

Merged
merged 4 commits into from
Dec 3, 2024
Merged

Support hashing directories of certificates #852

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
045b486
Support hashing directories
jannejy Dec 3, 2024
99163ef
Fix after fixes in libevse-security
jannejy Dec 3, 2024
d335bad
Move hashing directories out of libocpp
jannejy Dec 3, 2024
d643278
Update tag to libevse-security
jannejy Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion dependencies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ date:
options: ["BUILD_TZ_LIB ON", "HAS_REMOTE_API 0", "USE_AUTOLOAD 0", "USE_SYSTEM_TZ_DB ON"]
libevse-security:
git: https://github.com/EVerest/libevse-security.git
git_tag: v0.9.1
git_tag: v0.9.2
libwebsockets:
git: https://github.com/warmcat/libwebsockets.git
git_tag: v4.3.3
Expand Down
5 changes: 5 additions & 0 deletions include/ocpp/common/evse_security.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,11 @@ class EvseSecurity {
/// \return CA certificate file
virtual std::string get_verify_file(const CaCertificateType& certificate_type) = 0;

/// \brief Retrieves the PEM formatted CA bundle location for the given \p certificate_type
/// \param certificate_type
/// \return CA certificate file
virtual std::string get_verify_location(const CaCertificateType& certificate_type) = 0;

/// \brief Gets the expiry day count for the leaf certificate of the given \p certificate_type
/// \param certificate_type
/// \return day count until the leaf certificate expires
Expand Down
1 change: 1 addition & 0 deletions include/ocpp/common/evse_security_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ class EvseSecurityImpl : public EvseSecurity {
bool include_ocsp = false) override;
bool update_certificate_links(const CertificateSigningUseEnum& certificate_type) override;
std::string get_verify_file(const CaCertificateType& certificate_type) override;
std::string get_verify_location(const CaCertificateType& certificate_type) override;
int get_leaf_expiry_days_count(const CertificateSigningUseEnum& certificate_type) override;
};

Expand Down
4 changes: 4 additions & 0 deletions lib/ocpp/common/evse_security_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,10 @@ std::string EvseSecurityImpl::get_verify_file(const CaCertificateType& certifica
return this->evse_security->get_verify_file(conversions::from_ocpp(certificate_type));
}

std::string EvseSecurityImpl::get_verify_location(const CaCertificateType& certificate_type) {
return this->evse_security->get_verify_location(conversions::from_ocpp(certificate_type));
}

int EvseSecurityImpl::get_leaf_expiry_days_count(const CertificateSigningUseEnum& certificate_type) {
return this->evse_security->get_leaf_expiry_days_count(conversions::from_ocpp(certificate_type));
}
Expand Down
8 changes: 6 additions & 2 deletions lib/ocpp/common/websocket/websocket_libwebsockets.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,11 +374,15 @@ 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is below the check if this is a directory or file. What do you mean?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jannejy jannejy Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@AssemblyJohn AssemblyJohn Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


EVLOG_info << "Loading CA csms bundle to verify server certificate: " << ca_csms;

rc = SSL_CTX_load_verify_locations(ctx, ca_csms.c_str(), NULL);
if (std::filesystem::is_directory(ca_csms)) {
rc = SSL_CTX_load_verify_locations(ctx, NULL, ca_csms.c_str());
} else {
rc = SSL_CTX_load_verify_locations(ctx, ca_csms.c_str(), NULL);
}

if (rc != 1) {
EVLOG_error << "Could not load CA verify locations, error: " << ERR_error_string(ERR_get_error(), NULL);
Expand Down
1 change: 1 addition & 0 deletions tests/lib/ocpp/common/evse_security_mock.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ class EvseSecurityMock : public EvseSecurity {
(override));
MOCK_METHOD(bool, update_certificate_links, (const CertificateSigningUseEnum&), (override));
MOCK_METHOD(std::string, get_verify_file, (const CaCertificateType&), (override));
MOCK_METHOD(std::string, get_verify_location, (const CaCertificateType&), (override));
MOCK_METHOD(int, get_leaf_expiry_days_count, (const CertificateSigningUseEnum&), (override));
};

Expand Down