Skip to content

Conversation

@clwluvw
Copy link

@clwluvw clwluvw commented Nov 14, 2025

What this PR does / why we need it:

Expose tls.Config.GetClientCertificate to allow supplying a callback that loads the client certificate and key on each request. This enables mTLS setups where certificates are rotated automatically without restarting the application.

Inspired by the etcd approach: etcd-io/etcd#7829

Which issue(s) this PR fixes:

Needed for: grafana/grafana#113982
Related to grafana/grafana#44296

Expose tls.Config.GetClientCertificate to allow supplying a callback
that loads the client certificate and key on each request. This enables
mTLS setups where certificates are rotated automatically without
restarting the application.

Inspired by the etcd approach: etcd-io/etcd#7829

Signed-off-by: Seena Fallah <[email protected]>
@CLAassistant
Copy link

CLAassistant commented Nov 14, 2025

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@andresmgot
Copy link
Contributor

Hello,

Thank you for the contribution. Unfortunately this goes against plugin best practices: Plugins should not use the local file system. This is a security measure to avoid plugins accessing restricted files from the system or other plugin files.

The recommended approach is still using a provisioning file, which could read files from the system, even though that requires a restart of the pod running the plugin. This shouldn't have any impact or downtime if served in HA.

@clwluvw
Copy link
Author

clwluvw commented Nov 17, 2025

@andresmgot - thanks for the feedback. i'm still little confused about it.
The goal here isn’t avoiding downtime, but avoiding the need for scheduled restarts every time mTLS client certificates rotate.
What confuses me is that Grafana already allows reading certificate files during provisioning, and the SDK itself already loads TLS certs from disk using tls.LoadX509KeyPair. The patch doesn’t expand access beyond what the SDK already does today, it only reloads the same files using the same API, but on demand instead of once at startup. Since the paths are controlled and only used for mTLS, I don’t see how this introduces a new security issue.
I mean we perhaps should be able to find a way to sort this out, and i don't think this practice is really a problem to not have it supported.

@andresmgot
Copy link
Contributor

Let me try to clarify.

What confuses me is that Grafana already allows reading certificate files during provisioning,

Yes, the provisioning system is part of Grafana core, where the code that's being executed is controlled and verified that it works as expected. In the case of plugins, we don't own the code and therefore we need to be more careful of what's being allowed.

and the SDK itself already loads TLS certs from disk using tls.LoadX509KeyPair.

I'm not sure if you mean this middleware. As you can see, the struct has those file paths deprecated and has not been removed (yet) to avoid breaking changes.

The patch doesn’t expand access beyond what the SDK already does today, it only reloads the same files using the same API, but on demand instead of once at startup. Since the paths are controlled and only used for mTLS, I don’t see how this introduces a new security issue.

This patch introduces a new set of paths for local files and that's what is not allowed. Apart from the security implications (plugins could try to read unexpected files from those locations), plugins should not assume any state of the machine they are running since that may change over time and datasources will not.

Hope that makes sense.

@tuunit
Copy link

tuunit commented Nov 18, 2025

@andresmgot understood that you don't want filesystem access through plugins. Do you have another idea in mind on how to realise mTLS cert rotation in a generic way to be supported by any kind of datasource?

@andresmgot
Copy link
Contributor

See this suggestion: grafana/grafana#113982 (comment)

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

Labels

None yet

Projects

Status: 🔬 In review

Development

Successfully merging this pull request may close these issues.

4 participants