nvaccess / addon-datastore

Other
20 stars 14 forks source link

Ensure add-on release origin is authentic #41

Open feerrenrut opened 2 years ago

feerrenrut commented 2 years ago

PR #35 demonstrates a potential issue with automatic approval of PRs. Data in a prior submission can be changed, pass verification, and potentially auto-merged. In this case the author and license are changed. It would also be possible to change the download URL and SHA.

Some ways to address this:

nvdaes commented 2 years ago

For the following:

  • Introduce a way for authors to digitally sign add-on releases, so their origin can be confirmed.

PR #37 may provide this, since the publisher in metadata is set from the GitHub username that creates the issue that creates the PR to automerge.

feerrenrut commented 2 years ago

No, this isn't enough. To prevent hijacking of the add-on there needs to be a way to check that the person (or people) that prepared the add-on release haven't changed.

I've considered whether it could be mandated the addon release is tied to a github user / org, but there are some problems I don't like the inflexibility for the author. It also brings up problems when a new maintainer takes over.

Instead, if the addon package is signed, we know that the author of that version had access to the private key. We would have to store the public key of the author, or perhaps a URL and hash of it with each release.

nvdaes commented 2 years ago

Maybe good to test with public keys to know the possible difficulties and a way to document the procedure so that it"s easy and comfortable, and well understood why to do things like that.

Enviado desde mi iPhone

El 16 may 2022, a las 5:12, Reef Turner @.***> escribió:

 No, this isn't enough. To prevent hijacking of the add-on there needs to be a way to check that the person (or people) that prepared the add-on release haven't changed.

I've considered whether it could be mandated the addon release is tied to a github user / org, but there are some problems I don't like the inflexibility for the author. It also brings up problems when a new maintainer takes over.

Instead, if the addon package is signed, we know that the author of that version had access to the private key. We would have to store the public key of the author, or perhaps a URL and hash of it with each release.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.

feerrenrut commented 2 years ago

Yes, this will require some investigation. We don't intend to require it early on. But it is worth being aware.

nvdaes commented 2 years ago

Personally, I'll experiment signing releases as explained at https://wiki.debian.org/Creating%20signed%20GitHub%20releases

This may not be the final approach for the store, but I'll try to comment if I find special difficulties in case this can be useful.

nvdaes commented 2 years ago

I'm using the procedure described i the GitHub help to create a new GPG and related documents (options by default). I installed Cleopatra, which has a gui for Windows, but seems not to be very accessible for me with NVDA since I cannot see each certificate. Imo the process is not too difficult, but my advice would be to include this requirement as soon as the store is open for general testing, or alternatively make the signature process optional, not required, showing signed add-ons as verified and not signed add-ons without the verified state. Otherwise, imo some people may feel this as something tricky and may not be motivated to share add-ons in the store. At least, if this is included ASAP, people maybe less frustrated with further requirements that may not be well understood, and unless this store has a great advantage over other existing procedures/websites to share add-ons, this probably will be an fforz by NV Access and testers that won't be very used by add-on authors. Anyway people may prefer to share add-ons in the most easy way, but requiring this in a later stage may cause people to feel bad is this is not optional.

feerrenrut commented 2 years ago

I don't want to hold back that the store trying to add this requirement now. Given this problem has not previously been solved, I think we'll go on with the current approach. We may introduce it as an optional feature later.

nvdaes commented 2 years ago

Ok, anyway I will start signing add-on packages with detached-sign, attaching the .sig file to releases, since it is a good idea. When this optional feature is ready, I would like to test or use it, for example, we can provide an URL where the .sig file can be found, and public key maybe stored by you or shared in releases as an .asc file. This is very interesting as an optional feature.

Enviado desde mi iPhone

El 20 may 2022, a las 4:38, Reef Turner @.***> escribió:

 I don't want to hold back that the store trying to add this requirement now. Given this problem has not previously been solved, I think we'll go on with the current approach. We may introduce it as an optional feature later.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.

nvdaes commented 2 years ago

Hi again:

After investigation, I'll probably sign add-ons with GPG using GitHub Actions. I've done it just with my manualRelease.yaml workflow, which requires to provide the add-on version and if it's a prerelease. I've stored my private key and passphrase as repo secrets, and stored my public key in a publicKey.asc file in the rot of the project. I'll share this on add-ons mailing list in case it's helpful:

nvdaes commented 4 months ago

Now Github has Artifact Attestations in beta.