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.

Sign up for GitHub

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

Jump to bottom

Add ipv6 support to should_bypass_proxies #5953

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add ipv6 support to should_bypass_proxies #5953

wants to merge 1 commit into from

Conversation

derekhiggins
Copy link

Add support to should_bypass_proxies to support
IPv6 ipaddresses and CIDRs in no_proxy. Includes
adding IPv6 support to various other helper functions.

dtantsur
dtantsur approved these changes Oct 11, 2021
View reviewed changes
requests/utils.py Outdated Show resolved Hide resolved
derekhiggins
derekhiggins commented Oct 18, 2021
View reviewed changes
requests/utils.py Outdated
netmask = struct.unpack('=L', socket.inet_aton(dotted_netmask(int(bits))))[0]
network = struct.unpack('=L', socket.inet_aton(netaddr))[0] & netmask
if is_ipv4_address(ip) and is_ipv4_address(netaddr):
ipaddr = struct.unpack('>L', socket.inet_aton(ip))[0]
Copy link
Author

Choose a reason for hiding this comment

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

I've tested this switch to 'big-endian' from 'native' on both big and little endian systems. Unit tests passed on both.

@derekhiggins derekhiggins mentioned this pull request Dec 21, 2022
Open
@derekhiggins
Copy link
Author

Hi @nateprewitt @sethmlarson , is this something you'd consider merging ?

@melewitz
Copy link

I'm facing this same issue. @nateprewitt @sethmlarson Any update on your thoughts?

@derekhiggins derekhiggins force-pushed the ipv6-noproxy branch 2 times, most recently from f2e8dc6 to 4f89a66 Compare September 8, 2023 14:38
@novacain1
Copy link

Running into the same problem with this, it sure would be nice to have the ability to specify IPv6 CIDRs in no_proxy. What's holding this up?

Ousret added a commit to jawah/niquests that referenced this pull request Oct 16, 2023
@Ousret
🔖 Release 3.1.2
77fa29b 
**Fixed**
- Static type checker not accepting **list\[str\]** in values for argument **data**.

**Misc**
- Changed the documentation theme by **furo**.

**Added**
- IPv6 support in the `NO_PROXY` environment variable or in the **proxies** (key no_proxy) argument.
  Patch taken from idle upstream PR psf#5953
- Preemptively register a website to be HTTP/3 capable prior to the first TLS over TCP handshake.
  You can do so by doing like:

  ```python
  from niquests import Session

  s = Session()
  s.quic_cache_layer.add_domain("cloudflare.com")
  ```
- Passed **data** will be converted to form-data if headers have a Content-Type header and is set to `multipart/form-data`.
  Otherwise, by default, it is still urlencoded. If you specified a boundary, it will be used, otherwise, a random one will
  be generated.
@Ousret Ousret mentioned this pull request Oct 16, 2023
Merged
Ousret added a commit to jawah/niquests that referenced this pull request Oct 16, 2023
@Ousret
🔖 Release 3.1.2
1d8561f 
**Fixed**
- Static type checker not accepting **list\[str\]** in values for argument **data**.

**Misc**
- Changed the documentation theme by **furo**.

**Added**
- IPv6 support in the `NO_PROXY` environment variable or in the **proxies** (key no_proxy) argument.
  Patch taken from idle upstream PR psf#5953
- Preemptively register a website to be HTTP/3 capable prior to the first TLS over TCP handshake.
  You can do so by doing like:

  ```python
  from niquests import Session

  s = Session()
  s.quic_cache_layer.add_domain("cloudflare.com")
  ```
- Passed **data** will be converted to form-data if headers have a Content-Type header and is set to `multipart/form-data`.
  Otherwise, by default, it is still urlencoded. If you specified a boundary, it will be used, otherwise, a random one will
  be generated.
Ousret added a commit to jawah/niquests that referenced this pull request Oct 16, 2023
@Ousret
🔖 Release 3.1.2
0cc9b8a 
**Fixed**
- Static type checker not accepting **list\[str\]** in values for argument **data**.

**Misc**
- Changed the documentation theme by **furo**.

**Added**
- IPv6 support in the `NO_PROXY` environment variable or in the **proxies** (key no_proxy) argument.
  Patch taken from idle upstream PR psf#5953
- Preemptively register a website to be HTTP/3 capable prior to the first TLS over TCP handshake.
  You can do so by doing like:

  ```python
  from niquests import Session

  s = Session()
  s.quic_cache_layer.add_domain("cloudflare.com")
  ```
- Passed **data** will be converted to form-data if headers have a Content-Type header and is set to `multipart/form-data`.
  Otherwise, by default, it is still urlencoded. If you specified a boundary, it will be used, otherwise, a random one will
  be generated.
Ousret added a commit to jawah/niquests that referenced this pull request Oct 16, 2023
@Ousret
🔖 Release 3.1.2 (#37)
5cb7c7b 
**Fixed**
- Static type checker not accepting **list\[str\]** in values for argument **data**.

**Misc**
- Changed the documentation theme by **furo**.

**Added**
- IPv6 support in the `NO_PROXY` environment variable or in the **proxies** (key no_proxy) argument.
  Patch taken from idle upstream PR psf#5953
- Preemptively register a website to be HTTP/3 capable prior to the first TLS over TCP handshake.
  You can do so by doing like:

  ```python
  from niquests import Session

  s = Session()
  s.quic_cache_layer.add_domain("cloudflare.com")
  ```
- Passed **data** will be converted to form-data if headers have a Content-Type header and is set to `multipart/form-data`.
  Otherwise, by default, it is still urlencoded. If you specified a boundary, it will be used, otherwise, a random one will
  be generated.
@derekhiggins
Copy link
Author

derekhiggins commented Nov 28, 2023 •
edited
Loading

@nateprewitt @sigmavirus24 @sethmlarson would you have a chance to look at this please, its been hanging around for over 2 years and still causing me problems.

@frenzymadness
Copy link
Contributor

It'd be nice to have this implemented. Is there any way I can help?

@derekhiggins
Copy link
Author

Hi @frenzymadness , if you can review or confirm that this works for your use case then that would be appreciated but
ultimately this PR has been waiting on reviews from one of the maintainers I'm not sure how best to get it some traction besides pinging them on here.

frenzymadness
frenzymadness reviewed Dec 20, 2023
View reviewed changes
Copy link
Contributor

@frenzymadness frenzymadness left a comment

Choose a reason for hiding this comment

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

I checked the code and it looks good to me. I'm looking for more ways to test it.

src/requests/utils.py
)
network = (network_msb << 64) ^ network_lsb
else:
return False
Copy link
Contributor

Choose a reason for hiding this comment

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

Is returning False the correct thing to do when this function is called with IPv4 IP and IPv6 network or vice versa? What about raising an exception here?

Copy link
Author

Choose a reason for hiding this comment

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

I think this is fine as is (i.e. returning false), the function is used to test if a IP address is a member of any of the CIDR's listed in no_proxy, having a no_proxy with mixed IPv4 and IPv6 CIDR's would be valid, I don't think I'd expect an exception to the raised in this instance.

@frenzymadness
Copy link
Contributor

We are going to test this change internally to get some confidence.

@frenzymadness
Copy link
Contributor

Derek tested the package in OpenShift 4.15 and the functionality proposed here works as expected. Is there any chance to merge this?

@frenzymadness
Copy link
Contributor

During additional testing, my colleague raised an interesting question: Are ::/0 and 0.0.0.0/0 valid values for no_proxy? The current implementation for IPv4 does not allow masks to be lower than 1 and the new implementation for IPv6 does the same.

I see a use-case for ::/0 - let's say I have a proxy running on both protocols but I want it to be used only for IPv4 - then it might make sense to use NO_PROXY for all IPv6 addresses. What do you think?

@jadhaj jadhaj mentioned this pull request Feb 21, 2024
Closed
@frenzymadness
Copy link
Contributor

What can I do to get this merged? We've reviewed the change and tested it in a real environment with real use cases. Is there anything else we can do?

@derekhiggins
Add ipv6 support to should_bypass_proxies
bc8624b 
Add support to should_bypass_proxies to support
IPv6 ipaddresses and CIDRs in no_proxy. Includes
adding IPv6 support to various other helper functions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frenzymadness frenzymadness frenzymadness left review comments

@dtantsur dtantsur dtantsur approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@derekhiggins @melewitz @novacain1 @frenzymadness @dtantsur