Skip to content

WIP Feat[MQB]: add authentication with basic logic #696

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

WIP Feat[MQB]: add authentication with basic logic #696

wants to merge 9 commits into from

Conversation

emelialei88
Copy link
Collaborator

@emelialei88 emelialei88 commented Apr 9, 2025 •
edited
Loading

Broker side:

  • Added support for the authentication protocol.
  • Introduced a new AuthenticationEvent event type.
  • Implemented the authenticator component to handle authentication logic, using AuthenticationContext to encapsulate relevant context data.
  • Updated InitialConnectionHandler to handle the authentication flow.

Client side:

  • Renamed NegotiationChannelFactory to InitialConnectionChannelFactory, which now handles both authentication and negotiation.
  • Updated bmqimp_application to pass in an AuthenticationMessage.

chrisbeard
chrisbeard reviewed Apr 9, 2025
View reviewed changes
src/groups/bmq/bmqp/bmqp_ctrlmsg.xsd Outdated
Comment on lines 1612 to 1613
<element name='authenticateRequest' type='tns:AuthenticateRequest'/>
<element name='authenticateResponse' type='tns:AuthenticateResponse'/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From our discussion today, we're leaning toward moving these types to a new, top-level complexType that just contains the authentication protocols.

@emelialei88 emelialei88 force-pushed the feat/authn-protocol branch 4 times, most recently from d923a9d to 6737422 Compare April 11, 2025 17:55
@emelialei88 emelialei88 force-pushed the feat/authn-protocol branch 2 times, most recently from fe5f4fb to 515517d Compare April 24, 2025 20:37
@emelialei88 emelialei88 changed the title Feat[MQB]: add authn protocol Feat[MQB]: add authentication Apr 25, 2025
@emelialei88 emelialei88 changed the title Feat[MQB]: add authentication Feat[MQB]: add authentication with basic logic Apr 25, 2025
emelialei88
emelialei88 commented Apr 29, 2025
View reviewed changes
src/groups/bmq/bmqimp/bmqimp_application.cpp
InitialConnectionChannelFactoryConfig(
&d_statChannelFactory,
authenticationMessage,
sessionOptions.connectTimeout(), // TODO: different for authn?
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a different timeout for authentication?

@emelialei88 emelialei88 force-pushed the feat/authn-protocol branch from 515517d to e6554bf Compare April 29, 2025 18:03
@emelialei88 emelialei88 requested a review from 678098 April 29, 2025 18:04
emelialei88 added 8 commits May 9, 2025 11:23
@emelialei88 emelialei88 force-pushed the feat/authn-protocol branch from e6554bf to cef4a2d Compare May 9, 2025 15:23
678098
678098 reviewed May 12, 2025
View reviewed changes
Copy link
Collaborator

@678098 678098 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Continue looking, but here are some comments

src/groups/bmq/bmqa/bmqa_session.cpp
authenticationMessage.makeAuthenticateRequest();
bsl::string str = "username:password";
ar.mechanism() = "basic";
ar.data() = bsl::vector<char>(str.begin(), str.end()); // hexBinary
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few notes:

  1. Binary hex introduces +100% protocol overhead to data.size() after encoding.
    If we use base64, we can have +33% protocol overhead.
    Probably it's not a problem, because we don't intend to send authn requests too often and don't want to send too much data here.

  2. Should binary hex be enforced? If so, it's worth to add checks in both client library (before sending a request) and on the broker side (when request is received).
    This also means that "username:password" data provided here will be rejected.

  3. Another way to enforce binary hex is to hide it from the library user.
    Let the user to just assign bsl::vector<char> data to anything, and convert it to binary hex when message is packed.

  4. What variations of capital letters does this hex binary support?
    abcdef, ABCDEF, or both abcdefABCDEF?

src/groups/bmq/bmqimp/bmqimp_application.h
@@ -226,7 +226,8 @@ class Application {
const bsl::shared_ptr<bmqp::HeartbeatMonitor>& monitor);
bsl::shared_ptr<bmqp::HeartbeatMonitor>
createMonitor(const bsl::shared_ptr<bmqio::Channel>& channel);
void startHeartbeat(const bsl::shared_ptr<bmqio::Channel>& channel,
void
startHeartbeat(const bsl::shared_ptr<bmqio::Channel>& channel,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a formatter bug @dorjesinpo encountered

@emelialei88 emelialei88 force-pushed the feat/authn-protocol branch 2 times, most recently from b174b92 to a191d1e Compare May 12, 2025 16:22
@emelialei88
minor fix
f750a50 
Signed-off-by: Emelia Lei <[email protected]>
@emelialei88 emelialei88 force-pushed the feat/authn-protocol branch from a191d1e to f750a50 Compare May 12, 2025 16:28
chrisbeard
chrisbeard reviewed May 12, 2025
View reviewed changes
src/groups/bmq/bmqa/bmqa_session.cpp
@@ -311,6 +319,7 @@ int SessionUtil::createApplication(SessionImpl* sessionImpl)
sessionImpl->d_application_mp.load(
new (*(sessionImpl->d_allocator_p))
bmqimp::Application(options,
authenticationMessage,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In its proper form, this probably won't be a concrete object, but rather something user-provided via SessionOptions that provides a callback to generate the raw information needed to construct this message.

@emelialei88 emelialei88 changed the title Feat[MQB]: add authentication with basic logic WIP Feat[MQB]: add authentication with basic logic May 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisbeard chrisbeard chrisbeard left review comments

@678098 678098 678098 left review comments

@pniedzielski pniedzielski Awaiting requested review from pniedzielski

At least 1 approving review is required to merge this pull request.

Assignees

@pniedzielski pniedzielski

@678098 678098

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@emelialei88 @chrisbeard @678098 @pniedzielski