[CDRIVER-3728] GSSAPI auth commands must not use implicit sessions Created: 26/Jun/20  Updated: 28/Oct/23  Resolved: 05/Aug/20

Status: Closed
Project: C Driver
Component/s: None
Affects Version/s: None
Fix Version/s: 1.18.0, 1.17.3, 1.18.0-alpha

Type: Bug Priority: Major - P3
Reporter: Kevin Albertson Assignee: Unassigned
Resolution: Fixed Votes: 0
Labels: new-eng
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
is related to CDRIVER-2449 Session ID is included in authenticat... Closed

 Description   

Authentication commands must not append a session ID per the driver session spec.

CDRIVER-2449 discovered that most auth commands were including the session ID unintentionally. The resolution was to set prohibit_lsid=true in the mongoc_cmd_parts_t used to construct the command. For example, in _mongoc_cluster_auth_node_cr:

   mongoc_cmd_parts_init (
      &parts, cluster->client, auth_source, MONGOC_QUERY_SLAVE_OK, &command);
   parts.prohibit_lsid = true;

However, _mongoc_cluster_auth_node_cyrus and _mongoc_cluster_auth_node_sspi do not set prohibit_lsid. I believe they may still be appending a session ID unintentionally.



 Comments   
Comment by Pierre Mickael GONZALO [ 06/Aug/20 ]

Alright thank you I'll know for the next time

Comment by Kevin Albertson [ 05/Aug/20 ]

Thank you pierremickael.gonzalo@gmail.com! I authorized the patch build and merged the PR. Evergreen, MongoDB's continuous integration test platform. Evergreen tests on all platforms we support. Alas, it does require employee intervention to run user submitted pull requests. But, Travis tasks should still run on all PRs. And you can also run all tests locally. See https://github.com/mongodb/mongo-c-driver/blob/master/CONTRIBUTING.md#testing.

Comment by Kevin Albertson [ 05/Aug/20 ]

PR: https://github.com/mongodb/mongo-c-driver/pull/675
Commit: https://github.com/mongodb/mongo-c-driver/commit/cd511a80005cc8c24b1d7eb8221ac62873bc2829

Comment by Pierre Mickael GONZALO [ 03/Aug/20 ]

Hi,
As requested I've made the change but as you can see one test failed about evergreen with a short line description that says:

patch must be manually authorized 

But I do not know what it means if the error comes from my side or not. Maybe it is related to this cluster's test that we talked previously but you said that it is manually tested so could you give me some hint on what's going wrong ?
Thank you

Comment by Kevin Albertson [ 31/Jul/20 ]

Alright never mind I've just misunderstood. So the manually test is intended to be tested by myself but how does it works ? Should I send you a screenshot of the server logs ?

No need. We can run manual tests on our end when reviewing the change since we already have Kerberos clusters set up to test against.

Comment by Pierre Mickael GONZALO [ 31/Jul/20 ]

Yes, if it is not reasonable to add automated tests, then we'd have to rely on manually looking at the sent command to check that the behavior is correct. That was the decision in CDRIVER-2449 (the fix was applied, no tests were added, but behavior was validated by inspecting trace logs

Alright never mind I've just misunderstood. So the manually test is intended to be tested by myself but how does it works ? Should I send you a screenshot of the server logs ?

If you are interested in attempting the fix, I'd suggest proceeding with the change you proposed.

Yes I am interrested in attempting the fix, I've actually already done but for now I can't create a remote branch so I can ask for a pull request.

Comment by Kevin Albertson [ 30/Jul/20 ]

Hi, so both functions calls "mongoc_cluster_run_command_private" and according to his description no APM callbacks are executed. I think is that you meant by: "And authentication commands are not capture in command monitoring".

Yes. That is what I meant.

So you would like to know if is there an alternative to APM ? I would say to add an additionnal argument to the function that enables us to test it. I've seen the parts variable set his attribute "has_temp_server" to true in case the prohibit_lsid is set, maybe we can use it to complete our argument. Honestly I think it's not a good idea to do that way.

If you are referring to the field has_temp_session in mongoc_cmd_parts_t, that is used internally to determine whether an implicit session is applied. It's not clear to me how you'd access that from within a test.

When you say "rely on manual inspection of the commands" what do you mean exactly ? That the function cannot be tested or from now it doesn't including a session id even if it is never supposed to do ?

Yes, if it is not reasonable to add automated tests, then we'd have to rely on manually looking at the sent command to check that the behavior is correct. That was the decision in CDRIVER-2449 (the fix was applied, no tests were added, but behavior was validated by inspecting trace logs).

If you are interested in attempting the fix, I'd suggest proceeding with the change you proposed. Manually testing this will require authenticating against a Kerberos cluster. We have existing Kerberos clusters so we can validate the change.

Comment by Pierre Mickael GONZALO [ 30/Jul/20 ]

Hi, so both functions calls "mongoc_cluster_run_command_private" and according to his description no APM callbacks are executed. I think is that you meant by: "And authentication commands are not capture in command monitoring". So you would like to know if is there an alternative to APM ? I would say to add an additionnal argument to the function that enables us to test it. I've seen the parts variable set his attribute "has_temp_server" to true in case the prohibit_lsid is set, maybe we can use it to complete our argument. Honestly I think it's not a good idea to do that way.
When you say "rely on manual inspection of the commands" what do you mean exactly ? That the function cannot be tested or from now it doesn't including a session id even if it is never supposed to do ?

Comment by Kevin Albertson [ 29/Jul/20 ]

Hello pierremickael.gonzalo@gmail.com, I agree, that seems like the most straightforward solution. If you would like to submit a PR with the change, we'd happily review it.

Ideally, we would like to additionally add a test to validate the change. The C driver does not have much infrastructure for testing authentication conversations. The existing integration tests check that authentication works against MongoDB servers when expected, and fails when we do not. Since the session ID in the authentication command does not appear to affect whether authentication succeeds. And authentication commands are not capture in command monitoring. I would like to explore whether it is reasonable to add automated testing. If it is not feasible, we may need to rely on manual inspection of the commands.

Comment by Pierre Mickael GONZALO [ 29/Jul/20 ]

Hi, I would set the prohibit_lsid attribute to true to the parts variable in both functions right after the call to mongoc_cmd_parts_init, did I miss something ? 

Generated at Wed Feb 07 21:18:52 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.