1070188 - Modify the validator to warn about pre-existing signatures

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Add-on Validation, defect, P3)

Description

[Wil Clouser [:clouserw]]

When we sign add-ons we are going to clobber any pre-existing signatures (including our own signatures).  We should warn authors about this if they upload an add-on with an existing signature.  How about:

> Add-ons which are already signed will be re-signed when published 
> on AMO.  This will replace any existing signatures on the add-on.

Comment 1

[Olivier Meunier [:aspyrine]]

Here is a PR for this new test:
https://github.com/mozilla/amo-validator/pull/261

Is the tier=1 argument on the decorator correct?

The test only checks the presence of signature files, not their contents.

Comment 2

[Olivier Meunier [:aspyrine]]

Needs to be reviewed: https://github.com/mozilla/olympia/pull/386

Comment 3

[Mathieu Agopian [:magopian]]

New PR: https://github.com/mozilla/amo-validator/pull/268

Comment 4

[Mathieu Agopian [:magopian]]

Fixed in https://github.com/mozilla/amo-validator/commit/7eff2a1d4f4b099a0ebee17d7d15f8881bbe7c7f

STR:
1/ submit an addon or a new version of an existing addon
2/ give it a full or preliminary review (it is thus signed)
3/ download the (signed) XPI from the website (to make sure it's signed: it should now contain a META-INF folder with three files: manifest.mf, zigbert.sf and zigbert.rsa)
4/ delete the addon or the version from the website
5/ submit the addon or version again, this time uploading the signed xpi from step 3

It should display a validation warning saying "Add-ons which are already signed will be re-signed when published on AMO. This will replace any existing signatures on the add-on."

Comment 5

[Mathieu Agopian [:magopian]]

The step 5 of the STRs doesn't work because of the blacklisting of the UUID.

Instead of first uploading the addon to have it signed, maybe do:
1/ download an already signed addon (I believe https://github.com/mozilla/olympia/blob/master/apps/files/fixtures/files/signed.xpi should do the trick, haven't tested though)
2/ submit the addon

I don't know how to sign addons the old way, maybe :jorgev or :kmag can tell? With those STR, you'll only be able to test once, as the UUID will be blacklisted also...

Comment 6

[Madalin Cotetiu]

I have submitted the add-on from comment 5 but the mentioned warning message was not displayed.
Screenshot: http://screencast.com/t/wQU1WzSsm

Comment 7

[Mathieu Agopian [:magopian]]

I understand why: this addon I've linked doesn't have the manifest.mf file. Is that file always present when an addon is signed (the old way)?

If not, we'll have to modify the amo-validator code (https://github.com/mozilla/amo-validator/pull/268/files#diff-55a89dd8d238cf41af9ae392adfb7ad8R264) to be less strict and just look for the zigbert.rsa and zigbert.sf files.

:dveditz :rtilder, any thoughts?

Comment 8

[Ryan Tilder [:rtilder]]

The manifest.mf should be created by the call to signing_clients.app.JarExtractor.make_signed in olympia.  It will ignore one if it already exists but should always create one, I believe.

Comment 9

[Mathieu Agopian [:magopian]]

Ryan, does that also apply with the "old" way of (self?) signing the XPIs?

Comment 10

[Ryan Tilder [:rtilder]]

Meaning before support for re-signing a signed XPI?  Yes and no.  manifest.mf will be generated fresh.y but it will contain stanzas for the pre-existing manifest.mf and *.rsa and *.sf files.  That would mean that the generated manifest.mf is incorrect which would cause later signature verification errors in the client.

Comment 11

[Mathieu Agopian [:magopian]]

Ryan, please let me rephrase: for the STRs that the QA team will use, I said they could use this addon which is supposed to be signed (it's a fixture we have for our tests): https://github.com/mozilla/olympia/blob/master/apps/files/fixtures/files/signed.xpi

However, this addon doesn't have the manifest.mf file, and thus isn't detected as "signed" by the amo-validator (it looks for the presence of the three files: the manifest.mf and the two zigbert.rsa and zigbert.sf files).

My question is thus: should I fix the amo-validator code (to detect an xpi as signed if it only has the two zigbert files, but not the manifest.mf file), or is it the fixture that's broken?

Comment 12

[Mathieu Agopian [:magopian]]

After a quick chat with Ryan on irc, it seems the fixture (the signed xpi that I recommended to use for the STRs) is faulty, and should have the manifest.mf file in there.

Thus, Madalin, could you please try again with those steps:

1/ submit a new addon, and review it (so it gets signed)
2/ download the signed addon, unzip it, change the UUID in the install.rdf (modify what's in the <em:id> tag), and re-zip it
3/ submit this new addon (the signature won't be correct, because the content has been modified, but it doesn't matter for this test, it should still detect that the addon is signed, and display the warning in the report)

Comment 13

[Madalin Cotetiu]

Attachment: signedtest001-.1-fx UpdatedUUID.xpi

I have followed the above steps, there is no warning message regarding the signing.
The upload your add-on page: http://screencast.com/t/4jZbYL6pby
The full validation page: http://screencast.com/t/q8VHZMxMr3c

I am attaching the final xpi file which I used for the final test (The one submitted, approved, downloaded and then updated the UUID).

Reopening the bug.

Comment 14

[Mathieu Agopian [:magopian]]

Madalin, I can see the security warning in the second screencast: "Package already signed". That's what the bug is about, so I believe it's verified as fixed?

Comment 15

[Madalin Cotetiu]

Shouldn't that message be also be displayed on the Upload Your Add-on page, just on the full validation report? If yes please mark the bug again as fixed so we can close it.

Comment 16

[Mathieu Agopian [:magopian]]

I don't think so, on the "upload your add-on" page, there's only this three items checklist I believe.

I'll close this now, if :jorgev believes it should be displayed on the submission page, he'll hopefully reopen it.

Thanks!

Comment 17

[Madalin Cotetiu]

Ok we will wait for :jorgev opinion on this.

Comment 18

[Jorge Villalobos [:jorgev] (he/him)]

I think it's fine to only show it in the full report. It's not such a critical message.

Comment 19

[Madalin Cotetiu]

Thanks for the response.
Closing bug.

Comment 20

[Madalin Cotetiu]

The issue started to reproduce again.
Screencast for this issue: http://screencast.com/t/cYnkdNWGAw
Reopening.

Comment 21

[Mathieu Agopian [:magopian]]

Thanks Madalin, indeed we changed the signature filenames in olympia, but forgot to replicate the change in the amo-validator:

https://github.com/mozilla/amo-validator/blob/7c88632c655d9e60cae32523a9d97f542c024ae8/validator/testcases/content.py#L264

The bug related to the signature filenames change: bug 1152102

Comment 22

[Mathieu Agopian [:magopian]]

PR: https://github.com/mozilla/amo-validator/pull/283

Comment 23

[Mathieu Agopian [:magopian]]

Fixed in https://github.com/mozilla/amo-validator/commit/3477b97134c7446b93532608a6a8cf8b828272a3

Comment 24

[Madalin Cotetiu]

Issue is still reproducing in -dev http://screencast.com/t/Ml0FFJ1oKrR
Reopening bug.

Comment 25

[Mathieu Agopian [:magopian]]

Yes, two issues here:
1/ I forgot to retag and publish the amo-validator with the changes
2/ I suddenly realized that we don't want to only test for mozilla.* files... we want to test for any signature, so instead just check if there's a META-INF/manifest.mf file

Comment 26

[Mathieu Agopian [:magopian]]

Third PR : https://github.com/mozilla/amo-validator/pull/284

Comment 27

[Mathieu Agopian [:magopian]]

Fixed in https://github.com/mozilla/amo-validator/commit/833139f55ee90c052f528241bbbf27a45d06a751 for the amo-validator

Fixed in https://github.com/mozilla/olympia/commit/c9c7302af2c2c0340dd079444fee1d00a623129c for the bump of the validator in olympia

Comment 28

[Madalin Cotetiu]

Verified as fixed in Ff38(Win7) in addons-dev.allizom.org
Postfix screencast: http://screencast.com/t/SiaU4wo1TzZ
Closing bug.