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

LDAP configuration: paths to root CA cert files should contain spaces #53942

Closed
DragoonAethis opened this issue Aug 19, 2022 · 7 comments · Fixed by #85735
Closed

LDAP configuration: paths to root CA cert files should contain spaces #53942

DragoonAethis opened this issue Aug 19, 2022 · 7 comments · Fixed by #85735

Comments

@DragoonAethis
Copy link

DragoonAethis commented Aug 19, 2022

What happened: LDAP configuration does not allow pointing to CA certs with spaces in the path. Grafana tries to split the provided path on spaces and load each space-separated path as a separate file. There's nothing in the config file or the associated struct that suggests this is happening.

What you expected to happen: Grafana loads the provided path without any splitting by spaces et al.

How to reproduce it (as minimally and precisely as possible):

  • In LDAP configuration, point root_ca_certs to a path with spaces in it (for example /opt/certs/Nice Cert.crt)
  • Open the Grafana LDAP config page, where it'll complain about not being able to open /opt/certs/Nice.

Anything else we need to know?:

  • This behavior was introduced back in 2015 with this commit - it looks like the logic for that was just a copy-paste from the host reading logic below.
  • It looks like someone else hit the same issue here.
  • A new root_ca_certs: []string config value would be the best, where each path from the TOML list gets used as is.
  • If not, a comment needs to be added that spaces in this single string separate multiple paths.

Environment:

  • Grafana version: 9.1.0 (but affects all versions since ~2015)
  • OS Grafana is installed on: Official Alpine container (Docker on Ubuntu 20.04).
@DragoonAethis
Copy link
Author

I'm not much of a Go dev, but it looks simple enough to handle - if you decide that adding a new config value is the way to go, I could give it a try and send out a PR :)

@zuchka zuchka added area/auth/ldap area/backend triage/needs-confirmation used for OSS triage rotation - reported issue needs to be reproduced area/auth team/identity-access labels Aug 25, 2022
@Jguer Jguer removed their assignment Sep 22, 2022
@mgyongyosi mgyongyosi changed the title LDAP configuration: paths to root CA cert files cannot contain spaces LDAP configuration: paths to root CA cert files should contain spaces Jan 5, 2023
@cindy
Copy link

cindy commented Mar 3, 2023

@mgyongyosi can you assign me to this one? :)

@mgyongyosi mgyongyosi removed the triage/needs-confirmation used for OSS triage rotation - reported issue needs to be reproduced label Mar 3, 2023
@mgyongyosi
Copy link
Contributor

mgyongyosi commented Mar 3, 2023

@cindy Thanks for your interest in fixing this 💯 . I assigned the issue to you. In case you need assistance feel free to reach out. :)

@Jguer
Copy link
Contributor

Jguer commented Mar 3, 2023

#61288 this PR introduces a good standard to follow

@cindy cindy removed their assignment Jun 26, 2023
@david-shiko
Copy link

Closed?

@mgyongyosi
Copy link
Contributor

It's been fixed. When the certs' path contain spaces please enclose it with quotes. root_ca_cert = "/path/to/certificate one.crt", "/path/to/other/certificate two.crt"

@DragoonAethis
Copy link
Author

Thank you!

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

Successfully merging a pull request may close this issue.

6 participants