593651 - Make it easier for jetpack packages to be used as examples and collaborated-on

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Atul Varma [:atul]]

It's currently very difficult to collaborate on a jetpack-based addon with others using the SDK because it requires that the developers either share the same private key, or that they all have different "id" values in their package.json file, which complicates version control a lot.

Furthermore, example packages such as the ones contained in the SDK's 'examples' directory also become problematic to experiment with, as just issuing 'cfx run' on them destructively modifies their package.json, which complicates SDK version control.

Running 'cfx develop' also destructively modifies packages/development-mode/package.json, complicating SDK version control.

We need some kind of solution around this. Since there isn't actually any way to sign/verify an XPI with an addon's key, it seems like the easiest way around this right now is to simply not complain if an addon's private key can't be found--or perhaps display a warning only if the user is creating an XPI, telling them that in the future they'd need the private key to sign the addon, but continuing with the process.

Comment 1

[Atul Varma [:atul]]

Note that the SDK version control complications are relatively minor, since they're really just paper cuts to anyone who has a cloned repository rather than the clientele of the SDK at large.

However, the collaboration issue is big. As a real-world example, Jono and I were trying to work on the jetpack at http://github.com/toolness/pakreport-jetpack together and the private key issues derailed this.

I think it's reasonable to expect the private key to be required to publish an addon to the public at large, but requiring it for simple development and testing is a frustrating barrier that was never present in XUL-based addon development.

I know this probably isn't a p1 blocker, but I'm setting it to one for now so it at least gets noticed as a massive cardboard-induced paper cut. I'm willing to work on a patch for it, I just want to make sure the solution we decide on is ok with warner.

Comment 2

[Atul Varma [:atul]]

Attachment: patch that just warns when privkey isn't found

Comment 3

[Atul Varma [:atul]]

Comment on attachment 472224 [details] [diff] [review]
patch that just warns when privkey isn't found

Here's an implementation of my proposed solution. Explanation text displayed when a private key can't be found is now changed to:

  In the future, you won't be able to publish your addon to the rest of
  the world unless you either remove the 'id' property from package.json,
  so that we can generate a new id and keypair, or put your private key
  somewhere that we can find it.

and cfx continues to run instead of aborting.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Right, this isn't a P1 blocker for the 0.8 release, since it doesn't have to be fixed in order for us to go beta.  Nevertheless, this definitely does feel like a blocker for 1.0 final, and I'd love to get a fix for it as soon as possible.

Note: Brian is away for a week, and I don't think he'll be reviewing patches until he returns.  He gets back on the 13th, though, so still in time to land something for 0.8.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 472224 [details] [diff] [review]
patch that just warns when privkey isn't found

Reviewing in the interest of moving this forward, as it is a significant developer ergonomics issue.


> Note that the SDK version control complications are relatively minor, since
> they're really just paper cuts to anyone who has a cloned repository rather
> than the clientele of the SDK at large.

Right, although it's still worth improving the experience for those folks where we can; and if we ever implement automatic updates, then this becomes a problem for all developers.


>+In the future, you won't be able to publish your addon to the rest of
>+the world unless you either remove the 'id' property from package.json,
>+so that we can generate a new id and keypair, or put your private key
>+somewhere that we can find it.

I definitely agree with the goal of this patch, and the code looks great.

This message, however, unlike the original, is cryptic, ominous, and discomfiting.  I feel like I'm being chastened for doing something wrong, but it's not clear what (hiding my private key somewhere you can't find it, like a kid with his Halloween candy?); "the future" is a pretty wide time range (albeit narrowing all the time, according to some physical cosmologists); and "publish your addon to the rest of the world" could mean several things (at least one of which is certainly incorrect).

Also, while this patch accomplishes the primary goal of enabling developers to collaborate on addon software development projects without copying private keys, it doesn't accomplish the secondary goal of enabling them to try the example applications included in the SDK without destructively modifying them.


I can imagine a couple of options to address the issue with the new message:

1. Change the message to matter-of-factly explain the problem and its possible solutions (i.e. something like the original message, but converting it from an error to a warning).
2. Stop displaying the message, given that we don't currently have a specific plan to require signatures to distribute addons on AMO and/or install them into Firefox, so ignoring the problem does no harm and won't trip up developers for the foreseeable future.

And several to resolve the secondary problem:

3. Move the ID reference to a separate file that can be gitignored (and would not conflict with an automatic update to a newer version of an example app's package.json file).
4. Allow the keys file to live in the package directory (f.e. at package-dir/.keys), and ship example apps with their keys in that location (using a single private key that we revoke in any application that checks them), although continue to write them to ~/.jetpack/keys/ by default.
5. Stop generating keys and signing XPI bundles by default, but add a configuration option to enable the feature, so we can iterate on it (and the AMO/Firefox features that it requires to achieve its goals) without inconveniencing developers in the meantime.  We might also label the feature (and others we haven't yet spent enough time on and that haven't yet matured, like `cfx develop`) as "experimental".


Of these, I strongly encourage you to do least #1 or #2 (preferably the latter), although I won't block review on it; and consider doing either #3, #4, or #5 (although I am amenable to doing that in a separate bug).

Comment 6

[Atul Varma [:atul]]

Comment on attachment 472224 [details] [diff] [review]
patch that just warns when privkey isn't found

Cool, I am asking warner for feedback so it shows up on his radar when he gets back, and we can discuss it then.

Comment 7

[Atul Varma [:atul]]

Oh, and note that another option, which isn't ideal but works for now, is to simply have all the examples and development-mode come with an ID that simply has no publicly-available private key. i.e., one of us could just cd into all the relevant package directories, run "cfx run" to generate a keypair for them, and then commit the modified package.json's to the repository. This would effectively make them un-publishable XPIs, at least once we actually add signing and verifying logic to the platform.

Comment 8

[Brian Warner [:warner :bwarner]]

Hmm.

The reason we added the cryptographic JIDs and preflight check so early
(before having a full plan for using them) was to have a stable add-on
identifier, and to get developers into the habit of keeping track of those
private keys. We could back off on that, but it might make it hard to bring
it back later.

What about a class of non-cryptographic JIDs which mean "don't apply any sort
of enforcement during add-on installation"? We could publish examples with
these IDs (which would still need to be unique, of course, so they could
continue to be used to index simple-storage/etc), and the preflight code
would skip its private-key-is-available check when it sees a JID in this
format.

We currently use "jid0-"+XYZ (where XYZ is the hash of the pubkey) as the
JID, and "jid0-"+XYZ+"@jetpack" as the "program id". What if
"jid-anonymous-"+ABC (where ABC was a short randomly-generated string) were
used as the "don't check this" marker? Or "jid-sample-"+ABC?

I'm not entirely sure how to address the shared-source-repo with this,
however. In the examples that we publish, it's easy enough to ship a
package.json with the non-enforced value. But when Alice creates an add-on,
then shares the code with Bob, how should she learn to edit her package.json
and replace it with the marker? Maybe the "you don't seem to have a private
key" message could mention "replace jid0-XYZ with
jid-anonymous-DIRECTORYNAME" as a third option.


I'm ok with this patch if we can't find a better way, and I fully acknowledge
the pain-point of collaboration and example addons with the current scheme,
but I'm slightly nervous about effectively disabling the check without having
a way to teach people how to generate correct long-term-stable JIDs for
production (i.e. published) add-ons.

Comment 9

[Brian Warner [:warner :bwarner]]

Comment on attachment 472224 [details] [diff] [review]
patch that just warns when privkey isn't found

(f+ to make it clear that this patch does what it's supposed to do, and is a good way of accomplishing that goal, even if I'm still concerned about the goal itself)

Comment 10

[Atul Varma [:atul]]

Attachment: updated warning text

Comment 11

[Atul Varma [:atul]]

Attachment: added list of EXAMPLE_JIDS in preflight.py to ignore if no privkey is found.

Comment 12

[Atul Varma [:atul]]

Attachment: replaced EXAMPLE_JIDS with warner's jid0-anonymous- suggestion.

Comment 13

[Atul Varma [:atul]]

Comment on attachment 475376 [details] [diff] [review]
replaced EXAMPLE_JIDS with warner's jid0-anonymous- suggestion.

Myk, can you take a quick look at the new copy and the implementation of warner's jid0-anonymous- proposal for not warning on examples and development-mode?

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 475376 [details] [diff] [review]
replaced EXAMPLE_JIDS with warner's jid0-anonymous- suggestion.

jid0-anonymous- seems like a fine fix for the examples problem.  And the warning is much better now, although I still think it's way too long and scary to display every time someone runs |cfx run| and will have wolf crying consequences.

Comment 15

[Atul Varma [:atul]]

Thanks! I agree about the wolf-crying consequences... Hmm, hopefully we'll be able to figure out a better long-term solution by the next release. At least this gets rid of the major paper cuts.

Bug 593651 - Make it easier for jetpack packages to be used as examples and collaborated-on
Atul Varma
http://hg.mozilla.org/labs/jetpack-sdk/rev/e6307bc5d5ad

Comment 16

[Brian Warner [:warner :bwarner]]

oh, one comment: please make it "jid-anonymous-XYZ", not "jid0-anonymous-XYZ", so we can distinguish them from real ones later. Or any other prefix than "jid0-".

Comment 17

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".