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.

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 open_basedir issues by AWS SDK #32740

Merged
merged 2 commits into from
Sep 19, 2022
Merged

Fix open_basedir issues by AWS SDK #32740

merged 2 commits into from
Sep 19, 2022

Conversation

jasperweyne
Copy link
Contributor

This PR sets the 'use_aws_shared_config_files' option to false in the S3ConnectionTrait, in order to disable configuration loading from ~/.aws/config by the AWS SDK. It is a continuation of #27040, as that PR only changed the behaviour of the CredentialsProvider; this change affects the ConfigurationProvider as well, fully disabling any access to ~/.aws/config and solving all open_basedir issues. It intends to solve the issues stated in #23555.

This commit sets the 'use_aws_shared_config_files' option to false, in order to disable configuration loading from ~/.aws/config by the AWS SDK, specifically the S3Client. It is a continuation of #27040, as that PR only changed the behaviour of the CredentialsProvider; this change affects the ConfigurationProvider as well.

Signed-off-by: Jasper Weyne <[email protected]>
@jasperweyne
Copy link
Contributor Author

I see that the behat CI run fails, however I'm not exactly sure how this change affected those tests, as it should be relatively unrelated. Moreover, when running the test suite locally in docker, the same problem doesn't occur. Anyone any thoughts on this?

I feel like this might be a magic bug and a rerun of the CI pipeline will "fix" this. However, if I messed something up, please point me in the right direction and I will fix it!

@kesselb
Copy link
Contributor

kesselb commented Jun 7, 2022

Some of the integrations tests are also failing on master. Should be safe to ignore it.

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Makes sense. Failing test definitely seems unrelated 👍

@jasperweyne
Copy link
Contributor Author

@juliushaertl Thanks for the review! I see that you've added the Nextcloud 25 milestone, I assume this is standard for a PR to master? In any case, I'd like to note that this fix would be applicable to versions 22-24 as well and shouldn't introduce any merge conflicts, as the relevant portion of code hasn't changed since (looking at the stable22 branch).

@jasperweyne
Copy link
Contributor Author

@icewind1991 If you have any feedback, I'd love to hear it!

@jasperweyne
Copy link
Contributor Author

Hi @icewind1991 do you perhaps have a chance to review this? I'd like to avoid this PR becoming stale. If there's anything I can do to help, please let me know!

@jasperweyne
Copy link
Contributor Author

Hi @CarlSchwan , you've assigned the reviewers for this PR, but unfortunately @icewind1991 's review has been left open for more than two months now. I'm assuming he's unavailable to review this, can you perhaps assign another reviewer?

Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍 makes sense

@PVince81 PVince81 added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 11, 2022
@skjnldsv skjnldsv mentioned this pull request Aug 12, 2022
@skjnldsv skjnldsv mentioned this pull request Aug 18, 2022
This was referenced Aug 24, 2022
This was referenced Sep 6, 2022
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
Copy link
Member

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

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

Ci failure unrelated

@CarlSchwan CarlSchwan merged commit 3950deb into nextcloud:master Sep 19, 2022
@welcome
Copy link

welcome bot commented Sep 19, 2022

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@CarlSchwan
Copy link
Member

/backport to stable24

@CarlSchwan
Copy link
Member

/backport to stable23

@backportbot-nextcloud
Copy link

The backport to stable23 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable24 failed. Please do this backport manually.

@jasperweyne jasperweyne deleted the patch-2 branch November 8, 2022 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug feature: object storage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants