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

Update DNS configuration to favor Cisco AnyConnect adapters when connected and routeable adapters when not #2

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

Conversation

Omegaice
Copy link
Contributor

The current code looks for networks that have a route to 0.0.0.0/0 or ::/0 which only works if the vpn routes all traffic. This change updates the DNS selection code to always pick the DNS servers from the AnyConnect interface when it connects and any routeable adapters when not.

This works for the VPN setup that I have access to but I am not sure if it breaks anything outside of that.

…d routes if it exists and any internet routed adapters if not
Copy link
Owner

@jacob-pro jacob-pro left a comment

Choose a reason for hiding this comment

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

I only learnt the other day that I need to click the "finish review" button for these comments to show up!
i actually wrote these ages ago - sorry!

.split(|&c| c == 0)
.next()
.unwrap();
let description = String::from_utf16_lossy(&description_buffer);
Copy link
Owner

Choose a reason for hiding this comment

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

Please can you use this helper function I have already written here (from win32_utils)
https://github.com/jacob-pro/win32-utils/blob/master/src/str.rs#L19
I realise this isn't well documented (yet)

String::from_pwstr_lossy(adapter.Description)

IpAddr::V4(_) => route.interface_index == adapter.ipv4_interface_index,
IpAddr::V6(_) => route.interface_index == adapter.ipv6_interface_index,
})
})
Copy link
Owner

Choose a reason for hiding this comment

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

could this whole thing be simplified, so instead of making all the above changes, just changing this filter?

.filter(|adapter| {
    let is_internet = internet_routes
        .iter()
        .any(|route| match route.destination_prefix_ip {
            IpAddr::V4(_) => route.interface_index == adapter.ipv4_interface_index,
            IpAddr::V6(_) => route.interface_index == adapter.ipv6_interface_index,
        });
    return is_internet || adapter.description.contains("Cisco AnyConnect")
})

It would mean that the AnyConnect adapter is not excluded from the list if it is not an internet route. I would like the behaviour of this tool to remain as close as possible to the way that windows chooses DNS servers, which is based on interface metrics:

https://github.com/jacob-pro/wsl2-dns-agent/blob/master/docs/ROUTING.md#step-3---working-windows-dns

i.e. if your AnyConnect adapter is not the highest metric then Windows won't use it as the first DNS choice, and neither should WSL

@wp4nuv
Copy link

wp4nuv commented Jan 30, 2023

I found this project while researching a connectivity issue with WSL in Windows 11 where the WSL vEthernet adapter is hidden. I also use a VPN for work, but it's not Cisco-based. I would like to know if this change can be worked, so it uses ANY adapter with the lowest metric. Considering that VPNs usually want to be the default adapter, this change would make sense instead of looking for a particular adapter name.

@jacob-pro
Copy link
Owner

Hi @wp4nuv

Considering that VPNs usually want to be the default adapter, this change would make sense instead of looking for a particular adapter name.

I'm a bit confused, the existing code already just chooses the adapter with the lowest metric, like you are asking for?

.sorted_by_key(Adapter::interface_metric)

@wp4nuv
Copy link

wp4nuv commented Jan 31, 2023 via email

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