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

Use system certs by default for oauth2 gitlab calls #1560

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hightoxicity
Copy link

If no httpClient is defined for gitlab connector, create one with system trusted ca bundle.

@hightoxicity hightoxicity force-pushed the feat-gitlab-use-system-certs branch from 9feab08 to 0fc723d Compare November 28, 2019 14:15
@bonifaido bonifaido self-requested a review December 20, 2019 07:53
@bonifaido
Copy link
Member

Hi @hightoxicity, why is this change needed? I'm concerned a bit, that client authentication by certificates is not used in this case.

@bonifaido bonifaido self-assigned this Dec 20, 2019
@hightoxicity
Copy link
Author

hightoxicity commented Dec 20, 2019

Hi guy, our gitlab instance is presenting a self-signed certificate (signed under our internal root CA), dex must trust the root CA used to issue the gitlab certificate, I have not found any way in dex to provide a trusted cert bundle... Thx

@jenting
Copy link

jenting commented Dec 21, 2019

There exists a config tlsClientCA used to specify the root CA.

@hightoxicity
Copy link
Author

hightoxicity commented Dec 21, 2019

Afaik tlsClientCA is only used to configure grpc client

@jenting
Copy link

jenting commented Dec 23, 2019

Afaik tlsClientCA is only used to configure grpc client

Okay, I was thinking is it better to add tlsClientCA by configuration? Because in this way, the user provides the CA certificate in config like LDAP connector

// Path to a trusted root certificate file.
RootCA string `json:"rootCA"`
and
// Base64 encoded PEM data containing root CAs.
RootCAData []byte `json:"rootCAData"`

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.

3 participants