slackapi / java-slack-sdk

Slack Developer Kit (including Bolt for Java) for any JVM language
https://slack.dev/java-slack-sdk/
MIT License
573 stars 214 forks source link

AmazonS3InstallationService should accept aws credential in constructor #1165

Open smallufo opened 1 year ago

smallufo commented 1 year ago

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 commented 1 year ago

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

smallufo commented 1 year ago

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 commented 1 year ago

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.