Feat[bmq, mqb]: Support TLS listeners #549

hallfox · 2024-12-16T19:05:16Z

Added

TLS configuration in broker config
Helper script for generating test certs and CAs
TLS options for NtcChannel
Loading certificates and authority data specified from bmqbrkrcfg.json
SessionOptions to bmq package for configuring client sessions
--tls-authority and --tls-version options to bmqtool to configure session options
Client sessions will now require broker TLS sessions when TLS protocol versions are specified
Create CertificateStore component for bmqio
Integration tests for TLS

Changed

Update ntf-core and bde dependencies

chrisbeard

Very quick pass on the draft

chrisbeard · 2024-12-18T15:02:21Z

src/applications/bmqbrkr/etc/bmqbrkrcfg.json

+            "loggingVerbosity": "TRACE",
+            "consoleSeverityThreshold": "TRACE",


Is this change intentional?

No, seems like a leftover from local testing

src/groups/bmq/bmqimp/bmqimp_application.cpp

chrisbeard · 2024-12-18T15:17:32Z

src/groups/bmq/bmqimp/bmqimp_application.cpp

+    ntca::EncryptionClientOptions encryptionClientOptions;
+    // Set the minimum version to TLS 1.3
+    encryptionClientOptions.setMinMethod(ntca::EncryptionMethod::e_TLS_V1_3);
+    encryptionClientOptions.setMaxMethod(ntca::EncryptionMethod::e_TLS_V1_3);


Curious to see what others think here. From the ntc docs, it looks like we could use e_DEFAULT here. The client can then use v1.3 up to ntc's max supported version without needing to make code changes here (when 1.4 eventually lands)?

struct EncryptionMethod { public: /// Enumerate the methods of encryption. enum Value { /// When specified as a minimum version, the minimum version is /// interpreted as the minimum version suggested by the current /// standards of cryptography. When specified as a maximum version, the /// maximum version is interpreted as the maximum version supported by /// the implementation. e_DEFAULT, ...

src/groups/bmq/bmqio/bmqio_ntcchannel.h

678098 · 2025-03-03T14:14:15Z

src/applications/bmqtool/m_bmqtool_parameters.h

    // A name of a property to put auto-incremented values
    // in batch-posting mode.

+    bsl::string d_certificateAuthority;


Let's initialize this field in this constructor:

blazingmq/src/applications/bmqtool/m_bmqtool_parameters.cpp

Line 254 in db86b73

Parameters::Parameters(bslma::Allocator* allocator)

And add it to this printer:

blazingmq/src/applications/bmqtool/m_bmqtool_parameters.cpp

Line 272 in db86b73

Parameters::print(bsl::ostream& stream, int level, int spacesPerLevel) const

678098 · 2025-03-03T14:16:40Z

src/groups/bmq/bmqimp/bmqimp_application.cpp

+                                            statContextCreator,
+    const bmqp_ctrlmsg::NegotiationMessage& negotiationMessage,
+    BlobSpPool*                             blobSpPool,
+    bslma::Allocator*                       allocator)


The args list for this method can be rearranged so the non const ptrs (except allocator) go first. Can do it later

678098 · 2025-03-04T14:28:04Z

src/groups/bmq/bmqimp/bmqimp_application.cpp

+void Application::ChannelFactoryPipeline::stop()
+{
+    d_reconnectingChannelFactory.stop();
+    d_channelFactory.stop();


bmqio::NtcChannelFactory d_channelFactory; bmqio::ResolvingChannelFactory d_resolvingChannelFactory; bmqio::ReconnectingChannelFactory d_reconnectingChannelFactory; bmqio::StatChannelFactory d_statChannelFactory; NegotiatedChannelFactory d_negotiatedChannelFactory;

There are 5 different factories stored as fields, should all of them be stopped here? Or either there should be a comment that some of these factories are stopped from other factories that hold them.

678098 · 2025-03-04T14:29:10Z

src/groups/bmq/bmqimp/bmqimp_application.cpp

+    return bmqt::GenericResult::e_SUCCESS;
+}
+
+void Application::ChannelFactoryPipeline::stop()


Maybe we miss calling stop in destructor, or either checking that everything was stopped there. There are a lot of new code and classes in this PR, so it's worth adding more state conditions checks

678098 · 2025-03-04T14:54:15Z

src/groups/bmq/bmqimp/bmqimp_application.h

    typedef bslma::ManagedPtr<bmqio::ChannelFactory::OpHandle>
        ChannelFactoryOpHandleMp;

+    class ChannelFactoryPipeline : public bmqio::ChannelFactory {


bmqimp::Application::ChannelFactoryPipeline and mqbnet::TCPSessionFactory::ChannelFactoryPipeline have very similar interfaces. There are some differences though:

bmqimp::Application::ChannelFactoryPipeline: extra field NegotiatedChannelFactory d_negotiatedChannelFactory

fields are stored as managed pointers or direct values

Is it possible to unify interfaces, make a new component bmqio_channelfactorypipeline and reuse it from both places?

678098 · 2025-03-06T13:35:12Z

src/groups/bmq/bmqio/bmqio_certificatestore.h

+    bmqvt::ValueOrError<Certificate, Error>
+    loadCertificateFromPath(const bsl::string& path) BSLS_KEYWORD_OVERRIDE;


The return value type here makes us use Error that is inherited from ntsa::Error, so we expose ntf symbols and header.

There are different ways of returning results, in this header we also have this:

int loadCertificateAuthority(CertificateLoader& loader, const bsl::string& caPath);

We can return Certificate and/or Error via pointer args. We can also return (possibly null) shared pointer to Certificate as a return value and pass output stream to log any errors, to hide ntsa::Error

678098 · 2025-03-06T14:49:50Z

src/groups/bmq/bmqio/bmqio_ntcchannel.cpp

+    const ntca::UpgradeEvent&                upgradeEvent,
+    const ntci::UpgradeFunction&             cb)
+{
+    BSLS_ASSERT(cb);


An interesting question if we should check this.
If cb is empty, an exception will be thrown when we try to call it a few lines below anyway. We check the same thing twice.
However, we don't do this often, it's not the data path.

678098 · 2025-03-06T15:08:00Z

src/groups/bmq/bmqio/bmqio_ntcchannelfactory.h

+    const ntci::EncryptionClient& encryptionClient() const;
+
+    /// @brief Access this factory's upgrade options.
+    const ntca::UpgradeOptions& upgradeOptions() const;


These added accessors are not used.
If we don't plan to use them, we should probably remove them to simplify the code base.

678098 · 2025-03-06T15:19:36Z

src/groups/bmq/bmqio/bmqio_ntcchannelfactory.t.cpp

+void Tester::setEncryptionAuthority()
+{
+    if (d_authorityCertificate && d_authorityPrivateKey) {
+        // Authority root has already been initialized, no need to set it again


Can also fail the test here and in other set methods in this file. If we try to set something twice it's a problem with a test design probably

678098 · 2025-03-06T15:26:26Z

src/groups/bmq/bmqio/bmqio_ntcchannelfactory.t.cpp

+        // Shorten the timeout interval
+        ntca::UpgradeOptions upgradeOptions;
+        bsls::TimeInterval   deadline = bdlt::CurrentTime::now();
+        // 5ms doesn't seem to interfere with the success test case, so this


Should we set a bigger timeout in case we run on a slower host?

678098 · 2025-03-06T16:10:40Z

@hallfox I have applied my fixes in this PR:
hallfox#1

emelialei88 · 2025-03-25T20:08:20Z

src/groups/bmq/bmqio/bmqio_certificatestore.cpp

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// bmqio_certificatestore.cpp                                         -*-C++-*-


Is this component still being used?

emelialei88 · 2025-03-25T20:09:26Z

.gitignore

 .pytest_cache/
 .cache/
 .venv/
+venv/


maybe not adding an extra virtual environment path to the .gitignore but rather having the user put them in .git/info/exclude if they want to

emelialei88 · 2025-03-25T20:10:31Z

bin/gen-tls-certs.sh

+#
+# Based off of
+#   https://github.com/confluentinc/librdkafka/blob/master/tests/gen-ssl-certs.sh
+


Would be great to be clear where this script is being used.

emelialei88 · 2025-03-26T20:17:16Z

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp

-            negotiate(channel, context);
-        }
+
+        negotiationInit(channel, context);


Shouldn't this function be called only in else branch?

dorjesinpo

few questions

src/groups/bmq/bmqex/bmqex_promise_cpp03.h

src/groups/mqb/mqbcfg/mqbcfg.xsd

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp

dorjesinpo · 2025-04-03T14:52:06Z

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp

+        // during intrusion testing) would trigger this trace.
        BALL_LOG_INFO << "#TCP_UNEXPECTED_STATE "
-                      << "TCPSessionFactory '" << d_config.name()
+                      << "TCPSessionFactory '" << d_threadName


Is the idea of changing d_config.name() to d_threadName to tell TLS from cleartext?

But d_threadName = "bmqIO_" + d_config.name().substr(0, 15 - 6);, would it look the same fro both?

Think this is an old artifact of a previous iteration, cleaning them up.

dorjesinpo · 2025-04-03T14:55:50Z

src/groups/bmq/bmqio/bmqio_ntcchannelfactory.cpp

    return d_limitSignaler.connect(cb);
 }

+bdlmt::SignalerConnection NtcChannelFactory::onUpgrade(const UpgradeFn& cb)


Where is onUpgrade called?

dorjesinpo · 2025-04-03T15:01:14Z

src/groups/bmq/bmqio/bmqio_ntcchannelfactory.cpp

+                            bdlf::PlaceHolders::_2,  // upgradeEvent
+                            callback));
+                    if (rc != 0) {
+                        callback(event, upgradeStatus, channel);


Is this not enough to inject the upgrade before calling TCPSessionFactory::channelStateCallback?

Why do we need d_upgradeSignaler?

Same for processChannelResult

Added ===== - TLS configuration in broker config - Helper script for generating test certs and CAs - TLS options for NtcChannel - Loading certificates and authority data specified from bmqbrkrcfg.json - SessionOptions to bmq package for configuring client sessions - --tls-authority and --tls-version options to bmqtool to configure session options - Client sessions will now require broker TLS sessions when TLS protocol versions are specified - Create CertificateStore component for bmqio - Integration tests for TLS Changed ======= - Update ntf-core and bde dependencies Signed-off-by: Taylor Foxhall <[email protected]> Signed-off-by: Evgeny Malygin <[email protected]>

Signed-off-by: Taylor Foxhall <[email protected]>

hallfox changed the title ~~Support TLS listeners~~ Feat[bmq, mqb]: Support TLS listeners Dec 16, 2024

hallfox force-pushed the tls-poc branch 5 times, most recently from 93f478c to a1e957d Compare December 17, 2024 21:15

chrisbeard reviewed Dec 18, 2024

View reviewed changes

hallfox force-pushed the tls-poc branch from a1e957d to cb68f2a Compare December 18, 2024 20:26

hallfox force-pushed the tls-poc branch 3 times, most recently from 7b352d4 to 634c1e4 Compare January 10, 2025 23:57

dorjesinpo self-assigned this Feb 20, 2025

dorjesinpo self-requested a review February 20, 2025 16:12

chrisbeard reviewed Feb 26, 2025

View reviewed changes

src/groups/bmq/bmqio/bmqio_ntcchannel.h Outdated Show resolved Hide resolved

dorjesinpo assigned 678098 Feb 27, 2025

678098 reviewed Mar 6, 2025

View reviewed changes

emelialei88 requested changes Mar 26, 2025

View reviewed changes

emelialei88 reviewed Mar 26, 2025

View reviewed changes

dorjesinpo reviewed Apr 3, 2025

View reviewed changes

dorjesinpo assigned hallfox and unassigned 678098 and dorjesinpo Apr 9, 2025

hallfox force-pushed the tls-poc branch 3 times, most recently from 2a44dc8 to 10c64a9 Compare May 30, 2025 20:05

hallfox marked this pull request as ready for review May 30, 2025 20:05

hallfox requested a review from a team as a code owner May 30, 2025 20:05

hallfox force-pushed the tls-poc branch from 10c64a9 to a0f6ac8 Compare May 30, 2025 20:07

hallfox force-pushed the tls-poc branch from 3f3fb53 to 91f11f4 Compare November 5, 2025 23:31

hallfox force-pushed the tls-poc branch from 91f11f4 to 0b0f8ce Compare November 5, 2025 23:33

hallfox force-pushed the tls-poc branch from 0b0f8ce to 3442464 Compare November 7, 2025 23:35

WIP: Better abstraction for channel factory pipeline

09af27c

Signed-off-by: Taylor Foxhall <[email protected]>

hallfox force-pushed the tls-poc branch from 3442464 to 09af27c Compare November 14, 2025 23:03

		"loggingVerbosity": "TRACE",
		"consoleSeverityThreshold": "TRACE",

		bmqvt::ValueOrError<Certificate, Error>
		loadCertificateFromPath(const bsl::string& path) BSLS_KEYWORD_OVERRIDE;

Feat[bmq, mqb]: Support TLS listeners #549

Are you sure you want to change the base?

Feat[bmq, mqb]: Support TLS listeners #549

Uh oh!

Conversation

hallfox commented Dec 16, 2024

Added

Changed

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!

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!

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 commented Mar 6, 2025

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 Mar 26, 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!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

emelialei88 Mar 26, 2025 •

edited

Loading