-
Notifications
You must be signed in to change notification settings - Fork 22
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.
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
MRE with working OCPP 2.0.1 security profile 1, 2 and 3 (all) #31
Conversation
@activeshadow and @sahabulh you need to sign off on your commits. This is required for LFE projects to ensure clear copyright for the generated code https://developercertificate.org/, https://stackoverflow.com/a/1962112 If you want to go back and sign your commits, you can rebase with -s https://stackoverflow.com/a/15667644 or close this PR and submit a new one
Where are you checking the logs? The fact that your command starts with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sahabulh I am a bit confused. I thought that with the most recent release of EVerest, we still had issues with the connection. These MREs are still using the most recent version, but appear to be passing.
Is it possible to summarize what changes were required to get this to work? It would be helpful both for the review and for the historical record.
I also have some high-level cleanup feedback below.
echo "Adding host.docker.internal address to EVerest manager" | ||
docker exec -it everest-ac-demo-manager-1 sh -c "echo -e \"\$(ip route | grep default | cut -d' ' -f3)\thost.docker.internal\" >> /etc/hosts" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not required on all hosts. Concretely, I have a mac and host.docker.internal
works just fine. I assume you are trying to run this on linux - host.docker.internal
not working on Linux is a well known issue for docker. I would suggest using the recommended solution instead of mucking around with the /etc/hosts
moby/libnetwork#2348 (comment)
or
https://medium.com/@TimvanBaarsen/how-to-connect-to-the-docker-host-from-inside-a-docker-container-112b4c71bc66
If you must run this command, please do so only for linux.
# echo "All configuration done, please run 'docker exec -it everest-ac-demo-manager-1 /bin/bash' and then (in the container) 'sh ./build/run-scripts/run-sil-ocpp201.sh'" | ||
# echo "Note that this is currently expected to fail https://github.com/EVerest/everest-demo/issues/25#issuecomment-1991954008" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are going to run it directly, we should just remove the echo
statements. But we may also want to not start the server directly from the script because it makes it harder to start and stop the server and debug it. I would suggest removing the docker exec -it
and re-enabling the echo for now, until we feel like this is fully stable.
@sahabulh can you please address the review feedback? Also here's the list of commits that are currently not signed off |
496e868
to
0857bed
Compare
@shankari This PR is ready to be looked at again |
0857bed
to
c1f6da8
Compare
@sahabulh can you please comment on where the EonTI certificates come from? Did you generate them from the root, or are they generated by EonTI? I would like to respond and provide feedback to EonTI on the certificates that they have generated and the ability to use them in software settings |
@shankari sorry to jump in this conversation, especially since I'm not answering your EonTI question specifically, but I did want to point out that in general we (@sahabulh and I) did not include anything specific to EonTI in this PR. This PR is still using the same certs as the SP2 demo. If/When the EonTI stuff gets figured out, the plan is to have that in a separate commit/PR to keep things clean. |
@shankari Craig is already talking with the EonTi team about this. I don't have the details. But I will let you know what happens. |
@shankari also worth noting that if you want to test the SP3 demo script locally, you'll need to be sure to change line 34 in the demo script to clone the forked repo and branch this PR is based on instead of the main repo. Just FYSA, though I'm sure you're already aware. |
@activeshadow understood. I hijacked this PR a bit because I would really like the response to the EonTI question and I wasn't seeing a response to it otherwise. @sahabulh so are you saying that Craig provided the EonTI certificates to you? I would just like clarity on where the certificates came from because they do not appear to match the certificates in the Google Drive shared by EonTI. I can go out to EonTI saying that I don't know where the certificates came from, but in general, it is better to provide as much information as possible upfront. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sahabulh Once you revert that change, I am happy to merge this. But when you submit the EonTI PR, I would like to see a description of how the certs are generated. This is an MRE after all so we need to understand all the steps properly!
I also assume you have tested, and am merging optimistically! We can always fix if it turns out that this doesn't work.
@shankari @sahabulh @activeshadow Certificates we're using come from the CRP PKI prototype instance Sandia has access to under an MOU with SAE. They might differ from those EonTi is providing for NREL testing early next month. BTW I've invited SAE and EonTi leads to join the EVerest General weekly call tomorrow and requested a slot on the agenda for them to share the current situation re: availability of their planned test platform (APIs) and roadmap. To move beyond uncertainty/confusion about cert provenance, intended use, etc. I've suggested EVerest modules use a local repository (for now, perhaps APIs later) that supports multiple well-documented cert bundles. |
@sahabulh you didn't sign off on your most recent commit that reverted the changes to |
4e21b4b
to
c1f6da8
Compare
@activeshadow Sorry, fixed it in a different way. Is it okay? |
Yes sir, all good! |
Signed-off-by: Md Sahabul Hossain <[email protected]>
Signed-off-by: Md Sahabul Hossain <[email protected]>
Signed-off-by: Md Sahabul Hossain <[email protected]>
241992f
to
1f06b16
Compare
@activeshadow Respecting your suggestion, I squashed the last commit into the first one using the interactive rebase and then force pushed it. |
Please review again. I reverted the changes to the config file. |
Noice! 👍 |
@CRR-SNL thank you for that clarification! That explains why I could not find any of these certs in the files provided by EonTI for the PKI testing event. wrt
The EVerest libevsecurity module supports cert bundles - if you check the script included here, you will see that we upload a cert bundle to the EVSE container and copy the certs to the correct locations. Any arbitrary cert bundle can be supported in the same way. However, I am not sure that EVerest will want to have multiple cert bundles installed in the EVSE at the same time; in production, an EVSE will have the cert bundle for the CSMS that they are connecting to. Having said that, from an internoperability perspective, maybe it would be better to have multiple V2G roots in a root bundle to make it easier to switch between networks. To go back to my favorite example of browser + website interop, most browsers will store multiple CA roots, which they keep updated through periodic updates. Did you mean multiple roots or multiple bundles? |
@sahabulh the checks are failing since you renamed device_model_storage_maeve_preconfigured.db to manager/device_model_storage_maeve_sp1.db Please either restore, or change the manager Dockerfile |
Signed-off-by: Md Sahabul Hossain <[email protected]>
Done! |
Checks now pass. We should fix the static analysis across all the scripts so that all the checks pass 😄 but that doesn't need to hold up this release. |
Based on Bryan's fix for the first MRE. I have just added scripts and databases for all profiles.
Two issues: