Fix to use default credential providers #3193
Conversation
| AWS_ACCESS_KEY_ID: ${{ secrets.S3_ACCESS_KEY }} | ||
| AWS_SECRET_ACCESS_KEY: ${{ secrets.S3_SECRET_ACCESS_KEY }} |
There was a problem hiding this comment.
The provider checks these environment variables.
| - name: Prepare Google Cloud Credentials | ||
| run: | | ||
| echo '${{ env.CLOUD_STORAGE_SERVICE_ACCOUNT_KEY }}' > $HOME/gcloud_service_account.json | ||
| export GOOGLE_APPLICATION_CREDENTIALS=$HOME/gcloud_service_account.json | ||
There was a problem hiding this comment.
The provider checks GOOGLE_APPLICATION_CREDENTIALS to get the path to the key.
I will replace this part with more secure ways when I have the chance.
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly updates the S3 and Google Cloud Storage adapters to use their respective default credential providers, which is a great security enhancement. The changes are well-implemented across the configuration, wrapper, and test files, removing the need to manage credentials directly. The code is clean and the changes are consistent with the PR's objective.
core/src/main/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageWrapper.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This PR enhances security by migrating S3 and Google Cloud Storage adapters to use default credential providers instead of explicit username/password credentials. The changes remove hardcoded credential handling in favor of environment-based authentication mechanisms provided by AWS SDK and Google Cloud SDK.
- Removed explicit credential configuration from S3 and Cloud Storage adapters
- Updated the CI/CD workflow to use Application Default Credentials for Google Cloud
- Cleaned up configuration classes and interfaces by removing credential-related methods
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/object-storage-adapter-check.yaml | Added setup for Google Cloud credentials file and removed password parameter from integration test |
| core/src/main/java/com/scalar/db/common/CoreError.java | Removed Cloud Storage service account key error codes that are no longer needed |
| core/src/main/java/com/scalar/db/storage/objectstorage/ObjectStorageConfig.java | Removed getPassword() method from interface as it's no longer common to all implementations |
| core/src/main/java/com/scalar/db/storage/objectstorage/blobstorage/BlobStorageConfig.java | Changed getPassword() from interface override to regular public method (still needed for Azure) |
| core/src/main/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageConfig.java | Removed password field, getPassword() method, and getCredentials() method |
| core/src/main/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageWrapper.java | Removed explicit credential provider configuration, now uses Application Default Credentials |
| core/src/main/java/com/scalar/db/storage/objectstorage/s3/S3Config.java | Removed username and password fields and their getter methods |
| core/src/main/java/com/scalar/db/storage/objectstorage/s3/S3Wrapper.java | Removed static credential provider configuration, now uses AWS default credential chain |
| core/src/test/java/com/scalar/db/storage/objectstorage/cloudstorage/CloudStorageConfigTest.java | Removed assertions for password field that no longer exists |
| core/src/test/java/com/scalar/db/storage/objectstorage/s3/S3ConfigTest.java | Removed assertions for username and password fields that no longer exist |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
KodaiD
requested review from
a team,
Torch3333,
brfrn169,
feeblefakie and
komamitsu
and removed request for
a team
| - name: Setup Gradle | ||
| uses: gradle/actions/setup-gradle@v5 | ||
|
|
||
| - name: Prepare Google Cloud Credentials |
There was a problem hiding this comment.
It's a question for the following.
I'm not very familiar with IAM role, is it still regarded as using a secret key, and IAM roles are not used?
https://github.com/scalar-labs/scalardb/pull/3193/files#diff-11fd3465a765a999577e6b617ec3aafeab4b8690954bd0b8504236ff35ba98f6R43-R44
I'm asking because we should use IAM roles since using secret keys is not recommended in AWS.
https://docs.aws.amazon.com/IAM/latest/UserGuide/id_credentials_access-keys.html
There was a problem hiding this comment.
It's for Google Cloud, not for AWS. As written in here, we should use a more secure way, like workload identity, to authenticate from outside Google Cloud. I'll address it after the release.
There was a problem hiding this comment.
The main goal of this PR is to allow users to choose authentication without using secret access keys or service account keys. Securing CI will be addressed in a separate PR.
| .region(Region.of(config.getRegion())) | ||
| .credentialsProvider( | ||
| StaticCredentialsProvider.create( | ||
| AwsBasicCredentials.create(config.getUsername(), config.getPassword()))) |
There was a problem hiding this comment.
So, does it change the way to deploy ScalarDB in case the underlying databases include S3?
(If so, do we need to change the Helm chart?)
There was a problem hiding this comment.
I don't think we need to change the Helm chart. This change just eliminates the need to enter credentials in a custom values file. ScalarDB Cluster users can assign an IAM Role to the Kubernetes service account by using EKS Pod Identity.
There was a problem hiding this comment.
So, do we need a particular section for configuring credentials for S3?
The current doc basically passes passwords to Cluster through K8s secrets.
There was a problem hiding this comment.
Yes, we do. I'll prepare it.
There was a problem hiding this comment.
@feeblefakie The current Helm chart has a feature that mounts the service account. So, we don't need to update the Helm chart. But users have to specify those values.
|
Sorry reviewers, since we should do more verifications, I'm closing this PR. I will address this after the release. |
2 participants
Description
This PR updates the Object Storage adapter to use default credentials providers, which are more secure.
Related issues and/or PRs
Changes made
Checklist
Additional notes (optional)
N/A
Release notes
N/A