Feat[MQB]: Add Authenticator and InitialConnectionContext #1004

emelialei88 · 2025-10-24T19:53:27Z

This PR introduces several refactors and enables authentication:

Adds an Authenticator class to encapsulate authentication logic.
Migrates connection initialization logic from InitialConnectionHandler to InitialConnectionContext.

dorjesinpo

interim review.
we have pairs of the following:
decodeInitialConnectionMessage,
scheduleRead,
readCallback
readBlob,
processBlob,
complete
... in both InitialConnectionHandler and InitialConnectionContext.

The point of FSM-like state management is that everything happens in handleEvent and there is only one place where we call complete
Otherwise, we can't rely on controlling actions by the state

Is it possible to engage handleEvent immediately in InitialConnectionHandler::handleInitialConnection and call InitialConnectionContext::complete from handleEvent only.

We seem to have InitialConnectionContext::handleInitialConnection but we call InitialConnectionHandler::handleInitialConnection instead which does not seem to support auth?

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp

src/groups/mqb/mqbnet/mqbnet_initialconnectioncontext.cpp

dorjesinpo · 2025-11-14T16:09:40Z

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp

+                context));

    // Register as observer of the channel to get the 'onClose'
+    bsl::weak_ptr<InitialConnectionContext> context_wp =


What is the motivation behind the use of weak_ptr?

Previously we have InitialConnectionContext bind to onClose, which means it won't be destroyed until the channel closes. It's works but I was hoping to clean it up once the initial connection part finishes, to make the lifetime match with the semantic.

since there is d_initialConnectionContextCache now, maybe we do not need to bind bsl::shared_ptr<InitialConnectionContext> anymore?

src/groups/mqb/mqba/mqba_authenticator.cpp

dorjesinpo · 2025-11-14T16:18:20Z

src/groups/mqb/mqbnet/mqbnet_authenticationcontext.cpp

+    bslmt::LockGuard<bslmt::Mutex> guard(&d_mutex);  // LOCKED
+
+    if (d_state != AuthenticationState::e_AUTHENTICATING) {
+        errorDescription << "State not AUTHENTICATING (was " << d_state << ")";


was -> is

emelialei88

Response

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp

emelialei88 · 2025-11-17T16:10:14Z

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp

+                context));

    // Register as observer of the channel to get the 'onClose'
+    bsl::weak_ptr<InitialConnectionContext> context_wp =


Previously we have InitialConnectionContext bind to onClose, which means it won't be destroyed until the channel closes. It's works but I was hoping to clean it up once the initial connection part finishes, to make the lifetime match with the semantic.

src/groups/mqb/mqbnet/mqbnet_initialconnectioncontext.cpp

dorjesinpo

interim review

dorjesinpo · 2025-11-17T17:59:02Z

src/groups/mqb/mqbmock/mqbmock_cluster.cpp

 // BDE
 #include <bdlbb_pooledblobbufferfactory.h>
 #include <bsl_iostream.h>
+#include <bsl_memory.h>


just double checking if this is necessary

Indirectly included in mqbmock_cluster.h but maybe we shouldn't depend on indirect includes. Needed for shared_ptr.

dorjesinpo · 2025-11-17T18:06:07Z

src/groups/mqb/mqba/mqba_application.cpp

+                                           d_scheduler_p,
+                                           d_allocators.get("Authenticator")),
+        d_allocator_p);
+


why not creating SessionNegotiator below in the same way (vs. using raw pointer and then bslma::ManagedPtr)?

setAdminCommandEnqueueCallback is a function for SessionNegotiator, but not exposed for mqbnet::Negotiator.

This is very minor (since we do not throw exceptions) but what I meant is

bslma::ManagedPtr<mqbnet::Negotiator> negotiatorMp( new (*d_allocator_p) SessionNegotiator(&d_bufferFactory, d_dispatcher_mp.get(), d_statController_mp->clientsStatContext(), &d_blobSpPool, d_scheduler_p, d_allocators.get("SessionNegotiator"), d_allocator_p); negotiatorMp ->setAdminCommandEnqueueCallback( bdlf::BindUtil::bind(&Application::enqueueCommand, this, bdlf::PlaceHolders::_1, // source bdlf::PlaceHolders::_2, // cmd bdlf::PlaceHolders::_3, // onProcessedCb bdlf::PlaceHolders::_4)); // fromReroute

Yeah I understand. But mqbnet::Negotiator doesn't expose an interface setAdminCommandEnqueueCallback.

That is not a problem (we could do bslma::ManagedPtr<mqbnet::Negotiator> negotiatorMp(impl); where impl is bslma::ManagedPtr<mqbnet::Negotiator>). The problem is setClusterCatalog below.
This is ok since we do not throw exceptions, do early returns

src/groups/mqb/mqba/mqba_authenticator.h

src/groups/mqb/mqba/mqba_authenticator.cpp

dorjesinpo · 2025-11-17T22:57:46Z

src/groups/mqb/mqbnet/mqbnet_initialconnectioncontext.cpp

+}
+
+void InitialConnectionContext::handleEvent(
+    int                                                   statusCode,


the meaning of statusCode is not clear. It seems that all errors are communicated as InitialConnectionEvent::e_ERROR and TCPSessionFactory::initialConnectionComplete just needs statusCode != 0 (which is not enforced by InitialConnectionEvent::e_ERROR case).
Unless we want to communicate exact value to bmqio::Channel::close for some reason, the code can be simplified by removing statusCode argument.

Most of the errors that occur during initial connection don’t print the error code or logs until initialConnectionComplete is called. If we were to follow this convention, it still feels necessary to pass the error code through handleEvent to initialConnectionComplete so it can be properly logged.

Understood. But d_state is primary and statusCode is secondary. On the line 710, there is no check for statusCode, if it is 0, there will be no complete call.

Maybe, we should rely on InitialConnectionState::e_FAILED and InitialConnectionState::e_NEGOTIATED being the only final states and call complete as soon as we reach them. Assuming, any other state is transitory and awaits for more event(s).

dorjesinpo · 2025-11-17T23:00:31Z

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp

+                context));

    // Register as observer of the channel to get the 'onClose'
+    bsl::weak_ptr<InitialConnectionContext> context_wp =


since there is d_initialConnectionContextCache now, maybe we do not need to bind bsl::shared_ptr<InitialConnectionContext> anymore?

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp

dorjesinpo · 2025-11-17T23:11:27Z

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp

            }  // close mutex lock guard // UNLOCK

-            // Keep track of active channels, for logging purposes
-            ++d_nbActiveChannels;


moving ++d_nbActiveChannels changes the semantics of it. It used to track all channels, including incomplete ones.

I think this might be the race condition we discussed earlier, though I don’t recall the exact scenario. All the tests are passing now — possibly because the fuzz tests have sped up and no longer reproduce the issue. Definitely a good reminder for me to document these cases better next time.

I don't recall having any problems with d_nbActiveChannels, we do not seem to check it, only log

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp

chrisbeard

Gave the authenticator interfaces a first pass, looking good overall, some comments about the implementation details and minor design bits.

chrisbeard · 2025-11-20T17:23:36Z

src/groups/mqb/mqbnet/mqbnet_authenticationcontext.h

+    /// The authentication message received.
+    bmqp_ctrlmsg::AuthenticationMessage d_authenticationMessage;


What is the motivation for storing this for the lifetime of the connection? We should not store this material once authentication completes. Can that be avoided?

chrisbeard · 2025-11-20T17:27:46Z

src/groups/mqb/mqbnet/mqbnet_authenticationcontext.h

+    /// The encoding type used for sending a message.  It should match with the
+    /// encoding type of the received message.


nit: this comment feels like it can be made clearer, perhaps something like:

// The encoding type used for sending authentication responses. The value matches // the encoding type set by the client during authentication and ensures that all // responses are sent using the same encoding.

chrisbeard · 2025-11-20T17:31:20Z

src/groups/mqb/mqbnet/mqbnet_authenticationcontext.h

+        const bsl::shared_ptr<mqbplug::AuthenticationResult>& value);
+
+    // NOTE: AuthenticationMessage and encodingType are set only when
+    // AuthenticationState is e_AUTHENTICATED.  authenticationMessage() and


Is the state mentioned in this comment correct? e_AUTHENTICATED feels wrong here

These two functions are called only during reauthentication. The state should be e_AUTHENTICATED at that time.

chrisbeard · 2025-11-20T17:43:32Z

src/groups/mqb/mqbnet/mqbnet_authenticator.h

+    /// Produce and send outbound authentication message with the specified
+    /// `context`.  Return 0 on success, or a non-zero error code and populate
+    /// the specified `errorDescription` with a description of the error
+    /// otherwise.
+    virtual int authenticationOutbound(
+        bsl::ostream&                                 errorDescription,
+        const bsl::shared_ptr<AuthenticationContext>& context) = 0;


This method feels a little out of place. Maybe that means design-wise we should rethink this, leave it out for now, or documentation on the method and interface should better define its purpose.

chrisbeard · 2025-11-20T17:44:38Z

src/groups/mqb/mqba/mqba_authenticator.cpp

+const int k_MIN_THREADS = 1;  // Minimum number of threads in the thread pool
+const int k_MAX_THREADS = 3;  // Maximum number of threads in the thread pool


This should probably be configuration-driven.

chrisbeard · 2025-11-20T17:58:30Z

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp

+    context->setAuthenticationMessage(authenticationMessage);
+    context->setAuthenticationEncodingType(
+        event.authenticationEventEncodingType());


Similar comment here, we should prefer to not store the authentication message for the lifetime of the connection.

chrisbeard · 2025-11-20T18:01:59Z

src/groups/mqb/mqba/mqba_authenticator.cpp

+    int         rc = rc_SUCCESS;
+    bsl::string error;
+
+    // Setup error handler based on whether this is initial authn or reauthn


Suggested change

// Setup error handler based on whether this is initial authn or reauthn

// Set up error handler based on whether this is initial authn or reauthn

chrisbeard · 2025-11-20T18:02:13Z

src/groups/mqb/mqba/mqba_authenticator.cpp

+    bsl::optional<bdlb::ScopeExitAny>                   scopeGuard;
+
+    if (isReauthn) {
+        // For reauthentication: setup error guard to handle failures


Suggested change

// For reauthentication: setup error guard to handle failures

// For reauthentication: set up error guard to handle failures

chrisbeard · 2025-11-20T18:02:22Z

src/groups/mqb/mqba/mqba_authenticator.cpp

+            channel));
+    }
+    else {
+        // For initial authentication: setup state machine transition


Suggested change

// For initial authentication: setup state machine transition

// For initial authentication: set up state machine transition

chrisbeard · 2025-11-20T18:06:20Z

src/groups/mqb/mqbnet/mqbnet_authenticationcontext.cpp

+        }
+
+        bmqu::MemOutStream errorStream;
+        errorStream << "Authentication timeout after " << lifetime << " ms";


This error could be made clearer. If I understand correctly, it's trying to convey that the client did not reauthenticate within before the lifetime expired?

dorjesinpo

few comments, mostly minor

dorjesinpo · 2025-11-20T15:49:39Z

src/groups/mqb/mqba/mqba_application.cpp

+                                           d_scheduler_p,
+                                           d_allocators.get("Authenticator")),
+        d_allocator_p);
+


This is very minor (since we do not throw exceptions) but what I meant is

bslma::ManagedPtr<mqbnet::Negotiator> negotiatorMp( new (*d_allocator_p) SessionNegotiator(&d_bufferFactory, d_dispatcher_mp.get(), d_statController_mp->clientsStatContext(), &d_blobSpPool, d_scheduler_p, d_allocators.get("SessionNegotiator"), d_allocator_p); negotiatorMp ->setAdminCommandEnqueueCallback( bdlf::BindUtil::bind(&Application::enqueueCommand, this, bdlf::PlaceHolders::_1, // source bdlf::PlaceHolders::_2, // cmd bdlf::PlaceHolders::_3, // onProcessedCb bdlf::PlaceHolders::_4)); // fromReroute

dorjesinpo · 2025-11-20T16:00:11Z

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp


+    BSLS_ASSERT_SAFE(
+        d_initialConnectionContextCache.contains(initialConnectionContext_p));
+    bsl::shared_ptr<InitialConnectionContext> initialConnectionContext_sp =


the purpose of this seems to be to keep the object alive until this method returns since there is no other reference left. Is that correct? If so, it is worth a comment.

There's a comment for d_initialConnectionContextCache in mqbnet::tcpsessionfactory.h.

src/groups/mqb/mqba/mqba_authenticator.cpp

dorjesinpo · 2025-11-20T19:21:55Z

src/groups/mqb/mqba/mqba_authenticator.cpp

+        if (scheduleRc != 0) {
+            rc    = (scheduleRc * 10) + rc_SCHEDULE_REAUTHN_FAILED;
+            error = scheduleErrStream.str();
+            return;  // RETURN


it is better still to respond (with an error)

I feel like failing to schedule reauthentication is more an internal error than something to notify the client.

we should always respond to request, client logic may depend on it. In this case, it looks like an internal error and the response shoudl indicate an error.

src/groups/mqb/mqbnet/mqbnet_initialconnectioncontext.cpp

dorjesinpo · 2025-11-20T19:46:36Z

src/groups/mqb/mqba/mqba_authenticator.cpp

+            mqbnet::AuthenticationState::e_AUTHENTICATING  // state
+        );
+
+    context->setAuthenticationContext(authenticationContext);


I meant something like BSLS_ASSERT_SAFE(!d_authenticationCtxSp); in InitialConnectionContext::setAuthenticationContext

src/integration-tests/test_authn.py

dorjesinpo · 2025-11-20T20:10:09Z

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp

            }  // close mutex lock guard // UNLOCK

-            // Keep track of active channels, for logging purposes
-            ++d_nbActiveChannels;


I don't recall having any problems with d_nbActiveChannels, we do not seem to check it, only log

Signed-off-by: Emelia Lei <[email protected]>

Signed-off-by: Emelia Lei <[email protected]> address feedback Signed-off-by: Emelia Lei <[email protected]>

Signed-off-by: Emelia Lei <[email protected]>

dorjesinpo

Still think we always need to respond to auth request even in the case of internal error.
A minor point - since d_initialConnectionContextCache, there no need for bsl::enable_shared_from_this<InitialConnectionContext>

emelialei88 requested a review from a team as a code owner October 24, 2025 19:53

emelialei88 marked this pull request as draft October 24, 2025 20:58

678098 self-assigned this Nov 6, 2025

678098 self-requested a review November 6, 2025 16:05

emelialei88 force-pushed the authn/change-negotiator branch 2 times, most recently from d2c4c40 to ea2450d Compare November 12, 2025 18:51

emelialei88 marked this pull request as ready for review November 12, 2025 19:38

emelialei88 requested a review from dorjesinpo November 12, 2025 19:38

emelialei88 assigned dorjesinpo Nov 12, 2025

dorjesinpo requested changes Nov 14, 2025

View reviewed changes

dorjesinpo assigned emelialei88 and unassigned dorjesinpo Nov 14, 2025

emelialei88 force-pushed the authn/change-negotiator branch 2 times, most recently from a009820 to e4183da Compare November 17, 2025 15:58

emelialei88 commented Nov 17, 2025

View reviewed changes

emelialei88 requested a review from dorjesinpo November 17, 2025 16:41

emelialei88 assigned dorjesinpo and unassigned emelialei88 Nov 17, 2025

dorjesinpo requested changes Nov 17, 2025

View reviewed changes

dorjesinpo assigned emelialei88 and unassigned dorjesinpo Nov 17, 2025

emelialei88 force-pushed the authn/change-negotiator branch from a0c0411 to 9d7a99a Compare November 19, 2025 20:48

emelialei88 requested a review from dorjesinpo November 19, 2025 21:24

emelialei88 assigned dorjesinpo and unassigned emelialei88 Nov 19, 2025

emelialei88 force-pushed the authn/change-negotiator branch from 9d7a99a to 3d2c808 Compare November 19, 2025 21:34

chrisbeard reviewed Nov 20, 2025

View reviewed changes

dorjesinpo requested changes Nov 20, 2025

View reviewed changes

dorjesinpo assigned emelialei88 and unassigned dorjesinpo Nov 20, 2025

emelialei88 added 7 commits November 21, 2025 14:25

Add Authenticator and InitialConnectionContext

3c08c31

Signed-off-by: Emelia Lei <[email protected]>

address feedback

b9d82a0

Signed-off-by: Emelia Lei <[email protected]> address feedback Signed-off-by: Emelia Lei <[email protected]>

DON'T MERGE: slow down fuzz test

7d234ae

Signed-off-by: Emelia Lei <[email protected]>

fix initialConnectionContextCache not erase when statusCode!=0

292e206

Signed-off-by: Emelia Lei <[email protected]>

address feedback

ff93cce

Signed-off-by: Emelia Lei <[email protected]>

remove status code for handleEvent

a426cce

Signed-off-by: Emelia Lei <[email protected]>

add min/max thread number into config

d7bfeb3

Signed-off-by: Emelia Lei <[email protected]>

emelialei88 force-pushed the authn/change-negotiator branch from f1e6e18 to d7bfeb3 Compare November 21, 2025 19:25

dorjesinpo requested changes Nov 24, 2025

View reviewed changes

		/// The authentication message received.
		bmqp_ctrlmsg::AuthenticationMessage d_authenticationMessage;

		/// The encoding type used for sending a message. It should match with the
		/// encoding type of the received message.

		const int k_MIN_THREADS = 1; // Minimum number of threads in the thread pool
		const int k_MAX_THREADS = 3; // Maximum number of threads in the thread pool

	// Setup error handler based on whether this is initial authn or reauthn
	// Set up error handler based on whether this is initial authn or reauthn

	// For reauthentication: setup error guard to handle failures
	// For reauthentication: set up error guard to handle failures

	// For initial authentication: setup state machine transition
	// For initial authentication: set up state machine transition

Feat[MQB]: Add Authenticator and InitialConnectionContext #1004

Are you sure you want to change the base?

Feat[MQB]: Add Authenticator and InitialConnectionContext #1004

Uh oh!

Conversation

emelialei88 commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dorjesinpo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emelialei88 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dorjesinpo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emelialei88 Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chrisbeard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emelialei88 commented Oct 24, 2025 •

edited

Loading

emelialei88 Nov 19, 2025 •

edited

Loading