Closed Bug 1508939 (improve-principal-serialization) Opened 6 years ago Closed 5 years ago

Evaluate and improve persistent serialization format of Principals between sessions

Categories

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

Product:
Core
Component:
DOM: Security
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla69
Tracking Flags:
Tracking Status
firefox69 --- fixed

People

(Reporter: ckerschb, Assigned: jkt)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-backlog1])

Attachments

(1 file, 2 obsolete files)

Bug 1508939 - cpp serialization
(deleted), text/x-phabricator-request
Details
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.
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
Blocks: 1507773
Depends on: 965637, 1346759
(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.
Depends on: 1535378
Alias: improve-principal-serialization
Assignee: nobody → jkt
Attached file Bug 1508939 - rust serialization (obsolete) (deleted) —

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).

Attachment #9058679 - Flags: feedback?(nika)
Depends on: 1547271
Depends on: 1547707
Attached file Bug 1508939 - cpp serialization (deleted) —

Migrating to pure Cpp

Attachment #9061392 - Attachment description: Bug 1508939 - rust serialization → Bug 1508939 - pure cpp serialization
Attachment #9061392 - Attachment description: Bug 1508939 - pure cpp serialization → Bug 1508939 - cpp serialization

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!

Attachment #9061392 - Flags: feedback?(ckerschb)

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

I left detailed notes in phabricator!

Attachment #9061392 - Flags: feedback?(ckerschb)
Depends on: 1549168
Depends on: 1549981
Depends on: 1549997
Pushed by jkingston@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2e5db6145d9b cpp serialization r=ckerschb,mccr8,mikedeboer
Blocks: 1519813

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

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox69: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Regressions: 1556699
Pushed by rgurzau@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/05f1a2a0ad69 disabled testSessionFilePreservation on Android 4.3 r=aryx

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.

Attachment #9069634 - Attachment is obsolete: true
Backout by rgurzau@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c6f2121b3ca2 Backed out changeset 05f1a2a0ad69 for having the wrong bug number.
Attachment #9058679 - Attachment is obsolete: true
Attachment #9058679 - Flags: feedback?(nika)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: