1508939 - (improve-principal-serialization) Evaluate and improve persistent serialization format of Principals between sessions

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P3)

Description

[Christoph Kerschbaumer [:ckerschb]]

We should evaluate and improve the current mechanisms for persistent serialization of principals between sessions.
We need to at least find and document answers for the following questions:

*) If a Principal gets serialized and code changes around serialization happen in the meantime, what do we do if we can't deserialize the Principal anymore? Save fallbacks? Should we fall back to using the SystemPrincipal to make sure loading is allowed (not blocked by the principal?)

*) If we fall back to using NullPrincipals, then we should definitely wait till Bug 1346759 has landed, because currently we evaluate equality of NullPrincipals based on pointer equality which definitely breaks equality assumptions of serialized NullPrincipals.

*) Currently the CSP hangs off the Principal (but is not serialized with Principals). Bug 965637 will move the CSP away from the Principal which should also simplify serialization efforts within this bug.

*) We need strong serialization/deserialization tests for all kinds of Principals (NullPrincipal, SystemPrincipal (which should be easier because it's a singleton), CodebasePrincipals as well as expanded Principals).

*) Probably I missed thing within this initial inventory of our serialization efforts, but we can document as we dig deeper into the problem.

Comment 1

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #0)
> *) Probably I missed thing within this initial inventory of our
> serialization efforts, but we can document as we dig deeper into the problem.

One more: they rely on how URIs get serialized, and we sometimes change how the same URI spec gets parsed and thus serialized, and the current code is not set up for that.

Comment 2

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1508939 - rust serialization

Comment 3

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 9058679 [details]
Bug 1508939 - rust serialization

Hey Nika, would you be able to give some initial feedback to this approach. Some neatening is needed (including making the proc-macro actually write the code and not be hard coded).

Comment 4

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1508939 - cpp serialization

Migrating to pure Cpp

Comment 5

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 9061392 [details]
Bug 1508939 - cpp serialization

Hey :ckerschb, could you give a first round of feedback on this pure cpp patch please?

Three notable changes I would like to do next:

• Share symbols between the serialization and deserialization code rather than hardcode "Content" "Null" etc.
• Move the BasePrincipal::FromJSON to have a Principal specific function on Content/NullPrincipal etc rather than a giant function (I had some issues with linking jsoncpp preventing adding Json::Value as a method signature)
• Make a function to check if a JSON member is present, if it's a string and then convert to a nsCString (Rather than have content.isMember("domain") code everywhere)

Thanks!

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 9061392 [details]
Bug 1508939 - cpp serialization

I left detailed notes in phabricator!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e5db6145d9b
cpp serialization r=ckerschb,mccr8,mikedeboer

Comment 8

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/2e5db6145d9b

Comment 9

[Raul Gurzau (:RaulG)]

Attachment: Bug 1508939 - disabled testSessionFilePreservation on Android 4.3 r=Aryx

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05f1a2a0ad69
disabled testSessionFilePreservation on Android 4.3 r=aryx

Comment 11

[Phabricator Automation]

Comment on attachment 9069634 [details]
Bug 1508939 - disabled testSessionFilePreservation on Android 4.3 r=Aryx

Revision D33627 was moved to bug 1556667. Setting attachment 9069634 [details] to obsolete.

Comment 12

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c6f2121b3ca2
Backed out changeset 05f1a2a0ad69 for having the wrong bug number.