Skip to content
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

Istiod's cacert.pem is under testdata/ #27574

Closed
myidpt opened this issue Sep 28, 2020 · 5 comments · Fixed by #27657, #27767 or #27766
Closed

Istiod's cacert.pem is under testdata/ #27574

myidpt opened this issue Sep 28, 2020 · 5 comments · Fixed by #27657, #27767 or #27766
Assignees

Comments

@myidpt
Copy link
Contributor

myidpt commented Sep 28, 2020

Bug description
The file tests/testdata/certs/cacert.pem is used by Istiod for JWKS endpoint authenticaiton:
https://github.com/istio/istio/blob/master/tools/istio-docker.mk#L102
https://github.com/istio/istio/blob/master/pilot/pkg/model/jwks_resolver.go#L77

[ ] Docs
[ ] Installation
[ ] Networking
[ ] Performance and Scalability
[ ] Extensions and Telemetry
[ X ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure

Expected behavior
We can use the Linux OS root cert pool, i.e., /etc/ssl/certs/ca-certificates.crt.
https://github.com/istio/istio/blob/master/docker/Dockerfile.base#L28
will update the pool when building the image.

Steps to reproduce the bug

Version (include the output of istioctl version --remote and kubectl version --short and helm version if you used Helm)
All versions.

How was Istio installed?

Environment where bug was observed (cloud vendor, OS, etc)
All

@myidpt myidpt changed the title cacert.pem is under testdata/ Istiod's cacert.pem is under testdata/ Sep 28, 2020
@myidpt myidpt added this to P1 in Prioritization Sep 28, 2020
@myidpt myidpt assigned yangminzhu and unassigned myidpt Sep 28, 2020
@howardjohn
Copy link
Member

Why is this better than /etc/ssl/certs/ca-certificates.crt? /etc/ssl/certs/ca-certificates.crt is updated for us, /cacert.pem is certainly outdated.

@xulingqing
Copy link
Member

@myidpt Hi Oliver, what's your suggestion?

One thing for implementation is on mac it do not have /etc/ssl/certs/ca-certificates.crt
I'm working on trying to install with pilot docker file and I will give you update shortly. Thanks!

@howardjohn
Copy link
Member

The dockerfile already has /etc/ssl/certs/ca-certificates.crt. We should probably just use x509.SystemCertPool() anyways though, which indirectly reads that and a few other locations?

@yangminzhu
Copy link
Contributor

Why is this better than /etc/ssl/certs/ca-certificates.crt? /etc/ssl/certs/ca-certificates.crt is updated for us, /cacert.pem is certainly outdated.

+1, we should fix the outdated problem.

@xulingqing I also checked the pilot image and it already has the certificate, so all you need to do is: 1) change to use x509.SystemCertPool() in jwks_resolver.go; 2) Remove the cacerts.pem from the docker image; 3) Test it a little bit to make sure it works (can use curl for a quick test)

@yangminzhu yangminzhu removed their assignment Sep 30, 2020
@myidpt
Copy link
Contributor Author

myidpt commented Sep 30, 2020

Sure, I'm OK with using the system root cert pool. Checked the code and found this:
https://github.com/istio/istio/blob/master/docker/Dockerfile.base#L28
This will update the system root cert pool when building the image.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment