Deprecate using environment variables for auth settings in sessions (#121)

iwysiu · web-flow · commit 2cf8fadbd684 · 2024-01-31T11:37:14.000-05:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 All notable changes to this project will be documented in this file.
 
+## v0.23.0
+
+- Deprecate using environment variables for auth settings in sessions [#121](https://github.com/grafana/grafana-aws-sdk/pull/121)
+
 ## v0.22.0
 
 - Add ReadAuthSettings to get config settings from context [#118](https://github.com/grafana/grafana-aws-sdk/pull/118)
diff --git a/pkg/awsds/authSettings.go b/pkg/awsds/authSettings.go
@@ -25,8 +25,8 @@ const (
 	// GrafanaAssumeRoleExternalIdKeyName is the string literal for the grafana assume role external id environment variable key name
 	GrafanaAssumeRoleExternalIdKeyName = "AWS_AUTH_EXTERNAL_ID"
 
-	// GrafanaListMetricsPageLimit is the string literal for the cloudwatch list metrics page limit key name
-	GrafanaListMetricsPageLimit = "AWS_CW_LIST_METRICS_PAGE_LIMIT"
+	// ListMetricsPageLimitKeyName is the string literal for the cloudwatch list metrics page limit key name
+	ListMetricsPageLimitKeyName = "AWS_CW_LIST_METRICS_PAGE_LIMIT"
 
 	defaultAssumeRoleEnabled         = true
 	defaultListMetricsPageLimit      = 500
@@ -91,12 +91,21 @@ func ReadAuthSettingsFromContext(ctx context.Context) (*AuthSettings, bool) {
 		hasSettings = true
 	}
 
-	if v := cfg.Get(GrafanaListMetricsPageLimit); v != "" {
+	if v := cfg.Get(SessionDurationEnvVarKeyName); v != "" {
+		sessionDuration, err := gtime.ParseDuration(v)
+		if err == nil {
+			settings.SessionDuration = &sessionDuration
+		} else {
+			backend.Logger.Error("could not parse env variable", "var", SessionDurationEnvVarKeyName)
+		}
+	}
+
+	if v := cfg.Get(ListMetricsPageLimitKeyName); v != "" {
 		listMetricsPageLimit, err := strconv.Atoi(v)
 		if err == nil {
 			settings.ListMetricsPageLimit = listMetricsPageLimit
 		} else {
-			backend.Logger.Error("could not parse context variable", "var", GrafanaListMetricsPageLimit)
+			backend.Logger.Error("could not parse context variable", "var", ListMetricsPageLimitKeyName)
 		}
 		hasSettings = true
 	}
@@ -111,17 +120,6 @@ func ReadAuthSettingsFromContext(ctx context.Context) (*AuthSettings, bool) {
 		hasSettings = true
 	}
 
-	// Users set session duration directly as an environment variable
-	sessionDurationString := os.Getenv(SessionDurationEnvVarKeyName)
-	if sessionDurationString != "" {
-		sessionDuration, err := gtime.ParseDuration(sessionDurationString)
-		if err == nil {
-			settings.SessionDuration = &sessionDuration
-		} else {
-			backend.Logger.Error("could not parse env variable", "var", SessionDurationEnvVarKeyName)
-		}
-	}
-
 	return settings, hasSettings
 }
 
@@ -159,15 +157,15 @@ func ReadAuthSettingsFromEnvironmentVariables() *AuthSettings {
 
 	authSettings.ExternalID = os.Getenv(GrafanaAssumeRoleExternalIdKeyName)
 
-	listMetricsPageLimitString := os.Getenv(GrafanaListMetricsPageLimit)
+	listMetricsPageLimitString := os.Getenv(ListMetricsPageLimitKeyName)
 	if len(listMetricsPageLimitString) == 0 {
-		backend.Logger.Warn("environment variable missing. falling back to default page limit", "var", GrafanaListMetricsPageLimit)
+		backend.Logger.Warn("environment variable missing. falling back to default page limit", "var", ListMetricsPageLimitKeyName)
 		listMetricsPageLimitString = "500"
 	}
 
 	authSettings.ListMetricsPageLimit, err = strconv.Atoi(listMetricsPageLimitString)
 	if err != nil {
-		backend.Logger.Error("could not parse env variable", "var", GrafanaListMetricsPageLimit)
+		backend.Logger.Error("could not parse env variable", "var", ListMetricsPageLimitKeyName)
 		authSettings.ListMetricsPageLimit = defaultListMetricsPageLimit
 	}
 
diff --git a/pkg/awsds/authSettings_test.go b/pkg/awsds/authSettings_test.go
@@ -49,7 +49,7 @@ func TestReadAuthSettingsFromContext(t *testing.T) {
 				AllowedAuthProvidersEnvVarKeyName:   "foo , bar,baz",
 				AssumeRoleEnabledEnvVarKeyName:      "false",
 				GrafanaAssumeRoleExternalIdKeyName:  "mock_id",
-				GrafanaListMetricsPageLimit:         "50",
+				ListMetricsPageLimitKeyName:         "50",
 				proxy.PluginSecureSocksProxyEnabled: "true",
 			}),
 			expectedSettings: &AuthSettings{
@@ -81,29 +81,27 @@ func TestReadAuthSettings(t *testing.T) {
 		os.Setenv(GrafanaAssumeRoleExternalIdKeyName, originalExternalId)
 	}()
 
-	expectedDuration, err := time.ParseDuration("20m")
-	require.NoError(t, err)
+	ctxDuration := 10 * time.Minute
+	envDuration := 20 * time.Minute
 	expectedSessionContextSettings := &AuthSettings{
 		AllowedAuthProviders:      []string{"foo", "bar", "baz"},
 		AssumeRoleEnabled:         false,
-		SessionDuration:           &expectedDuration, //20 minutes in nanoseconds count,
+		SessionDuration:           &ctxDuration,
 		ExternalID:                "mock_id",
 		ListMetricsPageLimit:      50,
 		SecureSocksDSProxyEnabled: true,
 	}
 
 	expectedSessionEnvSettings := &AuthSettings{
-		AllowedAuthProviders:      []string{"env1", "env2"},
+		AllowedAuthProviders:      []string{"default", "keys", "credentials"},
 		AssumeRoleEnabled:         true,
-		SessionDuration:           &expectedDuration,
+		SessionDuration:           &envDuration,
 		ExternalID:                "env_id",
 		ListMetricsPageLimit:      30,
 		SecureSocksDSProxyEnabled: false,
 	}
 
-	require.NoError(t, os.Setenv(AllowedAuthProvidersEnvVarKeyName, "env1,env2"))
-	require.NoError(t, os.Setenv(AssumeRoleEnabledEnvVarKeyName, "true"))
-	require.NoError(t, os.Setenv(GrafanaListMetricsPageLimit, "30"))
+	require.NoError(t, os.Setenv(ListMetricsPageLimitKeyName, "30"))
 	require.NoError(t, os.Setenv(SessionDurationEnvVarKeyName, "20m"))
 	require.NoError(t, os.Setenv(proxy.PluginSecureSocksProxyEnabled, "false"))
 	defer unsetEnvironmentVariables()
@@ -138,8 +136,9 @@ func TestReadAuthSettings(t *testing.T) {
 			cfg: backend.NewGrafanaCfg(map[string]string{
 				AllowedAuthProvidersEnvVarKeyName:   "foo , bar,baz",
 				AssumeRoleEnabledEnvVarKeyName:      "false",
+				SessionDurationEnvVarKeyName:        "10m",
 				GrafanaAssumeRoleExternalIdKeyName:  "mock_id",
-				GrafanaListMetricsPageLimit:         "50",
+				ListMetricsPageLimitKeyName:         "50",
 				proxy.PluginSecureSocksProxyEnabled: "true",
 			}),
 			expectedSettings: expectedSessionContextSettings,
@@ -155,3 +154,10 @@ func TestReadAuthSettings(t *testing.T) {
 		})
 	}
 }
+
+func unsetEnvironmentVariables() {
+	os.Unsetenv(AllowedAuthProvidersEnvVarKeyName)
+	os.Unsetenv(AssumeRoleEnabledEnvVarKeyName)
+	os.Unsetenv(SessionDurationEnvVarKeyName)
+	os.Unsetenv(ListMetricsPageLimitKeyName)
+}
diff --git a/pkg/awsds/sessions.go b/pkg/awsds/sessions.go
@@ -28,14 +28,12 @@ type envelope struct {
 type SessionCache struct {
 	sessCache     map[string]envelope
 	sessCacheLock sync.RWMutex
-	authSettings  *AuthSettings
 }
 
 // NewSessionCache creates a new session cache using the default settings loaded from environment variables
 func NewSessionCache() *SessionCache {
 	return &SessionCache{
-		sessCache:    map[string]envelope{},
-		authSettings: ReadAuthSettingsFromEnvironmentVariables(),
+		sessCache: map[string]envelope{},
 	}
 }
 
@@ -78,6 +76,7 @@ type SessionConfig struct {
 	Settings      AWSDatasourceSettings
 	HTTPClient    *http.Client
 	UserAgentName *string
+	AuthSettings  *AuthSettings
 }
 
 func isOptInRegion(region string) bool {
@@ -106,8 +105,15 @@ func (sc *SessionCache) GetSession(c SessionConfig) (*session.Session, error) {
 		// DefaultRegion is deprecated, Region should be used instead
 		c.Settings.Region = c.Settings.DefaultRegion
 	}
+
+	// If the datasource calling GetSession is getting the settings from the contexts, they'll pass
+	// the values through AuthSettings. Otherwise, we need to get them from the env variables.
+	if c.AuthSettings == nil {
+		c.AuthSettings = ReadAuthSettingsFromEnvironmentVariables()
+	}
+
 	authTypeAllowed := false
-	for _, provider := range sc.authSettings.AllowedAuthProviders {
+	for _, provider := range c.AuthSettings.AllowedAuthProviders {
 		if provider == c.Settings.AuthType.String() {
 			authTypeAllowed = true
 			break
@@ -118,7 +124,7 @@ func (sc *SessionCache) GetSession(c SessionConfig) (*session.Session, error) {
 		return nil, fmt.Errorf("attempting to use an auth type that is not allowed: %q", c.Settings.AuthType.String())
 	}
 
-	if c.Settings.AssumeRoleARN != "" && !sc.authSettings.AssumeRoleEnabled {
+	if c.Settings.AssumeRoleARN != "" && !c.AuthSettings.AssumeRoleEnabled {
 		return nil, fmt.Errorf("attempting to use assume role (ARN) which is disabled in grafana.ini")
 	}
 
@@ -159,7 +165,7 @@ func (sc *SessionCache) GetSession(c SessionConfig) (*session.Session, error) {
 		c.Settings.Region = ""
 	}
 	if c.Settings.Region != "" {
-		if c.Settings.AssumeRoleARN != "" && sc.authSettings.AssumeRoleEnabled && isOptInRegion(c.Settings.Region) {
+		if c.Settings.AssumeRoleARN != "" && c.AuthSettings.AssumeRoleEnabled && isOptInRegion(c.Settings.Region) {
 			// When assuming a role, the real region is set later in a new session
 			// so we use a well-known region here (not opt-in) to obtain valid credentials
 			regionCfg = &aws.Config{Region: aws.String("us-east-1")}
@@ -201,11 +207,11 @@ func (sc *SessionCache) GetSession(c SessionConfig) (*session.Session, error) {
 	}
 
 	duration := stscreds.DefaultDuration
-	if sc.authSettings.SessionDuration != nil {
-		duration = *sc.authSettings.SessionDuration
+	if c.AuthSettings.SessionDuration != nil {
+		duration = *c.AuthSettings.SessionDuration
 	}
 	expiration := time.Now().UTC().Add(duration)
-	if c.Settings.AssumeRoleARN != "" && sc.authSettings.AssumeRoleEnabled {
+	if c.Settings.AssumeRoleARN != "" && c.AuthSettings.AssumeRoleEnabled {
 		// We should assume a role in AWS
 		backend.Logger.Debug("Trying to assume role in AWS", "arn", c.Settings.AssumeRoleARN)
 
@@ -229,7 +235,7 @@ func (sc *SessionCache) GetSession(c SessionConfig) (*session.Session, error) {
 					p.Expiry.SetExpiration(expiration, 0)
 					p.Duration = duration
 					if c.Settings.AuthType == AuthTypeGrafanaAssumeRole {
-						p.ExternalID = aws.String(sc.authSettings.ExternalID)
+						p.ExternalID = aws.String(c.AuthSettings.ExternalID)
 					} else if c.Settings.ExternalID != "" {
 						p.ExternalID = aws.String(c.Settings.ExternalID)
 					}
diff --git a/pkg/awsds/sessions_test.go b/pkg/awsds/sessions_test.go
