Closed
Bug 1070188
Opened 10 years ago
Closed 9 years ago
Modify the validator to warn about pre-existing signatures
Categories
(addons.mozilla.org Graveyard :: Add-on Validation, defect, P3)
addons.mozilla.org Graveyard
Add-on Validation
Tracking
(Not tracked)
VERIFIED
FIXED
2015-05
People
(Reporter: clouserw, Assigned: magopian)
References
Details
Attachments
(1 file)
(deleted),
application/x-xpinstall
|
Details |
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.
Updated•10 years ago
|
Assignee: nobody → olivier
Comment 1•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
New PR: https://github.com/mozilla/amo-validator/pull/268
Assignee: olivier → mathieu
Target Milestone: --- → 2015-02
Assignee | ||
Comment 4•10 years ago
|
||
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."
Assignee | ||
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 5•10 years ago
|
||
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•10 years ago
|
||
I have submitted the add-on from comment 5 but the mentioned warning message was not displayed. Screenshot: http://screencast.com/t/wQU1WzSsm
Assignee | ||
Comment 7•10 years ago
|
||
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?
Flags: needinfo?(rtilder)
Flags: needinfo?(dveditz)
Comment 8•10 years ago
|
||
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.
Flags: needinfo?(rtilder)
Assignee | ||
Comment 9•10 years ago
|
||
Ryan, does that also apply with the "old" way of (self?) signing the XPIs?
Comment 10•10 years ago
|
||
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.
Assignee | ||
Comment 11•10 years ago
|
||
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?
Flags: needinfo?(dveditz) → needinfo?(rtilder)
Assignee | ||
Comment 12•10 years ago
|
||
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)
Flags: needinfo?(rtilder)
Comment 13•10 years ago
|
||
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.
Updated•10 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 14•10 years ago
|
||
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•10 years ago
|
||
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.
Assignee | ||
Comment 16•10 years ago
|
||
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!
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Comment 18•10 years ago
|
||
I think it's fine to only show it in the full report. It's not such a critical message.
Flags: needinfo?(jorge)
Comment 20•9 years ago
|
||
The issue started to reproduce again. Screencast for this issue: http://screencast.com/t/cYnkdNWGAw Reopening.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 21•9 years ago
|
||
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
Assignee | ||
Comment 23•9 years ago
|
||
Fixed in https://github.com/mozilla/amo-validator/commit/3477b97134c7446b93532608a6a8cf8b828272a3
Status: REOPENED → RESOLVED
Closed: 10 years ago → 9 years ago
Resolution: --- → FIXED
Target Milestone: 2015-02 → 2015-05
Comment 24•9 years ago
|
||
Issue is still reproducing in -dev http://screencast.com/t/Ml0FFJ1oKrR Reopening bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 25•9 years ago
|
||
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
Assignee | ||
Comment 27•9 years ago
|
||
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
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Comment 28•9 years ago
|
||
Verified as fixed in Ff38(Win7) in addons-dev.allizom.org Postfix screencast: http://screencast.com/t/SiaU4wo1TzZ Closing bug.
Status: RESOLVED → VERIFIED
Updated•9 years ago
|
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•