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 treating ethernet and VPN as home network/for internal URL #4872

Merged
merged 8 commits into from
Dec 13, 2024

Conversation

jpelgrom
Copy link
Member

@jpelgrom jpelgrom commented Dec 3, 2024

Summary

Add options to also treat ethernet or VPN as home network (for internal URL, app lock, websocket push, BLE monitor). It reads the value from the system, not a specific app, so it matches when the status bar icon shows a <...> for ethernet or key for VPN (no additional details are available).

This should help with users who want internal URL + VPN, which I've seen multiple times on forums/Discord etc., and does not require location access for the app or location services to be on. Addresses:

The new values are nullable so we can change defaults and possibly introduce an inverse option in the future.

Screenshots

The 'home network WiFi SSID' screen is extended with the new options.

Light Dark
image image

The check for location access is moved to this screen, instead of before the screen, and will display inline next to the WiFi networks setting.

Light Dark
image image

Link to pull request in Documentation repository

I don't see how adding this advanced information to the basic setup page helps, probably fine to leave it as it is

Any other notes

For review purposes: the first two commits include the actual logic. The third commit includes UI.

 - Add the option (in code) to use internal url based on the current connection being ethernet or having a VPN, instead of specific SSIDs.
 - Make ethernet/VPN an additional reason to consider a network internal, not alternative to SSIDs. The values default to null so we can change the default later or introduce a 3-way switch.
 - Updates the SSID view UI to be the home network management UI
 - Update app lock/BLE monitor to check internal instead of home Wi-Fi SSID
…ator

# Conflicts:
#	common/src/main/res/values/strings.xml
Copy link
Member

@dshokouhi dshokouhi left a comment

Choose a reason for hiding this comment

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

Tested the APK and works well over here with Google VPN and also my home wifi configured. Great PR! Should make a lot of people happy I think :)

@marazmarci
Copy link
Contributor

marazmarci commented Dec 4, 2024

FYI there's a minor security concern associated to this feature in case of Ethernet connections*. I don't really have a feasible idea on how to address it, other than showing some kind of disclaimer / warning text for the users before allowing turning this on.

((But here's an unfeasible idea, which still isn't 100% secure: get the IP of the current default gateway's IP and based on that, query its MAC address, which is a unique (but spoofable) identifier of the router. This MAC address could act as the identifier for the network, similarly to the SSID in case of WiFi networks.))

*edit: it also applies to VPN connections, if there's no differentiation between different VPN providers / connections. I think there's also no reliable way to do it.

@marazmarci
Copy link
Contributor

marazmarci commented Dec 4, 2024

If only there would be an unauthenticated* endpoint on the HA server, through which it could prove its identity towards the app, before the app making an authenticated connection, then it could be solved pretty easily, I think. But I'm pretty sure there's no such endpoint, so we'd need it to be developed in the core repository...

*: so the app won't "leak" the session token too early

@jpelgrom
Copy link
Member Author

jpelgrom commented Dec 4, 2024

I was aware of that as I read the issues, but believe this feature is limited enough in scope (users will probably not use different ethernets/vpns) and 'advanced' enough for normal users to leave the settings at the default (off) that they are/should be aware.

But here's an unfeasible idea, which still isn't 100% secure

As I see it we could explore that for a v1.1, but that may increase the permissions required for the app.

If only there would be an unauthenticated* endpoint on the HA server, through which it could prove its identity towards the app

This would also apply to SSIDs. MITM is always a concern, so let's not introduce that discussion here.

@marazmarci
Copy link
Contributor

And what do you think of my suggestion about adding a warning text to the UI?

Something along these lines:

Warning: this could pose a security risk when using the device on multiple Ethernet networks / VPNs.

@jpelgrom
Copy link
Member Author

jpelgrom commented Dec 4, 2024

Your warning is very specific for http + internal connection, and "a security risk" doesn't tell users a lot, while this feature (home network) is more general. At the same time I want to avoid writing a whole paragraph of text. What do you think about:

Make sure to set up your home network correctly. Adding public Wi-Fi networks or using multiple ethernet/VPN connections may unintentionally expose information about or access to your app or server.

@marazmarci
Copy link
Contributor

Your warning is very specific for http + internal connection

Oh, yes indeed 🤦‍♂️

What do you think about:

Make sure to set up your home network correctly. Adding public Wi-Fi networks or using multiple ethernet/VPN connections may unintentionally expose information about or access to your app or server.

This is to the point, and concise. Well done, thanks!

 - Adds a warning based on PR comments about how incorrect home network settings can unintentionally expose information
 - Small refactor to allow for a different colored alert (info instead of warning)
@jpelgrom
Copy link
Member Author

jpelgrom commented Dec 4, 2024

Warning alert added as discussed:
image

@dshokouhi dshokouhi merged commit 7695bea into home-assistant:master Dec 13, 2024
4 checks passed
@jpelgrom jpelgrom deleted the internal-determinator branch December 13, 2024 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants