-
Notifications
You must be signed in to change notification settings - Fork 619
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
Add trusted CA certificate support #783
Conversation
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 PR is missing the default.yaml
and override.yaml
handling in limayaml.FillDefault()
, and then should get corresponding test data filled in for default_test.go
as well.
Also please add DCO signature to your commit: https://github.com/lima-vm/lima/pull/783/checks?check_run_id=5879125698 |
Added. |
@jandubois I think I did this correctly. The tests were a little confusing to me. Let me know if its sufficient. |
Signed-off-by: Nick Petrovic <[email protected]>
caFiles := unique(append(append(d.CACertificates.Files, y.CACertificates.Files...), o.CACertificates.Files...)) | ||
y.CACertificates.Files = caFiles | ||
|
||
caCerts := unique(append(append(d.CACertificates.Certs, y.CACertificates.Certs...), o.CACertificates.Certs...)) | ||
y.CACertificates.Certs = caCerts |
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.
It would probably be better to check for duplicates once all the files have been read and combined with the literal certs. You might have multiple files containing the same cert, or having a file and a literal entry be the same. Or you might have ~/ca.pem
and /Users/jan/ca.pem
, which would be the same file, but not recognized as duplicates.
On the other hand I don't think having duplicate certs is a problem, so not marking this as a blocker.
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.
LGTM, thanks!
Note that this feature will not work with alpine-lima until lima-vm/alpine-lima#54 is implemented! |
resolves #594