Add clean and complete session save handler interface

[yohgaki]

With "User defined session serializer RFC", OO session save handler does not have to be intrusive to C written save handlers.
https://wiki.php.net/rfc/user_defined_session_serializer

This PR adds clean and complete session save handler interface (SessionSaveHandlerInterface) that does not use base class. i.e. No C written save handler base class.

SessionSaveHandlerInterface requires all required handlers for users to implement proper/optimal session save handler.

interface SessionSaveHandlerInterface {
  function bool open(string $save_path);
  function bool close(void);
  function string read(string $key);
  function bool write(string $key, string $data);
  function bool destroy(string $key);
  function int gc(int $maxlifetime);
  function string createId(void);
  function bool validateId($key);
  function bool updateTimestamp($key, $data);
}

Note: Older save handler has "create_sid" method that does not comply CODING_STANDARDS. It renamed to "createId".

When obsolete session save handler object/classes are removed, session module code could be cleaned up. i.e. State management codes to prevent session save handler misuse/abuse can be removed.

[Ocramius]

SessionHandlerInter?

[yohgaki]

Thanks. It's typo of "SessionHandlerInterface".

[Ocramius]

This interface seems to be extremely bloated and extremely implementation-detailed (which is why the savehandler interfaces out there are so full of edge cases).

Isn't it simpler to have following?

interface SessionSaveHandlerInterface
{
    /** just save or refresh the damn thing - metadata can contain arbitrary info, such as TTL too */
    public function save(SessionId $id, SessionMetadata $metadata, SessionData $session);
    public function destroy(SessionId $id);
    public function load(SessionId $id);
}

[yohgaki]

@Ocramius
Bloated code is intentional.
Older interfaces (and object) are obsolete and should be removed in the future. This is the reason why.

If we could remove older OO API, session module code, as well as user code, became a lot simpler and cleaner.

[yohgaki]

@Ocramius
About simplicity, the API is simple enough.

Simpler is better, but not too simple.
The proposed interface is not able to do the session management task correctly/fully. Too simple is wrong.

e.g. Without gc(), how to perform GC efficiently for the session data storage? Each handler has reason why it is needed. Exception is createId(), it is not needed strictly unless user want special session ID such as user id prefixed session id.

BTW, current session module does not manage session data by timestamp, it cannot work correctly. Session module is too simple to do the required task correctly. This has to be fixed also, but this issue is irrelevant to session save handler.

[Ocramius]

Bloated code is intentional.

Seriously, when designing a new interface, get this out of your mind.

If you want to design an API, do it properly or don't do it at all. This interface, once (and if) merged, will stick around for AGES, so it better be good.

Details like:

• close
• gc
• createId
• validateId
• updateTimestamp

are not necessary, and just move time-based coupling from the domain of the extension, making things much easier to break.

[Ocramius]

Also don't take the previously existing API as a starting point: that's a perfect way to shoot yourself in the foot.

[yohgaki]

@Ocramius

• close
• gc
• createId
• validateId
• updateTimestamp

are not necessary, and just move time-based coupling from the domain of the extension, making things much easier to break.

Your statement is simply wrong. It seems you are only considering your specific needs.

• close() is mandatory for long lived process like session data management helper tasks. i.e. Think of start and commit session multiple times. You may ends up with resource error. PHP does not need fclose() mostly, but there is fclose() API because it is mandatory in some cases. close() is mandatory for this reason.
• gc() is mandatory. Session module is supposed to do GC. How could session module know how to perform GC without gc()?
• createId() is the only exception that could be optional, but there is session_create_id(). Users can simply call it if they don't need custom session ID. BTW, keeping track of user's session IDs is very important for security. Easiest and efficient technique for this is to use custom session ID. It is mandatory for apps that requires user ID security.
• validateId() is mandatory to make sure session ID has no collision and supplied session ID is valid or not. i.e. It's mandatory for security reasons.
• updateTImestamp() is mandatory for faster session operations. If handler cannot support it, user may call write() instead. It's simple one liner.

Just because you don't need features and/or ignore some security issues, it does not mean it's the recommended and/or correct way to do the task. The API is "For general purpose", "Not for your specific needs".

just move time-based coupling from the domain of the extension, making things much easier to break.

I don't understand what you exactly mean by this. Are you insisting GC should be performed like memcached backend? (i.e. Automatic expiration by storage engine)

Also don't take the previously existing API as a starting point: that's a perfect way to shoot yourself in the foot.

The API is good mostly.
Reinventing wheels is waste of time.

The reason why new handler (validateId, etc) being optional is "compatibility" for older/obsolete save handlers.

Your suggested new API simply does not work as low level session save handler API. It seems you mixed up higher level API design and low level API design.

interface SessionSaveHandlerInterface
{
    /** just save or refresh the damn thing - metadata can contain arbitrary info, such as TTL too */
    public function save(SessionId $id, SessionMetadata $metadata, SessionData $session);
    public function destroy(SessionId $id);
    public function load(SessionId $id);
}

[yohgaki]

@Ocramius
Since you insists validateId() is not needed, you seems you don't understand the reasons why session.use_strict_mode=1 is important. Please research why this is important.

[bwoebi]

validateId() is not needed. Your session identifier is supposed to be sufficiently random so that a collision is exceedingly unlikely and not to be expected until the end of universe.

updateTimestamp() is not needed either, just call write(). A write() handler may check itself whether the session has been changed since read, if it's worth it.

gc() is not mandatory. A cron job may do it. And in case it's not, the session handler may take care of it itself in e.g. constructor (i.e. execute if (mt_rand(0, 10000) == 0) { … } and delete any sessions older than a certain time). There's no need for a gc() function called from within ext/session.

And createId() … not necessary either, just grab a few pseudorandom bytes and use them. [Everything else is a security risk IMHO.]

[yohgaki]

Since session is closely related to authentication and privacy, it must be as secure as possible by default. IMO.

For example, shopping sites that allow to add product without authentication can leak cart contents to attacker without session ID validation. When attackers has physical access device, (e.g. colleague, shared pc, etc), it's easy to set session ID for attack. Attackers do not even have to keep session ID alive. There are other attack scenarios.
(BTW、this mitigation is less effective without timestamp mangement and automatic session ID regeneration. i.e. https://wiki.php.net/rfc/precise_session_management )

One may argue "these risk are negligible", but this isn't how we secure softwares. We should remove risks that can be removed easily.

Currently, validating session ID has extra overhead, but I'm going to propose remove this overhead. So there will be no overhead at all soon.

[Ocramius]

128 bits of entropy are a bit much for most DDoSs. Regardless of that, you could add it to the interface, and it would still be better than the old legacy API:

interface SessionSaveHandlerInterface
{
    public function create() : SessionId;
    /** just save or refresh the damn thing - metadata can contain arbitrary info, such as TTL too */
    public function save(SessionId $id, SessionMetadata $metadata, SessionData $session) : void;
    public function destroy(SessionId $id) : void;
    public function load(SessionId $id) : SessionData;
}

[yohgaki]

BTW, could anyone tell the best way to introspect and validate function/method parameters and return type? It seems Zend does not have dedicated API currently.

[KalleZ]

Honestly, after watching this discussion for the better part of the day, I think to make both parties happy and the many changes to the module. Perhaps you two should collab on creating a session module with a solid API from day one, and then later on a compatibility layer for session_*() functions and such can be added. Seems like a much more flawless way to improve the extension as a whole.

[Ocramius]

@KalleZ the only session module needed over here is one that just defines the $_SESSION superglobal - the rest can be done 100% in userland.

[KalleZ]

I know. It just seems more healthy for the extension and the future session API that its gonna be clean, now @yohgaki have done a great job and improving it in steps, but when I look at the API it seems like a patch on a previous hack and I think it is time to clean it up, hence my suggestion for you two to perhaps work something out :)

[php-pulls]

Comment on behalf of krakjoe at php.net:

labelling

[krakjoe]

This RFC was declined, closing.