ddvk / rmfakecloud

host your own cloud for the remarkable
GNU Affero General Public License v3.0
767 stars 62 forks source link

`STORAGE_URL` is not used globally: `local.appspot.com` is still hardcoded somewhere #162

Closed flomlo closed 11 months ago

flomlo commented 2 years ago

Hi,

the function locateService in line https://github.com/ddvk/rmfakecloud/blob/3ff893c2497435167e8fb951316b808ee1f50f4e/internal/app/handlers.go#L522 does not make use of the config parameter STORAGE_URL, but is instead hardcoded to local.appspot.com.

This means that every client of the rmfakecloud has to resolve the host local.appspot.com. Whilst this poses no major problem for the remarkable device itself, it is a nuisance for 3rd-party cloud clients like rmcl, rmfuse, etc: The device running these clients must modify /etc/hosts and further accept the ca-certificate used to sign local.appspot.com.

thejonny has drafted a solution already, see #161.

It introduces STORAGE_HOST instead of STORAGE_URL (without a preceding https://) and allows us to run rmfakecloud without manually resolving local.appspot.com on the remarkable device (or the computer running rmfuse).

Whilst we are not sure of the precise use of STORAGE_URL in rmfakecloud it might be that is superseeded by STORAGE_HOST. But we are not sure yet.

flomlo commented 2 years ago

See https://github.com/rschroll/rmcl/pull/9 for the changes to rmcl which enable it to properly use rmfakecloud.

ddvk commented 2 years ago

yep, I've only used rmapi which has the storage urls hardcoded, and only STORAGE_URL has to be correct

flomlo commented 2 years ago

I think deprecating STORAGE_URL in favour of STORAGE_HOST would be the cleanest (although API-changing). Or we calculate STORAGE_HOST from STORAGE_URL by removing preceding https:// or http://-strings (potentially ugly and error-prone?)

nemunaire commented 2 years ago

rmfakecloud could run either on HTTP and HTTPS, so deprecating STORAGE_URL in favor of a variable with less information is awkward.

Moreover, you can use the net/url go package to extract the scheme and host reliably.

For me STORAGE_HOST has to be determined from STORAGE_URL, and let to the user the choice to use http:// with software supporting it.

But :+1: for removing hardcoded local.appspot.com!

flomlo commented 2 years ago

That is a good objection, I didn't consider http://-only clients!

@ddvk: If I understand it correctly: rmapi in the current version would break if this patch would be included and someone were to set STORAGE_URL other than local.appspot.com? I guess that should be considered a bug and will be fixed at some point(?). But a rmfakecloud where STORAGE_URL is not set and defaults to http[s]://local.appspot.com should continue to work with unpatched versions of rmapi.

Are you ok with including the net/url go package to reliably generate STORAGE_HOST from STORAGE_URL? Then we will modify the patch appropriately.

TheJonny commented 2 years ago

rmfakecloud could run either on HTTP and HTTPS, so deprecating STORAGE_URL in favor of a variable with less information is awkward.

Moreover, you can use the net/url go package to extract the scheme and host reliably.

For me STORAGE_HOST has to be determined from STORAGE_URL, and let to the user the choice to use http:// with software supporting it.

But +1 for removing hardcoded local.appspot.com!

Hi :)

can STORAGE_URL contain a subdirectory? or is the most general valid form $scheme://$host (i think $host could also contain a port)

does the remarkable support http or is it for use with the local forwarding proxy? there are queries that are answerd by the bare hostname, so i guess xochitl/sync adds "https://"

TheJonny commented 2 years ago

rmfakecloud could run either on HTTP and HTTPS, so deprecating STORAGE_URL in favor of a variable with less information is awkward.

you are right - that's why the PR is marked as draft :)

nemunaire commented 11 months ago

There is no more hardcoded local.appspot.com. I've just tested deleting the entry in /etc/hosts on my remarkable, and all the expected features continue to work well.