mozilla-services / autograph

Mozilla's digital signature service
https://hub.docker.com/r/mozilla/autograph/
Mozilla Public License 2.0
151 stars 35 forks source link

Support 'browser_specific_settings' in add-ons manifest #174

Closed wagnerand closed 5 years ago

wagnerand commented 5 years ago

Firefox supports 'browser_specific_settings' as a synonym for applications, however, autograph does not know about this, which leads to signing the file using the wrong add-on ID.

More info in https://github.com/mozilla/addons/issues/6180

g-k commented 5 years ago

Per discussion on slack, this is being added to addons-server: https://github.com/mozilla-services/autograph/blob/2bc1af88b684316e9dca7501f3f910c3b128329d/tools/sign-addon/sign.py#L73

But autograph's XPI /sign/file endpoint should support this (which might block https://github.com/mozilla/addons/issues/5936 too).

g-k commented 5 years ago

Could be handy to set or check the strict_min_version property for the COSE signature migration too.

g-k commented 5 years ago

As far as I can tell autograph's XPI /sign/file endpoint always takes the ID as a request param and doesn't try to pull it out of the XPI manifest: https://github.com/mozilla-services/autograph/blob/d33b9c90e377b962a70c5a6dc5fef0b7135ce643/signer/xpi/xpi.go#L292-L293

We might need to change the EE cert we generate though since we use the signer CN and fall back to the request ID:

https://github.com/mozilla-services/autograph/blob/d33b9c90e377b962a70c5a6dc5fef0b7135ce643/signer/xpi/xpi.go#L302-L311

then hash the CN to include in the cert DNSNames:

https://github.com/mozilla-services/autograph/blob/master/signer/xpi/x509.go#L52-L62

g-k commented 5 years ago

Maybe autograph should return a 400 error if the request ID doesn't match the ID from the manifest.json.

g-k commented 5 years ago

Per :aswan on slack:

If they're both present, the browser favors browser_specific_settings and warns that "applications" is ignored

EnTeQuAk commented 5 years ago

I'm starting to work on https://github.com/mozilla/addons/issues/5936 and I'm wondering if this is a blocker for this work to go live eventually, especially since that synonym will probably become more and more used in the future.

wagnerand commented 5 years ago

As far as I understand, as long as AMO always provides the add-on ID in the request, we should be good.

jvehent commented 5 years ago

Maybe autograph should return a 400 error if the request ID doesn't match the ID from the manifest.json.

I think AMO should do this. We should keep Autograph as focused on signing as possible.

As far as I understand, as long as AMO always provides the add-on ID in the request, we should be good.

This is correct, the ID needs to be set in the request and Autograph will simply use that value. https://github.com/mozilla-services/autograph/tree/master/signer/xpi#signature-request No parsing of manifest happens during signing, beyond the generation of the signature files.

wagnerand commented 5 years ago

@jvehent Can I ask that we reconsider the decision of closing this issue?

autograph does parse the manifest in https://github.com/mozilla-services/autograph/blob/2bc1af88b684316e9dca7501f3f910c3b128329d/tools/sign-addon/sign.py#L73

That part parses applications.gecko.id which is incomplete. I believe we should either completely remove that function (get_guid) and solely rely on the ID being part of the request or add browser_specific_settings.gecko.id to make autograph compliant to the specification.

jvehent commented 5 years ago

@wagnerand this tool a helper that's not in use anywhere, and I think we could remove it from the repository completely.