fix: allow downloading S3 files from MinIO over http #570
Conversation
get_s3.go
Outdated
| // If it's not an AWS domain, set the custom endpoint | ||
| if !isAWSDomain { | ||
| opts.BaseEndpoint = aws.String("https://" + url.Host) | ||
| opts.BaseEndpoint = aws.String(url.Scheme + "://" + url.Host) |
There was a problem hiding this comment.
Just a small edge-case thought: url.Scheme should always be present for non-AWS S3 endpoints, but if anything upstream ever passes a malformed URL (e.g., s3::127.0.0.1:9000/... with no scheme), this would produce an invalid endpoint like ://host.
Not blocking, but adding a tiny fallback would make this more robust without changing existing behavior will be a better option. Let me know your thoughts. (I'm just Thinking around a defensive improvement)
There was a problem hiding this comment.
Would you be open to adding a small test for this?
Right now this logic is untested, which might make it easy to regress later.
There was a problem hiding this comment.
I have added verification on empty scheme with fallback to https.
I'm not very clear on the extent of the test; ideally the entire newS3Client() should be tested.
3 participants
Allow downloading files from S3 over custom HTTP endpoints like MinIO.
Without this the download fails with:
Fixes #569
PCI review checklist
I have documented a clear reason for, and description of, the change I am making.
If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
If applicable, I've documented the impact of any changes to security controls.