Added 'allow_redirects' parameter in perform_request function for RequestsHttpConnection

[saimedhi]

Description

Fixed CVE-2023-32681 linked to Requests

Issues Resolved

fixes GHSA-j8r2-6x86-q33q

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[codecov]

Codecov Report

Merging #401 (86d21a2) into main (dafc966) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #401   +/-   ##
=======================================
  Coverage   72.12%   72.12%           
=======================================
  Files          77       77           
  Lines        7190     7190           
=======================================
  Hits         5186     5186           
  Misses       2004     2004           

Impacted Files	Coverage Δ
opensearchpy/connection/http_requests.py	86.36% <100.00%> (ø)

[harshavamsi]

Thanks @saimedhi for this change. Can you write a simple test on the prepare_request method mocking it and trying to do a redirect and failing(since allow_redirects) is false. This gives us confidence in this change working.

[saimedhi]

PR ready for review

[dblock]

Thanks! Looks like this adds an allow_redirects option, aka it's a new feature?

It also changes the existing behavior, which would be a breaking change for users, isn't it? We should discuss making it. Is that CVE critical enough to warrant a backwards incompatible change?

0. Assuming the CVE is not critical/high, I would make this PR without breaking backwards compatibility, aka keep allow_redirects=True.
1. Instead of calling out the CVE in CHANGELOG let's call out the new feature.
2. Document the new feature in user guide.

[saimedhi]

Hello @dblock, Are you recommending that we need to capture 3xx response codes and ensure a new request is made to the redirect destination. This prevents breaking change for users currently relying on redirect behaviors. I agree with your suggestion and will implement it.

[dblock]

Maybe, but I also think that you can start by turning this PR into just adding allow_redirects and defaulting it to True. Users that care about this CVE could set it to False understanding the consequences. We would merge that because it's just a new option, doesn't break anything and helps users fix their problem themselves.

Then, as a separate PR, we can discuss the vulnerability and how we can either switch that option to False by default or implement something that can keep it at True and fix the vulnerability.

[wbeckler]

If we merge it, it's important that we don't leave the impression that we "fixed" the CVE. That requires a default behavior of no auto-redirect. I'd be okay with 2 merges or 1 big one, but we need to get to the point where we default to the safe code path either way. So we'll need to keep moving toward the catching of the 3xx and the redirect from there.