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

Added support for TLS client auth for datasource proxies (#5801) #5899

Closed

Conversation

joelanford
Copy link
Contributor

@joelanford joelanford commented Aug 25, 2016

Fixes #5801 (and #2316)

The basic functionality is now there to add client cert and key paths (local to the grafana server) to the data source HTTP auth configuration when using proxy access. I thought about adding another boolean for TLS server verification (its currently hardcoded to skip server verification), but I wanted to submit a small checkpoint to make sure I'm on the right track.

Any comments or questions? Is there any preference for the layout of the new form fields on the datasource edit screen? I kind of just took a stab at it.

@bergquist bergquist added the pr/needs-manual-testing Before merge some help with manual testing & verification is requested label Aug 25, 2016
@joelanford
Copy link
Contributor Author

Just wanted to check in on the status of this PR. Any chance this gets pulled in to an upcoming release? Any changes you'd like to see?

@bergquist
Copy link
Contributor

@joelanford I will review and merge this as soon as I find time. It will most likely be included in the next release. Its something that we really want its just a matter of how :)

@tomkozlowski
Copy link
Contributor

@bergquist / @torkelo / @DanCech any chance this is going to make it into 4.0.0-beta2 ? This is a fairly important feature for me and having to build artifacts off of munged PRs is sub-optimal

@bergquist
Copy link
Contributor

@tomkozlowski This is something we want internally so we decided to add support for it our self. It should be available soonish.

The reason we decided to do it our self is that we wanted to improve UX for configuring certs on the data source edit page. Sometimes its just easier to start coding then starting a feedback process.

@tomkozlowski
Copy link
Contributor

@bergquist how soon is soonish? Is there a ticket or a branch we should be following? Should this PR be closed with a pointer to the new work?

@daniellee
Copy link
Contributor

Hi @joelanford

Thanks for your great work here! I based #6692 on it and used your code for the TLS authentication. The difference is that we wanted to save the TLS Cert data rather than filepaths to the cert/key files. I also took the chance to pretty up the html/css for http settings.

The other change was to allow upload of CA Certs for self-signed certs instead of setting InsecureSkipVerify to true.

Do you have any thoughts on not saving filepaths and not using InsecureSkipVerify? Does that work for you?

@daniellee
Copy link
Contributor

Closing as this is included in #6692.

@daniellee daniellee closed this Nov 24, 2016
@joelanford
Copy link
Contributor Author

Hi @daniellee

Looks good to me! File paths were the easiest solution to my use case and I just carried InsecureSkipVerify: true forward. I fully expected feedback on my pull request, but I'm just as happy to see this get picked up, improved, and merged into 4.0.

@ying-jeanne ying-jeanne added the pr/external This PR is from external contributor label Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/external This PR is from external contributor pr/needs-manual-testing Before merge some help with manual testing & verification is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] elasticsearch with client cert authentication
5 participants