Implement cert.bundle in jwcrypto

RESOLVED INVALID

Status

()

Core
Identity
RESOLVED INVALID
5 years ago
5 years ago

People

(Reporter: jedp, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

For the Firefox Accounts client in FirefoxOS, we need to be able to bundle certs and assertions.
Whiteboard: [qa-]
Created attachment 822177 [details] [diff] [review]
implements jwcrypto.cert.bundle([certs], signedAssertion)
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!
Attachment #822177 - Flags: feedback?(warner-bugzilla)
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"
Attachment #822177 - Flags: feedback?(warner-bugzilla) → feedback+
Created attachment 824955 [details] [diff] [review]
implements jwcrypto.cert.bundle([certs], signedAssertion)

Fixed patch to include valid headers
Attachment #822177 - Attachment is obsolete: true
Blocks: 935245
No longer blocks: 929388
(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.
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.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.