Closed Bug 930792 Opened 7 years ago Closed 7 years ago

Implement cert.bundle in jwcrypto

Categories

(Core Graveyard :: Identity, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INVALID

People

(Reporter: jedp, Unassigned)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

For the Firefox Accounts client in FirefoxOS, we need to be able to bundle certs and assertions.
Whiteboard: [qa-]
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+
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
Closed: 7 years ago
Resolution: --- → INVALID
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.