EVerest / cbexigen

cbExiGen - The V2GTP EXI codec generator for cbV2G
Apache License 2.0
32 stars 18 forks source link

need different approach for exi fragment on iso20 message #57

Closed kwoyhope closed 10 months ago

kwoyhope commented 10 months ago

It seems that 'encode_iso20_exiFragment()'(also decode) is mis-designed. ('iso20_exiFragment.SignedInfo' could be fine, but others(Req/Res message types) has problem.)

For example, iso2_exiFragment.AuthorizationReq == body, but iso20_exiFragment.AuthorizationReq != body (ie, iso20_AuthorizationReqType == whole message including Header)

FYI, According to "[V2G20-1063] In case of PnC, the EVCC shall sign the PnC_AReqAuthorizationMode element using the private key associated with the contract certificate." - ie, only part(specific) of AuthorizationReq should be used. So, "encode_iso20_AuthorizationReqType(stream, &exiFrag->AuthorizationReq)" in the 'encode_iso20_exiFragment()' leads to an error(wrong exi bit streams).

kwoyhope commented 10 months ago

I find that it(fragment) element could be custimized by config. I'll try it first and get back with result.

barsnick commented 10 months ago

Correct, there is a configuration key to handle additional elements to be fragment-coded. We handled it in this unrelated ticket: https://github.com/EVerest/cbexigen/issues/53#issuecomment-1797968420

Currently, there are restrictions to which elements can be coded. We are working on making the fragment coding available for all types.

barsnick commented 10 months ago

By the way, you opened our eyes.

For ISO 15118-20 CommonMessages, we have currently configured the following elements for fragment coding:

iso20_fragments = [
    'SignedInfo',
    'AuthorizationReq',
    'CertificateInstallationReq',
    'CertificateInstallationRes',
    'MeteringConfirmationReq',
]

We have come to realize that we read Table 17 of the standard incorrectly. These should be:

iso20_fragments = [
    'SignedInfo',
    'PnC_AReqAuthorizationMode',
    'CertificateInstallationReq',
    'SignedInstallationData',
    'MeteringConfirmationReq',
    'AbsolutePriceSchedule',
]

(PnC_AReqAuthorizationMode is incorrectly listed as PnC_AReqIdentificationMode in the table in our version of the standard.)

@kwoyhope: Do you see the need for signing any other fields? If so, which are these, and why?

kwoyhope commented 10 months ago

I also think that the 'Table 17' is enough, because we should follow [V2G20-2312] and [V2G20-2313]. FYI, my document(downloaded from ISO directly) also have same typo(?). - I can't find 'PnC_AReqIdentificationMode' in any schema files. I hope that it is clarified at 'ISO 15118-20:2022/AWI Amd 1'(Corrigenda/Amendments, currently DIS 40.99(2023-10-02) stage).

And in my humble opinion, 'iso20_fragments' would be better to be configured as below, iso20_fragments = [ 'PnC_AReqAuthorizationMode', 'OEMProvisioningCertificateChain', -- not 'CertificateInstallationReq' 'SignedInstallationData', 'SignedMeteringData', -- not 'MeteringConfirmationReq' 'AbsolutePriceSchedule', ] , because (1) As I know 'SignedInfo' should be used not by 'iso20_fragments'(encode_iso20_exiFragment) but by 'xmldsigFragment'(encode_iso20_xmldsigFragment). (2) 'CertificateInstallationReq' is also whole(including header) type, and [V2G20-1548] tells me 'sign the body element of CertificateInstallationReq', but as I know there's no way to do that. (ISO-20 has no separated body only structure), and 'OEMProvisioningCertificateChain' is a meaningful(worthy to be signed) item in the body of 'CertificateInstallationReq'. Other items(ListOfRootCertificateIDs,MaximumContractCertificateChains,PrioritizedEMAIDs) might not be so important to be signed. - I also hope that it is clarified at the 'ISO 15118-20:2022/AWI Amd 1'. (3) according to [V2G20-1808], 'SignedMeteringData' should be a fragement to be signed.

By the way, I've tested fragment encoding, but unfortunately the result(fragment encoded stream) is different from SwitchEV. In this case, it's not easy to tell which one is correct. (As I know, SwitchEv has already tested ISO-20(including PnC?) with Vector anyway.)

I attach the 'comparison_fragment_result_of_PnC_AReqAuthorizationMode.txt' including full contents of below. [ Fragment(part of AuthorizationReq) to encode ] {"PnC_AReqAuthorizationMode": {"Id": "id1", "GenChallenge": "a4tFZzJ7I8ZkPJhpZjNIcw==", "ContractCertificateChain": ... [ by cbV2G, 1892 bytes EXI-encoded stream ] 804481..same..fb20 (starting event code = 137(0x89, 0100/0100/1)) [ by SwitchEV, 1893 bytes EXI-encoded stream ] 804b81..same..fb2468 (starting event code = 151(0x97, 0100/1011/1)) comparison_fragment_result_of_PnC_AReqAuthorizationMode.txt

barsnick commented 10 months ago

I hope that it is clarified at 'ISO 15118-20:2022/AWI Amd 1'(Corrigenda/Amendments, currently DIS 40.99(2023-10-02) stage).

Thanks for the pointer. (Also https://www.iso.org/standard/87920.html.) We will try to get hold of it.

And in my humble opinion, 'iso20_fragments' would be better to be configured as below,

Many interesting observations. We think you may be correct, but hope for the Addendum to clarify this.

We were also wondering how to sign complete message "bodies", and have been guessing. We thought perhaps fragment-encoding the message without the (optional) Signature element in the Header, creating the Digest and the Signature based on that, then inserting it into the Header, and then using this as the complete message. But it makes more sense assuming that something else was meant to be signed.

Remember that any element to be signed individually requires an Id attribute of type xs:ID, otherwise it cannot be referenced in the Reference. And that ISO 15118-2 and ISO 15118-20 have totally different elements with IDs. Complete messages do not happen to have IDs either.

By the way, I've tested fragment encoding, but unfortunately the result(fragment encoded stream) is different from SwitchEV.

Very interesting! The difference at the end of the stream can be explained by issue https://github.com/EVerest/cbexigen/issues/53, as commented here: https://github.com/EVerest/cbexigen/issues/53#issuecomment-1798172711. We are working on fixing this.

The difference at the beginning hints that we are missing some elements when counting the enumeration in encode_iso20_exiFragment(). We currently don't know which these are. (In your fragment, you got enum 137 with cbV2G and 151 with Josev.) We will have a check.

Are you sure that Josev is actually doing it correctly? We have no other reference yet. We also assume they have done some testing.

Thanks for the excellent report!

kwoyhope commented 10 months ago

Are you sure that Josev is actually doing it correctly? We have no other reference yet. We also assume they have done some testing.

I am also not sure that SwitchEV had tested ISO-20 with Vector completely/successfully. (That's why I added a question mark("...(including PnC?)...") previously.) What I know is based on the description of "https://github.com/SwitchEV/josev", it tells me that SwitchEV has supported ISO-20 PnC(with some limitation(ex, not using SHA512 but SHA256 in open source)), and they tested successfully in the Vector vTESTival.(what test scenarios? by the way, they have another EXI codec named REXI(not open source), but I guess it's behavior might be same except the speed.)

And I have found a clue about wrong enumeration. For example, I can see that the comment "// AckMaxDelay (urn:iso:std:iso:15118:-20:CommonTypes)"(event 5) in the "encode_iso20_ac_exiFragment()", but there's no room(no event code comment) in the "encode_iso20_exiFragment()". So, I guess there might be some missing items of 'CommonTypes' which are not used by CommonMessage but used by only other Messages(20AC, 20DC, ...). In my understanding, even though the generated file name is the "iso20_CommonMessages_Encoder.c", we need to consider all items in all schema files(including CommonTypetypes) for fragment grammar regardless it is used in common message or not.

Thanks for your rapid response and many efforts.

barsnick commented 10 months ago

Hi @kwoyhope, fixes have been merged into branch main. Please be so kind to test them.

We can now properly decode your Josev EXI fragment. We have not tested creating of that fragment, perhaps you can do so.

kwoyhope commented 10 months ago

I've tested the 'AuthorizationReq' message including 'PnC_AReqIdentificationMode', and it works well as below,

encode_iso20_exiFragment(PnC_AReqAuthorizationMode) -> OK ('.Reference[0].DigestValue' correct(same with calculated hash)) encode_iso20_xmldsigFragment(SignedInfo) -> OK ('.Signature.SignatureValue' correct(verified by PubKey in certificate(ecdsa-sha256))

I didn't all other fragment items yet, but I think it('need different approach') could be closed without doubt. I very appreciate your effort on developing.