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

AmazonS3InstallationService should accept aws credential in constructor #1165

Open
smallufo opened this issue Jun 8, 2023 · 3 comments · May be fixed by #1168
Open

AmazonS3InstallationService should accept aws credential in constructor #1165

smallufo opened this issue Jun 8, 2023 · 3 comments · May be fixed by #1168
Labels
enhancement M-T: A feature request for new functionality good first issue
Milestone
1.x

Comments

@smallufo
Copy link

smallufo commented Jun 8, 2023

https://slack.dev/java-slack-sdk/guides/app-distribution

// The standard AWS env variables are expected
// export AWS_REGION=us-east-1
// export AWS_ACCESS_KEY_ID=AAAA*************
// export AWS_SECRET_ACCESS_KEY=4o7***********************

// Please be careful about the security policies on this bucket.
String awsS3BucketName = "YOUR_OWN_BUCKET_NAME_HERE";

InstallationService installationService = new AmazonS3InstallationService(awsS3BucketName);

I realize you utilize DefaultAWSCredentialsProviderChain.getInstance().getCredentials() to retrieve credentials from env. But it truly sucks (lacks flexibility)

You should provide a constructor that accepts these 3 parameters in constructor : awsAccessKey , awsSecretKey , region , and build AWS credential in the constructor.
@filmaj filmaj added enhancement M-T: A feature request for new functionality good first issue and removed untriaged labels Jun 9, 2023
@filmaj
Copy link
Contributor

filmaj commented Jun 9, 2023

Thanks for the feature suggestion! We would love a pull request for this addition if you think it is something you can contribute 😄

@seratch seratch added this to the 1.x milestone Jun 10, 2023
@smallufo smallufo linked a pull request Jun 10, 2023 that will close this issue
Open
6 tasks
@smallufo
Copy link
Author

BTW , it seems AWS_REGION , AWS_ACCESS_KEY_ID , AWS_SECRET_ACCESS_KEY is not sufficient.
I set 3 values , and I see error message :

2023-06-11 18:22:52.407 [http-nio-8080-exec-1] WARN  c.a.i.InstanceMetadataServiceResourceFetcher - Fail to retrieve token 
com.amazonaws.SdkClientException: Failed to connect to service endpoint: 
	at com.amazonaws.internal.EC2ResourceFetcher.doReadResource(EC2ResourceFetcher.java:112)
	at com.amazonaws.internal.InstanceMetadataServiceResourceFetcher.getToken(InstanceMetadataServiceResourceFetcher.java:91)
	at com.amazonaws.internal.InstanceMetadataServiceResourceFetcher.readResource(InstanceMetadataServiceResourceFetcher.java:69)
	at com.amazonaws.internal.EC2ResourceFetcher.readResource(EC2ResourceFetcher.java:66)
	at com.amazonaws.util.EC2MetadataUtils.getItems(EC2MetadataUtils.java:407)
	at com.amazonaws.util.EC2MetadataUtils.getData(EC2MetadataUtils.java:376)
	at com.amazonaws.util.EC2MetadataUtils.getData(EC2MetadataUtils.java:372)
	at com.amazonaws.util.EC2MetadataUtils.getEC2InstanceRegion(EC2MetadataUtils.java:287)
	at com.amazonaws.regions.InstanceMetadataRegionProvider.tryDetectRegion(InstanceMetadataRegionProvider.java:59)
	at com.amazonaws.regions.InstanceMetadataRegionProvider.getRegion(InstanceMetadataRegionProvider.java:50)
	at com.amazonaws.regions.AwsRegionProviderChain.getRegion(AwsRegionProviderChain.java:46)
	at com.amazonaws.client.builder.AwsClientBuilder.determineRegionFromRegionProvider(AwsClientBuilder.java:475)
	at com.amazonaws.client.builder.AwsClientBuilder.setRegion(AwsClientBuilder.java:458)
	at com.amazonaws.client.builder.AwsClientBuilder.configureMutableProperties(AwsClientBuilder.java:424)
	at com.amazonaws.client.builder.AwsSyncClientBuilder.build(AwsSyncClientBuilder.java:46)
	at com.amazonaws.services.s3.AmazonS3ClientBuilder.defaultClient(AmazonS3ClientBuilder.java:54)
	at com.slack.api.bolt.service.builtin.AmazonS3OAuthStateService.createS3Client(AmazonS3OAuthStateService.java:91)
	at com.slack.api.bolt.service.builtin.AmazonS3OAuthStateService.lambda$initializer$0(AmazonS3OAuthStateService.java:43)
	at com.slack.api.bolt.App.initialize(App.java:544)
	at com.slack.api.bolt.App.start(App.java:557)
	at com.slack.api.bolt.App.run(App.java:588)

After digging into s3's code , it seems it needs additional 3 system properties : aws.region , aws.accessKeyId , aws.secretKey
( see SystemPropertiesCredentialsProvider : https://github.com/aws/aws-sdk-java/blob/master/aws-java-sdk-core/src/main/java/com/amazonaws/auth/SystemPropertiesCredentialsProvider.java )

Anyway , I don't suggest using DefaultAWSCredentialsProviderChain as it assumes your server is running on AWS's image.

After I set the 3 values : aws.region , aws.accessKeyId , aws.secretKey . When event comes , bolt output :

INFO  c.s.a.b.m.b.MultiTeamsAuthorization - auth.test result: null, io error: null, api error: null

No stacktrace :(
And I see no folder created under my s3 bucket .

I think maybe the AWS storage didn't run test outside AWS's image ?

While using FileInstallationService(config, "/tmp") , it correctly creates folders under /tmp/xxx

/tmp/xxx
total 0
drwxr-xr-x   4 smallufo  wheel  128  6 11 00:29 .
drwxrwxrwt  15 root      wheel  480  6 11 17:02 ..
drwxr-xr-x   4 smallufo  wheel  128  6 10 20:26 installation
drwxr-xr-x   6 smallufo  wheel  192  6 11 17:29 state

@seratch
Copy link
Member

seratch commented Jun 11, 2023 •
edited

Hey @smallufo

BTW , it seems AWS_REGION , AWS_ACCESS_KEY_ID , AWS_SECRET_ACCESS_KEY is not sufficient.

I don't think so. At least, this is not always true. From my past experience, the default resolver works very well with those env variables. As long as you provide sufficient env variables, Java AWS SDK works for you without any issues.

I don't have time to discuss details right now, but I recommend checking https://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/auth/DefaultAWSCredentialsProviderChain.html The default credential resolver tries to read env variables first, then Java system properties, and so on. Also, I believe that it's the best recommended way to resolve credentials on AWS infra (and even on your local machine).

No stacktrace :( And I see no folder created under my s3 bucket .

What about enabling slf4j debug-level logging? The debug logs by this SDK and AWS SDK may provide more detailed information, which can be helpful for you.

Lastly, as for your PR to add a new constructor to add a new way to initialize the credentials, I am open to add such in addition to the current default one. But I still don't agree the current approach is not recommended. I will check your PR next week. Thanks again for your inputs and contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement M-T: A feature request for new functionality good first issue
Projects
None yet
Milestone
1.x
Development

Successfully merging a pull request may close this issue.

AmazonS3InstallationService add constructor smallufo/java-slack-sdk
3 participants
@seratch @filmaj @smallufo