-
Notifications
You must be signed in to change notification settings - Fork 599
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.
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
fix: enforce user must specify access_key and secret_key using aws auth #11120
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we reject it in fe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But might be ok as a quick fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still discussing with @arkbriar, whether we should continuously support public buckets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still discussing with @arkbriar, whether we should continuously support public buckets.
Prefer to keep it. It's up to user's choice, not our fault.
@@ -127,7 +123,7 @@ impl AwsAuthProps { | |||
pub async fn build_config(&self) -> anyhow::Result<SdkConfig> { | |||
let region = self.build_region().await?; | |||
let credentials_provider = self | |||
.with_role_provider(self.build_credential_provider().await?) | |||
.with_role_provider(self.build_credential_provider()?) | |||
.await?; | |||
let config_loader = aws_config::from_env() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should change this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the problem is about finding an alternative for from_env
.
Important: Using the aws-config crate to configure the SDK is preferred to invoking this builder directly. Using this builder directly won’t pull in any AWS recommended default configuration values.
---- from doc
It is not recommended to build SdkConfig
directly.
If the bucket is Publicly accessible, can we don't provide AK/SK? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still discussing with @arkbriar, whether we should continuously support public buckets.
Prefer to keep it. It's up to user's choice, not our fault.
Just change to an empty string as default. We won't use our env var any more. |
Well... Okay if that's too difficult, I can accept it |
Codecov Report
@@ Coverage Diff @@
## main #11120 +/- ##
=======================================
Coverage 69.92% 69.92%
=======================================
Files 1312 1312
Lines 223351 223347 -4
=======================================
- Hits 156171 156170 -1
+ Misses 67180 67177 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
resolve #11086
Checklist
./risedev check
(or alias,./risedev c
)Documentation
Click here for Documentation
Types of user-facing changes
Please keep the types that apply to your changes, and remove the others.
Release note
access_key and corresponding secret_key become a must for all aws auth components.