Skip to content

CDRIVER-4689 Additional partial implementation of OIDC authentication #2018

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 184 commits into
base: master
Choose a base branch
from

Conversation

mdbmes
Copy link
Contributor

@mdbmes mdbmes commented May 19, 2025

This is my attempt to revive a patch stack that Kyle Kloberdanz was working on about 1 year ago. Kyle has since left MongoDB. Unrelatedly, I'm also leaving. (To protest the "AI" hype here.) My last day will be this Wednesday May 21st and I'm losing access to my corporate accounts. This GitHub account will be deleted before I lose access, so if this PR won't be merged by then someone will need to copy the branch if the work is to be continued.

The commit stack is a bit harrowing and I would not recommend reviewing commit-by-commit. My process here was to (very interactively) rebase an old copy of Kyle's branch on main, while editing out changes that conflicted with the intervening work on OIDC callbacks. After the rebase, I added commits intended to fix specific problems as described in the messages.

What's maybe good?

  • Lots of the work here was necessary plumbing to support OIDC speculative auth
  • We try to handle OIDC re-authentication replies by doing a retry
  • There's an OIDC credential cache, with invalidation rules that try to follow the auth spec
  • Some refactoring (_mongoc_sasl_run_command)
  • Some new type checks for the unified test runner
  • Added and documented mongoc_oidc_callback_copy

What's a problem:

  • Evergreen changes add an "example-task" that was clearly a work in progress
  • New set_oidc_callback functions are missing documentation
  • Incomplete thread safety work: set_oidc_callback functions have no safety checks or locking, meanwhile we have a global mutex (_oidc_callback_mutex) that seems unjustified.
  • Changes intending to zero sensitive memory before free are incomplete (Sensitive data lingers in bson_t, and passes through growable buffers)
  • Might invalidate the wrong cached token (see TODO comment)
  • OIDC support in test framework is headed in a brittle direction, without a single source of truth for the overall intended authentication mode (see the various places where mechanism is checked, and everywhere we use the "OIDC" environment variable)
  • I'm not sure the choice of ownership rules for oidc_callback_t make sense: The call sites for set_oidc_callback here would all be improved by having set_oidc_callback take ownership of the callback_t instead of copying it.

What's just incomplete:

  • Testing still needs lots of work, especially on the evergreen infrastructure side.
  • "ENVIRONMENT" options are not yet hooked up, and the environment-specific callbacks are still just stubs.

@mdbmes mdbmes requested a review from a team as a code owner May 19, 2025 16:15
@mdbmes mdbmes requested review from rcsanchez97, kevinAlbs and eramongodb and removed request for rcsanchez97 May 19, 2025 16:15
@mdbmes
Copy link
Contributor Author

mdbmes commented May 20, 2025

Note, I've been debugging the mock-server-test failure but I'm not confident I'll be able to provide a solution before my accounts lock at some unspecified time.

The failure is in _test_topology_scanner: It calls test_framework_get_uri to set up the topology scanner (mongoc_topology_scanner_new) before any mock servers are started, and without knowledge of the mock server ports. It will try to connect to the default test server, to check whether it's a single server or a member of a replica set. In the mock-server-test configuration, my understanding so far (though I have not verified this) is that it's intended to run without a default server, so this approach will fail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants