EVerest Config Option #45
Conversation
…n work with EVerest
There was a problem hiding this comment.
@louisg1337 at a high level, the goal of this PR is to remove the patches. Patches are good as a short-term solution, but they are fragile and can break as the base code changes. So the goal here was to change the raw code instead of submitting a series of patches.
However:
- most of these patches are already to config files (e.g.
docker-composeorconfig.toml) - some of them (e.g. disabling the OCSP) will not be accepted
So I think that the main change that you will need to make is:
- the one around wss:// if no certs are available, we want to turn off wss:// but not shut down the entire program; submit this as a PR, remove patch after PR is merged
- move the commit in the everest demo script forward to pick up the LB changes, and remove that patch
| timeout: 10s | ||
| retries: 3 | ||
|
|
||
| - lb: |
There was a problem hiding this comment.
I think that this is already in the maeve docker compose.
We may want to just move to a newer commit on maeve to pick up the change.
We will then need to re-create the patches that fail and re-test, but we can remove this patch.
There was a problem hiding this comment.
Oh that was my fault, I think I was using an older version of everest-demo by mistake when I was copying over the patch commands. The latest version of everest-demo/main doesn't include the lb patch so we should be all good on that front.
There was a problem hiding this comment.
Instead of applying this patch, we should change the maeve code so that instead of crashing the entire program if no certs are defined, it only stops listening to wss https://github.com/US-JOET/base-camp/issues/14#issuecomment-1980213695
We can then remove this patch.
If the MaEVe community does not agree with this change, we have to retain the patch
There was a problem hiding this comment.
I think we will have to leave this unchanged; this should be in a config, but it is already in a config, so it doesn't really help. We should really understand how this will work with the SAE PKI infrastructure, but that is not really in our scope.
| _, err = a.CertificateValidationService.ValidatePEMCertificateChain(ctx, []byte(*req.Certificate), req.IdToken.IdToken) | ||
| idTokenInfo.Status, certificateStatus = handleCertificateValidationError(err) | ||
| - if err != nil { | ||
| + if err.Error() == "failed to perform ocsp check after 1 attempts" { |
There was a problem hiding this comment.
I don't think MaEVe will accept this either since it is a hack. We should really have OCSP set up correctly.
…aned up the bash script by getting rid of wss and lb patches
|
Thank you @shankari for the feedback on all of this. I just pushed out two commits that addresses the following issues. LB Patch
You were totally right as per my response to the comment above. In the first commit, I removed the WSS Patch
To work around this issue, I added in a check before the WSS initialization that determines whether or not any certs were provided. If at least one was provided, we can go through to the initialization, but if none were provided, then we skip over the initialization. When approaching this problem, the main issue I struggled with was how to determine whether or not the user wanted to use SP1 or SP2/3. In my implementation, I am assuming that if the user provided no certificates at all, they didn't plan on using SP2/3. On the flip side, if they supplied at least one certificate, then we know they at least tried to provide something, implying they would want to use SP2/3. From there, the error handling in place already would pick up the slack and determine if they supplied all the correct ones or not. Testing Done For the WSS change, I ran our |
There was a problem hiding this comment.
@louisg1337 a few more changes here. Then once this is merged, we can change the demo script to use these patches instead of keeping two copies!
config/everest/everest-patch.sh
Outdated
| echo "Patching the CSMS to enable local mo root" | ||
| patch -p1 -i config/everest/maeve-csms-local-mo-root.patch | ||
|
|
||
| echo "Patching the CSMS to enable local mo root" |
gateway/cmd/serve.go
Outdated
| "net/url" | ||
| "os" | ||
| "time" | ||
|
|
There was a problem hiding this comment.
extraneous change; move them back so there is no commit churn
|
LGTM, let's see what the MaEVe community thinks! |
Hello again! In case anyone didn't see my last PR, I am on a team that is working on setting up software simulated demos for the charging software EVerest.
In some of those demos, we have patch files that we use to get MaEVe to work with EVerest. Our idea was that we could create some type of config within MaEVe for future use by whomever wants to setup a MaEVe and EVerest environment. This PR is our attempt at that, creating a single script that would be ran before running
docker compose up, and would apply all the patches.We would greatly appreciate suggestions against this to see if there are any better, more eloquent ways to create this config option.