Skip to content

fix: service path expected uri for signature to match. #815

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

re1709
Copy link

@re1709 re1709 commented May 23, 2025

Using authentication with a service path (e.g. under kubernetes deployment) does not work, as the expected uri is incorrect (contains the service path twice).

String uriForSigning = presignedUrl ? originalUri :
this.servicePath + originalUri;
String uriForSigning = presignedUrl ? uri :
this.servicePath + uri;
Copy link
Owner

Choose a reason for hiding this comment

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

I am happy to merge this but is there any kind of test that you can add that failed before and succeeds now?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry just tested this, and realised it breaks virtual host style bucket names - please don't merge.

Copy link
Author

@re1709 re1709 May 28, 2025

Choose a reason for hiding this comment

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

This is going to be more complicated than thought to fix, to work with path style and virtual host style buckets. We've now switched to use host style buckets and no longer need the service path - everything works great! If i find some time i'll try to correct this change to not break host style.

Would it be feasible to block setting service path and virtual host at the same time in the S3Proxy builder? The two properties seem to conflict. Could then reliably make a change that checks one of them to not break functionality for the other.

@re1709 re1709 marked this pull request as draft May 28, 2025 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants