Mg c07 authorize contract certs #527
Conversation
…ase C07 requirements Signed-off-by: Menno de Graaf <[email protected]>
Signed-off-by: Menno de Graaf <[email protected]>
Signed-off-by: Menno de Graaf <[email protected]>
Signed-off-by: Menno de Graaf <[email protected]>
… chain Signed-off-by: Menno de Graaf <[email protected]>
Signed-off-by: Menno de Graaf <[email protected]>
Signed-off-by: Marc Emmers <[email protected]>
Signed-off-by: Menno de Graaf <[email protected]>
Signed-off-by: Menno de Graaf <[email protected]>
Signed-off-by: Menno de Graaf <[email protected]>
| std::vector<OCSPRequestData> ChargePoint::generate_ocsp_data(const CiString<5500>& certificate) { | ||
| std::vector<OCSPRequestData> ocsp_request_data_list; | ||
| const auto ocsp_data_list = | ||
| this->evse_security->get_ocsp_request_data(certificate.get(), ocpp::CaCertificateType::MO); |
There was a problem hiding this comment.
I don't get where the MO type comes from here. I would either pass it to the function that it needs to be an MO root or change the function name to show that its OCSP data for an MO certificate chain.
There was a problem hiding this comment.
During P&C the OCSP data needs to be generated for a contract chain, therefore MO is the default. If there are other use cases, we could indeed add the CertificateType as an argument.
There was a problem hiding this comment.
Just noticed that EVerest/libevse-security#61 is another use case.
| @@ -335,6 +335,31 @@ CaCertificateType string_to_ca_certificate_type(const std::string& s); | |||
| /// \returns an output stream with the CaCertificateType written to | |||
| std::ostream& operator<<(std::ostream& os, const CaCertificateType& ca_certificate_type); | |||
|
|
|||
| enum class CertificateValidationResult { | |||
There was a problem hiding this comment.
I see this done before so maybe not the right time, but I think we should prevent making a copy of an enum definition at all cost. This is going to be really error prone.
If we want it in scope I would suggest using some kind of using statement here.
There was a problem hiding this comment.
I guess how I did it is similar to how it is typically done in libocpp. @Pietfried any suggestions?
|
|
||
| // C07.FR.01: When CS is online, it shall send an AuthorizeRequest | ||
| // C07.FR.02: The AuthorizeRequest shall at least contain the OCSP data | ||
| // TODO: local validation results are ignored if response is based only on OCSP data, is that acceptable? |
There was a problem hiding this comment.
I am not sure about this. One could even argue that we should always generate the OCSP data in libocpp instead of allowing this data coming from the outside but I don't know if that is possible. Are there sources that only give us the OCSP data?
The way it is now a caller to this function can give a cert chain with different data then the OCSP contains so then we might need to check. In that case not even having OCSP as an argument would be just as quick.
There was a problem hiding this comment.
I implemented it this way to avoid breaking existing functionality. AFAIK in EVerest, it is possible that local chain validation and generation of OCSP data is done already by the ISO15118 module.
| } | ||
| } else { | ||
| // C07.FR.07: CS shall not allow charging | ||
| response.idTokenInfo.status = AuthorizationStatusEnum::NotAtThisTime; |
There was a problem hiding this comment.
I would suggest a different return type here. NotAtThisTime holds a special meaning an could be used for UI purposes later. In this case I would just go with Unknown.
There was a problem hiding this comment.
I have set it to NotAtThisTime for this purpose as the CSMS is offline. If the user comes back some other time if the CSMS if online again, authorization might be ok. The user gets feedback that the failed authorization is because of some problem in the CS, not with his contract.
There was a problem hiding this comment.
Fair enough but that would be for other modules to decide what to show to the used based on the online status. NotAtThisTime is a specific return type specified in OCPP and in my opinion should hold special meaning if the CSMS reports this.
In other cases when we are offline and not able to authorize we also respond with unknown.
lib/ocpp/v201/charge_point.cpp
Outdated
| // Temporary variable that is set to true to avoid immediate response to allow the local auth list | ||
| // or auth cache to be tried | ||
| bool tryLocalAuthListOrCache = false; | ||
| bool forwardToCsms = false; |
There was a problem hiding this comment.
Nit: rename to forwardedToCsms. I now expected you to still take an action in the if statement checking this.
Signed-off-by: Menno de Graaf <[email protected]>
Signed-off-by: Menno de Graaf <[email protected]>
|
Closed because replaced by: #536 |
Describe your changes
verify_certificateso that it can also be used for ISO P&C chain validationIssue ticket number and link
#324
Checklist before requesting a review