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

Create setting to allow to trust the system root CAs #2430

Merged

Conversation

dhaus67
Copy link
Contributor

@dhaus67 dhaus67 commented Mar 8, 2022

Overview

This introduces a new function for the OpenShift connector with the ability to inject a http.Client for usage. This will enable us to i.e. modify the x509.CertPool and support adding system specific certificates as trusted within library usage.

What this PR does / why we need it

We are currently using dex as a library within our application running on OpenShift as a containerized workload.
When using the connector in combination with the internal OAuth server of OpenShift, they OAuth server may be either

  1. internally exposed, whereas the certificate will be explicitly added under a specific mount point.
  2. exposed externally, behind a OpenShift route and a certificate the cluster admin chose.

Since we want to cover both cases ideally when using it as a library, it would be nice to have some way of telling the connector to not only include custom root CAs, but also the system root CAs.
The best way would be to extract the list of trusted CAs from Golang's x509.CertPool and use this for configuration, but sadly this is currently not possible.

Does this PR introduce a user-facing change?

No.

@dhaus67 dhaus67 force-pushed the openshift-connector-system-root-cas branch from 626db56 to f2a804e Compare March 8, 2022 05:27
@nabokihms nabokihms self-requested a review March 9, 2022 18:19
@nabokihms
Copy link
Member

Hello, @dhaus67. I finally managed to take a look at this PR. We do not imply that someone uses Dex as a library, but I do not have concerns about rearranging the code to make it usable as a library. It still will be hard to guarantee the compatibility of your changes for future releases, though.

Speaking about this particular solution, I don't think we are willing to expose this option to end-users. If users want to append a certificate to the system root pool, they should follow the way you have mentioned (about adding the mount point).

From my point of view, this is ok to add a method to substitute the whole HTTP transport or client to make this connector more usable as a library. Something like this:

func (c *Config) OpenWithHttpClient(id string, logger log.Logger, httpClient *http.Client) (conn connector.Connector, err error) {
  ...
}

func (c *Config) Open(id string, logger log.Logger) (conn connector.Connector, err error) {
   httpClient, err := newHTTPClient(c.InsecureCA, c.RootCA); err != nil {
     return nil, fmt.Errorf("failed to create HTTP client: %v", err)
   }
   return c.OpenWithHttpClient(id, logger, httpClient)  
}

@nabokihms
Copy link
Member

@sagikazarmark /cc

Previously, when rootCA was set, the trusted system root CAs were ignored. Now, allow for both being able to be configured and used

Signed-off-by: Daniel Haus <dhaus@redhat.com>
Signed-off-by: Daniel Haus <dhaus@redhat.com>
@dhaus67 dhaus67 force-pushed the openshift-connector-system-root-cas branch from d2e2519 to 4088d4f Compare April 12, 2022 15:39
@dhaus67
Copy link
Contributor Author

dhaus67 commented Apr 12, 2022

Thanks for the feedback @nabokihms ! I've changed it to not expose it directly as a setting for the end-user, but rather as you mentioned by providing the ability to inject the http.Client. Feel free to take a look once more.

@dhaus67
Copy link
Contributor Author

dhaus67 commented May 2, 2022

@nabokihms any update on this one?

Copy link
Member

@nabokihms nabokihms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you. I don't see any strong reasons not to merge this.

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

Successfully merging this pull request may close these issues.

None yet

3 participants