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

convert_domain_to_ip does not support ipv6, only ipv4 #573

Closed
mrchrisadams opened this issue Apr 10, 2024 · 0 comments · Fixed by #575
Closed

convert_domain_to_ip does not support ipv6, only ipv4 #573

mrchrisadams opened this issue Apr 10, 2024 · 0 comments · Fixed by #575
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@mrchrisadams
Copy link
Member

mrchrisadams commented Apr 10, 2024

We curently have a method in our platform that is responsible for converting domain names to ip address objects in python.

It's linked below:
https://github.com/thegreenwebfoundation/admin-portal/blob/master/apps/greencheck/domain_check.py#L128

The problem is that right now, it only supports ipv4, when it should support ipv6 too. the code looks like this:

    def convert_domain_to_ip(
        self, domain
    ) -> ipaddress.IPv4Network or ipaddress.IPv6Network or None:
        """
        Accepts a domain name or IP address, and returns an IPV4 or IPV6
        address
        """
        try:
            ip_string = socket.gethostbyname(domain)
            return ipaddress.ip_address(ip_string)
        except socket.gaierror as err:
            logger.warning(f"Unable to lookup domain: {domain} - error: {err}")

This also method right now unfortunately will return a None value when socket.gethostbyname(), which causes a problem later in the code path.

Really we should be using socket.getaddrinfo, which does work with ipv6, but instead of just returning a string representation of an ip address, returns a different object., which we'd need to change. the docs for python are below:

https://docs.python.org/3.12/library/socket.html#socket.getaddrinfo

We'd need convert_domain_to_ip to return a string representation of ipv4 addresses AND ipv6 addresses, which involves a bit of work to fetch the string out of the return value of socket.getaddrinfo.

When there is no IPv4 or IPv6 returned, we should probably raise an appropriate exception, so we can catch it early, rather than blindly passing None values along to the caller of the method.

What the code returns

I've put together a quick marimo notebook example of what the socket.getaddrinfo method returns, to give an idea of what datastructures any code would need to fetch the string representation of an IP address out of:

https://static.marimo.app/static/get-addrinfo-check-e861

This isn't mega complex, but it's likely a few more lines than we have in the file - the good news it doesn't rely require existing knowledge of the rest of the app, so presumably anyone with a bit of python experience could make a PR with an improved implementation of convert_domain_to_ip.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant