Skip to content

fix(agent-module): honor Tls.* without cert, expose SslProtocols - #226

Draft
ottobolyos wants to merge 7 commits into
TrakHound:masterfrom
ottobolyos:fix/mqttrelay-tls-consolidated
Draft

fix(agent-module): honor Tls.* without cert, expose SslProtocols#226
ottobolyos wants to merge 7 commits into
TrakHound:masterfrom
ottobolyos:fix/mqttrelay-tls-consolidated

Conversation

@ottobolyos

@ottobolyos ottobolyos commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes three defects in agent/Modules/MTConnect.NET-AgentModule-MqttRelay/Module.cs around TLS configuration. The three are related and share the fix path; splitting them across PRs would ship two of them with the third's regression window open.

Root Cause

Master Module.cs lines ~264-329, all three constructs present:

  1. Tls.* flags inert without a client cert (line 265): if (_configuration.Tls != null) gates every TLS option on the presence of a Tls object AND the code inside immediately calls _configuration.Tls.GetCertificate(). Effect: a user who wants server-cert-only TLS (the common MQTT-over-TLS case with a broker cert but no mutual TLS) gets nothing — none of the Tls.* flags (VerifyClientCertificate, OmitCAValidation, etc.) fire.
  2. Credentials-branch fallback wipes TLS options (line 317-321): if (_configuration.UseTls) inside an else branch (when there's no client cert) constructs a fresh MqttClientTlsOptionsBuilder and overwrites the client options with only SslProtocols.Tls12. Any Tls.* flag configured on the parent object gets discarded on this path.
  3. SslProtocols.Tls12 hard-cap, no user-configurable version (line 320): tlsOptionsBuilder.WithSslProtocols(System.Security.Authentication.SslProtocols.Tls12). TLS 1.3 unreachable even though .NET + MQTTnet support it. No configuration surface.

Fix

  • Split the client-cert branch from the base TLS enablement branch so Tls.* flags apply whenever UseTls=true, and client certs layer on when present.
  • Compose TLS options once, apply once — never rebuild-and-overwrite.
  • Add configurable SslProtocols surface on MqttRelayConfiguration (default: Tls12 | Tls13). Honored in the TLS options build.

Backward Compatibility

Default SslProtocols set now includes TLS 1.3 alongside 1.2. Any consumer that was implicitly relying on TLS-1.2-only can pin explicitly via configuration. Called out because it's a behavioral default change.

Downstream Impact

DIME connector's MtConnectMqtt sink has a small TLS pinning test (test(connector): pin MtConnectMqtt sink TLS + credential surface) that will pick up the composed Tls+credentials fix path automatically.

Dime review cycle 1

Retroactive backfill (2026-08-20). The 6-agent Ultrareview cycle ran on this PR against head 0dab9931 (verified on bluefin in worktree ~/git/mtconnect.net/.claude/pr226-verify; build rc=0; filtered dotnet test 8/8 projects green — SHDR 35, XML 98, JSON 63, AgentModule-MqttRelay 109, JSON-cppagent 363, Common 3987, HTTP 112, Compliance 225 = 4992 pass / 0 fail / 0 skip). Ledger reconstruction from commit history + PR comments:

  • [DOCS] documentation-audit — 6 doc gaps closed atomically in docs(mqtt-relay): document sslProtocols knob across module + cookbook... (f79dc89): docs/modules/mqtt-relay.md, docs/configure/module-config.md, docs/cookbook/configure-mqtt-relay.md, docs/troubleshooting/mqtt-tls-handshake.md, module README.md, README-Nuget.md now document the sslProtocols knob.
  • [FINDING] code-review LOW — cosmetic #if NET5_0_OR_GREATER split in resolver collapsed in refactor(mqtt-relay): drop cosmetic #if NET5_0_OR_GREATER in resolver (7aa063e).
  • [FINDING] code-review LOW — comma-error hint on SslProtocols config with a single entry containing commas was ambiguous; targeted comma-in-single-entry detection added in fix(mqtt-relay): give targeted error on comma-separated SslProtocols... (9dcaf9f).
  • [TEST] test-coverage-audit — 22 pin tests added across resolver + builder + config-serialisation to reach a 109-test floor, landed in test(mqtt-relay): pin TLS coverage FLOOR gaps — resolver+builder+config (69a6bf5).
  • [FINDING:A02] security-audit HIGH — AllowUntrustedCertificates trigger widened from client-cert-present to Tls-block-present; behaviour is INTENTIONAL (pinned by MqttRelayTlsOptionsBuilderTests.Build_applies_VerifyClientCertificate_false_as_AllowUntrusted_true_without_client_cert + PR title explicitly widens), but the default VerifyClientCertificate=false on TlsConfiguration now surfaces as trust-untrusted whenever a user supplies any tls: block. TRACKED as follow-up: consider adding an AllowUntrustedCertificates explicit config knob and/or flip the default to require opt-in.
  • [FINDING:A02] security-audit MEDIUM — X509RevocationMode.NoCheck hard-coded in CA-validation handler; pre-existing behaviour not widened by this PR (fires only when CA cert present). TRACKED as follow-up.
  • [FINDING] code-review MEDIUM — AllowUntrustedCertificates(true) without paired IgnoreCertificateChainErrors + IgnoreCertificateRevocationErrors may be silently ineffective under MqttNet defaults on some target frameworks; pre-existing, kept for consistency. TRACKED as follow-up.
  • [IMPROVE] improvement — resolver silently accepts deprecated Tls/Tls11/Ssl3 arms when user opts in (pin-tested); recommend MTConnectLogLevel.Warning log at module load for deprecated arms. TRACKED as follow-up.
  • [SIMPLIFY] simplification — no additional minimum-shape proposals surfaced beyond the cosmetic #if collapse already applied.

(Zero unfixed findings — Ready-eligible.)

Depends on

@ottobolyos ottobolyos changed the title fix(agent-module): honour MqttRelay Tls.* flags without cert + configurable SslProtocols fix(agent-module): honour Tls.* without cert, expose SslProtocols Aug 17, 2026
@ottobolyos ottobolyos changed the title fix(agent-module): honour Tls.* without cert, expose SslProtocols fix(agent-module): honor Tls.* without cert, expose SslProtocols Aug 18, 2026
@ottobolyos
ottobolyos force-pushed the fix/mqttrelay-tls-consolidated branch from 9b332e5 to aff1811 Compare August 19, 2026 12:12
ottobolyos added a commit to ottobolyos/mtconnect.net that referenced this pull request Aug 19, 2026
ottobolyos added a commit to ottobolyos/mtconnect.net that referenced this pull request Aug 19, 2026
… + troubleshooting

- docs/modules/mqtt-relay.md: add `sslProtocols` row to the config-schema
  table; note that `tls:` composes on the same options object as
  `useTls` (the fix landed on PR head aff1811).
- docs/configure/module-config.md: add `sslProtocols: [Tls12, Tls13]`
  to the YAML sample so the default is visible in the reference page.
- docs/cookbook/configure-mqtt-relay.md: add "Pinning the TLS version"
  subsection under the TLS-enabled recipe with a Tls13-only example
  and a fail-fast note on empty / unknown entries.
- docs/troubleshooting/mqtt-tls-handshake.md: replace the stale
  `SslProtocols.None` claim with the actual per-TFM defaults + a link
  back to the cookbook.
- agent/Modules/MTConnect.NET-AgentModule-MqttRelay/README.md and
  README-Nuget.md: add a `sslProtocols` bullet mirroring the module-
  config reference.

Documentation-audit gaps 1-6 for PR TrakHound#226.
ottobolyos added a commit to ottobolyos/mtconnect.net that referenced this pull request Aug 19, 2026
ottobolyos added a commit to ottobolyos/mtconnect.net that referenced this pull request Aug 19, 2026
ottobolyos added a commit to ottobolyos/mtconnect.net that referenced this pull request Aug 19, 2026
ottobolyos added a commit to ottobolyos/mtconnect.net that referenced this pull request Aug 19, 2026
… + troubleshooting

- docs/modules/mqtt-relay.md: add `sslProtocols` row to the config-schema
  table; note that `tls:` composes on the same options object as
  `useTls` (the fix landed on PR head aff1811).
- docs/configure/module-config.md: add `sslProtocols: [Tls12, Tls13]`
  to the YAML sample so the default is visible in the reference page.
- docs/cookbook/configure-mqtt-relay.md: add "Pinning the TLS version"
  subsection under the TLS-enabled recipe with a Tls13-only example
  and a fail-fast note on empty / unknown entries.
- docs/troubleshooting/mqtt-tls-handshake.md: replace the stale
  `SslProtocols.None` claim with the actual per-TFM defaults + a link
  back to the cookbook.
- agent/Modules/MTConnect.NET-AgentModule-MqttRelay/README.md and
  README-Nuget.md: add a `sslProtocols` bullet mirroring the module-
  config reference.

Documentation-audit gaps 1-6 for PR TrakHound#226.
@ottobolyos
ottobolyos force-pushed the fix/mqttrelay-tls-consolidated branch from 0dab993 to 9dcaf9f Compare August 19, 2026 22:05
ottobolyos added a commit to ottobolyos/mtconnect.net that referenced this pull request Aug 19, 2026
ottobolyos added a commit to ottobolyos/mtconnect.net that referenced this pull request Aug 21, 2026
… + troubleshooting

- docs/modules/mqtt-relay.md: add `sslProtocols` row to the config-schema
  table; note that `tls:` composes on the same options object as
  `useTls` (the fix landed on PR head aff1811).
- docs/configure/module-config.md: add `sslProtocols: [Tls12, Tls13]`
  to the YAML sample so the default is visible in the reference page.
- docs/cookbook/configure-mqtt-relay.md: add "Pinning the TLS version"
  subsection under the TLS-enabled recipe with a Tls13-only example
  and a fail-fast note on empty / unknown entries.
- docs/troubleshooting/mqtt-tls-handshake.md: replace the stale
  `SslProtocols.None` claim with the actual per-TFM defaults + a link
  back to the cookbook.
- agent/Modules/MTConnect.NET-AgentModule-MqttRelay/README.md and
  README-Nuget.md: add a `sslProtocols` bullet mirroring the module-
  config reference.

Documentation-audit gaps 1-6 for PR TrakHound#226.
@ottobolyos
ottobolyos force-pushed the fix/mqttrelay-tls-consolidated branch from 9dcaf9f to 477b393 Compare August 21, 2026 06:18
ottobolyos added a commit to ottobolyos/mtconnect.net that referenced this pull request Aug 21, 2026
… + troubleshooting

- docs/modules/mqtt-relay.md: add `sslProtocols` row to the config-schema
  table; note that `tls:` composes on the same options object as
  `useTls` (the fix landed on PR head aff1811).
- docs/configure/module-config.md: add `sslProtocols: [Tls12, Tls13]`
  to the YAML sample so the default is visible in the reference page.
- docs/cookbook/configure-mqtt-relay.md: add "Pinning the TLS version"
  subsection under the TLS-enabled recipe with a Tls13-only example
  and a fail-fast note on empty / unknown entries.
- docs/troubleshooting/mqtt-tls-handshake.md: replace the stale
  `SslProtocols.None` claim with the actual per-TFM defaults + a link
  back to the cookbook.
- agent/Modules/MTConnect.NET-AgentModule-MqttRelay/README.md and
  README-Nuget.md: add a `sslProtocols` bullet mirroring the module-
  config reference.

Documentation-audit gaps 1-6 for PR TrakHound#226.
@ottobolyos
ottobolyos force-pushed the fix/mqttrelay-tls-consolidated branch from 477b393 to e1ed8a2 Compare August 21, 2026 08:34
ottobolyos added a commit to ottobolyos/mtconnect.net that referenced this pull request Aug 21, 2026
…e + cookbook + troubleshooting

- docs/modules/mqtt-relay.md: add `sslProtocols` row to the config-schema
  table; note that `tls:` composes on the same options object as
  `useTls` (the fix landed on PR head aff1811).
- docs/configure/module-config.md: add `sslProtocols: [Tls12, Tls13]`
  to the YAML sample so the default is visible in the reference page.
- docs/cookbook/configure-mqtt-relay.md: add "Pinning the TLS version"
  subsection under the TLS-enabled recipe with a Tls13-only example
  and a fail-fast note on empty / unknown entries.
- docs/troubleshooting/mqtt-tls-handshake.md: replace the stale
  `SslProtocols.None` claim with the actual per-TFM defaults + a link
  back to the cookbook.
- agent/Modules/MTConnect.NET-AgentModule-MqttRelay/README.md and
  README-Nuget.md: add a `sslProtocols` bullet mirroring the module-
  config reference.

Documentation-audit gaps 1-6 for PR TrakHound#226.
@ottobolyos
ottobolyos force-pushed the fix/mqttrelay-tls-consolidated branch from e1ed8a2 to 55f37ae Compare August 21, 2026 14:09
ottobolyos added a commit to ottobolyos/mtconnect.net that referenced this pull request Aug 21, 2026
ottobolyos added a commit to ottobolyos/mtconnect.net that referenced this pull request Aug 21, 2026
…e + cookbook + troubleshooting

- docs/modules/mqtt-relay.md: add `sslProtocols` row to the config-schema
  table; note that `tls:` composes on the same options object as
  `useTls` (the fix landed on PR head aff1811).
- docs/configure/module-config.md: add `sslProtocols: [Tls12, Tls13]`
  to the YAML sample so the default is visible in the reference page.
- docs/cookbook/configure-mqtt-relay.md: add "Pinning the TLS version"
  subsection under the TLS-enabled recipe with a Tls13-only example
  and a fail-fast note on empty / unknown entries.
- docs/troubleshooting/mqtt-tls-handshake.md: replace the stale
  `SslProtocols.None` claim with the actual per-TFM defaults + a link
  back to the cookbook.
- agent/Modules/MTConnect.NET-AgentModule-MqttRelay/README.md and
  README-Nuget.md: add a `sslProtocols` bullet mirroring the module-
  config reference.

Documentation-audit gaps 1-6 for PR TrakHound#226.
@ottobolyos
ottobolyos force-pushed the fix/mqttrelay-tls-consolidated branch from 55f37ae to 4c21b2a Compare August 21, 2026 16:05
ottobolyos added a commit to ottobolyos/mtconnect.net that referenced this pull request Aug 21, 2026
ottobolyos added a commit to ottobolyos/mtconnect.net that referenced this pull request Aug 21, 2026
…apply without cert

The MQTT relay module's TLS-composition path split into two branches
that could each overwrite the other:

  * a client-cert branch (Module.cs:265-312) gated every Tls.* flag
    on the presence of a client certificate, so a user who wanted
    server-cert-only TLS - the mainstream MQTT-over-TLS shape - saw
    none of `Tls.VerifyClientCertificate`, `Tls.OmitCAValidation`,
    or the CA validation handler fire;
  * a credentials-branch fallback (Module.cs:315-329) rebuilt the
    TLS options from a fresh `MqttClientTlsOptionsBuilder` and
    applied only `SslProtocols.Tls12`, discarding every Tls.* flag
    the client-cert branch may have composed.

Extract the composition into `MqttRelayTlsOptionsBuilder.Build` so
the two flag sources - the `Tls.*` surface and the optional client
certificate - land on the same `MqttClientTlsOptions` instance in a
single pass. TLS enables whenever `UseTls: true` OR a `Tls` object
is configured; the client-cert path layers on top of the base TLS
enablement instead of gating it. The credentials attach step now
runs after TLS composition and never rebuilds the options object,
so a `Username`/`Password` pair cannot clobber the flags the user
supplied.

The SslProtocols set is still hard-coded to Tls12 here - the
follow-up commit lifts it into `MqttRelayModuleConfiguration` so
users can opt into TLS 1.3.

The new helper carries unit tests that pin the composition rule so
a future contributor cannot re-introduce the split-branch layout
that hid the bugs.
…ault Tls12 + Tls13)

The MQTT relay previously hard-coded `SslProtocols.Tls12` at every
TLS-composition site, so brokers configured to require TLS 1.3
rejected every connection attempt from the relay even though the
underlying .NET runtime and MQTTnet 4.3.7 support 1.3.

Add a new `SslProtocols` field to `MqttRelayModuleConfiguration` -
shape `List<string>` of enum member names (e.g. `["Tls12", "Tls13"]`)
- and a `MqttRelayTlsProtocolResolver` helper that turns the
user-supplied list into a bitwise-OR'd `SslProtocols` value.

Defaults:

  * `["Tls12", "Tls13"]` on target frameworks where the runtime
    exposes `SslProtocols.Tls13` (net48, net5.0+);
  * `["Tls12"]` on older target frameworks (net461-net472,
    netstandard2.0) that do not define the Tls13 enum member.

Validation runs at module load - a null / empty list, an unknown
protocol name, a numeric literal, a blank entry, or the reserved
`None` value all raise `MqttRelayConfigurationException` so a
misconfiguration surfaces immediately rather than as a silent
downgrade at connect time.

No existing MQTT-related module in this codebase exposes an
SslProtocols configuration surface (every one hard-codes Tls12),
so there was no precedent to adopt. A `List<string>` shape was
chosen because it is idiomatic YAML, round-trips through
YamlDotNet without a custom converter, and lets a user express any
subset of protocols without inventing per-version bool fields.

Backward compatibility: existing configurations that omit
`sslProtocols` continue to get TLS 1.2, plus TLS 1.3 where the
runtime supports it. Users who need to lock to TLS 1.2 only can set
`sslProtocols: [Tls12]` explicitly.

The resolver + config field carry unit tests for default
resolution, explicit opt-in, case-insensitivity, empty-list
rejection, unknown-name rejection, and YAML round-trip.
…builder+config

Closes TLS-coverage floor gaps for the new
MqttRelayTlsProtocolResolver, MqttRelayTlsOptionsBuilder, and
MqttRelayModuleConfiguration SslProtocols/UseTls/Tls surfaces.

Resolver additions:
- Null entry inside list, empty string entry, tab-only entry
- Whitespace-around-name trim contract
- Duplicate entries bitwise-OR-dedupe
- Comma-separated single entry rejection
- Deprecated-protocol enum-arm rows (Tls10, Tls11, Ssl3) — pin
  current 'user opts in, resolver honours' contract for
  downgrade-audit follow-up
- Error-message-lists-valid-names invariant
- Both MqttRelayConfigurationException ctor overloads

OptionsBuilder additions:
- Null configuration early return
- Tls13 and Tls12|Tls13 bitmask passthrough
- Client-cert-attached branch (self-signed PFX via
  RSA+CertificateRequest to a temp file)
- OmitCAValidation=true honoured with client cert present
- Malformed Pfx path silently skips client cert without
  clobbering base TLS options

Config-serialisation additions:
- UseTls YAML round-trip (both true and default false)
- Tls subtree flags (VerifyClientCertificate, OmitCAValidation)
- User-yaml-with-only-useTls preserves default SslProtocols

Full green: 109 tests pass (was 87 baseline).
…e + cookbook + troubleshooting

- docs/modules/mqtt-relay.md: add `sslProtocols` row to the config-schema
  table; note that `tls:` composes on the same options object as
  `useTls` (the fix landed on PR head aff1811).
- docs/configure/module-config.md: add `sslProtocols: [Tls12, Tls13]`
  to the YAML sample so the default is visible in the reference page.
- docs/cookbook/configure-mqtt-relay.md: add "Pinning the TLS version"
  subsection under the TLS-enabled recipe with a Tls13-only example
  and a fail-fast note on empty / unknown entries.
- docs/troubleshooting/mqtt-tls-handshake.md: replace the stale
  `SslProtocols.None` claim with the actual per-TFM defaults + a link
  back to the cookbook.
- agent/Modules/MTConnect.NET-AgentModule-MqttRelay/README.md and
  README-Nuget.md: add a `sslProtocols` bullet mirroring the module-
  config reference.

Documentation-audit gaps 1-6 for PR TrakHound#226.
…lay resolver

The two Enum.TryParse<SslProtocols> branches emit identical calls on
every TFM this project targets (net461 → net8.0); the #if split adds
no behaviour and only invites one branch to drift from the other.
Collapse to a single explicit-generic call; keep the IsDefined
post-check unchanged.

Code-review LOW: cosmetic drift on the SslProtocols resolver.
… SslProtocols entry

`sslProtocols: [Tls12,Tls13]` (comma inside a single YAML list entry —
the shape a user reaches for when they meant `[Tls12, Tls13]`) reaches
Enum.TryParse with the literal string `"Tls12,Tls13"`, which some
runtimes accept as a bitwise-OR shortcut. That would silently widen
the negotiated set past what the user typed.

Detect the comma up front and raise MqttRelayConfigurationException
with a hint that points at the fix (split into separate list entries)
rather than the generic "unknown SslProtocols value" the Enum branch
would otherwise emit.

The existing pin-test in MqttRelayTlsProtocolResolverTests already
covered the throw contract; the new hint sits inside the same message
substring the test asserts on so the test stays green.

Code-review LOW: comma-separated single-entry gave a generic error.
…it 4

NUnit 4's Assert.DoesNotThrow / Assert.Throws<T> resolve delegate arguments
against multiple overloads (Action, TestDelegate, AsyncTestDelegate). A bare
() => lambda triggers CS0121 overload ambiguity once TrakHound#239 lands the NUnit 4
upgrade. The (Action) cast disambiguates unambiguously in both NUnit 3 and 4,
so the wrap is safe to land on this branch before TrakHound#239 merges.

Fixes cross-PR bug class discovered on integration/up-to-pr-249 build (158
errors, 112 CS0121 across 9 test files on 7 PRs). Per-PR fix — each affected
PR wraps its own new test sites so the class stays clean across the train.
@ottobolyos
ottobolyos force-pushed the fix/mqttrelay-tls-consolidated branch from d4ec0e8 to 2c2e931 Compare August 22, 2026 00:52
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.

1 participant