1392598 - [Shield] [Admin] [Opt-Out] /Extension/ page edit logic should be reviewed

Status: RESOLVED FIXED

(Shield :: General, defect)

Description

[Adrian Florinescu [:aflorinescu]]

My concern with the current logic of the admin is that /control/extension/ page has no control/review mechanism in place. 
As far as I can guess, the fact that you can edit extensions at any time might impact ongoing studies and result in unexpected results.

From testing point of view it is a good testing environment to simulate what happens when an Opt-Out study gets its add-on changed, but from functionality point of view, I believe that once a recipe is using an extension, that extension should be locked in place. 

The above calls for a delete function to be able to correct mistakes and a logical delete should be good enough for the purpose.

Comment 1

[Adrian Florinescu [:aflorinescu]]

As per todays' discussion with :Osmose, we arrived to the conclusion that the admin /control/extension/ should perform a check when uploading/saving a new extension:
- if legacy add-on, it should be signed (with !!! on the thing that we need it to be Mozilla signed in order for it not to get bounced back by an update);
- if web extension, it should be signed and should have ID.

Comment 2

[Michael Cooper [:mythmon]]

Talking with Osmose, we decided that this feature should be a convenience feature for authors, not a security feature to prevent bad add-ons from being uploaded. Because of this, I chose to only verify that add-ons appear to be signed, trusting that users are acting in good faith. Users not acting in good faith get brokenness, and I'm not trying to help those users.

Because of this, we cannot verify the signing key used for an extension is the special key that allows legacy add-ons to work on 57+. However, there also aren't any standard methods for signing legacy add-ons with non-special keys, so I think this is also ok.

The code to do this has landed on master. I'll resolve this bug when we deploy it.

Comment 3

[Adrian Florinescu [:aflorinescu]]

(In reply to Mike Cooper [:mythmon] from comment #2)
> Talking with Osmose, we decided that this feature should be a convenience
> feature for authors, not a security feature to prevent bad add-ons from
> being uploaded. Because of this, I chose to only verify that add-ons appear
> to be signed, trusting that users are acting in good faith. Users not acting
> in good faith get brokenness, and I'm not trying to help those users.
> 
> Because of this, we cannot verify the signing key used for an extension is
> the special key that allows legacy add-ons to work on 57+. However, there
> also aren't any standard methods for signing legacy add-ons with non-special
> keys, so I think this is also ok.
> 
> The code to do this has landed on master. I'll resolve this bug when we
> deploy it.

Mike, could you please clarify the above? 
The original discussion with :osmose (comment 2) was to have a validation in /extension/ admin side for:
-web-extension has an ID 
-add-ons are signed. 
From my understanding, your comment/fix should cover that?!? 

So what would remain to do is to actually check if the add-on is signed with the correct key - which would be really cool thing to have, but not required from my point of view, since the creator of the Opt-Out would need to be responsible for using the right add-on, which to my understanding will be tested separately.

Comment 4

[Adrian Florinescu [:aflorinescu]]

typo -> I was referring to comment 1 (The original discussion with :osmose (comment 1) was to have a validation in /extension/ admin side for:)

Comment 5

[Michael Cooper [:mythmon]]

All those words were mostly to say that I am not implementing this part of comment 1:

> (with !!! on the thing that we need it to be Mozilla signed in order for it not to get bounced back by an update)

and that I'm not actually cryptographically verifying the signatures, since this is a convenience feature, not a security one.

These changes landed in v71 of Normandy, and v72 is deployed to prod now, so I'm marking this as resolved.