home-assistant / android

:iphone: Home Assistant Companion for Android
https://companion.home-assistant.io/
Apache License 2.0
2.3k stars 636 forks source link

Path support for saving HAAS URL on the initial, setup screen #1651

Open juliandroid opened 3 years ago

juliandroid commented 3 years ago

I have HASS setup with NGINX reverse proxy that works properly with URL like this: https://haas.domain.com/secretpath/

Basically NGINX converts that secretpath to HttpOnly Secure cookie that will be returned back with the response.

Since HASS don't support https://haas.domain.com/path/ installations, the very next request will be called directly to the https://haas.domain.com/ but since the cookie is provided by the browser the NGINX will forward the request correctly to the reverse proxy.

That works great in browser, so you only need once to open the https://haas.domain.com/secretpath/ and after that the cookie helps to keep the HASS secure.

The only problem is that when I want to use the Android application the path section is trimmed by the saveUrl method and would be great if the path section is also reconstructed by the Builder. If the path is provided as part of the URL then this setup would work correctly.

The benefit of having this setup is to mitigate the attacks that happens since the sub domain is exposed by the public DNS and all my public services typically face DoS attacks time to time. Hiding HAAS will help greatly from security perspective.

override suspend fun saveUrl(url: String, isInternal: Boolean?) {
        val trimUrl = if (url == "") null else try {
            val httpUrl = url.toHttpUrl()
            HttpUrl.Builder()
                .scheme(httpUrl.scheme)
                .host(httpUrl.host)
                .port(httpUrl.port)
                .toString()
        } catch (e: IllegalArgumentException) {
            throw MalformedHttpUrlException(
                e.message
            )
        }
        localStorage.putString(if (isInternal ?: isInternal()) PREF_LOCAL_URL else PREF_REMOTE_URL, trimUrl)
    }

Current NGINX PoC configuration looks like this:

# https server section below

server {
# ....
    set $secretKey shhtKeepItLow;

        location ~* ^/([^/]*)/? {
                set $secret $1;
                set $authenticated 0;
                if ($secret = $secretKey) {
                        set $authenticated 1;
                }
                if ($http_cookie ~* "shhtKeepItLow=1") {
                        set $authenticated 1;
                }
                if ($authenticated = 0) {
                        return 404;
                }

            rewrite /shhtKeepItLow/(.*) /$1 break;

            proxy_pass http://192.168.0.2:8123;    # my local HAAS instance
            proxy_set_header Host $host;
            proxy_redirect http:// https://;
            proxy_http_version 1.1;
            proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
            proxy_set_header Upgrade $http_upgrade;
            proxy_set_header Connection $connection_upgrade;
            add_header Set-Cookie "shhtKeepItLow=1; Path=/; HttpOnly; Secure";
        }
# ....
}
dshokouhi commented 3 years ago

So heres the main issue. HA itself does not support this URL with a path. What you have done is on top of HA. I think we may need HA to first support the path properly before we can continue here.

juliandroid commented 3 years ago

Indeed deployment with URL path is not implemented in HA and judging on the other tickets it seems a lot of work is need in order such path to be supported. I don't expect such thing to be implemented anytime soon.

From another hand NGINX reverse proxy deployment is quite standard it is seems kind a official, especially if you want to have TLS support. I guess having a reverse proxy is not strange with HA deployments.

The suggested change seems to be very small and a very good low hanging fruit based on the benefits of adding extra security - after all most of the home users don't really want their HA to be publicly available. Not sure what complications could happen if the path is attached for those who provide the URL with the path. It looks rather like just a sanity check, not really intended to prevent someone of adding path to the domain part.

dshokouhi commented 3 years ago

From another hand NGINX reverse proxy deployment is quite standard it is seems kind a official, especially if you want to have TLS support. I guess having a reverse proxy is not strange with HA deployments.

Its not supported and the user needs to be able to support themselves in these situations. Only really advanced users know how to set this, we see proxy issues often. By making this change I am sure there will be more proxy requests when those should really be handled by the HA server.

after all most of the home users don't really want their HA to be publicly available.

I don't agree many users actually use Nabu Casa to make it easily accessible externally, not to mention certain services require a publicly accessible URL.

Not sure what complications could happen if the path is attached for those who provide the URL with the path. It looks rather like just a sanity check, not really intended to prevent someone of adding path to the domain part.

We actually make modifications to the path in order to load a specific lovelace view so we do indeed modify it.

juliandroid commented 3 years ago

From another hand NGINX reverse proxy deployment is quite standard it is seems kind a official, especially if you want to have TLS support. I guess having a reverse proxy is not strange with HA deployments.

Its not supported and the user needs to be able to support themselves in these situations. Only really advanced users know how to set this, we see proxy issues often. By making this change I am sure there will be more proxy requests when those should really be handled by the HA server.

I was referring to the available UI where you can setup the trusted proxy, but I guess it was invented for other purposes. I actually don't need or want doing any changes to the request path - only to not have the restriction. At least any possible workaround? I was looking at the other tickets and the frustration of not having a way to protect your home device from prying eyes.

Not sure what complications could happen if the path is attached for those who provide the URL with the path. It looks rather like just a sanity check, not really intended to prevent someone of adding path to the domain part.

We actually make modifications to the path in order to load a specific lovelace view so we do indeed modify it.

Sure, my change in NGINX doesn't affect that in any way. It does not intervene.

juliandroid commented 3 years ago

It should be something like this:

diff --git a/common/src/main/java/io/homeassistant/companion/android/common/data/url/UrlRepositoryImpl.kt b/common/src/main/java/io/homeassistant/companion/android/common/data/url/UrlRepositoryImpl.kt
index e4e1a75..65961a4 100644
--- a/common/src/main/java/io/homeassistant/companion/android/common/data/url/UrlRepositoryImpl.kt
+++ b/common/src/main/java/io/homeassistant/companion/android/common/data/url/UrlRepositoryImpl.kt
@@ -94,13 +94,17 @@ class UrlRepositoryImpl @Inject constructor(
                 .scheme(httpUrl.scheme)
                 .host(httpUrl.host)
                 .port(httpUrl.port)
+                .addEncodedPathSegments(httpUrl.encodedPathSegments.joinToString(separator = "/"))
                 .toString()
         } catch (e: IllegalArgumentException) {
             throw MalformedHttpUrlException(
                 e.message
             )
         }

I did some quick tests and it seems to work just fine! I could've fix the spec: src/test/java/io/homeassistant/companion/android/common/data/url/UrlRepositoryImplSpec.kt but I couldn't make tests of the app, as whole, to run, otherwise I would've created a PR request.

dshokouhi commented 3 years ago

I did some quick tests and it seems to work just fine!

did you check every feature asides from webview? Location updates, sensor updates, notifications, widgets, quick settings

I have a feeling only webview works and none of the other features would work if that is the case then you might as well stick with the PWA because its the same thing there.

juliandroid commented 3 years ago

I'll try that tomorrow, but basically the WS was working and I was getting updates state change of the devices. The NGINX config uses the /path/ part only once and then the app continues to work without the path part as it does right now, so the change affects only the first call and after that all requests are enriched with extra HttpOnly cookie that is used instead of the path part.

If you do other requests, outside the webview, then it should still work since the saved Url includes the path section which will transparently authenticate the request and will be forwarded to HA with that part trimmed. Basically the "rewrite" NGINX rule does the trimming and that /path/ never reaches the HA.

juliandroid commented 3 years ago

I've spent some time testing. One thing that is not related - I couldn't make push notification works (no matter whether I use reverse proxy or not) with my debug apk build, but they work fine with the official build from play store (maybe something related to firebase).

Aside from that everything looks good:

Using the android app gives you some perks like using phone location, quick settings and the widgets, so it will be great to have that, not only the PWA.

jurriaan commented 3 years ago

I would like this feature as well as an alternative to using basic auth (which seems harder to support due to issues with the Authorization headers).

You could also use a custom subdomain like Nabu Casa Remote UI does, but this 'secret' URL then leaks over DNS.

stale[bot] commented 2 years ago

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue now has been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.

juliandroid commented 2 years ago

Can we have that merged?

mkowalczuk commented 1 year ago

+1 to add support for the paths in URL. I have the same use case – I'd like to configure a secret path that will be stripped by a reverse proxy and never be sent to my HA installation.

danielbrunt57 commented 1 year ago

Another +1 here too!