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

Add Mozilla CA cert store in the docker image and enable building from macos #172

Closed
wants to merge 2 commits into from
Closed

Add Mozilla CA cert store in the docker image and enable building from macos #172

wants to merge 2 commits into from

Conversation

nicolaevladescu
Copy link

@nicolaevladescu nicolaevladescu commented Mar 4, 2019

Hi.

The initial reason of this PR has to do with us using a valid *.random.com certificate for our ssl enabled prometheus server, inside AWS EKS. And getting this error x509: failed to load system roots and no roots provided.

While looking at the source code i found the undocumented (and not available in the official stable chart) option --prometheus-ca-file that we could have used to mitigate this, but i decided to open this PR anyway, maybe somebody else could use the features that it introduces.

Related issue: #119

Introduces changes:

  • Enable building from macos LOCAL_OS=macos make docker-build by using gsed (brew install gnu-sed)
  • Fix a docker-for-mac issue with shared directories (docker container mounts failed) not being canonicalize (because ls -l /tmp -> private/tmp in macos) by using readlink and greadlink (brew install coreutils)
  • Download and verify Mozilla CA cert store in PEM format provided by curl here
  • Add the cert store to the docker image in an arbitrary chosen path from this list

@Vlaaaaaaad
Copy link

@DirectXMan12 any chance @nicolaevladescu can get a review on this? This is a blocker for us.

@s-urbaniak
Copy link
Contributor

@Vlaaaaaaad @nicolaevladescu I am a bit worried about adding official certificates to the image. These needs long-term maintenance and is usually not idiomatic in other kubernetes components.

As you recognized, there was --prometheus-ca-file added, but you can also use --prometheus-auth-config which lets you specify a kubernetes-style kubeconfig which can also include the whole *.remote.com certificate chain.

So, I would rather recommend you to either a) synthesize a kubeconfig-style including the cert-chain, mount that into the container and specify --prometheus-auth-config or b) synthesize a PEM certificate chain, mount that into the container and specify --prometheus-ca-file.

As for the fixes to make this buildable from macos, do you mind to create a separate PR for this?

@Vlaaaaaaad
Copy link

@DirectXMan12: hm... would modifying the helm chart at stable/prometheus-adapter to mount the host's /etc/ssl/certs/( or a customizable path since this differs a bit) be a better idea? Looking into it that's what some charts seem to do.

@nicolaevladescu
Copy link
Author

nicolaevladescu commented Mar 11, 2019

@Vlaaaaaaad The host's /etc/ssl/certs/ has the same Mozilla CA store but yes, it would simplify the long-term maintenance by doing it on the host side. Aside from that, maybe we can cherry pick just the Makefile macos improvements if you guys see any value in those for local development.

@DirectXMan12
Copy link
Contributor

My suggestion is in line with @s-urbaniak -- I'd recommend mounting in your cert bundle somehow. Either with a Secret or ConfigMap is best -- you don't always know that you can or want to mount in the host's certs (different OS paths, different permissions, etc). Injecting CA bundles into dockerfiles means you have to rebuild if your CA bundle changes, and that we'd have to rebuild on bundle changes.

I'll like @brancz and @s-urbaniak make the final decision though, since they're the primary maintainers of this repo at this point.

@nicolaevladescu
Copy link
Author

Thanks guys, i think we can manage without the CA part, i'll open another PR with the Makefile improvements only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants