desktop-wallet icon indicating copy to clipboard operation
desktop-wallet copied to clipboard

Automatic update system

Open mvaivre opened this issue 3 years ago • 47 comments

If we want our users to always stay up to date, we could implement auto-updating. I've already deployed that for another electron project in the past, and it's quite straightforward : the app calls a file server where the latest binaries are stored, it checks if the version number is higher, if yes, it downloads and allows the user to install the update in a click, automagically.

@polarker WDYT?

mvaivre avatar Mar 03 '22 08:03 mvaivre

https://www.electronjs.org/docs/latest/tutorial/updates

nop33 avatar Mar 03 '22 09:03 nop33

This has some security implications. Could you check how it's done by other known crypto wallets?

polarker avatar Mar 03 '22 10:03 polarker

In general, I feel it's risky to update automatically for sensitive applications

polarker avatar Mar 03 '22 10:03 polarker

@polarker what are the security implications that you are referring to?

@mvaivre which crypto wallet desktop apps do you recommend checking? I remember you get inspired by the Ledger Live app a lot. Is there any other?

nop33 avatar Mar 03 '22 10:03 nop33

@nop33 My main 2 daily drivers are indeed Ledger Live and Firefly Wallet. Both I believe are using electron and offering automatic updates. Exodus seems to be relying on electron as well, and is also offering updates (to double check though).

I'm looking for other examples.

mvaivre avatar Mar 03 '22 10:03 mvaivre

  1. It's not my specialized topic, but I think we should only support this if we are sure that it's safe. We need to play safe when it comes to security.
  2. It happened in the past that a known wallet (cannot find the reference for now) got hacked due to automatic update. Basically, it's hard to ensure that you are downloading the right package.
  3. I have always been suggesting that we should investigate other projects for critical topics. I want to emphasize this again, please do investigate!!

polarker avatar Mar 03 '22 11:03 polarker

Here is a nice list of desktop wallets for ETH: https://docs.ethhub.io/using-ethereum/wallets/desktop/

polarker avatar Mar 03 '22 11:03 polarker

  1. It's not my specialized topic, but I think we should only support this if we are sure that it's safe. We need to play safe when it comes to security.
  2. It happened in the past that a known wallet (cannot find the reference for now) got hacked due to automatic update. Basically, it's hard to ensure that you are downloading the right package.
  3. I have always been suggesting that we should investigate other projects for critical topics. I want to emphasize this again, please do investigate!!
  1. Of course, hence this investigation issue.
  2. Very interested to know more about this story!
  3. As always: of course, this is what we've started to do and what we are doing here.

Here is a nice list of desktop wallets for ETH: https://docs.ethhub.io/using-ethereum/wallets/desktop/

Thanks! I already went through most of this list : many of those are either web wallets or closed source unfortunately. The interesting refs I could extract from that list are Exodus (cf. my previous message) and MyCrypto Desktop, which is also an Electron app (couldn't find their electron build config though).

MyCrypto also seems to signal when an update is available as they explain that this feature is not available when offline.

So: all the electron apps we've seen so far are proposing this feature. BUT, we're still young, and this is not a priority. If you feel like it could be a security issue, then we shouldn't take the risk.

A compromise I would propose

But let's keep in mind that it could also be a security issue if our users are running deprecated wallet version which are known to be unsafe. As proposed here, we could call a "ping" endpoint returning the wallet's minimum "safe" version number. By doing this we could - if needed - show a message asking the user to manually update their app in order to continue using it. WDYT?

mvaivre avatar Mar 03 '22 13:03 mvaivre

Thanks for this investigation @mvaivre

MyCrypto Desktop, which is also an Electron app (couldn't find their electron build config though).

I also looked deeply into the project and the organization on GitHub but couldn't find any more info regarding the electron build.

So: all the electron apps we've seen so far are proposing this feature. BUT, we're still young, and this is not a priority. If you feel like it could be a security issue, then we shouldn't take the risk.

It does look like the way to go method in electron apps is to use the autoUpdater. But I agree that we don't have to go this way.

A compromise I would propose

But let's keep in mind that it could also be a security issue if our users are running deprecated wallet version which are known to be unsafe. As proposed https://github.com/alephium/alephium-wallet/issues/137#issuecomment-1057103326, we could call a "ping" endpoint returning the wallet's minimum "safe" version number. By doing this we could - if needed - show a message asking the user to manually update their app in order to continue using it. WDYT?

I like this idea! To that, I'd like to add that every time the wallet app opens, we could call the GitHub API to get the version of the latest release, compare it to the version of the wallet, and show a message to the user that a new version is available and they can download it from GitHub. We then let the user install it manually.

The GitHub API can be called this way:

curl https://api.github.com/repos/alephium/alephium-wallet/releases/latest

and then we just read the tag_name of the response.

nop33 avatar Mar 03 '22 13:03 nop33

There are two questions to investigate with those known wallets:

  1. should we add automatic update for the wallet? I feel that we should not, as it's really not safe to download from a link under the hood. There are many ways this approach could go wrong. However, I am not an expert on this topic, so I would like to see if other desktop wallets have automatic update enabled.
  2. how to implement automatic update system if we decide to support it? I feel that we are mostly discussing this second questions in this thread.

polarker avatar Mar 03 '22 14:03 polarker

  1. how to implement automatic update system if we decide to support it? I feel that we are mostly discussing this second questions in this thread.

hm, I wonder why you feel that, since both @mvaivre and I in our last messages are talking about ways to inform the user about a new release and let them manually update themselves.

nop33 avatar Mar 03 '22 14:03 nop33

hm, I wonder why you feel that, since both @mvaivre and I in our last messages are talking about ways to inform the user about a new release and let them manually update themselves.

okay, sorry that I mis-read the inforrmation. Checking if there is a new release and suggesting users to manually update them sounds great to me. And it's a very easy one to implement

polarker avatar Mar 03 '22 14:03 polarker

I tried an old version (1.6.x) of MyCrypto wallet of ETH. There is no reminder of newer version it seems

polarker avatar Mar 03 '22 14:03 polarker

I tried an old version (1.6.x) of MyCrypto wallet of ETH. There is no reminder of newer version it seems

I didn't notice anything in 1.7.16 either, even though their release notes say: Add check if version is <2.0.0 for desktop app. I found multiple UI bugs though! :/

However, I am not an expert on this topic, so I would like to see if other desktop wallets have automatic update enabled.

As said before and just for clarity sake - all the other electron wallets cited above are proposing automatic updates (Exodus, Ledger Live, Firefly). Alright, let's now focus on the "compromise" approach proposed above :)

mvaivre avatar Mar 03 '22 14:03 mvaivre

Side note: curl https://api.github.com/repos/alephium/alephium-wallet/releases/latest this might be problematic sometimes as we release candidate version sometimes, like 1.2.3-rc0 or 1.2.3-beta for example

polarker avatar Mar 03 '22 14:03 polarker

Side note: curl https://api.github.com/repos/alephium/alephium-wallet/releases/latest this might be problematic sometimes as we release candidate version sometimes, like 1.2.3-rc0 or 1.2.3-beta for example

Good remark! A simple regex could allow us to take into account only "final" versions.

mvaivre avatar Mar 03 '22 14:03 mvaivre

A quick evaluation of the discussion: right on :+1:

Just one criticism: the problem manifests in three ways, which I haven't seen distinguished:

  • wallet<->local hosted backends
  • wallet<->official hosted backends
  • wallet<->non-official hosted backends

The "comparing the local version of the wallet with the one on GitHub" suggestion then will continue to manifest the problem for local and non-official hosted backends users.

@mvaivre was on a good track with a "ping" endpoint, but I think it should more appropriately be called "version" and return the version of the backend software in question (node or explorer). From my brief work with explorer-backend, it already can report its version via /infos. @nop33 is checking if node already does too. If it does then no additional work is needed from the backends.

This way in all scenarios, the wallet version will be compatible with the respective responding backends. We will have to keep a list internal to the wallet which tracks compatible version pairs / ranges.

lf94 avatar Mar 18 '22 13:03 lf94

We will have to keep a list internal to the wallet which tracks compatible version pairs / ranges.

I think the compatibility between wallet and node/expl-backend should be defined by the compatible versions of node/expl-backend in the alephium-js version the wallet is using. For example, since the current wallet is using alephium-js v1.1.0, and since this version of alephium-js supports the schemas of node 1.2.8 and expl-backend 1.5.0, then I think it makes sense that these should be the supported versions.

nop33 avatar Mar 18 '22 13:03 nop33

Yep, I think you're right, after thinking about it for a bit. I'll add a little graph to visualize what I see in my mind...

lf94 avatar Mar 18 '22 13:03 lf94

wallet-versions wallet-versions.svg

lf94 avatar Mar 18 '22 13:03 lf94

It'll be beneficial for anyone using alephium-js on the backend then too: it can throw an error immediately saying it's incompatible with the responding backends. For alephium-wallet we just propagate this error to the user basically in a nice way.

lf94 avatar Mar 18 '22 13:03 lf94

As we discussed in the meeting last Thursday, the upcoming deployment of mainnet (node v1.3.0) will make the current desktop wallet that users have installed (v1.1.0) obsolete. Users who do not update their wallet to v1.2.0 will receive strange error messages, and no notification to update their wallet. Unfortunately, we cannot do anything to avoid that this time, but we can for the next one. So, before releasing desktop wallet v1.2.0 together with mainnet node v1.3.0 we need to build in the wallet a way to notify the users that the version of the wallet they are using is incompatible with the network they are using.

@lf94 and @mvaivre suggest we call endpoints of the node and explorer backend that return their versions. Then we compare these versions with the supported versions configured in the desktop wallet. If there is a mismatch, we display a message to the user:

The version of this wallet (X.Y.Z) is incompatible with the selected network settings. Expected node version A.B.C, but the wallet is connected to a node with version D.E.F. (accordingly for explorer backend)

A simpler approach that would address 99% of the problem (assuming 99% of our users only use mainnet network settings) would be to simply:

  1. check if network settings are set to mainnet
  2. query the GitHub release page, get the latest (not rc) release version
  3. compare it with the wallet version, and show a message to the user:

Your wallet is out of date. Please, update it by downloading and installing the latest version.

I think we should at least implement approach 2. WDYT?

nop33 avatar Mar 28 '22 15:03 nop33

We can close this right, since we opted for update notification?

lf94 avatar May 04 '22 18:05 lf94

I don't think so. We haven't implemented an "Automatic update system". We settled for informing the user that a new version is available and letting them download it themselves from GitHub. But maybe we want to keep it open and work on it in the future, once we figure out how to eliminate the security concerns? Many wallets have this feature, and honestly, it's a great UX to not have to download installation files by simply clicking "Update". Maybe by adding a label "postponed" or sth? @mvaivre?

nop33 avatar May 05 '22 06:05 nop33

Yep, agreed with @nop33, I'd keep this open - we should really think about how to offer this in a secure way. It's a UX must have.

mvaivre avatar May 05 '22 08:05 mvaivre

I thought it was a matter of privacy and security; ok, sure!

I think the security concerns are inherent to the method itself and can't be removed entirely. If there is a supply chain attack or similar all our users will be infected at the same time. I think software companies like Google and Mozilla can offer automatic updates to their browsers because they have dedicated teams each worth probably a few million dollars to mitigate this. All other companies doing it probably shouldn't be but it's context-dependent since it's most likely all that software isn't holding onto potentially a million plus dollars in value. As a moral software developer I cannot vouch for this feature in our context... I believe though our current notification solution is fair.

by simply clicking "Update"

This is still different from "automatic updating" and don't see the problem, as long as we're displaying where the source of download is coming from.

Didn't occur to me sooner but we should also have a "turn off update notification" option.

Many wallets have this feature

And many don't too!

  • Bitcoin Core
  • Trezor
  • Bitbox

I know the latter two take the "update notification" approach as we have.

lf94 avatar May 05 '22 16:05 lf94

Didn't occur to me sooner but we should also have a "turn off update notification" option.

I think we shouldn't :)

nop33 avatar May 09 '22 08:05 nop33

Reopening the discussion:

Reminder: https://www.electronjs.org/docs/latest/tutorial/updates

I've already used the second approach, but the first one - fetching Github releases - seems super simple and would work great for us (at least for Windows and MacOS).

We should check again how other electron wallets are supporting this. I personally don't see any obvious security threats if we're pulling the new versions directly from our github repo.

mvaivre avatar Sep 28 '22 06:09 mvaivre

@killerwhile We would love your opinion on this, ie. how to guaranty a safe hosting of our releases and the safe deployment of a an update notification service. We could organize a quick intro call about this, I'll ping you on Slack. :)

mvaivre avatar Sep 28 '22 07:09 mvaivre

Independently of the release process, it will be highly recommanded to sign the releases with OS-approved keys.

Releases in Github and signed are pre-requisites to electron's update tooling, which I would highly recommend using. I would still host the updater API ourselves (such as https://update.alephium.org on AWS) to avoid an additional dependency on https://update.electronjs.org.

We need to trust Github anyway, so keeping the CI and releases on Github seems fair. Otherwise we could copy the release on AWS, which we trust for the public wallet api.

killerwhile avatar Sep 29 '22 15:09 killerwhile