Modify the validator to warn about pre-existing signatures

VERIFIED FIXED in 2015-05

Status

addons.mozilla.org Graveyard
Add-on Validation
P3
normal
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: clouserw, Assigned: magopian)

Tracking

unspecified
2015-05

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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.
Assignee: nobody → olivier
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
(Assignee)

Comment 3

3 years ago
New PR: https://github.com/mozilla/amo-validator/pull/268
Assignee: olivier → mathieu
Target Milestone: --- → 2015-02
(Assignee)

Comment 4

3 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

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 5

3 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

3 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

3 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)
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

3 years ago
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.
(Assignee)

Comment 11

3 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

3 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

3 years ago
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.

Updated

3 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 14

3 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

3 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

3 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
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED

Comment 17

3 years ago
Ok we will wait for :jorgev opinion on this.
Flags: needinfo?(jorge)
I think it's fine to only show it in the full report. It's not such a critical message.
Flags: needinfo?(jorge)

Comment 19

3 years ago
Thanks for the response.
Closing bug.
Status: RESOLVED → VERIFIED

Comment 20

2 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

2 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 22

2 years ago
PR: https://github.com/mozilla/amo-validator/pull/283
(Assignee)

Comment 23

2 years ago
Fixed in https://github.com/mozilla/amo-validator/commit/3477b97134c7446b93532608a6a8cf8b828272a3
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: 2015-02 → 2015-05

Comment 24

2 years ago
Issue is still reproducing in -dev http://screencast.com/t/Ml0FFJ1oKrR
Reopening bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 25

2 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 26

2 years ago
Third PR : https://github.com/mozilla/amo-validator/pull/284
(Assignee)

Comment 27

2 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
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED

Comment 28

2 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
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.