switch from token to instance ?
Closed this issue · 9 comments
Following the lengthy discussion in UnifiedPush/flutter-connector#56, and since we are potentially up for some breaking changes I would like to propose a change from token to instance in the spec.
They are supposed to be almost the same but doesn't have the exact same semantic, and this is both confusing and error-prone I believe.
Main change in the spec would be the rename + relax the constraint regarding the need to be random, but keep the uniqueness.
Implications (good or bad) I can think off:
- no potential misuse by an app/lib that would ignore the random token requirement
- mimic current lib interfaces
- Distributors can't rely on that to be a random string that can be used for auth and/or uniqueness. They will have to store a mapping. This is however already the case for most of them. People writing distributors should have better knowledge of the spec than people dealing with the connector interface, hence I think this is fine.
For backward compatibility:
- existing UP distributors that currently directly used the random token should be able to do
token=tokenin their initialinstance-tokenmapping when upgrading - we can keep support for
tokenEXTRA param in the Android code and treat it as aninstancefor a while until all apps/framework migrate - for D-Bus, parameters are unnamed I believe, so no change should be needed at the interface level. We can just use current positional string token parameter as instance.
I would prefer renaming instance to tokenIdentifier or something like this on library side (which may makes sense). This needs to be random, and it is deliberately not set by developers to avoid any mistake :)
The 3 implications you suggested are indeed good things, imho.
However, unregister on DBus doesn't know about what app (or appName) originated the request. This change (non-unique tokens) would require adding an app name parameter to unregister (like the one register has). This is probably not a big deal since we're doing a 2.0 release anyway for #18 or #17. That could be batched together with this change into one big release.
@karmanyaahm hum yes indeed I didn't think about that, good catch thanks :)
@p1gp1g randomness in the token will be needed for some distributors, while it will be useless to some that will have to create a token/private key/else in the expected format for the underlying push mechanism, or get a token directly from it. ntfy is already generating its own token, UP-FCM is ignoring it completely. I think we are better off simplify the spec for the app/client-lib developers and let the distributors deal with this complexity, since lot of them will do so anyway, and people writing distributors should be more UnifiedPush savvy than people trying to use it in an app.
I don't think saying that it's dealt by the native library is a good way to deal with it: a lot of people will implement an app or a client lib using D-Bus directly and the spec, it's quite often less work than bridging a native lib because of existing and well supported D-Bus libraries in a lot of languages and frameworks.
I think you have misinterpreted the meaning of this token.
This token is only between the connector and the distributor. So another app can't impersonate the distributor.
The token is given to the distributor during the registration. If this token is not random, let's even say static, then the following is easy to do :
C -registration--token1-> D
C <-new-endpoint--schem://real.tld--token1- D
C <-new-endpoint--schem://fake.tld--token1- M
C <-message--fakeMessage--token1- M
C: connector
D: distributor
M: malicious
As we can see, it is better to keep this token secret as possible, so it is better to not use it in the endpoint actually.
Since the token is not unique per application on distributor side, a connector can have many connection with the distributor. The connector can use it to have multiple endpoints for the same application. That's what we have done with the instance system. Another connector may use this non-uniqueness to create different priority channel for instance. This does not regard specifications: the specifications already allow that.
Also, I have written all the android distributors (until ntfy, and just the fork of gotify) : this token is definitely used by all of them. Only NoProvider2Push uses the connector token in the endpoint, which would be changed if we involve it to not be only for development. [Edit: UP-FCM too, that should be changed...]
On linux side, it is a bit different, the distributor and the connector are meant to be run by the same user. So the token can't be secret. Randomness may just avoid any side effect but if it is required for any reason, it is possible to change it for a non-random one. *
* That's one of the reason I'd like the token generation not on main lib part, all the platforms may not need one.
Ohhhh I completely failed to understand that indeed !
Okay all is a lot more clear now ! I thought the token was to be used for the endpoint indeed, hence me saying it was unused for at least some, I was looking at the endpoint registering logic.
token: with a random token to identify the connection between the connector and the distributor. It MUST be unique on distributor side.
It is a bit ambiguous to me because I thought distributor was more talking about the underlying distribution tech 😅 .
It could be worth adding some extra precision here in the spec, perhaps by saying that it shouldn't be used as a source for authentication or identification of the underlying distribution channel, or something like that ?
I am not sure this is really needed however since the source package of the broadcast intent should be trustable I think. Not sure for D-Bus messages. We could mandate to check messages origin to be equals to the distributor ID. But then is it really simpler :) ?
On linux side, it is a bit different, the distributor and the connector are meant to be run by the same user. So the token can't be secret
As flatpak and other isolation mechanisms start to become more popular, and linux becomes more similar to android, having a secret token will probably make sense for the same reasons as android.
Not sure for D-Bus messages. We could mandate to check messages origin to be equals to the distributor ID
I previously looked at that since would help remove the app id from 'Register', but I don't think there's any straightforward way of achieving it. https://stackoverflow.com/a/64809747/8919142
But then is it really simpler :) ?
I agree, probably not.
Ohhhh I completely failed to understand that indeed !
Okay all is a lot more clear now ! I thought the token was to be used for the endpoint indeed, hence me saying it was unused for at least some, I was looking at the endpoint registering logic.
No problem 😄
token: with a random token to identify the connection between the connector and the distributor. It MUST be unique on distributor side.It is a bit ambiguous to me because I thought
distributorwas more talking about the underlying distribution tech sweat_smile . It could be worth adding some extra precision here in the spec, perhaps by saying that it shouldn't be used as a source for authentication or identification of the underlying distribution channel, or something like that ?
Maybe adding link to the definitions would help ?
Also, this token may allow an application to be easily an unifiedpush settings app. For instance : if org.unifiedpush.settings is a distributor, always choose it ; and this app mitm to other app :)
Can this issue be closed ?