Introduce support to IAM authentication in the datastore #4828

amartinezfayo · 2024-01-23T16:21:23Z

This PR introduces support to authenticate to the datastore using IAM database authentication for PostgreSQL and MySQL databases hosted in AWS RDS.

This is done by supporting the new aws_postgres and aws_mysql database types in the datastore configuration.

Fixes #2283.

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

maxlambrecht · 2024-01-23T20:19:29Z

pkg/server/datastore/sqldriver/awsrds/awsrds.go

+// Open is the overridden method for opening a connection, using
+// AWS IAM authentication
+func (w *sqlDriverWrapper) Open(name string) (driver.Conn, error) {
+	ctx, cancel := context.WithTimeout(context.Background(), timeOut)


Is there a reason to not use a derived context here and instead using a context.Background here?

The main reason is that sql.Open does not allow to provide a context, so we are limited by the arguments that are there.

You could have injected the context to the wrapper so it could be used.

Thank you @marcofranssen for you feedback.
The wrapper driver instance is created at initialization time. The wrapper struct could still have a context that could be set externally, but we would need to have a map between that context and a future call to Open (probably with the string used in the Open call as the key). All that seemed to be error-prone and adding a complexity that we may not really need. I would rather explore alternatives if we see that having a separate context causes problems.

maxlambrecht · 2024-01-23T20:19:50Z

pkg/server/datastore/sqldriver/awsrds/awsrds.go

+	}
+	password, err := token.getAWSAuthToken(ctx, config, w.tokenBuilder)
+	if err != nil {
+		return nil, fmt.Errorf("could not get authorization token: %w", err)


Suggested change

return nil, fmt.Errorf("could not get authorization token: %w", err)

return nil, fmt.Errorf("could not get authentication token: %w", err)

maxlambrecht · 2024-01-23T20:24:54Z

doc/plugin_server_datastore_sql.md

@@ -135,6 +135,60 @@ If you need to use custom Root CA, just specify `root_ca_path` in the plugin con
    }
 ```

+### IAM Authentication
+
+With IAM database authentication, an authentication token is used instead of a password when connecting to the database. For that reason, a password must not be provided in the connection string.


Suggested change

With IAM database authentication, an authentication token is used instead of a password when connecting to the database. For that reason, a password must not be provided in the connection string.

AWS Identity and Access Management (IAM) authentication allows for secure authentication to databases hosted on AWS. Unlike traditional methods, it uses an authentication token instead of a password. When using IAM authentication, it is required to exclude the password from the connection string.

Thank you for the suggested change. Since IAM authentication is not tied to AWS IAM only (we will be supporting IAM authentication in other services from other cloud providers) the description should not be AWS-specific.
I'm taking this but without the AWS-specific parts. Thanks for the suggestion, it's a lot more clear! than the original text

maxlambrecht · 2024-01-23T20:28:41Z

doc/plugin_server_datastore_sql.md

+
+With IAM database authentication, an authentication token is used instead of a password when connecting to the database. For that reason, a password must not be provided in the connection string.
+
+The `database_type` configuration can have additional settings when a database type supporting IAM authentication is used, and always takes the following form:


Suggested change

The `database_type` configuration can have additional settings when a database type supporting IAM authentication is used, and always takes the following form:

The database_type configuration allows specifying the type of database with IAM support. The configuration always follows this structure:

maxlambrecht · 2024-01-23T20:30:02Z

doc/plugin_server_datastore_sql.md

+        ...
+    }
+```
+


Suggested change

Note: Replace `dbtype-with-iam-support` with the specific database type that supports IAM authentication.

maxlambrecht · 2024-01-23T21:13:42Z

pkg/server/datastore/sqlstore/postgres.go

+			return nil, "", false, err
+		}
+		if c.Password != "" {
+			return nil, "", false, errors.New("invalid postgres config: no password should be provided when using IAM authentication")


Suggested change

return nil, "", false, errors.New("invalid postgres config: no password should be provided when using IAM authentication")

return nil, "", false, errors.New("invalid postgres configuration: password should not be set when using IAM authentication.")

Thanks for the suggestion. Note that error strings should not end with punctuation, so I'll not end it with ".".

maxlambrecht · 2024-01-23T21:16:29Z

pkg/server/datastore/sqldriver/awsrds/auth_token.go

+}
+
+type tokenGetter interface {
+	getAWSAuthToken(ctx context.Context, params *Config, tokenBuilder authTokenBuilder) (string, error)


I'd rename to getAuthToken to keep it consistent with the naming in the authTokenBuilder interface.

maxlambrecht · 2024-01-23T21:21:33Z

pkg/server/datastore/sqldriver/awsrds/awsrds.go

+	MySQLDriverName    = "aws-rds-mysql"
+	PostgresDriverName = "aws-rds-postgres"
+	iso8601BasicFormat = "20060102T150405Z"
+	timeOut            = time.Second * 30


Suggested change

timeOut = time.Second * 30

connectionTimeout = time.Second * 30

This timeout is not a connection timeout to the database, but a timeout building the authentication token. Maybe buildAuthTokenTimeout is a better name.

maxlambrecht · 2024-01-23T21:21:57Z

pkg/server/datastore/sqldriver/awsrds/awsrds.go

+const (
+	MySQLDriverName    = "aws-rds-mysql"
+	PostgresDriverName = "aws-rds-postgres"
+	iso8601BasicFormat = "20060102T150405Z"


I'd move this constant to the other file where it's used.

maxlambrecht · 2024-01-23T21:28:06Z

pkg/server/datastore/sqldriver/awsrds/awsrds.go

+		return "", fmt.Errorf("could not parse connection string: %w", err)
+	}
+	if cfg.Password != "" {
+		return "", errors.New("password was provided in the connection string")


Suggested change

return "", errors.New("password was provided in the connection string")

return "", errors.New("unexpected password in connection string for IAM authentication")

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

azdagron · 2024-02-15T14:36:31Z

doc/plugin_server_datastore_sql.md

+```hcl
+    DataStore "sql" {
+        plugin_data {
+           database_type "aws_postgres" {


nit: indentation is off

azdagron · 2024-02-21T23:23:15Z

pkg/server/datastore/sqldriver/awsrds/auth_token.go

+
+func (a *authToken) isExpired() bool {
+	clockSkew := time.Minute // Make sure that the authentication token is valid for one more minute.
+	return nowFunc().Add(clockSkew).Sub(a.expiresAt) >= 0


Should we be subtracting a minute from the current time if we want the token to live a little longer?

Good catch, we should be subtracting the clock skew, not adding.

azdagron · 2024-02-21T23:27:22Z

pkg/server/datastore/sqldriver/awsrds/awsrds.go

+
+// FormatDSN returns a DSN string based on the configuration.
+func (c *Config) FormatDSN() (string, error) {
+	dsn, err := json.Marshal(c)


nit: this seems ok since it's wholly contained by the driver implementation. I wonder if encoded url.Values might be a little more natural choice...

I would keep it as is unless you have a strong opinion to change to url.Values.

azdagron · 2024-02-21T23:29:42Z

pkg/server/datastore/sqldriver/awsrds/awsrds.go

+type sqlDriverWrapper struct {
+	sqlDriver    driver.Driver
+	tokenBuilder authTokenBuilder
+	tokensMap    tokens


Can Open() be called concurrently? Do we need to protect this with a mutex?

I went back and forth on this when I originally thought about this. I now think that to be in the safe side we should have a mutex.

azdagron · 2024-02-21T23:31:17Z

pkg/server/datastore/sqldriver/awsrds/awsrds.go

+	if cfg.Password != "" {
+		return "", errors.New("unexpected password in connection string for IAM authentication")
+	}
+	return fmt.Sprintf("%s password=%s", connString, password), nil


Should the password value be quoted? (e.g. %q)

Although we don't really expect spaces in an AWS auth token, it's a good point to consider.
Using %q wouldn't work here, as the value should be surrounded with single quotes, not double quotes.
To have an even more robust implementation, I'm thinking that I should escape the special characters ' and \ from this value.
I'll make the changes to enclose the value in single quotes and escape single quotes and backslashes. Thank you for bringing this up!

azdagron · 2024-02-21T23:32:00Z

pkg/server/datastore/sqldriver/awsrds/awsrds.go

+	return cfg.FormatDSN(), nil
+}
+
+func init() {


nit: init() functions should be moved to the top where we typically have other things that are initialized at package init time

azdagron · 2024-02-21T23:33:18Z

pkg/server/datastore/sqldriver/awsrds/awsrds_test.go

+	return nil, d.err
+}
+
+func init() {


init should ideally go towards the top of the file

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

azdagron

LGTM; one non-blocking comment.

azdagron · 2024-02-27T16:26:36Z

pkg/server/datastore/sqldriver/awsrds/awsrds.go

@@ -85,6 +94,9 @@ func (w *sqlDriverWrapper) Open(name string) (driver.Conn, error) {
 		return nil, fmt.Errorf("could not unmarshal configuration: %w", err)
 	}

+	w.tokensMapMtx.Lock()


I don't know if sqlDriver.Open can block for some time, but if so, maybe we can shorten the scope of this lock so it doesn't block other attempts?

It's a good idea to shorten the scope.

maxlambrecht

Looks good to me! 👍

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

azdagron

Thanks @amartinezfayo !

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

* Introduce support to IAM authentication in the datastore Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

Introduce support to IAM authentication in the datastore

eeb7925

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

amartinezfayo added this to the 1.9.0 milestone Jan 23, 2024

amartinezfayo requested review from evan2645, azdagron, MarcosDY and rturner3 as code owners January 23, 2024 16:21

maxlambrecht reviewed Jan 23, 2024

View reviewed changes

amartinezfayo added 3 commits January 25, 2024 15:22

Address PR comments

a965a96

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

Fix typo

ad2c9b6

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

Improving documentation

ec47ebc

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

rturner3 assigned azdagron Jan 30, 2024

evan2645 modified the milestones: 1.9.0, 1.9.1 Feb 15, 2024

amartinezfayo added 5 commits February 16, 2024 09:10

Merge remote-tracking branch 'origin' into datastore-aws-rds

f124f29

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

Test fixes

e670ea9

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

Test fixes

e8fe102

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

Test fixes

f8706c3

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

Test fixes

46bdddd

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

azdagron reviewed Feb 21, 2024

View reviewed changes

Address PR comments

83fc7bb

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

azdagron previously approved these changes Feb 27, 2024

View reviewed changes

maxlambrecht approved these changes Feb 27, 2024

View reviewed changes

Address PR comment

c060ce7

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

amartinezfayo dismissed azdagron’s stale review via c060ce7 February 27, 2024 17:29

Merge remote-tracking branch 'origin' into datastore-aws-rds

59fd757

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

azdagron previously approved these changes Feb 27, 2024

View reviewed changes

Fix conflicts

8b47607

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

amartinezfayo dismissed azdagron’s stale review via 8b47607 February 27, 2024 17:51

azdagron approved these changes Feb 27, 2024

View reviewed changes

amartinezfayo merged commit 49f8857 into spiffe:main Feb 27, 2024
32 checks passed

rushi47 pushed a commit to rushi47/spire that referenced this pull request Apr 11, 2024

Introduce support to IAM authentication in the datastore (spiffe#4828)

74af5bd

* Introduce support to IAM authentication in the datastore Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce support to IAM authentication in the datastore #4828

Introduce support to IAM authentication in the datastore #4828

amartinezfayo commented Jan 23, 2024 •

edited

maxlambrecht Jan 23, 2024

amartinezfayo Jan 25, 2024

marcofranssen Feb 28, 2024

amartinezfayo Feb 29, 2024

maxlambrecht Jan 23, 2024

maxlambrecht Jan 23, 2024

amartinezfayo Jan 24, 2024

maxlambrecht Jan 23, 2024

maxlambrecht Jan 23, 2024

maxlambrecht Jan 23, 2024

amartinezfayo Jan 25, 2024

maxlambrecht Jan 23, 2024

maxlambrecht Jan 23, 2024

amartinezfayo Jan 25, 2024

maxlambrecht Jan 23, 2024

maxlambrecht Jan 23, 2024

azdagron Feb 15, 2024

azdagron Feb 21, 2024

amartinezfayo Feb 23, 2024

azdagron Feb 21, 2024

amartinezfayo Feb 23, 2024

azdagron Feb 21, 2024

amartinezfayo Feb 23, 2024

azdagron Feb 21, 2024

amartinezfayo Feb 23, 2024

azdagron Feb 21, 2024

azdagron Feb 21, 2024

azdagron left a comment

azdagron Feb 27, 2024

amartinezfayo Feb 27, 2024

maxlambrecht left a comment

azdagron left a comment

	return nil, fmt.Errorf("could not get authorization token: %w", err)
	return nil, fmt.Errorf("could not get authentication token: %w", err)

	With IAM database authentication, an authentication token is used instead of a password when connecting to the database. For that reason, a password must not be provided in the connection string.
	AWS Identity and Access Management (IAM) authentication allows for secure authentication to databases hosted on AWS. Unlike traditional methods, it uses an authentication token instead of a password. When using IAM authentication, it is required to exclude the password from the connection string.


		With IAM database authentication, an authentication token is used instead of a password when connecting to the database. For that reason, a password must not be provided in the connection string.

		The `database_type` configuration can have additional settings when a database type supporting IAM authentication is used, and always takes the following form:

	The `database_type` configuration can have additional settings when a database type supporting IAM authentication is used, and always takes the following form:
	The database_type configuration allows specifying the type of database with IAM support. The configuration always follows this structure:


	Note: Replace `dbtype-with-iam-support` with the specific database type that supports IAM authentication.

	return nil, "", false, errors.New("invalid postgres config: no password should be provided when using IAM authentication")
	return nil, "", false, errors.New("invalid postgres configuration: password should not be set when using IAM authentication.")

	timeOut = time.Second * 30
	connectionTimeout = time.Second * 30

	return "", errors.New("password was provided in the connection string")
	return "", errors.New("unexpected password in connection string for IAM authentication")

Introduce support to IAM authentication in the datastore #4828

Introduce support to IAM authentication in the datastore #4828

Conversation

amartinezfayo commented Jan 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azdagron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxlambrecht left a comment

Choose a reason for hiding this comment

azdagron left a comment

Choose a reason for hiding this comment

amartinezfayo commented Jan 23, 2024 •

edited