Feat[MQB]: Make authentication workflow use plugins #773

emelialei88 · 2025-06-10T18:45:33Z

This PR apply plugin into authentication and reauthentication when we as a broker receives authentication requests from clients.

Added component rawclient to support integration test test_authn.py.
Added plugin bmqauthnbasic for testing. It uses "basic" as mechanism and creates an in-memory map for username and password.
Used plugin to authenticate and reauthenticate.
- Authenticator is managed by TransportManager and referred to by InitialConnectionContext.
- When sending response, we use the same encoding type as the one used by the client AuthenticationRequest.
- Authentication with plugin is performed in a separate thread, and the channel is closed when there's a failure.
- When AuthenticationContext is created during initial connection, create a reauthenticateCb to be called when a client session later receives an AuthenticationEvent.
- Use AuthenticationContext::State to indicate the state of authentication. This is used as to make sure only one authentication is run for a given context, and when a channel closes, no more reauthentication timer will be scheduled.
Default authentication credential:
- Credential = mechanism + identity
- When we receive a ClientIdentity prior to any AuthenticateRequest, we use default credential to authenticate, unless it's explicitly set to Disallow.
- Added AnonyPassAuthenticator (implementing mqbplug::Authenticator) and the corresponding factory and result under mqbauthn as default Authenticator.
A minor refactor to clarify the responsibility
- Have TransportManager instead of InitialConnectionHandler own the Negotiator.
Added a timer to disconnect when the authenticated lifetime expires.
Added InitialConnectionContext::State to indicate the state of initial connection.

src/groups/mqb/mqba/mqba_clientsession.t.cpp

pniedzielski

partial review

src/groups/mqb/mqba/mqba_clientsession.t.cpp

src/integration-tests/test_authn.py

pniedzielski · 2025-07-16T18:59:37Z

src/groups/mqb/mqba/mqba_application.cpp

    STOP_OBJ(d_configProvider_mp, "ConfigProvider");
-    STOP_OBJ(d_statController_mp, "StatController");
    STOP_OBJ(d_authenticationController_mp, "AuthenticationController");
+    STOP_OBJ(d_statController_mp, "StatController");


This and the DESTROY_OBJ change below look important. Is this because we use stats in our auth controller?

Not yet. This change is made since StatController starts first before AuthenticationController, so when stopping I wanted to have them in reverse order.

src/groups/mqb/mqba/mqba_authenticator.cpp

Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>

Signed-off-by: Emelia Lei <wlei29@bloomberg.net>

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp

Signed-off-by: Emelia Lei <wlei29@bloomberg.net>

678098

Could you also rebase the branch to the latest main to allow Solaris build?

src/groups/bmq/bmqp/bmqp_protocol.h

src/groups/mqb/mqba/mqba_authenticator.cpp

src/plugins/bmqauthnbasic/bmqauthnbasic_basicauthenticator.cpp

src/plugins/bmqauthnbasic/bmqauthnbasic_pluginlibrary.cpp

src/plugins/bmqauthnpass/bmqauthnpass_passauthenticator.cpp

678098

Could you also rebase the branch to the latest main to allow Solaris build?

Signed-off-by: Emelia Lei <wlei29@bloomberg.net>

emelialei88

Addressed the review from @678098 and responded to some questions.

src/groups/mqb/mqbauthn/mqbauthn_anonpasspluginlibrary.cpp

emelialei88 · 2025-10-01T21:41:59Z

src/groups/mqb/mqbnet/mqbnet_initialconnectioncontext.cpp

+    int rc = decodeInitialConnectionMessage(errorDescription, &message, blob);
+
+    if (rc != rc_SUCCESS) {
+        return (rc * 10) + rc_INVALID_INITIALCONNECTION_MESSAGE;  // RETURN


Not sure what you mean. Isn't errorDescription populated from decodeInitialConnectionMessage?

src/groups/mqb/mqbnet/mqbnet_initialconnectioncontext.cpp

emelialei88 · 2025-10-02T15:56:27Z

src/groups/mqb/mqbauthn/mqbauthn_anonpassauthenticator.cpp

+{
+    // PRECONDITIONS
+    BSLS_ASSERT_OPT(!d_isStarted &&
+                    "start() can only be called once on this object");


I thought that's what I'm doing here?

emelialei88 · 2025-10-02T15:58:49Z

src/groups/mqb/mqbauthn/mqbauthn_authenticationcontroller.h

+
+    /// Return the anonymous credential used for authentication.
+    /// If no anonymous credential is set, return an empty optional.
+    const bsl::optional<mqbcfg::Credential>& anonymousCredential();


optional doesn't help to bypass including this file. mqbplug_pluginmanager does include it tho if you think we can just use the one from mqbplug_pluginmanager.

emelialei88 · 2025-10-02T16:03:04Z

src/groups/mqb/mqba/mqba_authenticator.h

+    bdlmt::EventScheduler* d_scheduler;
+
+    /// Allocator to use.
+    bslma::Allocator* d_allocator_p;


I don't mind reordering but kind of want to keep d_allocator_p at the end cuz it helps to eliminate one extra parameter when calling allocate_shared.

emelialei88 · 2025-10-02T16:08:32Z

src/groups/mqb/mqbnet/mqbnet_authenticationcontext.cpp

 namespace mqbnet {

+namespace {
+BALL_LOG_SET_NAMESPACE_CATEGORY("MQBNET.AUTHENTICATIONCONTEXT");


Yeah this was a misused but I'm sure there's no error or crash.

emelialei88 · 2025-10-02T16:46:36Z

src/groups/mqb/mqbauthn/mqbauthn_anonpassauthenticator.h

+    const mqbcfg::AuthenticatorPluginConfig* d_authenticatorConfig_p;
+
+    bool d_isStarted;
+
+    bslma::Allocator* d_allocator_p;


Maybe not a big deal but putting d_allocator_p to the front would cause one extra variable provided toallocate_shared.

emelialei88 · 2025-10-02T16:59:31Z

src/groups/mqb/mqbnet/mqbnet_authenticationcontext.cpp

+    {
+        bslmt::LockGuard<bslmt::Mutex> guard(&d_mutex);  // LOCKED
+
+        if (d_state == State::e_CLOSED) {


The point of using a single mutex was bcz it's simpler to reason about the potential race condition between scheduling a reauthn event and closing a channel, making sure changing the state to e_CLOSED and cancelEventAndWait happen at the same time. We could use an atomic but there's a few places I still have to hold a mutex while accessing it. So I'm leaning towards just use one mutex unless you have a strong opinion against this idea. Thanks!

emelialei88 · 2025-10-02T16:59:45Z

src/groups/mqb/mqbnet/mqbnet_initialconnectioncontext.cpp

+{
+    bslmt::LockGuard<bslmt::Mutex> guard(&d_mutex);
+
+    return d_state == state;


See the comments above.

678098

More comments
I can test the build on Solaris when the branch is rebased from the latest main

678098 · 2025-10-02T18:40:31Z

src/groups/mqb/mqba/mqba_authenticator.h

 class Authenticator : public mqbnet::Authenticator {
+  private:
+    // CLASS-SCOPE CATEGORY
+    BALL_LOG_SET_CLASS_CATEGORY("MQBNET.AUTHENTICATIONCONTEXT");


Suggested change

BALL_LOG_SET_CLASS_CATEGORY("MQBNET.AUTHENTICATIONCONTEXT");

BALL_LOG_SET_CLASS_CATEGORY("MQBA.AUTHENTICATOR");

678098 · 2025-10-02T18:44:48Z

src/groups/mqb/mqba/mqba_authenticator.h

+    bdlmt::EventScheduler* d_scheduler;
+
+    /// Allocator to use.
+    bslma::Allocator* d_allocator_p;


Do you mean the fact that we omit the last allocator arg when calling allocate_shared? allocate_shared looks at class traits and if there is allocator trait it always passes an allocator as the last arg to constructor automatically.
The important thing is to keep allocator arg the last in the constructor, but it can be placed in the first place in the class fields.

678098 · 2025-10-02T18:49:05Z

src/groups/mqb/mqbauthn/mqbauthn_anonpassauthenticator.cpp

+{
+    // PRECONDITIONS
+    BSLS_ASSERT_OPT(!d_isStarted &&
+                    "start() can only be called once on this object");


I think it should be symmetrical, to make the codebase simpler and predictable.
Might either gracefully handle multiple start/stop, or assert in both cases.
Probably it's better to handle without crash.

678098 · 2025-10-02T18:57:01Z

src/groups/mqb/mqbauthn/mqbauthn_anonpassauthenticator.h

+
+  public:
+    // TRAITS
+    BSLMF_NESTED_TRAIT_DECLARATION(Authenticator, bslma::UsesBslmaAllocator)


Suggested change

BSLMF_NESTED_TRAIT_DECLARATION(Authenticator, bslma::UsesBslmaAllocator)

BSLMF_NESTED_TRAIT_DECLARATION(AnonPassAuthenticator, bslma::UsesBslmaAllocator)

678098 · 2025-10-02T18:58:25Z

src/groups/mqb/mqbauthn/mqbauthn_anonpassauthenticator.h

+    const mqbcfg::AuthenticatorPluginConfig* d_authenticatorConfig_p;
+
+    bool d_isStarted;
+
+    bslma::Allocator* d_allocator_p;


The problem might be with missing/incorrect allocated nested trait that is used by allocate_shared, if it is present and correct, there should be no last allocator arg in allocate_shared

678098 · 2025-10-02T19:11:36Z

src/groups/mqb/mqbnet/mqbnet_initialconnectioncontext.h

+
  public:
+    // TRAITS
+    BSLMF_NESTED_TRAIT_DECLARATION(AuthenticationContext,


Suggested change

BSLMF_NESTED_TRAIT_DECLARATION(AuthenticationContext,

BSLMF_NESTED_TRAIT_DECLARATION(InitialConnectionContext,

678098 · 2025-10-02T19:13:24Z

src/plugins/bmqauthnpass/bmqauthnpass_passauthenticator.h

+
+  public:
+    // TRAITS
+    BSLMF_NESTED_TRAIT_DECLARATION(PassAuthenticationResult,


Suggested change

BSLMF_NESTED_TRAIT_DECLARATION(PassAuthenticationResult,

BSLMF_NESTED_TRAIT_DECLARATION(PassAuthenticator,

If the provided class name doesn't match the current class name the trait will not work

678098 · 2025-10-02T19:14:13Z

src/plugins/bmqauthnfail/bmqauthnfail_failauthenticator.h

+
+  public:
+    // TRAITS
+    BSLMF_NESTED_TRAIT_DECLARATION(FailAuthenticationResult,


Suggested change

BSLMF_NESTED_TRAIT_DECLARATION(FailAuthenticationResult,

BSLMF_NESTED_TRAIT_DECLARATION(FailAuthenticator,

678098 · 2025-10-02T19:15:04Z

src/plugins/bmqauthnbasic/bmqauthnbasic_basicauthenticator.h

+
+  public:
+    // TRAITS
+    BSLMF_NESTED_TRAIT_DECLARATION(BasicAuthenticationResult,


Suggested change

BSLMF_NESTED_TRAIT_DECLARATION(BasicAuthenticationResult,

BSLMF_NESTED_TRAIT_DECLARATION(BasicAuthenticator,

678098 · 2025-10-02T19:19:48Z

src/plugins/bmqauthnbasic/bmqauthnbasic_basicauthenticator.cpp

+// BMQ
+


Suggested change

// BMQ

emelialei88 requested a review from a team as a code owner June 10, 2025 18:45

emelialei88 marked this pull request as draft June 10, 2025 18:45

emelialei88 force-pushed the integration/authn-authz branch 2 times, most recently from d8e0447 to 0f8a365 Compare June 18, 2025 15:21

emelialei88 force-pushed the authn/use-plugin branch 4 times, most recently from 057a7c2 to ec64999 Compare June 20, 2025 19:16

emelialei88 force-pushed the integration/authn-authz branch 2 times, most recently from d74e1fe to 2d81dff Compare June 20, 2025 19:32

emelialei88 force-pushed the authn/use-plugin branch 3 times, most recently from 85e7ca0 to 6441933 Compare June 23, 2025 15:58

emelialei88 force-pushed the integration/authn-authz branch 3 times, most recently from a6885dd to c2820a5 Compare June 25, 2025 16:23

emelialei88 force-pushed the authn/use-plugin branch from 6441933 to dae302b Compare June 25, 2025 18:11

emelialei88 force-pushed the integration/authn-authz branch from c2820a5 to 1217636 Compare July 10, 2025 21:46

emelialei88 force-pushed the authn/use-plugin branch from 9f73e62 to 3508355 Compare July 11, 2025 14:25

emelialei88 marked this pull request as ready for review July 15, 2025 15:18

emelialei88 requested a review from pniedzielski July 15, 2025 15:18

emelialei88 assigned pniedzielski Jul 15, 2025

emelialei88 force-pushed the authn/use-plugin branch from 09f7274 to 57a2d8f Compare July 16, 2025 20:48

emelialei88 commented Jul 16, 2025

View reviewed changes

src/groups/mqb/mqba/mqba_clientsession.t.cpp Outdated Show resolved Hide resolved

emelialei88 force-pushed the authn/use-plugin branch from 57a2d8f to 57e5b06 Compare July 16, 2025 21:55

pniedzielski reviewed Jul 17, 2025

View reviewed changes

emelialei88 force-pushed the authn/use-plugin branch 3 times, most recently from a873f88 to fbb6bec Compare July 21, 2025 21:53

emelialei88 force-pushed the integration/authn-authz branch from 1217636 to 1128950 Compare July 25, 2025 17:54

dorjesinpo and others added 2 commits September 25, 2025 14:41

bugfix: counting early sessions

f632343

Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>

fix bugs pr-897

020b977

Signed-off-by: Emelia Lei <wlei29@bloomberg.net>

emelialei88 force-pushed the authn/use-plugin branch 3 times, most recently from 348789c to 5d15c0d Compare September 25, 2025 21:23

emelialei88 added 5 commits September 26, 2025 15:36

PR feedback: unlock guard before complete

9ff4e60

Signed-off-by: Emelia Lei <wlei29@bloomberg.net>

PR feedback: make TCPSessionFactory handle reauthn event

98f8846

Signed-off-by: Emelia Lei <wlei29@bloomberg.net>

PR feedback: call reauthn directly

50d3580

Signed-off-by: Emelia Lei <wlei29@bloomberg.net>

PR feedback: negotiationContext ctor

14f7c8b

Signed-off-by: Emelia Lei <wlei29@bloomberg.net>

PR feedback: race - scheduled reauthn and close

b31b0a5

Signed-off-by: Emelia Lei <wlei29@bloomberg.net>

emelialei88 force-pushed the authn/use-plugin branch from 86a08e2 to b31b0a5 Compare September 26, 2025 19:37

AuthenticationContext merge mutex

4a8933e

Signed-off-by: Emelia Lei <wlei29@bloomberg.net>

emelialei88 force-pushed the authn/use-plugin branch from c53a634 to 224314f Compare September 29, 2025 21:03

move functions from InitialConnectionHandler to context

6965cc7

Signed-off-by: Emelia Lei <wlei29@bloomberg.net>

emelialei88 force-pushed the authn/use-plugin branch from 224314f to 6965cc7 Compare September 29, 2025 21:04

emelialei88 commented Sep 30, 2025

View reviewed changes

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp Outdated Show resolved Hide resolved

emelialei88 assigned 678098 and dorjesinpo and unassigned pniedzielski Sep 30, 2025

emelialei88 added 2 commits October 1, 2025 11:53

rawclient able to send bytes

f856d6b

Signed-off-by: Emelia Lei <wlei29@bloomberg.net>

change lifetimeMs to nonNegative

0a4b319

Signed-off-by: Emelia Lei <wlei29@bloomberg.net>

678098 reviewed Oct 1, 2025

View reviewed changes

678098 assigned emelialei88 and unassigned 678098 Oct 1, 2025

emelialei88 added 2 commits October 2, 2025 13:01

address feedback: grammar and format

59d7361

Signed-off-by: Emelia Lei <wlei29@bloomberg.net>

protect d_nbActiveChannels

6715100

Signed-off-by: Emelia Lei <wlei29@bloomberg.net>

emelialei88 force-pushed the authn/use-plugin branch from 9360453 to 6715100 Compare October 2, 2025 17:01

emelialei88 commented Oct 2, 2025

View reviewed changes

678098 reviewed Oct 2, 2025

View reviewed changes

	BALL_LOG_SET_CLASS_CATEGORY("MQBNET.AUTHENTICATIONCONTEXT");
	BALL_LOG_SET_CLASS_CATEGORY("MQBA.AUTHENTICATOR");

	BSLMF_NESTED_TRAIT_DECLARATION(Authenticator, bslma::UsesBslmaAllocator)
	BSLMF_NESTED_TRAIT_DECLARATION(AnonPassAuthenticator, bslma::UsesBslmaAllocator)

	BSLMF_NESTED_TRAIT_DECLARATION(AuthenticationContext,
	BSLMF_NESTED_TRAIT_DECLARATION(InitialConnectionContext,

	BSLMF_NESTED_TRAIT_DECLARATION(PassAuthenticationResult,
	BSLMF_NESTED_TRAIT_DECLARATION(PassAuthenticator,

	BSLMF_NESTED_TRAIT_DECLARATION(FailAuthenticationResult,
	BSLMF_NESTED_TRAIT_DECLARATION(FailAuthenticator,

	BSLMF_NESTED_TRAIT_DECLARATION(BasicAuthenticationResult,
	BSLMF_NESTED_TRAIT_DECLARATION(BasicAuthenticator,

Feat[MQB]: Make authentication workflow use plugins #773

Are you sure you want to change the base?

Feat[MQB]: Make authentication workflow use plugins #773

Conversation

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

Uh oh!

Uh oh!

Uh oh!

pniedzielski left a comment

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!

Uh oh!

Uh oh!

678098 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

678098 left a comment

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!

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!

678098 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!

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 Jun 10, 2025 •

edited

Loading