Page MenuHomePhabricator

Add serialization options to RESTBagOStuff
Closed, ResolvedPublic

Description

Task T215533: Enable use of session storage service in MediaWiki, as part of T206016: Create a service for session storage and in support of T88445: MediaWiki active/active datacenter investigation and work (tracking), modified RESTBagOStuff for use in storing sessions in the external Kask service. With T161647: RFC: Deprecate using php serialization inside MediaWiki in mind, it serialized sessions to JSON.

However, this caused T233146: Cannot enable 2FA on testwiki and per discussion in T233537: Document and communicate potentially breaking session storage serialization change breaks backward compatibility without deprecation. Because the release of mediawiki 1.34 is fast approaching, we need a solution that allows Kask development/rollout to continue, while still respecting the deprecation policy.

To solve this, recognize an additional value in the $params passed to the constructor to specify serialization type, with choices "PHP" and "JSON", with a default of "PHP".

This has the following benefits (copied from T233537, for more context see the discussion there):

  1. maintains backward compatibility for any existing callers (even though we are not aware of any)
  2. still allows us to use JSON for sessions stored in Kask if we choose to (we'd just specify "JSON" in the config)
  3. permanently avoids the need for autodetect backward compatibility code. If we use JSON for sessions stored in Kask, we can migrate production via MultiWriteBagOStuff. Any other callers will find the new serialization type option helpful should they also choose to migrate
  4. allows us to deprecate then remove PHP serialization from RESTBagOStuff in a friendly and deprecation-policy-compliant way, should we choose to do so
  5. lets us get 1.34 out the door cleanly.

Additionally, add an optional 'hmac_key' $params value. If included, it will be used to protect the serialized blob.

Event Timeline

BPirkle created this task.Sep 26 2019, 4:03 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 26 2019, 4:03 PM

Change 539382 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[mediawiki/core@master] Add optional serialization_type and hmac_key param values to RESTBagOStuff

https://gerrit.wikimedia.org/r/539382

BPirkle triaged this task as High priority.Oct 4 2019, 2:42 PM
BPirkle added a project: MW-1.34-release.

Change 539382 merged by jenkins-bot:
[mediawiki/core@master] Add optional serialization_type and hmac_key param values to RESTBagOStuff

https://gerrit.wikimedia.org/r/539382

Reedy added a comment.Oct 7 2019, 4:05 PM

@BPirkle can I get you to chip in on https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/OATHAuth/+/538154/ and https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/WebAuthn/+/538155/ please?

Now the core patch is merged (and will be backported to REL1_34), I'm not sure if we should go ahead with the patches above as they exist, or we should make some other changes to them, or do something slightly/completely differently

Whatever happens, I will backport the changes to those extensions to REL1_34 to match

Thanks!

Change 541643 had a related patch set uploaded (by Jforrester; owner: BPirkle):
[mediawiki/core@REL1_34] Add optional serialization_type and hmac_key param values to RESTBagOStuff

https://gerrit.wikimedia.org/r/541643

Change 541643 merged by jenkins-bot:
[mediawiki/core@REL1_34] Add optional serialization_type and hmac_key param values to RESTBagOStuff

https://gerrit.wikimedia.org/r/541643

@Reedy, the OATHAuth and WebAuthn patches look like they're generally on the right track to me and unless that approach proves to be unnecessarily painful, I'd recommend continuing that direction.

This change (the one I'm commenting on, which modified RESTBagOStuff) gave us the flexibility to pick either PHP or JSON serialization for sessions stored in Kask. But that leaves the open question "which are we actually going to use", which was raised in T233537: Document and communicate potentially breaking session storage serialization change.

Which leaves the people working on OATHAuth and WebAuthn in a rough spot, because you need to know that things you store in sessions will work.

The safest approach is to do exactly what those patches are already doing - ensure that you aren't storing proper PHP objects. The alternative is to go ahead and have the discussion about which format we're actually going to use. But I think we're all on various short timelines and resolving that quickly might be challenging.

Is that helpful at all?

I made a comment on one of the patches regarding nesting, and some test code that I played around with locally.

Reedy added a comment.EditedOct 10 2019, 5:10 PM

@Reedy, the OATHAuth and WebAuthn patches look like they're generally on the right track to me and unless that approach proves to be unnecessarily painful, I'd recommend continuing that direction.
This change (the one I'm commenting on, which modified RESTBagOStuff) gave us the flexibility to pick either PHP or JSON serialization for sessions stored in Kask. But that leaves the open question "which are we actually going to use", which was raised in T233537: Document and communicate potentially breaking session storage serialization change.
Which leaves the people working on OATHAuth and WebAuthn in a rough spot, because you need to know that things you store in sessions will work.
The safest approach is to do exactly what those patches are already doing - ensure that you aren't storing proper PHP objects. The alternative is to go ahead and have the discussion about which format we're actually going to use. But I think we're all on various short timelines and resolving that quickly might be challenging.
Is that helpful at all?
I made a comment on one of the patches regarding nesting, and some test code that I played around with locally.

Thanks, yeah I mostly just wanted a sanity check. Will have a look at and talk to Dejan about the nested stuff if necessary.

Yeah, I think this makes sense. The code itself isn't actually turning things into json (I had that in my head from looking before, but that was definitely a blurred memory), it's mostly turning them into PHP arrays, rather than passing php objects (past behaviour that worked with serialize but doesn't with json_encode), which is where the problems with this stemmed. And as php arrays can be dealt with by both, the two extensions don't really need to care about how MW is encoding them for storage in the sessions (as it should be)

Will test them locally too myself, but I think that's more than enough to fix T233146, and hopefully prevent any future problems from the assumption that the session code would happily accept PHP objects and deal with them

And we've got the documentation and deprecation plan in place now (thanks!) as we shouldn't have to worry about unexpected and un-notified breaking changes happening

BPirkle closed this task as Resolved.Oct 16 2019, 10:31 PM