930792 - Implement cert.bundle in jwcrypto

Status: RESOLVED INVALID

(Core Graveyard :: Identity, defect)

Description

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

For the Firefox Accounts client in FirefoxOS, we need to be able to bundle certs and assertions.

Comment 1

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: implements jwcrypto.cert.bundle([certs], signedAssertion)

Comment 2

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Comment on attachment 822177 [details] [diff] [review]
implements jwcrypto.cert.bundle([certs], signedAssertion)

Hi, Brian, Do you mind taking a quick look?  Thumbs up?  Thumbs down?  Thanks!

Comment 3

[Brian Warner [:warner :bwarner]]

Comment on attachment 822177 [details] [diff] [review]
implements jwcrypto.cert.bundle([certs], signedAssertion)

Looks like a good idea.

I believe that generateAssertion() currently already bundles the cert with the generated signed assertion, so your test case might get a duplicate cert in "bundle" ("cert~cert~assertion"). I'm undecided as to whether generateAssertion() should do the bundle() for you, or if callers should always call bundle() on the result. I guess I tend towards the latter; passing "aCert" into generateAssertion() seems weird. But maybe we should just rename generateAssertion() to generateBundle or generateSignedAssertion or generateIdentityBackedAssertion or whatever the id-specs docs use. If we change it, we'd need to track down all callers and update them too, of course.

You might enhance the test to split the bundled string on "~" and confirm that the pieces match the input (which would also confirm-or-deny the double-"cert" problem). And also join more than just one cert to make sure that works correctly: bundle(["c1", "c2"], "a")=="c1~c2~a"

Comment 4

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: implements jwcrypto.cert.bundle([certs], signedAssertion)

Fixed patch to include valid headers

Comment 5

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Brian Warner [:warner :bwarner] from comment #3)
> Comment on attachment 822177 [details] [diff] [review]
> implements jwcrypto.cert.bundle([certs], signedAssertion)
> 
> Looks like a good idea.
> 
> I believe that generateAssertion() currently already bundles the cert with
> the generated signed assertion

Ah you are so right.  Fool that I am.

> ... I'm undecided as to whether
> generateAssertion() should do the bundle() for you, or if callers should
> always call bundle() on the result. I guess I tend towards the latter;

That was what I had in mind, yes.

> passing "aCert" into generateAssertion() seems weird. But maybe we should
> just rename generateAssertion() to generateBundle or generateSignedAssertion
> or generateIdentityBackedAssertion or whatever the id-specs docs use. If we
> change it, we'd need to track down all callers and update them too, of
> course.

I will maintain parity with the jwcrypto in the Persona shim.

There are no callers in Firefox at the moment.  I don't know about add-ons.

Comment 6

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

So these api bits are actually out of sync with the published spec.  I've opened bug 936146 to make the api conform.  That will resolve the weirdness pointed out in Comment 3, in which generateAssertion() takes a cert as its first argument.

I'll close this as invalid.  Please re-open if I'm wrong.