From 73e8c54a66791417c228be759e0fc141dd26496a Mon Sep 17 00:00:00 2001 From: multimo Date: Wed, 26 Nov 2025 15:08:13 +0100 Subject: [PATCH 01/10] remove the cache key with pdc bytes and check the expiry instead --- backend/config.go | 94 +++-- backend/config_test.go | 444 +++++++++++++++++++++++- backend/datasource/instance_provider.go | 9 +- 3 files changed, 494 insertions(+), 53 deletions(-) diff --git a/backend/config.go b/backend/config.go index 46dbb12d0..b6d5cc6fc 100644 --- a/backend/config.go +++ b/backend/config.go @@ -2,6 +2,7 @@ package backend import ( "context" + "crypto/x509" "encoding/base64" "encoding/pem" "errors" @@ -9,6 +10,7 @@ import ( "os" "strconv" "strings" + "time" "github.com/grafana/grafana-plugin-sdk-go/backend/proxy" "github.com/grafana/grafana-plugin-sdk-go/backend/useragent" @@ -90,12 +92,70 @@ func (c *GrafanaCfg) Equal(c2 *GrafanaCfg) bool { if len(c.config) != len(c2.config) { return false } + + configsMatch := true for k, v1 := range c.config { + // Ignore secure socks proxy config when checking if the configs match + if strings.Contains(k, "GF_SECURE_SOCKS_DATASOURCE_PROXY") { + continue + } + if v2, ok := c2.config[k]; !ok || v1 != v2 { - return false + configsMatch = false + } + } + + // Check if the secure socks proxy config is enabled on the datasource + // If it is load in the secure socks proxy config and check its expiration date + if v, ok := c.config[proxy.PluginSecureSocksProxyEnabled]; ok && v == strconv.FormatBool(true) { + if !c.shouldRefreshProxyClientCert() { + return true } } - return true + + return configsMatch +} + +// shouldRefreshProxyClientCert checks if the secure socks proxy config is expiring in less than 6 hours +// If it is, it returns true to trigger a refresh of the secure socks proxy config +// If it is not, it returns false to continue using the existing secure socks proxy config from cache +// the lifetime of these certificates is 3 months by default +func (c *GrafanaCfg) shouldRefreshProxyClientCert() bool { + p, err := c.proxy() + if err != nil { + Logger.Warn("shouldRefreshProxyClientCert(): failed to get proxy client config", "error", err) + return true + } + + if p.clientCfg == nil { + Logger.Warn("shouldRefreshProxyClientCert(): proxy client config is nil") + return true + } + + certPEM, err := base64.StdEncoding.DecodeString(p.clientCfg.ClientCertVal) + if err != nil { + Logger.Warn("shouldRefreshProxyClientCert(): failed to decode certificate", "error", err) + return true + } + + block, _ := pem.Decode(certPEM) + if block == nil { + return true + } + + cert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + Logger.Warn("shouldRefreshProxyClientCert(): failed to parse certificate", "error", err) + return true + } + + // Check if the certificate will expire in less than 6 hours + sixHoursFromNow := time.Now().Add(6 * time.Hour) + if cert.NotAfter.Before(sixHoursFromNow) { + return true + } + + return false } // Diff returns the names of config fields that differ between this config and c2. @@ -140,36 +200,6 @@ func (c *GrafanaCfg) Diff(c2 *GrafanaCfg) []string { return changed } -// ProxyHash returns the last four characters of the base64-encoded -// PDC client key contents, if present, for use in datasource instance -// caching. The contents should be PEM-encoded, so we try to PEM-decode -// them, and, if successful, return the base-64 encoding of the final three bytes, -// giving a four character hash. -func (c *GrafanaCfg) ProxyHash() string { - if c == nil { - return "" - } - contents := c.config[proxy.PluginSecureSocksProxyClientKeyContents] - if contents == "" { - return "" - } - block, _ := pem.Decode([]byte(contents)) - if block == nil { - Logger.Warn("ProxyHash(): key contents are not PEM-encoded") - return "" - } - if block.Type != "PRIVATE KEY" { - Logger.Warn("ProxyHash(): key contents are not PEM-encoded private key") - return "" - } - bl := len(block.Bytes) - if bl < 3 { - Logger.Warn("ProxyHash(): key contents too short") - return "" - } - return base64.StdEncoding.EncodeToString(block.Bytes[bl-3:]) -} - type FeatureToggles struct { // enabled is a set-like map of feature flags that are enabled. enabled map[string]struct{} diff --git a/backend/config_test.go b/backend/config_test.go index 4a965d1bd..ddbdbf625 100644 --- a/backend/config_test.go +++ b/backend/config_test.go @@ -1,13 +1,20 @@ package backend import ( + "bytes" "context" + "crypto" + "crypto/ed25519" "crypto/rand" + "crypto/sha1" + "crypto/x509" + "crypto/x509/pkix" + "encoding/base64" "encoding/pem" - "fmt" - mathrand "math/rand" + "math/big" "os" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -502,20 +509,425 @@ func TestGrafanaCfg_Diff(t *testing.T) { }) } -func BenchmarkProxyHash(b *testing.B) { - count := 0 - kBytes := randomProxyContents() - cm := map[string]string{ - proxy.PluginSecureSocksProxyClientKeyContents: string(kBytes), +// pEMEncodeEd25519PrivateKey encodes an Ed25519 private key in PEM format +func pEMEncodeEd25519PrivateKey(key crypto.PrivateKey) ([]byte, error) { + var b bytes.Buffer + kb, err := x509.MarshalPKCS8PrivateKey(key) + if err != nil { + return nil, err } - for i := 0; i < b.N; i++ { - kBytes[88] = b64chars[mathrand.Intn(64)] //nolint:gosec - cm[proxy.PluginSecureSocksProxyClientKeyContents] = string(kBytes) - cfg := NewGrafanaCfg(cm) - hash := cfg.ProxyHash() - if hash[0] == 'a' { - count++ - } + err = pem.Encode(&b, &pem.Block{ + Type: "PRIVATE KEY", + Bytes: kb, + }) + if err != nil { + return nil, err + } + return b.Bytes(), err +} + +// pEMEncodeCertificate encodes a certificate in PEM format +func pEMEncodeCertificate(cert []byte) ([]byte, error) { + var b bytes.Buffer + err := pem.Encode(&b, &pem.Block{ + Type: "CERTIFICATE", + Bytes: cert, + }) + if err != nil { + return nil, err + } + return b.Bytes(), nil +} + +// createTestCertificate generates a test X.509 certificate with Ed25519 key and the given expiry time +// This aligns with the GenerateEd25519ClientKeys pattern used in production +// Returns: (certPEM []byte, keyPEM []byte, error) +func createTestCertificate(notAfter time.Time) ([]byte, []byte, error) { + // Generate Ed25519 key pair + pubKey, privKey, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + return nil, nil, err + } + + // Generate SubjectKeyId by taking the SHA-1 hash of the ASN.1 encoding of the public key + // This follows RFC 5280 section 4.2.1.2 + kb, err := x509.MarshalPKIXPublicKey(pubKey) + if err != nil { + return nil, nil, err } - fmt.Printf("This should be about one in 64: %f\n", float64(count)/float64(b.N)) + + //nolint:gosec + keyHash := sha1.Sum(kb) + ski := keyHash[:] + + // Create certificate with realistic fields matching production pattern + clientCert := &x509.Certificate{ + SerialNumber: big.NewInt(2024), + Subject: pkix.Name{ + Organization: []string{"Grafana Labs"}, + CommonName: "test-client-cert", + }, + NotBefore: time.Now().Add(-24 * time.Hour), + NotAfter: notAfter, + SubjectKeyId: ski, + KeyUsage: x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, + } + + // Self-sign the certificate + certBytes, err := x509.CreateCertificate(rand.Reader, clientCert, clientCert, pubKey, privKey) + if err != nil { + return nil, nil, err + } + + // Encode both certificate and private key in PEM format using utility functions + certPEM, err := pEMEncodeCertificate(certBytes) + if err != nil { + return nil, nil, err + } + + keyPEM, err := pEMEncodeEd25519PrivateKey(privKey) + if err != nil { + return nil, nil, err + } + + return certPEM, keyPEM, nil +} + +func TestshouldRefreshProxyClientCert(t *testing.T) { + t.Run("returns false when proxy is nil", func(t *testing.T) { + cfg := NewGrafanaCfg(map[string]string{}) + result := cfg.shouldRefreshProxyClientCert() + require.True(t, result) + }) + + t.Run("returns false when proxy is not enabled", func(t *testing.T) { + cfg := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "false", + }) + result := cfg.shouldRefreshProxyClientCert() + require.True(t, result) + }) + + t.Run("returns true when certificate is already expired", func(t *testing.T) { + expiredCert, _, err := createTestCertificate(time.Now().Add(-1 * time.Hour)) + require.NoError(t, err) + + cfg := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(expiredCert), + }) + result := cfg.shouldRefreshProxyClientCert() + require.True(t, result) + }) + + t.Run("returns true when certificate expires within 6 hours", func(t *testing.T) { + soonExpiredCert, _, err := createTestCertificate(time.Now().Add(2 * time.Hour)) + require.NoError(t, err) + + cfg := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(soonExpiredCert), + }) + result := cfg.shouldRefreshProxyClientCert() + require.True(t, result) + }) + + t.Run("returns false when certificate expires after 6 hours", func(t *testing.T) { + validCert, _, err := createTestCertificate(time.Now().Add(12 * time.Hour)) + require.NoError(t, err) + + cfg := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(validCert), + }) + result := cfg.shouldRefreshProxyClientCert() + require.False(t, result) + }) + + t.Run("returns false when certificate expires just after 6 hour boundary", func(t *testing.T) { + // Expiry slightly after the 6 hour mark to avoid timing issues + justAfterBoundary := time.Now().Add(6*time.Hour + 1*time.Second) + cert, _, err := createTestCertificate(justAfterBoundary) + require.NoError(t, err) + + cfg := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(cert), + }) + result := cfg.shouldRefreshProxyClientCert() + // After the 6 hour boundary, should return false + require.False(t, result) + }) + + t.Run("returns true when certificate cert value is invalid base64", func(t *testing.T) { + cfg := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientCertContents: "not-valid-base64!@#$", + }) + result := cfg.shouldRefreshProxyClientCert() + require.True(t, result) + }) + + t.Run("returns true when certificate cert value is invalid PEM", func(t *testing.T) { + cfg := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString([]byte("not-a-pem-certificate")), + }) + result := cfg.shouldRefreshProxyClientCert() + require.True(t, result) + }) + + t.Run("returns true when certificate parsing fails", func(t *testing.T) { + invalidCert := pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", + Bytes: []byte("invalid-cert-bytes"), + }) + + cfg := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(invalidCert), + }) + result := cfg.shouldRefreshProxyClientCert() + require.True(t, result) + }) + + t.Run("returns false when certificate is valid for more than 6 hours (1 month validity)", func(t *testing.T) { + validLongCert, _, err := createTestCertificate(time.Now().Add(24 * 30 * time.Hour)) + require.NoError(t, err) + + cfg := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(validLongCert), + }) + result := cfg.shouldRefreshProxyClientCert() + require.False(t, result) + }) +} + +func TestGrafanaCfg_Equal(t *testing.T) { + t.Run("both configs nil should return true", func(t *testing.T) { + var cfg1, cfg2 *GrafanaCfg + result := cfg1.Equal(cfg2) + require.True(t, result) + }) + + t.Run("one config nil should return false", func(t *testing.T) { + cfg1 := NewGrafanaCfg(map[string]string{"key": "value"}) + var cfg2 *GrafanaCfg + require.False(t, cfg1.Equal(cfg2)) + require.False(t, cfg2.Equal(cfg1)) + }) + + t.Run("empty configs should return true", func(t *testing.T) { + cfg1 := NewGrafanaCfg(map[string]string{}) + cfg2 := NewGrafanaCfg(map[string]string{}) + require.True(t, cfg1.Equal(cfg2)) + }) + + t.Run("identical configs should return true", func(t *testing.T) { + config := map[string]string{ + "key1": "value1", + "key2": "value2", + "key3": "value3", + } + cfg1 := NewGrafanaCfg(config) + cfg2 := NewGrafanaCfg(config) + require.True(t, cfg1.Equal(cfg2)) + }) + + t.Run("different values should return false", func(t *testing.T) { + cfg1 := NewGrafanaCfg(map[string]string{ + "key1": "value1", + "key2": "value2", + }) + cfg2 := NewGrafanaCfg(map[string]string{ + "key1": "different_value", + "key2": "value2", + }) + require.False(t, cfg1.Equal(cfg2)) + }) + + t.Run("different sizes should return false", func(t *testing.T) { + cfg1 := NewGrafanaCfg(map[string]string{ + "key1": "value1", + "key2": "value2", + }) + cfg2 := NewGrafanaCfg(map[string]string{ + "key1": "value1", + }) + require.False(t, cfg1.Equal(cfg2)) + }) + + t.Run("proxy enabled with valid certificate should return true", func(t *testing.T) { + validCert, _, err := createTestCertificate(time.Now().Add(12 * time.Hour)) + require.NoError(t, err) + + config := map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(validCert), + "other_key": "other_value", + } + cfg1 := NewGrafanaCfg(config) + cfg2 := NewGrafanaCfg(config) + require.True(t, cfg1.Equal(cfg2)) + }) + + t.Run("proxy enabled with expiring certificate returns needsUpdate true", func(t *testing.T) { + expiringCert, _, err := createTestCertificate(time.Now().Add(2 * time.Hour)) + require.NoError(t, err) + + cfg1 := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(expiringCert), + "other_key": "other_value", + }) + cfg2 := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(expiringCert), + "other_key": "other_value", + }) + // Since certificate is expiring, shouldRefreshProxyClientCert returns true + // When proxy is enabled AND expiring, Equal returns needsUpdate value + // Other keys are identical, so needsUpdate = true, Equal returns true + require.True(t, cfg1.Equal(cfg2)) + }) + + t.Run("proxy enabled with expiring certificate returns needsUpdate true", func(t *testing.T) { + expiringCert, _, err := createTestCertificate(time.Now().Add(2 * time.Hour)) + require.NoError(t, err) + newCert, _, err := createTestCertificate(time.Now().Add(1 * time.Hour)) + require.NoError(t, err) + + cfg1 := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(expiringCert), + "other_key": "other_value", + }) + cfg2 := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(newCert), + "other_key": "other_value", + }) + // Since certificate is expiring, shouldRefreshProxyClientCert returns true + // When proxy is enabled AND expiring, Equal returns needsUpdate value + // Other keys are identical, so needsUpdate = true, Equal returns true + require.True(t, cfg1.Equal(cfg2)) + }) + + t.Run("proxy disabled should ignore certificate expiration", func(t *testing.T) { + expiringCert, _, err := createTestCertificate(time.Now().Add(2 * time.Hour)) + require.NoError(t, err) + + cfg1 := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "false", + proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(expiringCert), + }) + cfg2 := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "false", + proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(expiringCert), + }) + // Even though certificate is expiring, proxy is disabled so Equal should return true + require.True(t, cfg1.Equal(cfg2)) + }) + + t.Run("secure socks proxy keys should be ignored in comparison", func(t *testing.T) { + cfg1 := NewGrafanaCfg(map[string]string{ + "key1": "value1", + "GF_SECURE_SOCKS_DATASOURCE_PROXY": "different_value_should_be_ignored", + "GF_SECURE_SOCKS_DATASOURCE_PROXY2": "also_ignored", + }) + cfg2 := NewGrafanaCfg(map[string]string{ + "key1": "value1", + "GF_SECURE_SOCKS_DATASOURCE_PROXY": "different_value", + "GF_SECURE_SOCKS_DATASOURCE_PROXY2": "completely_different", + }) + // Proxy keys should be ignored, other keys are the same + require.True(t, cfg1.Equal(cfg2)) + }) + + t.Run("non-proxy keys should not be ignored", func(t *testing.T) { + cfg1 := NewGrafanaCfg(map[string]string{ + "key1": "value1", + "key2": "value2", + }) + cfg2 := NewGrafanaCfg(map[string]string{ + "key1": "value1", + "key2": "different_value", + }) + require.False(t, cfg1.Equal(cfg2)) + }) + + t.Run("proxy enabled with expired certificate returns needsUpdate", func(t *testing.T) { + expiredCert, _, err := createTestCertificate(time.Now().Add(-1 * time.Hour)) + require.NoError(t, err) + + cfg1 := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(expiredCert), + }) + cfg2 := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(expiredCert), + }) + // Certificate is expired, shouldRefreshProxyClientCert returns true + // All other keys are identical, needsUpdate = true + // Equal returns needsUpdate = true + require.True(t, cfg1.Equal(cfg2)) + }) + + t.Run("proxy enabled with invalid certificate returns needsUpdate", func(t *testing.T) { + cfg1 := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientCertContents: "invalid-cert", + }) + cfg2 := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientCertContents: "invalid-cert", + }) + // Invalid certificate causes shouldRefreshProxyClientCert to return true + // All other keys are identical, needsUpdate = true + // Equal returns needsUpdate = true + require.True(t, cfg1.Equal(cfg2)) + }) + + t.Run("different non-proxy keys with expiring certificate returns false", func(t *testing.T) { + expiringCert, _, err := createTestCertificate(time.Now().Add(2 * time.Hour)) + require.NoError(t, err) + + cfg1 := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(expiringCert), + "other_key": "value1", + }) + cfg2 := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(expiringCert), + "other_key": "value2", + }) + // Different non-proxy keys, so needsUpdate = false + // Certificate is expiring, so Equal returns needsUpdate = false + require.False(t, cfg1.Equal(cfg2)) + }) + + t.Run("different non-proxy keys with valid certificate returns true", func(t *testing.T) { + validCert, _, err := createTestCertificate(time.Now().Add(12 * time.Hour)) + require.NoError(t, err) + + cfg1 := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(validCert), + "other_key": "value1", + }) + cfg2 := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(validCert), + "other_key": "value2", + }) + // Different non-proxy keys, so needsUpdate = false + // But certificate is valid and proxy is enabled + // Equal returns true immediately (early return for valid certificate) + // This ignores the fact that other keys differ! + require.True(t, cfg1.Equal(cfg2)) + }) } diff --git a/backend/datasource/instance_provider.go b/backend/datasource/instance_provider.go index 21b096b6e..0a6d14c7d 100644 --- a/backend/datasource/instance_provider.go +++ b/backend/datasource/instance_provider.go @@ -67,9 +67,9 @@ func (ip *instanceProvider) NeedsUpdate(ctx context.Context, pluginContext backe cachedConfig := cachedInstance.PluginContext.GrafanaConfig configUpdated := !cachedConfig.Equal(curConfig) - curDataSourceSettings := pluginContext.DataSourceInstanceSettings - cachedDataSourceSettings := cachedInstance.PluginContext.DataSourceInstanceSettings - dsUpdated := !curDataSourceSettings.Updated.Equal(cachedDataSourceSettings.Updated) + curDataSourceSettingsUpdatedTime := pluginContext.DataSourceInstanceSettings.Updated + cachedDataSourceSettingsUpdatedTime := cachedInstance.PluginContext.DataSourceInstanceSettings.Updated + dsUpdated := !curDataSourceSettingsUpdatedTime.Equal(cachedDataSourceSettingsUpdatedTime) if dsUpdated || configUpdated { logger := backend.Logger.FromContext(ctx) @@ -95,7 +95,6 @@ func (ip *instanceProvider) NewInstance(ctx context.Context, pluginContext backe func instanceKey(ctx context.Context, pluginContext backend.PluginContext) string { dsID := pluginContext.DataSourceInstanceSettings.ID tenantID := tenant.IDFromContext(ctx) - proxyHash := pluginContext.GrafanaConfig.ProxyHash() - return fmt.Sprintf("%d#%s#%s", dsID, tenantID, proxyHash) + return fmt.Sprintf("%d#%s", dsID, tenantID) } From 17a27a669427fefcea31ebceb5d531344ee04327 Mon Sep 17 00:00:00 2001 From: multimo Date: Wed, 26 Nov 2025 15:24:29 +0100 Subject: [PATCH 02/10] fix linting --- backend/config.go | 7 +--- backend/config_test.go | 80 +++++++++++------------------------------- 2 files changed, 21 insertions(+), 66 deletions(-) diff --git a/backend/config.go b/backend/config.go index b6d5cc6fc..1e1cbab37 100644 --- a/backend/config.go +++ b/backend/config.go @@ -150,12 +150,7 @@ func (c *GrafanaCfg) shouldRefreshProxyClientCert() bool { } // Check if the certificate will expire in less than 6 hours - sixHoursFromNow := time.Now().Add(6 * time.Hour) - if cert.NotAfter.Before(sixHoursFromNow) { - return true - } - - return false + return cert.NotAfter.Before(time.Now().Add(6 * time.Hour)) } // Diff returns the names of config fields that differ between this config and c2. diff --git a/backend/config_test.go b/backend/config_test.go index ddbdbf625..9930aafe2 100644 --- a/backend/config_test.go +++ b/backend/config_test.go @@ -3,7 +3,6 @@ package backend import ( "bytes" "context" - "crypto" "crypto/ed25519" "crypto/rand" "crypto/sha1" @@ -370,18 +369,6 @@ func TestPluginAppClientSecret(t *testing.T) { }) } -func randomProxyContents() []byte { - key := make([]byte, 48) - _, _ = rand.Read(key) - pb := pem.Block{ - Type: "PRIVATE KEY", - Bytes: key, - } - return pem.EncodeToMemory(&pb) -} - -var b64chars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789/+" - func TestGrafanaCfg_Diff(t *testing.T) { t.Run("both configs nil should return empty slice", func(t *testing.T) { var cfg1, cfg2 *GrafanaCfg @@ -509,23 +496,6 @@ func TestGrafanaCfg_Diff(t *testing.T) { }) } -// pEMEncodeEd25519PrivateKey encodes an Ed25519 private key in PEM format -func pEMEncodeEd25519PrivateKey(key crypto.PrivateKey) ([]byte, error) { - var b bytes.Buffer - kb, err := x509.MarshalPKCS8PrivateKey(key) - if err != nil { - return nil, err - } - err = pem.Encode(&b, &pem.Block{ - Type: "PRIVATE KEY", - Bytes: kb, - }) - if err != nil { - return nil, err - } - return b.Bytes(), err -} - // pEMEncodeCertificate encodes a certificate in PEM format func pEMEncodeCertificate(cert []byte) ([]byte, error) { var b bytes.Buffer @@ -542,25 +512,21 @@ func pEMEncodeCertificate(cert []byte) ([]byte, error) { // createTestCertificate generates a test X.509 certificate with Ed25519 key and the given expiry time // This aligns with the GenerateEd25519ClientKeys pattern used in production // Returns: (certPEM []byte, keyPEM []byte, error) -func createTestCertificate(notAfter time.Time) ([]byte, []byte, error) { - // Generate Ed25519 key pair +func createTestCertificate(notAfter time.Time) ([]byte, error) { pubKey, privKey, err := ed25519.GenerateKey(rand.Reader) if err != nil { - return nil, nil, err + return nil, err } - // Generate SubjectKeyId by taking the SHA-1 hash of the ASN.1 encoding of the public key - // This follows RFC 5280 section 4.2.1.2 kb, err := x509.MarshalPKIXPublicKey(pubKey) if err != nil { - return nil, nil, err + return nil, err } //nolint:gosec keyHash := sha1.Sum(kb) ski := keyHash[:] - // Create certificate with realistic fields matching production pattern clientCert := &x509.Certificate{ SerialNumber: big.NewInt(2024), Subject: pkix.Name{ @@ -577,24 +543,18 @@ func createTestCertificate(notAfter time.Time) ([]byte, []byte, error) { // Self-sign the certificate certBytes, err := x509.CreateCertificate(rand.Reader, clientCert, clientCert, pubKey, privKey) if err != nil { - return nil, nil, err + return nil, err } - // Encode both certificate and private key in PEM format using utility functions certPEM, err := pEMEncodeCertificate(certBytes) if err != nil { - return nil, nil, err - } - - keyPEM, err := pEMEncodeEd25519PrivateKey(privKey) - if err != nil { - return nil, nil, err + return nil, err } - return certPEM, keyPEM, nil + return certPEM, nil } -func TestshouldRefreshProxyClientCert(t *testing.T) { +func TestShouldRefreshProxyClientCert(t *testing.T) { t.Run("returns false when proxy is nil", func(t *testing.T) { cfg := NewGrafanaCfg(map[string]string{}) result := cfg.shouldRefreshProxyClientCert() @@ -610,7 +570,7 @@ func TestshouldRefreshProxyClientCert(t *testing.T) { }) t.Run("returns true when certificate is already expired", func(t *testing.T) { - expiredCert, _, err := createTestCertificate(time.Now().Add(-1 * time.Hour)) + expiredCert, err := createTestCertificate(time.Now().Add(-1 * time.Hour)) require.NoError(t, err) cfg := NewGrafanaCfg(map[string]string{ @@ -622,7 +582,7 @@ func TestshouldRefreshProxyClientCert(t *testing.T) { }) t.Run("returns true when certificate expires within 6 hours", func(t *testing.T) { - soonExpiredCert, _, err := createTestCertificate(time.Now().Add(2 * time.Hour)) + soonExpiredCert, err := createTestCertificate(time.Now().Add(2 * time.Hour)) require.NoError(t, err) cfg := NewGrafanaCfg(map[string]string{ @@ -634,7 +594,7 @@ func TestshouldRefreshProxyClientCert(t *testing.T) { }) t.Run("returns false when certificate expires after 6 hours", func(t *testing.T) { - validCert, _, err := createTestCertificate(time.Now().Add(12 * time.Hour)) + validCert, err := createTestCertificate(time.Now().Add(12 * time.Hour)) require.NoError(t, err) cfg := NewGrafanaCfg(map[string]string{ @@ -648,7 +608,7 @@ func TestshouldRefreshProxyClientCert(t *testing.T) { t.Run("returns false when certificate expires just after 6 hour boundary", func(t *testing.T) { // Expiry slightly after the 6 hour mark to avoid timing issues justAfterBoundary := time.Now().Add(6*time.Hour + 1*time.Second) - cert, _, err := createTestCertificate(justAfterBoundary) + cert, err := createTestCertificate(justAfterBoundary) require.NoError(t, err) cfg := NewGrafanaCfg(map[string]string{ @@ -693,7 +653,7 @@ func TestshouldRefreshProxyClientCert(t *testing.T) { }) t.Run("returns false when certificate is valid for more than 6 hours (1 month validity)", func(t *testing.T) { - validLongCert, _, err := createTestCertificate(time.Now().Add(24 * 30 * time.Hour)) + validLongCert, err := createTestCertificate(time.Now().Add(24 * 30 * time.Hour)) require.NoError(t, err) cfg := NewGrafanaCfg(map[string]string{ @@ -760,7 +720,7 @@ func TestGrafanaCfg_Equal(t *testing.T) { }) t.Run("proxy enabled with valid certificate should return true", func(t *testing.T) { - validCert, _, err := createTestCertificate(time.Now().Add(12 * time.Hour)) + validCert, err := createTestCertificate(time.Now().Add(12 * time.Hour)) require.NoError(t, err) config := map[string]string{ @@ -774,7 +734,7 @@ func TestGrafanaCfg_Equal(t *testing.T) { }) t.Run("proxy enabled with expiring certificate returns needsUpdate true", func(t *testing.T) { - expiringCert, _, err := createTestCertificate(time.Now().Add(2 * time.Hour)) + expiringCert, err := createTestCertificate(time.Now().Add(2 * time.Hour)) require.NoError(t, err) cfg1 := NewGrafanaCfg(map[string]string{ @@ -794,9 +754,9 @@ func TestGrafanaCfg_Equal(t *testing.T) { }) t.Run("proxy enabled with expiring certificate returns needsUpdate true", func(t *testing.T) { - expiringCert, _, err := createTestCertificate(time.Now().Add(2 * time.Hour)) + expiringCert, err := createTestCertificate(time.Now().Add(2 * time.Hour)) require.NoError(t, err) - newCert, _, err := createTestCertificate(time.Now().Add(1 * time.Hour)) + newCert, err := createTestCertificate(time.Now().Add(1 * time.Hour)) require.NoError(t, err) cfg1 := NewGrafanaCfg(map[string]string{ @@ -816,7 +776,7 @@ func TestGrafanaCfg_Equal(t *testing.T) { }) t.Run("proxy disabled should ignore certificate expiration", func(t *testing.T) { - expiringCert, _, err := createTestCertificate(time.Now().Add(2 * time.Hour)) + expiringCert, err := createTestCertificate(time.Now().Add(2 * time.Hour)) require.NoError(t, err) cfg1 := NewGrafanaCfg(map[string]string{ @@ -859,7 +819,7 @@ func TestGrafanaCfg_Equal(t *testing.T) { }) t.Run("proxy enabled with expired certificate returns needsUpdate", func(t *testing.T) { - expiredCert, _, err := createTestCertificate(time.Now().Add(-1 * time.Hour)) + expiredCert, err := createTestCertificate(time.Now().Add(-1 * time.Hour)) require.NoError(t, err) cfg1 := NewGrafanaCfg(map[string]string{ @@ -892,7 +852,7 @@ func TestGrafanaCfg_Equal(t *testing.T) { }) t.Run("different non-proxy keys with expiring certificate returns false", func(t *testing.T) { - expiringCert, _, err := createTestCertificate(time.Now().Add(2 * time.Hour)) + expiringCert, err := createTestCertificate(time.Now().Add(2 * time.Hour)) require.NoError(t, err) cfg1 := NewGrafanaCfg(map[string]string{ @@ -911,7 +871,7 @@ func TestGrafanaCfg_Equal(t *testing.T) { }) t.Run("different non-proxy keys with valid certificate returns true", func(t *testing.T) { - validCert, _, err := createTestCertificate(time.Now().Add(12 * time.Hour)) + validCert, err := createTestCertificate(time.Now().Add(12 * time.Hour)) require.NoError(t, err) cfg1 := NewGrafanaCfg(map[string]string{ From 2aacb3950ad90a81ff6e29b2c16ff03b455d8d90 Mon Sep 17 00:00:00 2001 From: multimo Date: Thu, 27 Nov 2025 11:00:26 +0100 Subject: [PATCH 03/10] update tests and fix implementation --- backend/config.go | 43 ++++----- backend/config_test.go | 210 +++++++++++++++-------------------------- 2 files changed, 93 insertions(+), 160 deletions(-) diff --git a/backend/config.go b/backend/config.go index 1e1cbab37..8f858c94e 100644 --- a/backend/config.go +++ b/backend/config.go @@ -3,7 +3,6 @@ package backend import ( "context" "crypto/x509" - "encoding/base64" "encoding/pem" "errors" "fmt" @@ -93,59 +92,51 @@ func (c *GrafanaCfg) Equal(c2 *GrafanaCfg) bool { return false } - configsMatch := true for k, v1 := range c.config { - // Ignore secure socks proxy config when checking if the configs match + // Ignore secure socks proxy config values as they are always different. if strings.Contains(k, "GF_SECURE_SOCKS_DATASOURCE_PROXY") { continue } if v2, ok := c2.config[k]; !ok || v1 != v2 { - configsMatch = false + return false } } - // Check if the secure socks proxy config is enabled on the datasource - // If it is load in the secure socks proxy config and check its expiration date + // By Default, we will always fetch the current config from cloud-config -> hosted-grafana api. + // This will generate a new set of keys / certificates in c2.config. + // Since these new keys are always different we would never cache a datasource instance and cause a memory leak. + // We test them seperately and if the existing cached config keys are still valid we will continue to use them if not we will refresh the keys. if v, ok := c.config[proxy.PluginSecureSocksProxyEnabled]; ok && v == strconv.FormatBool(true) { - if !c.shouldRefreshProxyClientCert() { - return true - } + return !c.isProxyCertificateExpiring() // if the certificate is expiring, we need to refresh the keys } - return configsMatch + return true } -// shouldRefreshProxyClientCert checks if the secure socks proxy config is expiring in less than 6 hours -// If it is, it returns true to trigger a refresh of the secure socks proxy config -// If it is not, it returns false to continue using the existing secure socks proxy config from cache -// the lifetime of these certificates is 3 months by default -func (c *GrafanaCfg) shouldRefreshProxyClientCert() bool { +// isProxyCertificateExpiring checks if the secure socks proxy client key is expiring in less than 6 hours +// the default lifetime of these certificates is 3 months by default. +func (c *GrafanaCfg) isProxyCertificateExpiring() bool { p, err := c.proxy() if err != nil { - Logger.Warn("shouldRefreshProxyClientCert(): failed to get proxy client config", "error", err) + Logger.Warn("isProxyCertificateExpiring(): failed to get proxy client config", "error", err) return true } if p.clientCfg == nil { - Logger.Warn("shouldRefreshProxyClientCert(): proxy client config is nil") - return true - } - - certPEM, err := base64.StdEncoding.DecodeString(p.clientCfg.ClientCertVal) - if err != nil { - Logger.Warn("shouldRefreshProxyClientCert(): failed to decode certificate", "error", err) + Logger.Warn("isProxyCertificateExpiring(): proxy client config is nil") return true } - block, _ := pem.Decode(certPEM) - if block == nil { + block, _ := pem.Decode([]byte(p.clientCfg.ClientKeyVal)) + if block == nil || block.Bytes == nil { + Logger.Warn("isProxyCertificateExpiring(): failed to decode private key") return true } cert, err := x509.ParseCertificate(block.Bytes) if err != nil { - Logger.Warn("shouldRefreshProxyClientCert(): failed to parse certificate", "error", err) + Logger.Warn("isProxyCertificateExpiring(): failed to parse certificate", "error", err) return true } diff --git a/backend/config_test.go b/backend/config_test.go index 9930aafe2..21790f173 100644 --- a/backend/config_test.go +++ b/backend/config_test.go @@ -8,7 +8,6 @@ import ( "crypto/sha1" "crypto/x509" "crypto/x509/pkix" - "encoding/base64" "encoding/pem" "math/big" "os" @@ -155,8 +154,8 @@ func TestConfig(t *testing.T) { proxy.PluginSecureSocksProxyClientKey: "./clientKey", proxy.PluginSecureSocksProxyClientCert: "./clientCert", proxy.PluginSecureSocksProxyRootCAs: "./rootCACert ./rootCACert2", - proxy.PluginSecureSocksProxyClientKeyContents: "clientKey", proxy.PluginSecureSocksProxyClientCertContents: "clientCert", + proxy.PluginSecureSocksProxyClientKeyContents: "clientKey", proxy.PluginSecureSocksProxyRootCAsContents: "rootCACert,rootCACert2", proxy.PluginSecureSocksProxyAllowInsecure: "true", }), @@ -511,16 +510,19 @@ func pEMEncodeCertificate(cert []byte) ([]byte, error) { // createTestCertificate generates a test X.509 certificate with Ed25519 key and the given expiry time // This aligns with the GenerateEd25519ClientKeys pattern used in production -// Returns: (certPEM []byte, keyPEM []byte, error) -func createTestCertificate(notAfter time.Time) ([]byte, error) { +// should match something like this; +// -----BEGIN PRIVATE KEY----- +// MC4CAQAwBQYDK2VwBCIEII+zrobzViCQkpHXkteM6qmDs2UW6fKAXcuvhb9rdJ2+ +// -----END PRIVATE KEY----- +func createTestCertificate(notAfter time.Time) string { pubKey, privKey, err := ed25519.GenerateKey(rand.Reader) if err != nil { - return nil, err + panic(err) } kb, err := x509.MarshalPKIXPublicKey(pubKey) if err != nil { - return nil, err + panic(err) } //nolint:gosec @@ -543,21 +545,21 @@ func createTestCertificate(notAfter time.Time) ([]byte, error) { // Self-sign the certificate certBytes, err := x509.CreateCertificate(rand.Reader, clientCert, clientCert, pubKey, privKey) if err != nil { - return nil, err + panic(err) } certPEM, err := pEMEncodeCertificate(certBytes) if err != nil { - return nil, err + panic(err) } - return certPEM, nil + return string(certPEM) } -func TestShouldRefreshProxyClientCert(t *testing.T) { +func TestIsProxyCertificateExpiring(t *testing.T) { t.Run("returns false when proxy is nil", func(t *testing.T) { cfg := NewGrafanaCfg(map[string]string{}) - result := cfg.shouldRefreshProxyClientCert() + result := cfg.isProxyCertificateExpiring() require.True(t, result) }) @@ -565,76 +567,71 @@ func TestShouldRefreshProxyClientCert(t *testing.T) { cfg := NewGrafanaCfg(map[string]string{ proxy.PluginSecureSocksProxyEnabled: "false", }) - result := cfg.shouldRefreshProxyClientCert() + result := cfg.isProxyCertificateExpiring() require.True(t, result) }) t.Run("returns true when certificate is already expired", func(t *testing.T) { - expiredCert, err := createTestCertificate(time.Now().Add(-1 * time.Hour)) - require.NoError(t, err) + expiredCert := createTestCertificate(time.Now().Add(-1 * time.Hour)) cfg := NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(expiredCert), + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientKeyContents: expiredCert, }) - result := cfg.shouldRefreshProxyClientCert() + result := cfg.isProxyCertificateExpiring() require.True(t, result) }) t.Run("returns true when certificate expires within 6 hours", func(t *testing.T) { - soonExpiredCert, err := createTestCertificate(time.Now().Add(2 * time.Hour)) - require.NoError(t, err) + soonExpiredCert := createTestCertificate(time.Now().Add(2 * time.Hour)) cfg := NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(soonExpiredCert), + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientKeyContents: soonExpiredCert, }) - result := cfg.shouldRefreshProxyClientCert() + result := cfg.isProxyCertificateExpiring() require.True(t, result) }) t.Run("returns false when certificate expires after 6 hours", func(t *testing.T) { - validCert, err := createTestCertificate(time.Now().Add(12 * time.Hour)) - require.NoError(t, err) + validCert := createTestCertificate(time.Now().Add(12 * time.Hour)) cfg := NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(validCert), + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientKeyContents: validCert, }) - result := cfg.shouldRefreshProxyClientCert() + result := cfg.isProxyCertificateExpiring() require.False(t, result) }) t.Run("returns false when certificate expires just after 6 hour boundary", func(t *testing.T) { // Expiry slightly after the 6 hour mark to avoid timing issues justAfterBoundary := time.Now().Add(6*time.Hour + 1*time.Second) - cert, err := createTestCertificate(justAfterBoundary) - require.NoError(t, err) + cert := createTestCertificate(justAfterBoundary) cfg := NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(cert), + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientKeyContents: cert, }) - result := cfg.shouldRefreshProxyClientCert() - // After the 6 hour boundary, should return false + result := cfg.isProxyCertificateExpiring() require.False(t, result) }) t.Run("returns true when certificate cert value is invalid base64", func(t *testing.T) { cfg := NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientCertContents: "not-valid-base64!@#$", + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientKeyContents: "not-valid-base64!@#$", }) - result := cfg.shouldRefreshProxyClientCert() + result := cfg.isProxyCertificateExpiring() require.True(t, result) }) t.Run("returns true when certificate cert value is invalid PEM", func(t *testing.T) { cfg := NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString([]byte("not-a-pem-certificate")), + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientKeyContents: string([]byte("not-a-pem-certificate")), }) - result := cfg.shouldRefreshProxyClientCert() + result := cfg.isProxyCertificateExpiring() require.True(t, result) }) @@ -645,22 +642,21 @@ func TestShouldRefreshProxyClientCert(t *testing.T) { }) cfg := NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(invalidCert), + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientKeyContents: string(invalidCert), }) - result := cfg.shouldRefreshProxyClientCert() + result := cfg.isProxyCertificateExpiring() require.True(t, result) }) t.Run("returns false when certificate is valid for more than 6 hours (1 month validity)", func(t *testing.T) { - validLongCert, err := createTestCertificate(time.Now().Add(24 * 30 * time.Hour)) - require.NoError(t, err) + validLongCert := createTestCertificate(time.Now().Add(24 * 30 * time.Hour)) cfg := NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(validLongCert), + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientKeyContents: validLongCert, }) - result := cfg.shouldRefreshProxyClientCert() + result := cfg.isProxyCertificateExpiring() require.False(t, result) }) } @@ -720,12 +716,11 @@ func TestGrafanaCfg_Equal(t *testing.T) { }) t.Run("proxy enabled with valid certificate should return true", func(t *testing.T) { - validCert, err := createTestCertificate(time.Now().Add(12 * time.Hour)) - require.NoError(t, err) + validCert := createTestCertificate(time.Now().Add(12 * time.Hour)) config := map[string]string{ - proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(validCert), + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientKeyContents: string(validCert), "other_key": "other_value", } cfg1 := NewGrafanaCfg(config) @@ -734,58 +729,33 @@ func TestGrafanaCfg_Equal(t *testing.T) { }) t.Run("proxy enabled with expiring certificate returns needsUpdate true", func(t *testing.T) { - expiringCert, err := createTestCertificate(time.Now().Add(2 * time.Hour)) - require.NoError(t, err) + expiringCert := createTestCertificate(time.Now().Add(12 * time.Hour)) + newCert := createTestCertificate(time.Now().Add(1 * time.Hour)) cfg1 := NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(expiringCert), + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientKeyContents: string(expiringCert), "other_key": "other_value", }) cfg2 := NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(expiringCert), + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientKeyContents: string(newCert), "other_key": "other_value", }) - // Since certificate is expiring, shouldRefreshProxyClientCert returns true - // When proxy is enabled AND expiring, Equal returns needsUpdate value - // Other keys are identical, so needsUpdate = true, Equal returns true - require.True(t, cfg1.Equal(cfg2)) - }) - t.Run("proxy enabled with expiring certificate returns needsUpdate true", func(t *testing.T) { - expiringCert, err := createTestCertificate(time.Now().Add(2 * time.Hour)) - require.NoError(t, err) - newCert, err := createTestCertificate(time.Now().Add(1 * time.Hour)) - require.NoError(t, err) - - cfg1 := NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(expiringCert), - "other_key": "other_value", - }) - cfg2 := NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(newCert), - "other_key": "other_value", - }) - // Since certificate is expiring, shouldRefreshProxyClientCert returns true - // When proxy is enabled AND expiring, Equal returns needsUpdate value - // Other keys are identical, so needsUpdate = true, Equal returns true require.True(t, cfg1.Equal(cfg2)) }) t.Run("proxy disabled should ignore certificate expiration", func(t *testing.T) { - expiringCert, err := createTestCertificate(time.Now().Add(2 * time.Hour)) - require.NoError(t, err) + expiringCert := createTestCertificate(time.Now().Add(12 * time.Hour)) cfg1 := NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyEnabled: "false", - proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(expiringCert), + proxy.PluginSecureSocksProxyEnabled: "false", + proxy.PluginSecureSocksProxyClientKeyContents: string(expiringCert), }) cfg2 := NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyEnabled: "false", - proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(expiringCert), + proxy.PluginSecureSocksProxyEnabled: "false", + proxy.PluginSecureSocksProxyClientKeyContents: string(expiringCert), }) // Even though certificate is expiring, proxy is disabled so Equal should return true require.True(t, cfg1.Equal(cfg2)) @@ -819,75 +789,47 @@ func TestGrafanaCfg_Equal(t *testing.T) { }) t.Run("proxy enabled with expired certificate returns needsUpdate", func(t *testing.T) { - expiredCert, err := createTestCertificate(time.Now().Add(-1 * time.Hour)) - require.NoError(t, err) + expiredCert := createTestCertificate(time.Now().Add(-1 * time.Hour)) cfg1 := NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(expiredCert), + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientKeyContents: string(expiredCert), }) cfg2 := NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(expiredCert), + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientKeyContents: string(expiredCert), }) - // Certificate is expired, shouldRefreshProxyClientCert returns true - // All other keys are identical, needsUpdate = true - // Equal returns needsUpdate = true - require.True(t, cfg1.Equal(cfg2)) + + require.False(t, cfg1.Equal(cfg2)) }) t.Run("proxy enabled with invalid certificate returns needsUpdate", func(t *testing.T) { cfg1 := NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientCertContents: "invalid-cert", + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientKeyContents: "invalid-cert", }) cfg2 := NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientCertContents: "invalid-cert", + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientKeyContents: "invalid-cert", }) - // Invalid certificate causes shouldRefreshProxyClientCert to return true - // All other keys are identical, needsUpdate = true - // Equal returns needsUpdate = true - require.True(t, cfg1.Equal(cfg2)) - }) - - t.Run("different non-proxy keys with expiring certificate returns false", func(t *testing.T) { - expiringCert, err := createTestCertificate(time.Now().Add(2 * time.Hour)) - require.NoError(t, err) - cfg1 := NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(expiringCert), - "other_key": "value1", - }) - cfg2 := NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(expiringCert), - "other_key": "value2", - }) - // Different non-proxy keys, so needsUpdate = false - // Certificate is expiring, so Equal returns needsUpdate = false require.False(t, cfg1.Equal(cfg2)) }) - t.Run("different non-proxy keys with valid certificate returns true", func(t *testing.T) { - validCert, err := createTestCertificate(time.Now().Add(12 * time.Hour)) - require.NoError(t, err) + t.Run("different non-proxy keys with expiring certificate returns false", func(t *testing.T) { + expiringCert := createTestCertificate(time.Now().Add(2 * time.Hour)) cfg1 := NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(validCert), + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientKeyContents: string(expiringCert), "other_key": "value1", }) cfg2 := NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientCertContents: base64.StdEncoding.EncodeToString(validCert), + proxy.PluginSecureSocksProxyEnabled: "true", + proxy.PluginSecureSocksProxyClientKeyContents: string(expiringCert), "other_key": "value2", }) - // Different non-proxy keys, so needsUpdate = false - // But certificate is valid and proxy is enabled - // Equal returns true immediately (early return for valid certificate) - // This ignores the fact that other keys differ! - require.True(t, cfg1.Equal(cfg2)) + + require.False(t, cfg1.Equal(cfg2)) }) } From 92b72f58e93109adb3af3aa6443cdfe73a97905d Mon Sep 17 00:00:00 2001 From: multimo Date: Thu, 27 Nov 2025 11:10:19 +0100 Subject: [PATCH 04/10] linting fixes --- backend/config.go | 4 ++-- backend/config_test.go | 20 ++++++++++---------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/backend/config.go b/backend/config.go index 8f858c94e..917e5d48e 100644 --- a/backend/config.go +++ b/backend/config.go @@ -104,9 +104,9 @@ func (c *GrafanaCfg) Equal(c2 *GrafanaCfg) bool { } // By Default, we will always fetch the current config from cloud-config -> hosted-grafana api. - // This will generate a new set of keys / certificates in c2.config. + // This will generate a new set of keys / certificates in c2.config. // Since these new keys are always different we would never cache a datasource instance and cause a memory leak. - // We test them seperately and if the existing cached config keys are still valid we will continue to use them if not we will refresh the keys. + // We test them separately and if the existing cached config keys are still valid we will continue to use them if not we will refresh the keys. if v, ok := c.config[proxy.PluginSecureSocksProxyEnabled]; ok && v == strconv.FormatBool(true) { return !c.isProxyCertificateExpiring() // if the certificate is expiring, we need to refresh the keys } diff --git a/backend/config_test.go b/backend/config_test.go index 21790f173..b8c1387b5 100644 --- a/backend/config_test.go +++ b/backend/config_test.go @@ -5,7 +5,7 @@ import ( "context" "crypto/ed25519" "crypto/rand" - "crypto/sha1" + "crypto/sha1" //nolint:gosec" "crypto/x509" "crypto/x509/pkix" "encoding/pem" @@ -720,7 +720,7 @@ func TestGrafanaCfg_Equal(t *testing.T) { config := map[string]string{ proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientKeyContents: string(validCert), + proxy.PluginSecureSocksProxyClientKeyContents: validCert, "other_key": "other_value", } cfg1 := NewGrafanaCfg(config) @@ -734,12 +734,12 @@ func TestGrafanaCfg_Equal(t *testing.T) { cfg1 := NewGrafanaCfg(map[string]string{ proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientKeyContents: string(expiringCert), + proxy.PluginSecureSocksProxyClientKeyContents: expiringCert, "other_key": "other_value", }) cfg2 := NewGrafanaCfg(map[string]string{ proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientKeyContents: string(newCert), + proxy.PluginSecureSocksProxyClientKeyContents: newCert, "other_key": "other_value", }) @@ -751,11 +751,11 @@ func TestGrafanaCfg_Equal(t *testing.T) { cfg1 := NewGrafanaCfg(map[string]string{ proxy.PluginSecureSocksProxyEnabled: "false", - proxy.PluginSecureSocksProxyClientKeyContents: string(expiringCert), + proxy.PluginSecureSocksProxyClientKeyContents: expiringCert, }) cfg2 := NewGrafanaCfg(map[string]string{ proxy.PluginSecureSocksProxyEnabled: "false", - proxy.PluginSecureSocksProxyClientKeyContents: string(expiringCert), + proxy.PluginSecureSocksProxyClientKeyContents: expiringCert, }) // Even though certificate is expiring, proxy is disabled so Equal should return true require.True(t, cfg1.Equal(cfg2)) @@ -793,11 +793,11 @@ func TestGrafanaCfg_Equal(t *testing.T) { cfg1 := NewGrafanaCfg(map[string]string{ proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientKeyContents: string(expiredCert), + proxy.PluginSecureSocksProxyClientKeyContents: expiredCert, }) cfg2 := NewGrafanaCfg(map[string]string{ proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientKeyContents: string(expiredCert), + proxy.PluginSecureSocksProxyClientKeyContents: expiredCert, }) require.False(t, cfg1.Equal(cfg2)) @@ -821,12 +821,12 @@ func TestGrafanaCfg_Equal(t *testing.T) { cfg1 := NewGrafanaCfg(map[string]string{ proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientKeyContents: string(expiringCert), + proxy.PluginSecureSocksProxyClientKeyContents: expiringCert, "other_key": "value1", }) cfg2 := NewGrafanaCfg(map[string]string{ proxy.PluginSecureSocksProxyEnabled: "true", - proxy.PluginSecureSocksProxyClientKeyContents: string(expiringCert), + proxy.PluginSecureSocksProxyClientKeyContents: expiringCert, "other_key": "value2", }) From 4c3253dec35f4ed6f1a29136e0f5e81c445bdf9e Mon Sep 17 00:00:00 2001 From: multimo Date: Thu, 27 Nov 2025 11:12:31 +0100 Subject: [PATCH 05/10] re add back function due to compatibility reasons --- backend/config.go | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/backend/config.go b/backend/config.go index 917e5d48e..d2cea4c24 100644 --- a/backend/config.go +++ b/backend/config.go @@ -3,6 +3,7 @@ package backend import ( "context" "crypto/x509" + "encoding/base64" "encoding/pem" "errors" "fmt" @@ -186,6 +187,40 @@ func (c *GrafanaCfg) Diff(c2 *GrafanaCfg) []string { return changed } +// ProxyHash returns the last four characters of the base64-encoded +// PDC client key contents, if present, for use in datasource instance +// caching. The contents should be PEM-encoded, so we try to PEM-decode +// them, and, if successful, return the base-64 encoding of the final three bytes, +// giving a four character hash. +// +// Deprecated: Use Equal() instead for cache invalidation based on certificate expiration. +func (c *GrafanaCfg) ProxyHash(jsonData []byte) string { + if c == nil || jsonData == nil { + return "" + } + + contents := c.config[proxy.PluginSecureSocksProxyClientKeyContents] + if contents == "" { + return "" + } + + block, _ := pem.Decode([]byte(contents)) + if block == nil { + Logger.Warn("ProxyHash(): key contents are not PEM-encoded") + return "" + } + if block.Type != "PRIVATE KEY" { + Logger.Warn("ProxyHash(): key contents are not PEM-encoded private key") + return "" + } + bl := len(block.Bytes) + if bl < 3 { + Logger.Warn("ProxyHash(): key contents too short") + return "" + } + return base64.StdEncoding.EncodeToString(block.Bytes[bl-3:]) +} + type FeatureToggles struct { // enabled is a set-like map of feature flags that are enabled. enabled map[string]struct{} From 380fb302a87967918bd2360b3987453fa3be1acb Mon Sep 17 00:00:00 2001 From: multimo Date: Thu, 27 Nov 2025 11:14:10 +0100 Subject: [PATCH 06/10] lint --- backend/config_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/config_test.go b/backend/config_test.go index b8c1387b5..6f0b692b7 100644 --- a/backend/config_test.go +++ b/backend/config_test.go @@ -5,7 +5,7 @@ import ( "context" "crypto/ed25519" "crypto/rand" - "crypto/sha1" //nolint:gosec" + "crypto/sha1" //nolint:gosec "crypto/x509" "crypto/x509/pkix" "encoding/pem" From 61b32cb06c695a21a8641c83a59defdcf071f006 Mon Sep 17 00:00:00 2001 From: multimo Date: Thu, 27 Nov 2025 11:16:32 +0100 Subject: [PATCH 07/10] fix type sig change --- backend/config.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/config.go b/backend/config.go index d2cea4c24..9ca6d1f7b 100644 --- a/backend/config.go +++ b/backend/config.go @@ -194,8 +194,8 @@ func (c *GrafanaCfg) Diff(c2 *GrafanaCfg) []string { // giving a four character hash. // // Deprecated: Use Equal() instead for cache invalidation based on certificate expiration. -func (c *GrafanaCfg) ProxyHash(jsonData []byte) string { - if c == nil || jsonData == nil { +func (c *GrafanaCfg) ProxyHash() string { + if c == nil { return "" } From da6b8cda4ff5cafcbb531bd1a14d62fdbf8640cf Mon Sep 17 00:00:00 2001 From: multimo Date: Thu, 27 Nov 2025 11:22:01 +0100 Subject: [PATCH 08/10] fix test --- backend/datasource/instance_provider_test.go | 53 +------------------- 1 file changed, 1 insertion(+), 52 deletions(-) diff --git a/backend/datasource/instance_provider_test.go b/backend/datasource/instance_provider_test.go index 744f0679f..6600e0bdd 100644 --- a/backend/datasource/instance_provider_test.go +++ b/backend/datasource/instance_provider_test.go @@ -2,16 +2,11 @@ package datasource import ( "context" - "encoding/base64" - "encoding/pem" - "fmt" - "strings" "testing" "time" "github.com/grafana/grafana-plugin-sdk-go/backend" "github.com/grafana/grafana-plugin-sdk-go/backend/instancemgmt" - "github.com/grafana/grafana-plugin-sdk-go/backend/proxy" "github.com/stretchr/testify/require" ) @@ -35,53 +30,7 @@ func TestInstanceProvider(t *testing.T) { }, }) require.NoError(t, err) - require.Equal(t, "4##", key) - }) - - t.Run("When PDC is configured, datasource cache key should include its (so-called) hash", func(t *testing.T) { - // the value of Bytes below must be a multiple of three in length for this test - // to pass, but that's an artifact of how the target value is created. The code - // itself isn't affected by the length of the key as long as it's at least 3 bytes. - contents := pem.EncodeToMemory(&pem.Block{ - Type: "PRIVATE KEY", - Bytes: []byte("this will work."), - }) - want := base64.StdEncoding.EncodeToString([]byte("this will work.")) - want = strings.TrimRight(want, "=") - want = want[len(want)-4:] - cfg := backend.NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyClientKeyContents: string(contents), - }) - key, err := ip.GetKey(context.Background(), backend.PluginContext{ - DataSourceInstanceSettings: &backend.DataSourceInstanceSettings{ID: 5}, - GrafanaConfig: cfg, - }) - require.NoError(t, err) - require.Equal(t, fmt.Sprintf("5##%s", want), key) - }) - - t.Run("When PDC is configured but the key is empty, no problem", func(t *testing.T) { - cfg := backend.NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyClientKeyContents: "", - }) - key, err := ip.GetKey(context.Background(), backend.PluginContext{ - DataSourceInstanceSettings: &backend.DataSourceInstanceSettings{ID: 6}, - GrafanaConfig: cfg, - }) - require.NoError(t, err) - require.Equal(t, "6##", key) - }) - - t.Run("When PDC is configured but the key is not PEM-encoded, no problem", func(t *testing.T) { - cfg := backend.NewGrafanaCfg(map[string]string{ - proxy.PluginSecureSocksProxyClientKeyContents: "this is not\na valid string\n", - }) - key, err := ip.GetKey(context.Background(), backend.PluginContext{ - DataSourceInstanceSettings: &backend.DataSourceInstanceSettings{ID: 6}, - GrafanaConfig: cfg, - }) - require.NoError(t, err) - require.Equal(t, "6##", key) + require.Equal(t, "4#", key) }) t.Run("When both the configuration and updated field of current data source instance settings are equal to the cache, should return false", func(t *testing.T) { From 7b0102e0eaae01fa32e42130575c734925a9a825 Mon Sep 17 00:00:00 2001 From: multimo Date: Thu, 4 Dec 2025 14:11:14 +0100 Subject: [PATCH 09/10] add failure case --- backend/config.go | 8 ++++++-- backend/config_test.go | 15 +++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/backend/config.go b/backend/config.go index 9ca6d1f7b..1f268b09e 100644 --- a/backend/config.go +++ b/backend/config.go @@ -94,12 +94,16 @@ func (c *GrafanaCfg) Equal(c2 *GrafanaCfg) bool { } for k, v1 := range c.config { + v2, ok := c2.config[k] + if !ok { + return false + } // Ignore secure socks proxy config values as they are always different. - if strings.Contains(k, "GF_SECURE_SOCKS_DATASOURCE_PROXY") { + if k == proxy.PluginSecureSocksProxyClientCertContents || k == proxy.PluginSecureSocksProxyClientKeyContents { continue } - if v2, ok := c2.config[k]; !ok || v1 != v2 { + if v1 != v2 { return false } } diff --git a/backend/config_test.go b/backend/config_test.go index 6f0b692b7..1c9f153ed 100644 --- a/backend/config_test.go +++ b/backend/config_test.go @@ -832,4 +832,19 @@ func TestGrafanaCfg_Equal(t *testing.T) { require.False(t, cfg1.Equal(cfg2)) }) + + t.Run("Should return false when skipped key causes false positive", func(t *testing.T) { + validCert := createTestCertificate(time.Now().Add(12 * time.Hour)) + + cfg1 := NewGrafanaCfg(map[string]string{ + proxy.PluginSecureSocksProxyClientKeyContents: validCert, + "other_key": "value1", + }) + cfg2 := NewGrafanaCfg(map[string]string{ + "completely_different": "value1", // Same length (2 keys) but totally different key:value + "other_key": "value1", + }) + + require.False(t, cfg1.Equal(cfg2)) + }) } From 292986931eeea0d15bc106b4667f374a2ba98de4 Mon Sep 17 00:00:00 2001 From: multimo Date: Thu, 4 Dec 2025 14:15:26 +0100 Subject: [PATCH 10/10] remove unecessary test --- backend/config_test.go | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/backend/config_test.go b/backend/config_test.go index 1c9f153ed..629493c06 100644 --- a/backend/config_test.go +++ b/backend/config_test.go @@ -761,21 +761,6 @@ func TestGrafanaCfg_Equal(t *testing.T) { require.True(t, cfg1.Equal(cfg2)) }) - t.Run("secure socks proxy keys should be ignored in comparison", func(t *testing.T) { - cfg1 := NewGrafanaCfg(map[string]string{ - "key1": "value1", - "GF_SECURE_SOCKS_DATASOURCE_PROXY": "different_value_should_be_ignored", - "GF_SECURE_SOCKS_DATASOURCE_PROXY2": "also_ignored", - }) - cfg2 := NewGrafanaCfg(map[string]string{ - "key1": "value1", - "GF_SECURE_SOCKS_DATASOURCE_PROXY": "different_value", - "GF_SECURE_SOCKS_DATASOURCE_PROXY2": "completely_different", - }) - // Proxy keys should be ignored, other keys are the same - require.True(t, cfg1.Equal(cfg2)) - }) - t.Run("non-proxy keys should not be ignored", func(t *testing.T) { cfg1 := NewGrafanaCfg(map[string]string{ "key1": "value1",