Skip to content

Signature validation - #75

Merged
TheAssassin merged 31 commits into
rewritefrom
signature-validation
Jul 26, 2018
Merged

Signature validation#75
TheAssassin merged 31 commits into
rewritefrom
signature-validation

Conversation

@TheAssassin

@TheAssassin TheAssassin commented Feb 26, 2018

Copy link
Copy Markdown
Member

Fixes #16.

@TheAssassin
TheAssassin force-pushed the signature-validation branch from 0d75054 to 997ead9 Compare March 1, 2018 12:01
@TheAssassin

Copy link
Copy Markdown
Member Author

After hours of debugging, and a couple of fixes I committed, the hashing of the files works perfectly fine (I compared it to some files in which I replaced the signature). However, I still get "bad signature" errors:

gpg2: gpg: Signature made Mon Mar  5 13:11:13 2018 CET using RSA key ID 101B2DC4
gpg2: gpg: BAD signature from "appimage.org <probonopd@users.noreply.github.com>" [unknown]
Validation error: Bad signature

@probonopd I am not sure whether I'm doing it right, though, and get the validation errors because I screwed up something in the gpg2 calls. validate.c keeps segfaulting, I'll probably have to fix those issues first. A review would be appreciated.

@TheAssassin

Copy link
Copy Markdown
Member Author

Using validate.c confirms the BAD signature, when hacking the gpg2 call to use the AppImageUpdate keyring.

It doesn't make much sense anyway to calculate an SHA256 hash for which a signature is calculated (GPG actually calculates an SHA1 hash, which it could do for the entire file as well). Note for myself: Have GPG use a more secure hashing algorithm (e.g., SHA2 or SHA3) instead of SHA1.

The question is, how can we fix these verification issues? There is clearly something going wrong here...

@TheAssassin

Copy link
Copy Markdown
Member Author

Actually, thinking about it, to have GPG2 sign the entire file containing a signature already, we had to copy it and overwrite the signature with zeroes, or alter the existing file and then re-insert the signature. So, signing the separately calculated hash makes sense.

@TheAssassin

TheAssassin commented Mar 5, 2018

Copy link
Copy Markdown
Member Author

The signing and verification works fine with locally built AppImage (tested with validate.c, not yet with AppImageUpdate). This raises the question, why does the verification fail with the appimaged AppImages built on Travis CI?

@TheAssassin

Copy link
Copy Markdown
Member Author

The last issue before we can merge this PR is to make AppImageKit's signatures work. They're invalid, and I'm not sure why. @probonopd will have to have a look. Once it's resolved, we can merge this. The UI implementation will follow soon.

@probonopd

probonopd commented May 4, 2018

Copy link
Copy Markdown
Member

It is possible that my public key doesn't match the private key. I remember I had some trouble back when I was setting it up, and as a result had to generate a new private key. Maybe I mixed the keys up. At least I cannot 100% exclude the possibility right now. Can you exclude it as a possiblity? Could that be the reason?

@TheAssassin

Copy link
Copy Markdown
Member Author

@probonopd just download your private key file, it contains the public key, and re-export the pubkey from it. Then you can compare it.

@probonopd

Copy link
Copy Markdown
Member

I need to do this when I have access to the private key. It is stored in a place I am currently not at.

@TheAssassin

TheAssassin commented May 4, 2018

Copy link
Copy Markdown
Member Author

There's no rush on this.

@TheAssassin

Copy link
Copy Markdown
Member Author

Waiting for your feedback, @probonopd.

@TheAssassin

Copy link
Copy Markdown
Member Author

poke

@probonopd

Copy link
Copy Markdown
Member

Damnit. Now I am in a train again. Argh

@TheAssassin
TheAssassin force-pushed the signature-validation branch 2 times, most recently from f8ecb8c to 92dfbbb Compare May 11, 2018 13:34
@TheAssassin

Copy link
Copy Markdown
Member Author

@probonopd I've been waiting for a week now, do you think you can check it this weekend?

@TheAssassin

Copy link
Copy Markdown
Member Author

@probonopd I will have to consider disabling the signatures of the official AppImages if you won't provide more details in the next days.

@TheAssassin

Copy link
Copy Markdown
Member Author

@probonopd setting Jul 16 as a deadline. I will either generate a new key for signing our official AppImages or remove signing entirely by then. The current situation is really annoying, since it has prevented a security(!) feature from being merged since February.

@probonopd

Copy link
Copy Markdown
Member

Let's generate new keys. But we need to decide how to safely store them so that project members have access. Let's discuss over a private channel.

@TheAssassin
TheAssassin force-pushed the signature-validation branch from 05d4c0a to 51c2f64 Compare July 20, 2018 01:20
@TheAssassin

Copy link
Copy Markdown
Member Author

Signature validation works fine now. Now we need to implement it in the Qt UI.

@TheAssassin

Copy link
Copy Markdown
Member Author

I implemented the validation in the Qt UI, changing the progress bar (and progress label) to show the validation state. Red means error, yellow means warning, green means it worked.

As on some DEs (e.g., XFCE) which use the Gtk plugin, the color of the progress bar cannot be changed (read: changing it in Qt doesn't change the UI), therefore on errors a message box is shown.

We need to decide what to do when the signature validation fails after the update. I'd say, we should restore the original state, i.e., move the original file back into position (if the .zs-old suffix had to be appended).

@TheAssassin

Copy link
Copy Markdown
Member Author

@probonopd I would call this PR "finished" now. Can you please test the UX of the validation process before I finally merge the PR? I think it's quite good now, and also adds some security.

@TheAssassin
TheAssassin merged commit 84cdbe6 into rewrite Jul 26, 2018
@TheAssassin
TheAssassin deleted the signature-validation branch July 26, 2018 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants