-
Notifications
You must be signed in to change notification settings - Fork 397
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
feat(security): implement tls block for grafana external #1628
Merged
theSuess
merged 3 commits into
grafana:master
from
aboulay-numspot:feat/implement-tls-in-grafana-external
Aug 22, 2024
Merged
feat(security): implement tls block for grafana external #1628
theSuess
merged 3 commits into
grafana:master
from
aboulay-numspot:feat/implement-tls-in-grafana-external
Aug 22, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aboulay-numspot
requested review from
NissesSenap,
weisdd,
ishanjainn,
theSuess,
hubeadmin and
pb82
as code owners
July 31, 2024 10:18
Thanks for the PR! Looks great at first glance. It'll take me some time to review, but I'll try to fit it in next week |
theSuess
requested changes
Aug 6, 2024
…eSkipVerify and certSecretRef be set at the same time
aboulay-numspot
force-pushed
the
feat/implement-tls-in-grafana-external
branch
from
August 19, 2024 15:58
75433e7
to
02edfdb
Compare
@theSuess I let you review these changes to ensure everything is fine for you |
theSuess
requested changes
Aug 21, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last nitpick and then I'm good to merge this!
…ig is use for all the workflow
aboulay-numspot
force-pushed
the
feat/implement-tls-in-grafana-external
branch
from
August 22, 2024 12:11
02edfdb
to
27b2179
Compare
theSuess
approved these changes
Aug 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Aim of the merge request
Implement the possibility to use a custom CA and use a client certificate (tls.crt and tls.key) when declaring an external Grafana resource.
This will improve security of the solution because, after analysis, it seems to be in insecureSkipVerify per default in every tls connection.
Breaking change
Currently, every call is in insecureSkipVerify by default regardless if they are using https or not. This MR provides the possibility to use the tls block to make https connection and specify if the connection is insecure or not. If the tls block is not specified, http request will be sent instead.
Related documents
Design document: Add TLS management block in Grafana CR External block
Testing process
To test the tls with a self-signed certificate, we will need to deploy a grafana service (without the operator) with a nginx configured has a https reverse proxy.
Homemade testing process (click to open)
To do this, we will create a self-signed certifcate:
Then, we will declare the manifest for k8s resources:
And finaly, the Grafana CR
Then, initialize the kind cluster locally:
You can now start playing with the parameter from the Grafana CR to test the certificate connectivity.
Additional comments
The behavior of using insecure https connection have not been changed for the rest of the system (grafana_com_fetcher to retrieve dashboard or url_fetcher). The NewHTTPClient has been kept because the Grafana CR's getVersion method use an api path not declared into the grafana-openapi-client (GET /api/frontend/settings).
Just a contextual thing to finish this (too log message), I'll be in holiday for two weeks so the review will probably take a bunch of time before been handled.