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

Allow using hosted Redis instances by default #293

Closed
wants to merge 5 commits into from

Conversation

schneems
Copy link
Contributor

Hosted Redis servers use self signed certificates (because they do not own the domain they're running on such as compute-1.amazonaws.com) therefore a full SSL connection verification will fail. The fix for that behavior is to disable verification so a self signed certificate can be used docs from a hosted Redis/Key-value store provider.

This PR makes the default behavior to allow connecting to self signed Redis servers, and introduces a new flag --strict-ssl which will require that the certificate is not self-signed.

This failing SSL connection behavior was originally reported in #292, however that issue focues on raising visibility of such failures.

@casperisfine casperisfine marked this pull request as ready for review November 13, 2024 07:37
@casperisfine
Copy link
Contributor

Sorry, but you'll have to sign the CLA...

As for the PR, I'm not even sure we need a flag. I can't really think of a security issue caused by CI queue being pointed at the wrong Redis, unless somehow the list of test name is super secret.

Copy link
Contributor

@ChrisBr ChrisBr left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution

@schneems
Copy link
Contributor Author

I have signed the CLA!

@schneems
Copy link
Contributor Author

I'm not even sure we need a flag.

I don’t feel strongly about it. I generally find changes to defaults easier to get merged if there’s some sort of opt out so I did what I thought would be the path of least resistance. I have no strong reason to force full SSL checking.

It sounds like you prefer fewer config options over greater configurability. Seems reasonable.

Please confirm and I can rip that out (or feel free to take the otherwise quite simple change and run with it if it’s faster/easier for you).

schneems added a commit to schneems/ci-queue that referenced this pull request Nov 19, 2024
Hosted Redis servers use self signed certificates (because they do not own the domain they're running on such as `compute-1.amazonaws.com`) therefore a full SSL connection verification will fail. The fix for that behavior is to disable verification so a self signed certificate can be used [docs from a hosted Redis/Key-value store provider](https://devcenter.heroku.com/articles/connecting-heroku-redis#connecting-in-ruby).

This PR makes the default behavior to allow connecting to self signed Redis servers. 

This failing SSL connection behavior was originally reported in Shopify#292, however that issue focuses on raising visibility of such failures.

This is an alternative to Shopify#293 that does not introduce a new configuration flag.
@schneems
Copy link
Contributor Author

I also opened #294 which doesn't include the configuration flags.

@ChrisBr
Copy link
Contributor

ChrisBr commented Dec 3, 2024

Superseded by #294

@ChrisBr ChrisBr closed this Dec 3, 2024
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.

3 participants