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

Require HTTPS #20

Merged
merged 2 commits into from
Feb 3, 2025
Merged

Require HTTPS #20

merged 2 commits into from
Feb 3, 2025

Conversation

RealOrangeOne
Copy link
Member

HTTPS is now required for API access, too.

Sadly, there's no way to have S3 redirect to HTTPS instead. Chances are, S3 is running behind some kind of proxy for serving files, which itself would handle the redirect.

@RealOrangeOne RealOrangeOne force-pushed the require-https branch 2 times, most recently from 6adac89 to ad4ae92 Compare January 6, 2025 14:46
HTTPS is now required for API access, too
@tomusher
Copy link
Member

This looks good in theory to me. The only doubt I have is whether this tool needs to support the 'No' option. It adds more steps to the setup and is not something I can see Torchbox (as the main package users) needing much.
I'd lean towards making it a default without the prompt - but happy if you want to merge as-is.

@RealOrangeOne
Copy link
Member Author

whether this tool needs to support the 'No' option

I think it needs to. The HTTPS handling doesn't do a redirect, which means for sites which use the S3 URL directly may not work. I'm not sure if the S3 does HSTS preload for its URLs, but it feels like too big of a risk.

@RealOrangeOne
Copy link
Member Author

From discussion, I've enforced HTTPS. Most users should be using HTTPS, and if a user isn't, that's a problem in itself worthy of investigating.

@tomusher
Copy link
Member

Looks good to me!

@RealOrangeOne RealOrangeOne changed the title Allow requiring HTTPS for bucket access Require HTTPS Feb 3, 2025
@RealOrangeOne RealOrangeOne merged commit 6760afd into master Feb 3, 2025
1 check passed
@RealOrangeOne RealOrangeOne deleted the require-https branch February 3, 2025 11:18
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