Closed mvaivre closed 1 year ago
This has some security implications. Could you check how it's done by other known crypto wallets?
In general, I feel it's risky to update automatically for sensitive applications
@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 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.
Here is a nice list of desktop wallets for ETH: https://docs.ethhub.io/using-ethereum/wallets/desktop/
- 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.
- 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.
- I have always been suggesting that we should investigate other projects for critical topics. I want to emphasize this again, please do investigate!!
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?
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.
There are two questions to investigate with those known wallets:
- 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.
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
I tried an old version (1.6.x) of MyCrypto wallet of ETH. There is no reminder of newer version it seems
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 :)
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
Side note:
curl https://api.github.com/repos/alephium/alephium-wallet/releases/latest
this might be problematic sometimes as we release candidate version sometimes, like1.2.3-rc0
or1.2.3-beta
for example
Good remark! A simple regex could allow us to take into account only "final" versions.
A quick evaluation of the discussion: right on :+1:
Just one criticism: the problem manifests in three ways, which I haven't seen distinguished:
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.
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.
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...
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.
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:
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?
We can close this right, since we opted for update notification?
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?
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.
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!
I know the latter two take the "update notification" approach as we have.
Didn't occur to me sooner but we should also have a "turn off update notification" option.
I think we shouldn't :)
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.
@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. :)
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.
Update: @killerwhile has rolled out our own update delivery service: https://updates.testnet.alephium.org/alephium/desktop-wallet/darwin-x64/v1.3.0 🎉 (no binaries are hosted yet, but everything's in place!)
We now only need to add code signing in our toolchain (#412) before we can implement auto-update in electron.
I was not involved in the discussion, but I prefer to hosting our releases on Github because:
Am I missing some points here?
Edit: I'd prefer update.electronjs.org than self-hosted update service too. Like we don't host our release for our SDKs.
update.electronjs.org (or, same code, different deployment, https://update.alephium.org/) are just eyes catching and redirection proxies in front of Github releases. So the "where to store" the releases we all agree, i.e. Github. The question is which service we use to integrate electron update mechanism, since it can't pull directly from Github. Starting with update.electronjs.org is easier (no extra), but we have to trust electronjs infrastructure (which probably millions of other do), or we deploy their stack on our side (done, it results in https://update.alephium.org/)
@polarker What do you think?
You'd like us to : 1) Store the (signed) releases on Github 2) And use update.electronjs.org as an update server?
@mvaivre Yes, that sounds much easier to implement&provision and don't break any of security practices as far as I see.
But as I am not really familiar with electronjs, I'd like to confirm if update.electronjs.org
is as popular as npmjs.com
?
Alright, thanks for your answer!
But as I am not really familiar with electronjs, I'd like to confirm if update.electronjs.org is as popular as npmjs.com?
Those two have nothing in common actually :
update.electronjs.org
is just a proxy service that's called by electron apps in order to know if a new version is available (on Github in our case). The service compare the version numbers, and inform the app if an update is indeed available. npmjs
is used to serve modules used when developing.Btw, there are multiple ways to sign the releases. I am not sure which way we are going to use. The signing approach might affect our current release workflow. Please feel free to bring the discussion up later on.
Yes! This is tracked here and @killerwhile is looking into adding the signing step in the building/releasing process.
Those two have nothing in common actually :
Got it. By popular I meant if it's used by many apps. Massive adoption could give me the confidence that it's been tested well in production.
As I quickly checked, it seems the update repo is well maintained by the electron team. Do you know any big project using the service?
It seems update.electronjs.org
is not really that used: https://www.npmjs.com/package/update-electron-app 😅 This might change my preference.
Suppose that our releases are published on Github, what are the differences between fetching release information directly from Github vs using update.electronjs.org
?
Edit: if I recall correctly, we were using Github releases for automatic updating already? If so, what are the issues making us to change to other approaches?
That's a good point. The solution involving https://www.npmjs.com/package/update-electron-app is the "easiest" option mentioned by the official Electron documentation, and works only with update.electronjs.org
. I guess that many projects don't want to rely on this service.
I've dig into my past projects, and it appears that I've implemented an alternative solution in the past, using electron-updater
(300k downloads per week) which was querying a simple file hosting service. We might make it work directly with our Github releases.
I'll look more into this with @killerwhile (and check what other popular projects are using) then update the issue accordingly.
That big download number sounds really promising!
Here we go:
Firefly wallet is using electron-updater
Here's their GH build workflow.
We can see in the electron-builder
configuration file that they use their own download service: https://dl.firefly.iota.org/
electron-builder
and electron-updater
seem to allow for easy publishing and checking for update, directly on Github.
But I suspect that the reason why some prefer to use their own update server instead of directly querying Github :
The GitHub API currently has a rate limit of 5000 requests per user per hour. An update check uses up to 3 requests per check.
We need to figure out if this is enough for us or no.
Investigating other projects... updating... daaamn, all those wallets are closed source!
The GitHub API currently has a rate limit of 5000 requests per user per hour
The "user" here is Alephium organization, or desktop wallet user?
The "user" here is Alephium organization, or desktop wallet user?
If we don't have an intermediary server for caching, that would be the end user (making the calls from their wallets directly to Github).
Then the 5000 limit is more than enough.
Agreed. Sounds good! Let's start like this then :)
Just an update on this: the automatic update system works great on macOS and Windows (master
)! Benoit is working at the moment on Linux code-signing so that we can have this feature on Linux as well.
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?