[SERVER-38335] killAllSessions can kill itself if the command body contains a lsid field Created: 30/Nov/18  Updated: 29/Oct/23  Resolved: 04/Feb/19

Status: Closed
Project: Core Server
Component/s: Sharding
Affects Version/s: 4.1.5
Fix Version/s: 4.1.8

Type: Bug Priority: Major - P3
Reporter: Randolph Tan Assignee: Randolph Tan
Resolution: Fixed Votes: 1
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
related to SERVER-78647 Remove the attachLogicalSessionsToOpC... Backlog
is related to SERVER-44096 killAllSessionsByPattern can kill its... Closed
Backwards Compatibility: Fully Compatible
Operating System: ALL
Sprint: Sharding 2018-12-17, Sharding 2018-12-31, Sharding 2019-01-14, Sharding 2019-01-28, Sharding 2019-02-11
Participants:

 Description   

The command in mongos kills all sessions and then sends the same killAllSessions to all shards. If the matcher is empty and the killAllSessions command has a lsid field, it will also end up calling killOp on itself since it is technically using a session. The remote killAllSessions call to every shard will still succeed since it is using a different operation context than the original command, but the original command will get interrupted early and will not wait for the remote commands to finish.

This is kind of problematic for drivers (and mongo shell) since every request is using an implicit session. As a result, there is currently no reliable way to synchronously block and wait for killAllSessions to complete before proceeding.



 Comments   
Comment by Githook User [ 24/May/19 ]

Author:

{'name': 'Jeremy Mikola', 'email': 'jmikola@gmail.com', 'username': 'jmikola'}

Message: Note that drivers can ignore killAllSessions interruptions (#550)

Prior to SERVER-38335, killAllSessions might kill its own session (implicit or explicit). In practice, this does not pose a risk for blocking subsequent tests due to a lingering, open transaction so drivers should be OK to simply disregard this error.
Branch: master
https://github.com/mongodb/specifications/commit/309ba5e6ada77b27365d2c01ae640fee6442bbc6

Comment by Githook User [ 04/Feb/19 ]

Author:

{'name': 'Randolph Tan', 'email': 'randolph@10gen.com', 'username': 'renctan'}

Message: SERVER-38335 Make killAllSessions not attach the lsid from request to OperationContext

This is to prevent it from killing itself.
Branch: master
https://github.com/mongodb/mongo/commit/473e96a92ce798edf48cc31b8a9f123609df4714

Comment by Misha Tyulenev [ 10/Dec/18 ]

kaloian.manassiev I agree with adding the shouldIgnoreSession property. This will allow keeping the authentication to run but avoid adding a session to the logical cache. In this case, we can simplify the initializeOperationSessionInfo by taking requiresAuth outside so those checks will not overlap.

Comment by Kaloian Manassiev [ 10/Dec/18 ]

misha.tyulenev, this is not just about implementing an allowedInTransactions property, but more like shouldIgnoreSession, which if specified as true means that the command should not check-out the session and should behave as if it was not ran with a session. And initializeOperationSessionInfo will only be called when !command->shouldIgnoreSession() && requiresAuth.

Is this what you had in mind?

Comment by Misha Tyulenev [ 07/Dec/18 ]

The approach is safe but I dont support the implementation suggestion. I would rather suggest implementing a virtual bool allowedInTransactions() method and override it appropriately

Comment by Shane Harvey [ 06/Dec/18 ]

I was not trying to suggest removing the auth requirement for killAllSessions, merely to skip parsing/creating the OperationSessionInfoFromClient similar to what is done for commands that don't require auth. Like this:

    if (!requiresAuth || commandName == "killAllSessions") {
        uassert(ErrorCodes::OperationNotSupportedInTransaction,
                "This command is not supported in transactions",
                !osi.getAutocommit());
        uassert(
            50889, "It is illegal to provide a txnNumber for this command", !osi.getTxnNumber());
        return boost::none;
    }

Comment by Randolph Tan [ 06/Dec/18 ]

Hm... killAllSessions command does require auth, doesn't it? Doesn't sound right to just make it return false.

Comment by Misha Tyulenev [ 06/Dec/18 ]

kaloian.manassiev this approach may introduce a security problem as this will open this method to nonauthorized users.:

Comment by Kaloian Manassiev [ 05/Dec/18 ]

Ah you are proposing that we do it at the server side so that the killAllSessions operation doesn't mark itself as running under a session, by returning early here, right?
misha.tyulenev, is this safe to do for select commands? Basically we don't want the killAllSessions command to get interrupted because it is running under a session.

Comment by Shane Harvey [ 05/Dec/18 ]

I think there's a single place in the server where it could ignore the session related arguments for a killAllSessions command. The server already does the same for all commands that don't require auth. This should remove the need to ignore a session after the fact.

I believe the drivers already support that for other commands too, so can you just add that one as well?

Yes, drivers do this for some commands but this only the case when there's a helper method, for example pymongo's parallel_scan helper (which runs parallelCollectionScan) in PYTHON-1529. There is no public API to run an arbitrary command without a session however so drivers would either need to add a new RunCommandWithoutSession method or add helpers for killAllSessions. I don't think we want to do either of those options.

Comment by Kaloian Manassiev [ 05/Dec/18 ]

The killAllSessions command was intended to do exactly what its name says and that's to kill all sessions. I understand this might be inconvenient for the drivers, but making it skip the session under which it is running is not trivial.

Given the implementation difficulty and generally that this command is allowed to accept parameters where the session under which it is run is included, I think that the most prudent thing is to disallow killAllSessions from running under a session. I believe the drivers already support that for other commands too, so can you just add that one as well?

shane.harvey?

Comment by Shane Harvey [ 04/Dec/18 ]

Can we get this prioritized to ease the driver testing proposed in SPEC-1140? Omitting the implicit session or using killAllSessionsByPattern may be difficult in some drivers.

Generated at Thu Feb 08 04:48:40 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.