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.

Sign up for GitHub

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

Jump to bottom

secure socks proxy: add option to disable tls in the socks proxy dialer #833

Merged
merged 6 commits into from
Dec 14, 2023
Merged

secure socks proxy: add option to disable tls in the socks proxy dialer #833

merged 6 commits into from
Dec 14, 2023

Conversation

PoorlyDefinedBehaviour
Copy link
Member

What this PR does / why we need it:

Description taken from https://github.com/grafana/hosted-grafana/issues/4935:

Sometimes there's a use case for testing grafana's SOCKs backend locally, and this would be significantly easier if we could disable the TLS requirement for the socks backend.

Which issue(s) this PR fixes:

Fixes https://github.com/grafana/hosted-grafana/issues/4935

@PoorlyDefinedBehaviour PoorlyDefinedBehaviour added the enhancement New feature or request label Dec 5, 2023
@PoorlyDefinedBehaviour PoorlyDefinedBehaviour self-assigned this Dec 5, 2023
@PoorlyDefinedBehaviour PoorlyDefinedBehaviour requested a review from a team as a code owner December 5, 2023 15:20
@PoorlyDefinedBehaviour PoorlyDefinedBehaviour requested review from wbrowne, marefr and xnyo and removed request for a team December 5, 2023 15:20
marefr
marefr suggested changes Dec 6, 2023
View reviewed changes
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but some comments/questions

backend/config.go Outdated
@@ -103,13 +103,15 @@ type Proxy struct {

func (c *GrafanaCfg) proxy() Proxy {
if v, exists := c.config[proxy.PluginSecureSocksProxyEnabled]; exists && v == strconv.FormatBool(true) {
allowInsecure, _ := strconv.ParseBool(c.Get(proxy.PluginSecureSocksProxyAllowInsecure))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should check for error/unable to parse here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

backend/proxy/secure_socks_proxy.go
@@ -39,6 +39,9 @@ var (
// PluginSecureSocksProxyServerName is a constant for the GF_SECURE_SOCKS_DATASOURCE_PROXY_SERVER_NAME
// environment variable used to specify the server name of the secure socks proxy.
PluginSecureSocksProxyServerName = "GF_SECURE_SOCKS_DATASOURCE_PROXY_SERVER_NAME"
// PluginSecureSocksProxyAllowInsecure is a constant for the GF_SECURE_SOCKS_DATASOURCE_PROXY_ALLOW_INSECURE
// environment variable used to specify if the proxy should use a TLS dialer.
PluginSecureSocksProxyAllowInsecure = "GF_SECURE_SOCKS_DATASOURCE_PROXY_ALLOW_INSECURE"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this supposed to be populated? Planning to make further changes to grafana? If so would suggest a PR on grafana now using this branch as sdk dependency

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marefr have you got any guidance about what we'd need to change in grafana/grafana? I can see that when the previous config items for the socks proxy were defined, we made some changes like these and we have some more config here. I don't have the full picture on what we'd need to change, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sorry for not pointing out that. Changes here is needed to be able to change this behaviour on a "global" Grafana level:
https://github.com/grafana/grafana/blob/main/pkg/plugins/envvars/envvars.go#L138-L145
https://github.com/grafana/grafana/blob/9e71dc801d229eae5d791e89905ac864cfeb448a/pkg/plugins/envvars/envvars.go#L248-L260
https://github.com/grafana/grafana/blob/main/pkg/setting/setting_secure_socks_proxy.go

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

dafydd-t
dafydd-t reviewed Dec 6, 2023
View reviewed changes
Copy link
Contributor

@dafydd-t dafydd-t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Definitely along the right lines. Just need to address Marcus's comments.

backend/proxy/secure_socks_proxy.go Outdated
@@ -108,7 +112,7 @@ func (p *cfgProxyWrapper) ConfigureSecureSocksHTTPProxy(transport *http.Transpor
}

transport.DialContext = contextDialer.DialContext
return nil
return nil // No need to include the TLS config since the proxy won't use it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment meant to be here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

@PoorlyDefinedBehaviour PoorlyDefinedBehaviour mentioned this pull request Dec 7, 2023
Merged
1 task
marefr
marefr approved these changes Dec 8, 2023
View reviewed changes
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM great work. Some nits/comments

backend/proxy/secure_socks_proxy.go
if pemDecoded == nil || pemDecoded.Type != "CERTIFICATE" {
return nil, errors.New("root ca is invalid")
if p.opts.ClientCfg.AllowInsecure {
dialer = &net.Dialer{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit. Can maybe return early here to make the code easier to read.

Oops just realized dialer are used further down. If last part moved to a function can maybe simplify the code then? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, moved to a function.

backend/proxy/secure_socks_proxy_test.go Outdated
ServerName: "testServer",
ClientCert: "client.crt",
ClientKey: "client.key",
RootCA: "ca.crt",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit. Based on changes in Grafana this test case when allow insecure is true should never happen. Not sure if it make sense to treat that special here though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the test and updated getConfigFromEnv: it won't try to get the tls fields from the env vars when allowInsecure=true because they won't be used

@marefr
Copy link
Contributor

marefr commented Dec 8, 2023

Would suggest sync with main before merge.

@PoorlyDefinedBehaviour PoorlyDefinedBehaviour requested review from dafydd-t and removed request for dafydd-t December 11, 2023 14:11
@PoorlyDefinedBehaviour PoorlyDefinedBehaviour merged commit aa5d1fd into main Dec 14, 2023
1 check passed
@PoorlyDefinedBehaviour PoorlyDefinedBehaviour deleted the secure-socks-proxy/add-insecure-option branch December 14, 2023 13:14
@renovate renovate bot mentioned this pull request May 27, 2024
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marefr marefr marefr approved these changes

@wbrowne wbrowne Awaiting requested review from wbrowne wbrowne is a code owner automatically assigned from grafana/plugins-platform-backend

@xnyo xnyo Awaiting requested review from xnyo xnyo is a code owner automatically assigned from grafana/plugins-platform-backend

@dafydd-t dafydd-t Awaiting requested review from dafydd-t

Assignees

@PoorlyDefinedBehaviour PoorlyDefinedBehaviour

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PoorlyDefinedBehaviour @marefr @dafydd-t