palant / pfp

A simple and secure browser extension to be used with KeePass databases.
https://pfp.works/
Mozilla Public License 2.0
113 stars 14 forks source link

remoteStorage as sync backend #87

Closed CircleCode closed 5 years ago

CircleCode commented 6 years ago

current sync server are all proprietary. Sync over a standardized protocol could be interesting.

Actually, I read #85, and I deduce that the way webdav manage authentication is not acceptable for pfp? (why it is?)

palant commented 6 years ago

There are two issues here:

  1. Usually, using WebDAV requires the client to have full credentials, not merely a limited access token like with OAuth. This is an issue because sync info cannot be encrypted - PfP needs to sync even if the user isn't logged in. Storing a WebDAV password as plain text is often a bad idea however. This can be worked around by creating a limited account specifically for the one use case, but it is definitely suboptimal.
  2. Multiple PfP clients might sync at the same time. It is important that one of them won't simply overwrite changes made by the other. My understanding is that the protocol itself doesn't specify a way to prevent such conflicts. Some implementations (Owncloud/Nextcloud) allow sending If-Match header so that the operation will fail if the file changed in the meantime, but I don't think that this is always supported.

Either way, any particular WebDAV server you had in mind? I don't think that speaking about all of them at the same time makes sense here, despite the standardized protocol.

CircleCode commented 6 years ago

Thanks for the explanations. Regarding 1. I now understand (I was not thinking about the password storage, only about the transmission part) Regarding 2., I had no specific implementation in mind, it would have come after this discussion, but as you said, there can be significant differences between implementations, and the possibilities they offer


Maybe this should be another issue, but do you think to other non proprietary, self hostable backends?

palant commented 6 years ago

It seems that sabre is the prevalent WebDAV implementation, both Owncloud and Nextcloud are based on it. By itself, sabre supports If-Match but no clients with limited access rights. It's the same with Owncloud, in both cases one would have to create a user account with access to a single directory. Nextcloud supports OAuth but so far only in a suboptimal way - there are no scopes, every OAuth client gets full access. Looking at WsgiDAV for comparison, things are very similar there. I guess that limited clients aren't a typical use case for WebDAV.

In fact, WebDAV is way overdimensioned for this use case. So one consideration would be a simple PfP-specific server. A Python-based implementation wouldn't be a big deal, anybody could install it on their server then.

CircleCode commented 6 years ago

In fact, WebDAV is way overdimensioned for this use case. So one consideration would be a simple PfP-specific server.

I fully agree.

However, I wonder if we cannot adopt an already existing storage backend to avoid having to install something specific to pfp. For example something like https://remotestorage.io/ (I don't know it, only heard about it…)

What do you think?

palant commented 6 years ago

Looks good. Clean spec, everything necessary seems to be present. I morphed this issue into a request to support remoteStorage, should be easy to implement.

CircleCode commented 6 years ago

:+1: and enough different server implementations to allow users to choose (and no need to develop a backend on your side)

palant commented 6 years ago

For reference: according to https://github.com/remotestorage/design/issues/3#issuecomment-377173443, there is no logo+text combination for remoteStorage. The recommendation is to use the page's own font. I'll need to see whether it makes sense to do the same for Dropbox and Google Drive, to have things look consistently.

palant commented 5 years ago

Supporting remoteStorage in the web client is problematic, a flow that would involve copying and pasting the token manually is not supported. I created https://github.com/remotestorage/spec/issues/174 to ask for this possibility to be added. In the meantime, I solved this by redirecting to https://pfp.works/oauth-endpoint/. This is not optimal as it adds one more place which could be compromised. But at least this webpage will only see the token but not the user address it belongs to.

palant commented 5 years ago

Also, at least 5apps.com made quite a mess with the OAuth authentication. Once you authorize an app, redirect_uri is remembered and reused for any subsequent authorizations. This makes it impossible to authorize both the regular extension and the web client, they have to use different values for redirect_uri. It seems that using a different client_id for the web client will solve it here, but I need to see how other servers deal with this.

raucao commented 5 years ago

I do the same for rs-backup (source), which is a terminal client for backing up and restoring RS accounts.

The web server actually doesn't see the token at all, because with OAuth it is sent in a URL fragment and not in a query parameter. This also prevents it from appearing in access logs and such.

palant commented 5 years ago

The web server actually doesn't see the token at all, because with OAuth it is sent in a URL fragment and not in a query parameter. This also prevents it from appearing in access logs and such.

Sure, the web server won't see anything - as long as this page isn't modified. If the server is compromised, the web page could be modified to log the token. Meaning an unnecessary point of failure.

palant commented 5 years ago

The spec says:

if no client registration is required, the server MUST ignore the value of the client_id parameter in favor of relying on the origin of the redirect_uri parameter for unique client identification.

Looking at how this is implemented in various servers:

raucao commented 5 years ago

Sure, the web server won't see anything - as long as this page isn't modified. If the server is compromised, the web page could be modified to log the token. Meaning an unnecessary point of failure.

That's the very security model of OAuth redirect flow, RS, origins, Web Storage, and so on. Of course you want the trusted origin, which is authorized to acquire tokens on behalf of the user, to not be compromised.

When there's no client registration, this is also the only way for storage providers to direct the user to a place that can tell them what the app and who the app provider is, of course.

raucao commented 5 years ago

Also, at least 5apps.com made quite a mess with the OAuth authentication. Once you authorize an app, redirect_uri is remembered and reused for any subsequent authorizations. This makes it impossible to authorize both the regular extension and the web client, they have to use different values for redirect_uri.

IIRC that is exactly what the OAuth specs require. The redirect URI is the entire URI btw, so it's very easy to use multiple different ones for different clients (which is also recommended for RS, because of the missing client registration).

palant commented 5 years ago

The authentication behavior of 5apps.com is problematic because:

  1. OAuth servers usually support multiple endpoints per client, useful for development vs. production server at the very least. Sure, that's normally paired with client registration, but it's still the expected scenario.
  2. Most remoteStorage servers (as in: all but 5apps.com) won't require unchanged redirect URIs per client, neither is this required by the remoteStorage spec. So while most clients won't work with multiple OAuth endpoints, they won't expect changing endpoind URL (different path on the same domain) to cause issues either.
  3. 5apps.com does not reject an unexpected redirect_uri parameter, it will simply redirect to its stored redirect URI instead. If the two are different, I cannot see how this could possibly result in anything useful. Also, the user won't have any hint towards what went wrong or how to correct this (revoke app token and restart the process).
  4. Finally, by displaying client ID and even linking to it 5apps.com clearly violates the spec and facilitates phishing attacks. Anybody could set client_id to https://www.google.com/, it's redirect_uri that matters - and this one could point to some phishing website.

Note how none of this would be an issue if 5apps.com used the origin of redirect_uri as "client ID" as required per spec. Having different client IDs for different endpoints would happen automatically and not something that developers have to worry about.

palant commented 5 years ago

I think I'm done. As things are now, it should work with any remoteStorage servers. It has only really been tested against 5apps.com however, and there are significant differences between implementations.

raucao commented 5 years ago

I understand the issue now, and it's actually quite surprising to me, because I do not remember it working this way. Looks like a fairly recent change in preparation for the new dashboard caused this bug. We will fix it asap and require client ID to be the same as redirect URI again.

(For historic context: we were actually one of the first projects doing just that, and arguing for it to be a reasonable choice for open web apps. In the meantime, others have validated this opinion.)

raucao commented 5 years ago

... by the way, be sure to add this app to the list on the RS wiki, whenever RS support is released: https://wiki.remotestorage.io/Apps

Looks promising! Looking forward to trying it out myself.

palant commented 5 years ago

... by the way, be sure to add this app to the list on the RS wiki, whenever RS support is released: https://wiki.remotestorage.io/Apps

Done (release came out two days ago). Filed #104 on an outstanding issue here but otherwise everything seems to be working fine.