Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add support for AWS IAM authentication for postgres #1858

Merged
merged 5 commits into from
Apr 16, 2024
Merged

add support for AWS IAM authentication for postgres #1858

merged 5 commits into from
Apr 16, 2024

Conversation

j-white
Copy link
Contributor

@j-white j-white commented Apr 7, 2024

Here we add support for AWS IAM authentication for the postgres datastore

This fixes #659

Verified this works in two different cases:

  1. Tested the datastore migrate head command via shell with SSO authentication
  2. Tested the serve command on ECS (Fargate) with a task role that has the rds-db:connect permission

In each case we're able to load the AWS credentials, generate an IAM token for the DB and authenticate successfully

@j-white j-white requested a review from a team as a code owner April 7, 2024 00:24
@github-actions github-actions bot added area/CLI Affects the command line area/datastore Affects the storage system area/dependencies Affects dependencies labels Apr 7, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 7, 2024 •
edited
Loading

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@j-white
Copy link
Contributor Author

j-white commented Apr 7, 2024

I have read the CLA Document and I hereby sign the CLA

@j-white
Copy link
Contributor Author

j-white commented Apr 7, 2024

recheck

josephschorr
josephschorr reviewed Apr 7, 2024
View reviewed changes
internal/datastore/postgres/migrations/driver.go Outdated Show resolved Hide resolved
internal/datastore/postgres/migrations/driver.go Outdated Show resolved Hide resolved
pkg/cmd/migrate.go Outdated Show resolved Hide resolved
@j-white
Copy link
Contributor Author

j-white commented Apr 8, 2024 •
edited
Loading

Thanks for the prompt feedback @josephschorr! I hid the AWS details behind the CredentialsProvider interface now.

Can you let me know how this looks?

@github-actions github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Apr 11, 2024
@vroldanbet
Copy link
Contributor

@j-white please run mage lint locally to help you out with those linter errors

@j-white
Copy link
Contributor Author

j-white commented Apr 12, 2024

thanks @vroldanbet - de-lintified now

if the checks pass, I'll rebase and squash the commits

@vroldanbet
Copy link
Contributor

@j-white another one, sorry it's annoying, you have to go and run go mod tidy in the submodules

@j-white
Copy link
Contributor Author

j-white commented Apr 12, 2024

🤞

josephschorr
josephschorr reviewed Apr 13, 2024
View reviewed changes
internal/datastore/postgres/migrations/driver.go Show resolved Hide resolved
internal/testserver/datastore/postgres.go Outdated Show resolved Hide resolved
pkg/datastore/credentials.go Show resolved Hide resolved
pkg/datastore/credentials.go Outdated Show resolved Hide resolved
pkg/datastore/credentials.go Show resolved Hide resolved
pkg/datastore/credentials.go Outdated Show resolved Hide resolved
pkg/datastore/credentials_test.go Outdated Show resolved Hide resolved
@j-white
Copy link
Contributor Author

j-white commented Apr 13, 2024

thanks for the feedback, I'll work on adding support for MySQL next

@j-white
Copy link
Contributor Author

j-white commented Apr 13, 2024

thanks for the feedback, I'll work on adding support for MySQL next

MySQL support here: https://github.com/j-white/spicedb/compare/jw/aws-iam-auth...j-white:spicedb:jw/mysql-iam-auth?expand=1

Will open a separate PR once we get this one through.

@vroldanbet
Copy link
Contributor

@j-white sorry for the delayed review, it's been a slowish week with folks OoO, we'll get to this, thank you so much for the contribution!

josephschorr
josephschorr reviewed Apr 15, 2024
View reviewed changes
pkg/datastore/credentials.go Outdated Show resolved Hide resolved
pkg/datastore/credentials.go
Get(ctx context.Context, dbHostname string, dbPort uint16, dbUser string) (string, string, error)
}

var NoCredentialsProvider CredentialsProvider = nil
Copy link
Member

Choose a reason for hiding this comment

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

const (if it lets you)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no worky pkg/datastore/credentials.go:24:29: invalid constant type CredentialsProvider

pkg/datastore/credentials_test.go Outdated Show resolved Hide resolved
pkg/datastore/credentials.go Outdated Show resolved Hide resolved
@j-white
return -> returns
96d6342
@j-white
return an error for unknown credential providers
587fed6 
return the NoCredentialsProvider (aka nil) when given a empty string
j-white
j-white commented Apr 15, 2024
View reviewed changes
pkg/datastore/credentials.go Outdated
// NewCredentialsProvider create a new CredentialsProvider for the given name
// returns an error if no match is found, of if there is a problem creating the given CredentialsProvider
func NewCredentialsProvider(ctx context.Context, name string) (CredentialsProvider, error) {
if name == "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the default value for the cmd flag is "", so we treat this as a valid option and return the NoCredentialsProvider

I can add a comment to this effect, or take a different approach?

Copy link
Member

@josephschorr josephschorr Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ideally, the empty string is handled at the caller and NewCredentialsProvider is never called with an empty name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good, will go with that then

josephschorr
josephschorr approved these changes Apr 16, 2024
View reviewed changes
Copy link
Member

@josephschorr josephschorr left a comment

Choose a reason for hiding this comment

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

LGTM

@josephschorr josephschorr added this pull request to the merge queue Apr 16, 2024
Merged via the queue into authzed:main with commit 3d7871a Apr 16, 2024
22 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 16, 2024
@j-white j-white deleted the jw/aws-iam-auth branch April 16, 2024 19:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@josephschorr josephschorr josephschorr approved these changes

Assignees
No one assigned
Labels
area/CLI Affects the command line area/datastore Affects the storage system area/dependencies Affects dependencies area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support IAM database authentication for Postgres datastore
3 participants
@j-white @vroldanbet @josephschorr