rmfakecloud icon indicating copy to clipboard operation
rmfakecloud copied to clipboard

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

Open flomlo opened this issue 2 years ago • 7 comments

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 avatar Mar 30 '22 14:03 flomlo

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

flomlo avatar Mar 30 '22 16:03 flomlo

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

ddvk avatar Mar 31 '22 08:03 ddvk

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?)

flomlo avatar Mar 31 '22 08:03 flomlo

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!

nemunaire avatar Mar 31 '22 09:03 nemunaire

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.

flomlo avatar Mar 31 '22 10:03 flomlo

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 avatar Mar 31 '22 11:03 TheJonny

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 :)

TheJonny avatar Mar 31 '22 11:03 TheJonny

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.

nemunaire avatar Nov 08 '23 20:11 nemunaire