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

Jump to bottom

857 certificates handling over an unsecure connection in ocpp201 should not be allowed #863

Merged
merged 5 commits into from
Nov 18, 2024
Merged

857 certificates handling over an unsecure connection in ocpp201 should not be allowed #863

merged 5 commits into from
Nov 18, 2024

Conversation

shingoxx222
Copy link
Contributor

@shingoxx222 shingoxx222 commented Nov 8, 2024 •
edited
Loading

Describe your changes

Issue ticket number and link

Checklist before requesting a review

@shingoxx222 shingoxx222 linked an issue Nov 8, 2024 that may be closed by this pull request
Certificate installation with unsecure connection in OCPP2.0.1 should not be allowed #857
Closed
@shingoxx222
Copy link
Contributor Author

shingoxx222 commented Nov 8, 2024 •
edited
Loading

test with octt:
image

@shingoxx222 shingoxx222 force-pushed the 857-certificates-handling-over-an-unsecure-connection-in-ocpp201-should-not-be-allowed branch from 37ea94e to 8a54349 Compare November 8, 2024 12:04
AssemblyJohn
AssemblyJohn reviewed Nov 11, 2024
View reviewed changes
Copy link
Contributor

@AssemblyJohn AssemblyJohn left a comment

Choose a reason for hiding this comment

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

At an initial look, it looks good to me.

marcemmers
marcemmers requested changes Nov 13, 2024
View reviewed changes
include/ocpp/v201/smart_charging.hpp Outdated
@@ -26,6 +26,7 @@ enum class ProfileValidationResultEnum {
Valid,
EvseDoesNotExist,
ExistingChargingStationExternalConstraints,
InvalidSecurityProfile,
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is unrelated to smart charging we should not add this to a result enum of smart charging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

using hardcoded string now

lib/ocpp/v201/charge_point.cpp Outdated
"Installed certificate: " + conversions::install_certificate_use_enum_to_string(msg.certificateType);
this->security_event_notification_req(CiString<50>(security_event), CiString<255>(tech_info), true,
utils::is_critical(security_event));
if ((msg.certificateType == InstallCertificateUseEnum::CSMSRootCertificate ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Allowing installation of CSMSRootCertificate and ManufacturerRootCertificate should be configurable, not hardcoded. I think preferably they should be configurable separately from each other.

Default should also allow this since that is what the current implementation does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, I make them ReadWrite and not required

@shingoxx222 shingoxx222 force-pushed the 857-certificates-handling-over-an-unsecure-connection-in-ocpp201-should-not-be-allowed branch 5 times, most recently from 17058a0 to 23d04b3 Compare November 13, 2024 14:58
marcemmers
marcemmers requested changes Nov 14, 2024
View reviewed changes
config/v201/component_config/standardized/SecurityCtrlr.json Outdated
@@ -177,6 +177,38 @@
"maximum": 3,
"default": "1",
"type": "integer"
},
"AllowCSMSRootCertificateInstallWhenLowSecurityProfile": {
"variable_name": "AllowCSMSRootCertificateInstallWhenLowSecurityProfile",
Copy link
Contributor

Choose a reason for hiding this comment

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

"Low" security profile leaves room for interpretation, it could be a bit more explicit still ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is now AllowCSMSRootCertInstallWhenSecurityProfile1

lib/ocpp/v201/charge_point.cpp Outdated
@@ -3588,24 +3588,52 @@ void ChargePoint::handle_get_installed_certificate_ids_req(Call<GetInstalledCert
this->send<GetInstalledCertificateIdsResponse>(call_result);
}

bool ChargePoint::should_reject_certificate_install(InstallCertificateUseEnum cert_type) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice to make a function of this to clean up the handler 👍

I do always have a difficult time with the double negatives especially with value_or in the mix. Would it be an option to change this to should_accept_certificate_install instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

config/v201/component_config/standardized/SecurityCtrlr.json Outdated
"attributes": [
{
"type": "Actual",
"mutability": "ReadWrite"
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason you've chosen ReadWrite here? Keep in mind the reason for this change is to prevent parties to install wrong certificates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought ReadOnly is the same as unmutable. now it is ReadOnly

marcemmers
marcemmers reviewed Nov 14, 2024
View reviewed changes
lib/ocpp/v201/charge_point.cpp Outdated
response.status = InstallCertificateStatusEnum::Rejected;
response.statusInfo = StatusInfo();
response.statusInfo->reasonCode = "LowSecurityProfile";
response.statusInfo->additionalInfo = "SecurityProfileTooLowForCertificateInstall";
Copy link
Contributor

Choose a reason for hiding this comment

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

This one could also be improved a bit. For example "Installation of certificate is not allowed when security profile is less then 2"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made it CertificateInstallationNotAllowedWhenSecurityProfile1

marcemmers
marcemmers reviewed Nov 14, 2024
View reviewed changes
lib/ocpp/v201/ctrlr_component_variables.cpp Outdated
@@ -1116,6 +1116,20 @@ const RequiredComponentVariable& SecurityProfile = {
"SecurityProfile",
}),
};
const ComponentVariable& AllowCSMSRootCertificateInstallWhenLowSecurityProfile = {
ControllerComponents::SecurityCtrlr,
Copy link
Contributor

Choose a reason for hiding this comment

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

They should be part of the InternalCtrlr since they are configuration items used internally

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@shingoxx222 shingoxx222 force-pushed the 857-certificates-handling-over-an-unsecure-connection-in-ocpp201-should-not-be-allowed branch from 534a4eb to eb89ebe Compare November 14, 2024 14:16
@shingoxx222 shingoxx222 force-pushed the 857-certificates-handling-over-an-unsecure-connection-in-ocpp201-should-not-be-allowed branch from bcf645f to 7f3ce1f Compare November 15, 2024 13:41
@shingoxx222 shingoxx222 merged commit 0fbecf4 into main Nov 18, 2024
8 checks passed
@shingoxx222 shingoxx222 deleted the 857-certificates-handling-over-an-unsecure-connection-in-ocpp201-should-not-be-allowed branch November 18, 2024 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AssemblyJohn AssemblyJohn AssemblyJohn left review comments

@marcemmers marcemmers marcemmers approved these changes

@Pietfried Pietfried Awaiting requested review from Pietfried Pietfried is a code owner

@hikinggrass hikinggrass Awaiting requested review from hikinggrass hikinggrass is a code owner

@maaikez maaikez Awaiting requested review from maaikez maaikez is a code owner

Assignees

@marcemmers marcemmers

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Certificate installation with unsecure connection in OCPP2.0.1 should not be allowed
3 participants
@shingoxx222 @AssemblyJohn @marcemmers