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.
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.
Needs to be reviewed: https://github.com/mozilla/olympia/pull/386
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."
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...
I have submitted the add-on from comment 5 but the mentioned warning message was not displayed. Screenshot: http://screencast.com/t/wQU1WzSsm
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?
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.
Ryan, does that also apply with the "old" way of (self?) signing the XPIs?
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.
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?
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)
Created attachment 8573834 [details] 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.
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?
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.
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!
Ok we will wait for :jorgev opinion on this.
I think it's fine to only show it in the full report. It's not such a critical message.
Thanks for the response. Closing bug.
The issue started to reproduce again. Screencast for this issue: http://screencast.com/t/cYnkdNWGAw Reopening.
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
Issue is still reproducing in -dev http://screencast.com/t/Ml0FFJ1oKrR Reopening bug.
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
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
Verified as fixed in Ff38(Win7) in addons-dev.allizom.org Postfix screencast: http://screencast.com/t/SiaU4wo1TzZ Closing bug.