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

feat(file-s3): Add support for IAM role authentication to file-s3 provider #10528

Merged
merged 1 commit into from
Dec 11, 2024
Merged

feat(file-s3): Add support for IAM role authentication to file-s3 provider #10528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions packages/core/types/src/file/providers/s3.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
export interface S3FileServiceOptions {
file_url: string
access_key_id: string
secret_access_key: string
access_key_id?: string
secret_access_key?: string
authentication_method?: "access-key" | "s3-iam-role"
Copy link
Member Author

Choose a reason for hiding this comment

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

One open point we had with @sgirones is whether deciding the auth method should be an explicit action (it defaults to access-key), or implicit by the fact that the access key ID and secret access key are not passed.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

The current approach makes sense to me – enforcing a clear intent for auth seems reasonable

Copy link
Contributor

@sgirones sgirones Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

for more context: the AWS SDKs usually follow the standard node configuration, where they look for env vars and files in the standard paths.

Usually, you would do something like: AWS_ACCESS_KEY_ID=AKIAIOSFODNN7EXAMPLE AWS_SECRET_ACCESS_KEY=wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY npm run start

In this specific case, instead of s3-iam-role we should have something like from-env, otherwise the user may expect to be able to set the iam role through the config.

region: string
bucket: string
prefix?: string
Expand Down
2 changes: 1 addition & 1 deletion packages/modules/providers/file-s3/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
"license": "MIT",
"scripts": {
"test": "jest --passWithNoTests src",
"test:integration": "jest --forceExit -- integration-tests/**/__tests__/**/*.spec.ts",
"test:integration": "jest --forceExit -- integration-tests/__tests__/*.spec.ts",
"build": "rimraf dist && tsc --build ./tsconfig.json",
"watch": "tsc --watch"
},
Expand Down
34 changes: 26 additions & 8 deletions packages/modules/providers/file-s3/src/services/s3-file.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,10 @@ type InjectedDependencies = {
}

interface S3FileServiceConfig {
// TODO: We probably don't need this as either the service should return it or we should be able to calculate it.
fileUrl: string
accessKeyId: string
secretAccessKey: string
accessKeyId?: string
secretAccessKey?: string
authenticationMethod?: "access-key" | "s3-iam-role"
region: string
bucket: string
prefix?: string
Expand All @@ -36,7 +36,6 @@ interface S3FileServiceConfig {
additionalClientConfig?: Record<string, any>
}

// FUTURE: At one point we will probably need to support authenticating with IAM roles instead.
export class S3FileService extends AbstractFileProviderService {
static identifier = "s3"
protected config_: S3FileServiceConfig
Expand All @@ -46,10 +45,23 @@ export class S3FileService extends AbstractFileProviderService {
constructor({ logger }: InjectedDependencies, options: S3FileServiceOptions) {
super()

const authenticationMethod = options.authentication_method ?? "access-key"

if (
authenticationMethod === "access-key" &&
(!options.access_key_id || !options.secret_access_key)
) {
throw new MedusaError(
MedusaError.Types.INVALID_DATA,
`Access key ID and secret access key are required when using access key authentication`
)
}

this.config_ = {
fileUrl: options.file_url,
accessKeyId: options.access_key_id,
secretAccessKey: options.secret_access_key,
authenticationMethod: authenticationMethod,
region: options.region,
bucket: options.bucket,
prefix: options.prefix ?? "",
Expand All @@ -63,11 +75,17 @@ export class S3FileService extends AbstractFileProviderService {
}

protected getClient() {
// If none is provided, the SDK will use the default credentials provider chain, see https://docs.aws.amazon.com/cli/v1/userguide/cli-configure-envvars.html
const credentials =
this.config_.authenticationMethod === "access-key"
? {
accessKeyId: this.config_.accessKeyId!,
secretAccessKey: this.config_.secretAccessKey!,
}
: undefined

const config: S3ClientConfigType = {
credentials: {
accessKeyId: this.config_.accessKeyId,
secretAccessKey: this.config_.secretAccessKey,
},
credentials,
region: this.config_.region,
endpoint: this.config_.endpoint,
...this.config_.additionalClientConfig,
Expand Down
Loading