Move and integrate Receiver configuration#484
Open
yuri-tceretian wants to merge 4 commits intomainfrom
Open
Conversation
yuri-tceretian
force-pushed
the
yuri-tseretyan/move-v0-integrations
branch
from
083b53b to
d56bd27
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| MSTeamsConfigs []*teams_v0mimir1.Config `yaml:"msteams_configs,omitempty" json:"msteams_configs,omitempty"` | ||
| MSTeamsV2Configs []*teams_v0mimir2.Config `yaml:"msteamsv2_configs,omitempty" json:"msteamsv2_configs,omitempty"` | ||
| JiraConfigs []*jira_v0mimir1.Config `yaml:"jira_configs,omitempty" json:"jira_configs,omitempty"` | ||
| } |
There was a problem hiding this comment.
Receiver type changes silently bypass Mimir validation
High Severity
The new Receiver struct uses v0mimir1.*Config types (e.g., discord_v0mimir1.Config, email_v0mimir1.Config), but ValidateAlertmanagerConfig in mimir_validation.go uses reflection to match against the old upstream types (e.g., config.DiscordConfig, config.EmailConfig, commoncfg.HTTPClientConfig). Since these are different types, none of the switch cases match when recursively walking the new Receiver, silently skipping all Mimir-specific validation. This bypasses security-relevant checks that prevent file-based credentials, disallowed proxy settings, and TLS file paths.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
Or push these changes by commenting: Preview (6226a16f55)diff --git a/definition/mimir_validation.go b/definition/mimir_validation.go
--- a/definition/mimir_validation.go
+++ b/definition/mimir_validation.go
@@ -8,6 +8,19 @@
"github.com/prometheus/alertmanager/config"
commoncfg "github.com/prometheus/common/config"
+
+ httpcfg "github.com/grafana/alerting/http/v0mimir1"
+ discord_v0mimir1 "github.com/grafana/alerting/receivers/discord/v0mimir1"
+ email_v0mimir1 "github.com/grafana/alerting/receivers/email/v0mimir1"
+ opsgenie_v0mimir1 "github.com/grafana/alerting/receivers/opsgenie/v0mimir1"
+ pagerduty_v0mimir1 "github.com/grafana/alerting/receivers/pagerduty/v0mimir1"
+ pushover_v0mimir1 "github.com/grafana/alerting/receivers/pushover/v0mimir1"
+ slack_v0mimir1 "github.com/grafana/alerting/receivers/slack/v0mimir1"
+ teams_v0mimir1 "github.com/grafana/alerting/receivers/teams/v0mimir1"
+ teams_v0mimir2 "github.com/grafana/alerting/receivers/teams/v0mimir2"
+ telegram_v0mimir1 "github.com/grafana/alerting/receivers/telegram/v0mimir1"
+ victorops_v0mimir1 "github.com/grafana/alerting/receivers/victorops/v0mimir1"
+ webhook_v0mimir1 "github.com/grafana/alerting/receivers/webhook/v0mimir1"
)
var (
@@ -118,6 +131,72 @@
if err := validateWebhookConfig(v.Interface().(config.WebhookConfig)); err != nil {
return err
}
+
+ // v0mimir1 types
+ case reflect.TypeOf(httpcfg.HTTPClientConfig{}):
+ if err := validateV0Mimir1HTTPConfig(v.Interface().(httpcfg.HTTPClientConfig)); err != nil {
+ return err
+ }
+
+ case reflect.TypeOf(httpcfg.TLSConfig{}):
+ if err := validateV0Mimir1TLSConfig(v.Interface().(httpcfg.TLSConfig)); err != nil {
+ return err
+ }
+
+ case reflect.TypeOf(discord_v0mimir1.Config{}):
+ if err := validateV0Mimir1DiscordConfig(v.Interface().(discord_v0mimir1.Config)); err != nil {
+ return err
+ }
+
+ case reflect.TypeOf(email_v0mimir1.Config{}):
+ if err := validateV0Mimir1EmailConfig(v.Interface().(email_v0mimir1.Config)); err != nil {
+ return err
+ }
+
+ case reflect.TypeOf(slack_v0mimir1.Config{}):
+ if err := validateV0Mimir1SlackConfig(v.Interface().(slack_v0mimir1.Config)); err != nil {
+ return err
+ }
+
+ case reflect.TypeOf(opsgenie_v0mimir1.Config{}):
+ if err := validateV0Mimir1OpsGenieConfig(v.Interface().(opsgenie_v0mimir1.Config)); err != nil {
+ return err
+ }
+
+ case reflect.TypeOf(victorops_v0mimir1.Config{}):
+ if err := validateV0Mimir1VictorOpsConfig(v.Interface().(victorops_v0mimir1.Config)); err != nil {
+ return err
+ }
+
+ case reflect.TypeOf(pagerduty_v0mimir1.Config{}):
+ if err := validateV0Mimir1PagerDutyConfig(v.Interface().(pagerduty_v0mimir1.Config)); err != nil {
+ return err
+ }
+
+ case reflect.TypeOf(pushover_v0mimir1.Config{}):
+ if err := validateV0Mimir1PushoverConfig(v.Interface().(pushover_v0mimir1.Config)); err != nil {
+ return err
+ }
+
+ case reflect.TypeOf(telegram_v0mimir1.Config{}):
+ if err := validateV0Mimir1TelegramConfig(v.Interface().(telegram_v0mimir1.Config)); err != nil {
+ return err
+ }
+
+ case reflect.TypeOf(webhook_v0mimir1.Config{}):
+ if err := validateV0Mimir1WebhookConfig(v.Interface().(webhook_v0mimir1.Config)); err != nil {
+ return err
+ }
+
+ case reflect.TypeOf(teams_v0mimir1.Config{}):
+ if err := validateV0Mimir1MSTeamsConfig(v.Interface().(teams_v0mimir1.Config)); err != nil {
+ return err
+ }
+
+ case reflect.TypeOf(teams_v0mimir2.Config{}):
+ if err := validateV0Mimir2MSTeamsConfig(v.Interface().(teams_v0mimir2.Config)); err != nil {
+ return err
+ }
}
// If the input config is a struct, recursively iterate on all fields.
@@ -325,3 +404,145 @@
}
return nil
}
+
+// validateV0Mimir1HTTPConfig validates the v0mimir1 HTTP config and returns an error if it contains
+// settings not allowed by Mimir.
+func validateV0Mimir1HTTPConfig(cfg httpcfg.HTTPClientConfig) error {
+ if cfg.BasicAuth != nil && cfg.BasicAuth.PasswordFile != "" {
+ return errPasswordFileNotAllowed
+ }
+ if cfg.Authorization != nil && cfg.Authorization.CredentialsFile != "" {
+ return errPasswordFileNotAllowed
+ }
+ if cfg.BearerTokenFile != "" {
+ return errPasswordFileNotAllowed
+ }
+ if cfg.OAuth2 != nil {
+ if cfg.OAuth2.ClientSecretFile != "" {
+ return errOAuth2SecretFileNotAllowed
+ }
+ // Mimir's "firewall" doesn't protect OAuth2 client, so we disallow Proxy settings here.
+ if cfg.OAuth2.ProxyURL.URL != nil && cfg.OAuth2.ProxyURL.String() != "" {
+ return errProxyURLNotAllowed
+ }
+ if cfg.OAuth2.ProxyFromEnvironment {
+ return errProxyFromEnvironmentURLNotAllowed
+ }
+ }
+ // We allow setting proxy config (cfg.ProxyConfig), because Mimir's "firewall" protects those calls.
+ return validateV0Mimir1TLSConfig(cfg.TLSConfig)
+}
+
+// validateV0Mimir1TLSConfig validates the v0mimir1 TLS config and returns an error if it contains
+// settings not allowed by Mimir.
+func validateV0Mimir1TLSConfig(cfg httpcfg.TLSConfig) error {
+ if cfg.CAFile != "" || cfg.CertFile != "" || cfg.KeyFile != "" || cfg.CA != "" || cfg.Cert != "" || cfg.Key != "" {
+ return errTLSConfigNotAllowed
+ }
+ return nil
+}
+
+// validateV0Mimir1DiscordConfig validates the v0mimir1 Discord config and returns an error if it
+// contains settings not allowed by Mimir.
+func validateV0Mimir1DiscordConfig(cfg discord_v0mimir1.Config) error {
+ if cfg.WebhookURLFile != "" {
+ return errWebhookURLFileNotAllowed
+ }
+ return nil
+}
+
+// validateV0Mimir1EmailConfig validates the v0mimir1 Email config and returns an error if it
+// contains settings not allowed by Mimir.
+func validateV0Mimir1EmailConfig(cfg email_v0mimir1.Config) error {
+ if cfg.AuthPasswordFile != "" {
+ return errPasswordFileNotAllowed
+ }
+ return nil
+}
+
+// validateV0Mimir1SlackConfig validates the v0mimir1 Slack config and returns an error if it
+// contains settings not allowed by Mimir.
+func validateV0Mimir1SlackConfig(cfg slack_v0mimir1.Config) error {
+ if cfg.APIURLFile != "" {
+ return errSlackAPIURLFileNotAllowed
+ }
+ return nil
+}
+
+// validateV0Mimir1OpsGenieConfig validates the v0mimir1 OpsGenie config and returns an error if it
+// contains settings not allowed by Mimir.
+func validateV0Mimir1OpsGenieConfig(cfg opsgenie_v0mimir1.Config) error {
+ if cfg.APIKeyFile != "" {
+ return errOpsGenieAPIKeyFileFileNotAllowed
+ }
+ return nil
+}
+
+// validateV0Mimir1VictorOpsConfig validates the v0mimir1 VictorOps config and returns an error if it
+// contains settings not allowed by Mimir.
+func validateV0Mimir1VictorOpsConfig(cfg victorops_v0mimir1.Config) error {
+ if cfg.APIKeyFile != "" {
+ return errVictorOpsAPIKeyFileNotAllowed
+ }
+ return nil
+}
+
+// validateV0Mimir1PagerDutyConfig validates the v0mimir1 PagerDuty config and returns an error if it
+// contains settings not allowed by Mimir.
+func validateV0Mimir1PagerDutyConfig(cfg pagerduty_v0mimir1.Config) error {
+ if cfg.ServiceKeyFile != "" {
+ return errPagerDutyServiceKeyFileNotAllowed
+ }
+ if cfg.RoutingKeyFile != "" {
+ return errPagerDutyRoutingKeyFileNotAllowed
+ }
+ return nil
+}
+
+// validateV0Mimir1PushoverConfig validates the v0mimir1 Pushover config and returns an error if it
+// contains settings not allowed by Mimir.
+func validateV0Mimir1PushoverConfig(cfg pushover_v0mimir1.Config) error {
+ if cfg.UserKeyFile != "" {
+ return errPushoverUserKeyFileNotAllowed
+ }
+ if cfg.TokenFile != "" {
+ return errPushoverTokenFileNotAllowed
+ }
+ return nil
+}
+
+// validateV0Mimir1TelegramConfig validates the v0mimir1 Telegram config and returns an error if it
+// contains settings not allowed by Mimir.
+func validateV0Mimir1TelegramConfig(cfg telegram_v0mimir1.Config) error {
+ if cfg.BotTokenFile != "" {
+ return errTelegramBotTokenFileNotAllowed
+ }
+ return nil
+}
+
+// validateV0Mimir1WebhookConfig validates the v0mimir1 Webhook config and returns an error if it
+// contains settings not allowed by Mimir.
+func validateV0Mimir1WebhookConfig(cfg webhook_v0mimir1.Config) error {
+ if cfg.URLFile != "" {
+ return errWebhookURLFileNotAllowed
+ }
+ return nil
+}
+
+// validateV0Mimir1MSTeamsConfig validates the v0mimir1 MS Teams config and returns an error if it
+// contains settings not allowed by Mimir.
+func validateV0Mimir1MSTeamsConfig(cfg teams_v0mimir1.Config) error {
+ if cfg.WebhookURLFile != "" {
+ return errWebhookURLFileNotAllowed
+ }
+ return nil
+}
+
+// validateV0Mimir2MSTeamsConfig validates the v0mimir2 MS Teams V2 config and returns an error if it
+// contains settings not allowed by Mimir.
+func validateV0Mimir2MSTeamsConfig(cfg teams_v0mimir2.Config) error {
+ if cfg.WebhookURLFile != "" {
+ return errWebhookURLFileNotAllowed
+ }
+ return nil
+}
diff --git a/definition/mimir_validation_test.go b/definition/mimir_validation_test.go
--- a/definition/mimir_validation_test.go
+++ b/definition/mimir_validation_test.go
@@ -3,13 +3,35 @@
package definition
import (
+ "net/url"
"testing"
"github.com/prometheus/alertmanager/config"
commoncfg "github.com/prometheus/common/config"
"github.com/stretchr/testify/assert"
+
+ httpcfg "github.com/grafana/alerting/http/v0mimir1"
+ discord_v0mimir1 "github.com/grafana/alerting/receivers/discord/v0mimir1"
+ email_v0mimir1 "github.com/grafana/alerting/receivers/email/v0mimir1"
+ opsgenie_v0mimir1 "github.com/grafana/alerting/receivers/opsgenie/v0mimir1"
+ pagerduty_v0mimir1 "github.com/grafana/alerting/receivers/pagerduty/v0mimir1"
+ pushover_v0mimir1 "github.com/grafana/alerting/receivers/pushover/v0mimir1"
+ slack_v0mimir1 "github.com/grafana/alerting/receivers/slack/v0mimir1"
+ teams_v0mimir1 "github.com/grafana/alerting/receivers/teams/v0mimir1"
+ teams_v0mimir2 "github.com/grafana/alerting/receivers/teams/v0mimir2"
+ telegram_v0mimir1 "github.com/grafana/alerting/receivers/telegram/v0mimir1"
+ victorops_v0mimir1 "github.com/grafana/alerting/receivers/victorops/v0mimir1"
+ webhook_v0mimir1 "github.com/grafana/alerting/receivers/webhook/v0mimir1"
)
+func mustParseURL(s string) *url.URL {
+ u, err := url.Parse(s)
+ if err != nil {
+ panic(err)
+ }
+ return u
+}
+
func TestValidateAlertmanagerConfig(t *testing.T) {
tests := map[string]struct {
input interface{}
@@ -180,6 +202,202 @@
},
expected: errTLSConfigNotAllowed,
},
+
+ // v0mimir1 types tests
+ "v0mimir1 HTTPClientConfig.BasicAuth.PasswordFile": {
+ input: httpcfg.HTTPClientConfig{
+ BasicAuth: &httpcfg.BasicAuth{
+ PasswordFile: "/secrets",
+ },
+ },
+ expected: errPasswordFileNotAllowed,
+ },
+ "v0mimir1 HTTPClientConfig.BearerTokenFile": {
+ input: httpcfg.HTTPClientConfig{
+ BearerTokenFile: "/file",
+ },
+ expected: errPasswordFileNotAllowed,
+ },
+ "v0mimir1 HTTPClientConfig.Authorization.CredentialsFile": {
+ input: httpcfg.HTTPClientConfig{
+ Authorization: &httpcfg.Authorization{
+ CredentialsFile: "/file",
+ },
+ },
+ expected: errPasswordFileNotAllowed,
+ },
+ "v0mimir1 HTTPClientConfig.OAuth2.ClientSecretFile": {
+ input: httpcfg.HTTPClientConfig{
+ OAuth2: &httpcfg.OAuth2{
+ ClientID: "client-id",
+ TokenURL: "http://token-url",
+ ClientSecretFile: "/file",
+ },
+ },
+ expected: errOAuth2SecretFileNotAllowed,
+ },
+ "v0mimir1 HTTPClientConfig.OAuth2.ProxyURL": {
+ input: httpcfg.HTTPClientConfig{
+ OAuth2: &httpcfg.OAuth2{
+ ClientID: "client-id",
+ TokenURL: "http://token-url",
+ ClientSecret: "secret",
+ ProxyConfig: httpcfg.ProxyConfig{
+ ProxyURL: commoncfg.URL{URL: mustParseURL("http://proxy:8080")},
+ },
+ },
+ },
+ expected: errProxyURLNotAllowed,
+ },
+ "v0mimir1 HTTPClientConfig.OAuth2.ProxyFromEnvironment": {
+ input: httpcfg.HTTPClientConfig{
+ OAuth2: &httpcfg.OAuth2{
+ ClientID: "client-id",
+ TokenURL: "http://token-url",
+ ClientSecret: "secret",
+ ProxyConfig: httpcfg.ProxyConfig{
+ ProxyFromEnvironment: true,
+ },
+ },
+ },
+ expected: errProxyFromEnvironmentURLNotAllowed,
+ },
+ "v0mimir1 TLSConfig.CAFile": {
+ input: httpcfg.TLSConfig{
+ CAFile: "/file",
+ },
+ expected: errTLSConfigNotAllowed,
+ },
+ "v0mimir1 TLSConfig.CertFile": {
+ input: httpcfg.TLSConfig{
+ CertFile: "/file",
+ },
+ expected: errTLSConfigNotAllowed,
+ },
+ "v0mimir1 TLSConfig.KeyFile": {
+ input: httpcfg.TLSConfig{
+ KeyFile: "/file",
+ },
+ expected: errTLSConfigNotAllowed,
+ },
+ "v0mimir1 TLSConfig.CA": {
+ input: httpcfg.TLSConfig{
+ CA: "ca-content",
+ },
+ expected: errTLSConfigNotAllowed,
+ },
+ "v0mimir1 TLSConfig.Cert": {
+ input: httpcfg.TLSConfig{
+ Cert: "cert-content",
+ },
+ expected: errTLSConfigNotAllowed,
+ },
+ "v0mimir1 TLSConfig.Key": {
+ input: httpcfg.TLSConfig{
+ Key: "key-content",
+ },
+ expected: errTLSConfigNotAllowed,
+ },
+ "v0mimir1 DiscordConfig.WebhookURLFile": {
+ input: discord_v0mimir1.Config{
+ WebhookURLFile: "/file",
+ },
+ expected: errWebhookURLFileNotAllowed,
+ },
+ "v0mimir1 EmailConfig.AuthPasswordFile": {
+ input: email_v0mimir1.Config{
+ AuthPasswordFile: "/file",
+ },
+ expected: errPasswordFileNotAllowed,
+ },
+ "v0mimir1 SlackConfig.APIURLFile": {
+ input: slack_v0mimir1.Config{
+ APIURLFile: "/file",
+ },
+ expected: errSlackAPIURLFileNotAllowed,
+ },
+ "v0mimir1 OpsGenieConfig.APIKeyFile": {
+ input: opsgenie_v0mimir1.Config{
+ APIKeyFile: "/file",
+ },
+ expected: errOpsGenieAPIKeyFileFileNotAllowed,
+ },
+ "v0mimir1 VictorOpsConfig.APIKeyFile": {
+ input: victorops_v0mimir1.Config{
+ APIKeyFile: "/file",
+ },
+ expected: errVictorOpsAPIKeyFileNotAllowed,
+ },
+ "v0mimir1 PagerDutyConfig.ServiceKeyFile": {
+ input: pagerduty_v0mimir1.Config{
+ ServiceKeyFile: "/file",
+ },
+ expected: errPagerDutyServiceKeyFileNotAllowed,
+ },
+ "v0mimir1 PagerDutyConfig.RoutingKeyFile": {
+ input: pagerduty_v0mimir1.Config{
+ RoutingKeyFile: "/file",
+ },
+ expected: errPagerDutyRoutingKeyFileNotAllowed,
+ },
+ "v0mimir1 PushoverConfig.UserKeyFile": {
+ input: pushover_v0mimir1.Config{
+ UserKeyFile: "/file",
+ },
+ expected: errPushoverUserKeyFileNotAllowed,
+ },
+ "v0mimir1 PushoverConfig.TokenFile": {
+ input: pushover_v0mimir1.Config{
+ TokenFile: "/file",
+ },
+ expected: errPushoverTokenFileNotAllowed,
+ },
+ "v0mimir1 TelegramConfig.BotTokenFile": {
+ input: telegram_v0mimir1.Config{
+ BotTokenFile: "/file",
+ },
+ expected: errTelegramBotTokenFileNotAllowed,
+ },
+ "v0mimir1 WebhookConfig.URLFile": {
+ input: webhook_v0mimir1.Config{
+ URLFile: "/file",
+ },
+ expected: errWebhookURLFileNotAllowed,
+ },
+ "v0mimir1 MSTeamsConfig.WebhookURLFile": {
+ input: teams_v0mimir1.Config{
+ WebhookURLFile: "/file",
+ },
+ expected: errWebhookURLFileNotAllowed,
+ },
+ "v0mimir2 MSTeamsV2Config.WebhookURLFile": {
+ input: teams_v0mimir2.Config{
+ WebhookURLFile: "/file",
+ },
+ expected: errWebhookURLFileNotAllowed,
+ },
+ "Receiver with v0mimir1 DiscordConfig.WebhookURLFile": {
+ input: Receiver{
+ Name: "test",
+ DiscordConfigs: []*discord_v0mimir1.Config{{
+ WebhookURLFile: "/file",
+ }},
+ },
+ expected: errWebhookURLFileNotAllowed,
+ },
+ "Receiver with v0mimir1 HTTPClientConfig": {
+ input: Receiver{
+ Name: "test",
+ WebhookConfigs: []*webhook_v0mimir1.Config{{
+ HTTPConfig: &httpcfg.HTTPClientConfig{
+ BasicAuth: &httpcfg.BasicAuth{
+ PasswordFile: "/secrets",
+ },
+ },
+ }},
+ },
+ expected: errPasswordFileNotAllowed,
+ },
}
for testName, testData := range tests { |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Description
This pull request includes the following changes:
Receivercomponent to align with the updated architecture.HTTPClientConfig.HTTPClientConfig.Checklist
Note
Medium Risk
Touches core alerting receiver config types and integration instantiation paths; mis-mapped
HTTPConfigdefaults or config unmarshalling differences could break notification delivery across multiple integrations.Overview
Switches the API/config model from upstream
config.Receiverto a newdefinition.Receiverthat embeds Grafana-maintainedv0mimir1receiver config types (Discord/Email/Slack/Webhook/etc., plus Jira) and enforces receiver-name validation during YAML unmarshalling.Introduces
http/v0mimir1with a standaloneHTTPClientConfigplus conversion helpers to/fromprometheus/commonconfigs, then updates compat loading/validation to populate each integration’sHTTPConfigvia this conversion.Updates notifier construction to use the Grafana
v0mimir1integration implementations (and adds Jira toBuildPrometheusReceiverIntegrations), removes the now-unusedGrafanaToUpstreamConfigconversion path, and refactors tests/schema mapping to the new types and secrets mapping logic.Written by Cursor Bugbot for commit d56bd27. This will update automatically on new commits. Configure here.