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

Allow additional trusted hosts #849

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

Conversation

matrujillo98
Copy link

@matrujillo98 matrujillo98 commented Apr 9, 2024

Add an option to support additional trusted endpoints. This will allow the users to trust specific hosts, and not need to drop the whole validation of endpoints.

I tested it locally, but unfortunately did not have proper proxy to fully test. Nonetheless, I used a local DNS forwarding to verify, but was not able to fully check due to the TLS certs.

image

@matrujillo98 matrujillo98 requested a review from a team as a code owner April 9, 2024 01:48
@CLAassistant
Copy link

CLAassistant commented Apr 9, 2024

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.

@aangelisc
Copy link
Contributor

Hi @matrujillo98! Thank you for the contribution! Before I review this I'd like to understand the use-case here. Is this to have a proxy in front of an ADX cluster with a custom DNS or something else?

@matrujillo98
Copy link
Author

Hey @aangelisc, so Kusto natively does not support failing over other clusters, thus you could use proxies on top of it with a traffic manager to allow such feature and have BCDR. Additionally, when talking about national clouds, it is not possible for a dev to query a Kusto cluster unless it has a national cloud identity which is not necessarily the case in all situations. Thus, you could use a proxy to centralize the access, and only allow grafana identity to get the information.

@matrujillo98
Copy link
Author

matrujillo98 commented Apr 9, 2024

Btw, compared with a non-dev grafana instance and the result is indeed an issue with endpoint validation before TLS:

image

@aangelisc
Copy link
Contributor

aangelisc commented Apr 17, 2024

Hi @matrujillo98, apologies for the delay and thank you for the additional context! Unfortunately, in order to implement this we'd require some additional changes (that would require us to potentially rework some aspects of the grafana-azure-sdk or another area of Grafana.

We'd need to ensure that we only display the component for specifying additional trusted hosts when the trusted endpoints setting is enabled to avoid confusion. Additionally, we'd also need to add some config to allow server admins to disable this feature entirely for security reasons.

I do agree this is a valuable feature, especially for the use-case you've described and I will create an issue in our backlog to investigate how this should be done.

@matrujillo98
Copy link
Author

Hey @aangelisc, thanks for the response!

Sure, I could add such a flag for admins to block the feature, and make sure this only appears if the overall enforcement is in place.

About this:

Unfortunately, in order to implement this we'd require some additional changes (that would require us to potentially rework some aspects of the grafana-azure-sdk or another area of Grafana.

I'm not quite sure why it would be the case, but I may be able to confirm e2e that this could work with current code if you'd like.

@aangelisc
Copy link
Contributor

Hi @matrujillo98,

Thank you for offering to take a look at this! The reason is the way that configuration gets passed from the Grafana instance to the plugin at the moment is via environment variables but this isn't sustainable and going forward we'd like to change this. I will investigate how that can be done and if it looks like it'll be too long of a timeframe then I'll figure out an alternative method in order to get this PR in and unblock you 😊

@aangelisc
Copy link
Contributor

Hi @matrujillo98,

I haven't forgotten about this work! Would you be comfortable creating a PR in Grafana to add the additional configuration variable required to allow Grafana admins to enable/disable this functionality?

Thanks!

@aangelisc
Copy link
Contributor

Hi @matrujillo98! So, on rethinking this, I think we should make it so that the trusted URLs can only be specified by the server admin (to avoid someone modifying a data source and trusting an endpoint that the server admin may not want to trust. Are you okay to modify this PR to support this use-case or would you prefer if I did so?

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