Skip to content

fix(coap): keep credentials out of packet debug logs - #19037

Open
hjianbo wants to merge 5 commits into
dev-60from
fix/coap-session-token-and-clientid-override-r60
Open

hjianbo wants to merge 5 commits into
dev-60from
fix/coap-session-token-and-clientid-override-r60

Conversation

@hjianbo

@hjianbo hjianbo commented Sep 17, 2026

Copy link
Copy Markdown
Member

Fixes: https://github.com/emqx/emqx-dev-team-tasks/issues/441
Release version: 6.0.4, 6.1.6, 6.2.4, 6.3.2, 7.0.0
Introduced in: pre-5.8

Summary

CoAP credentials could be written to the connection debug logs in plaintext.

emqx_utils:redact/1 only masks values stored under sensitive keys, so it did
not cover two shapes used by the CoAP gateway:

  • the session token returned as the payload of the 2.01 Created response
    to POST /mqtt/connection. It is the value check_token/2 compares on
    subsequent requests and it is also used on takeover, i.e. a live session
    credential;
  • credentials sent with the short query aliases (t, p), which are not
    recognised as sensitive keys.

The token is now wrapped with emqx_secret where it is produced
(emqx_coap_channel:process_connect/4) and unwrapped in
emqx_coap_frame:serialize_pkt/2 before it is put on the wire, so it can never
reach the shared send_packet / packet_received debug logs. Short-alias
values are redacted in place by emqx_coap_frame:format/1; the query keys stay
exactly as the client sent them, so the log is not misleading.

Channel#channel{token = ...} keeps the raw token, so the token comparison on
heartbeat and takeover is unchanged.

Most relevant modules:

  • apps/emqx_gateway_coap/src/emqx_coap_channel.erl (wrap at the source)
  • apps/emqx_gateway_coap/src/emqx_coap_frame.erl (unwrap for the wire, redact for logs)

Tests:

  • emqx_coap_frame_tests: sensitive query keys, short aliases, wrapped-secret
    payload redaction, and a serialize/parse round-trip proving the raw token is
    still delivered on the wire.
  • emqx_coap_SUITE: an end-to-end check that a live session token never shows
    up in send_packet / packet_received logs, plus a regression test pinning
    the runtime clientinfo to drop clientid_override ([6.3.1]CoAP Gateway retains clientid_override after PR #18842 warning path #18992, not
    reproducible on current code).

Design trade-offs

  • Redacting by value (wrapped secret) instead of by response method keeps the
    frame module free of protocol-semantic guesses, and automatically covers any
    payload a credential is placed in later.
  • The raw token is still present in the channel state, which is required to
    validate subsequent requests; only the logged and serialized representations
    are handled.
  • emqx_coap_frame:format/1 keeps the original query keys (only values are
    redacted) so a client that sent ?t=/?p= is not logged as if it had sent
    ?token=/?password=.

PR Checklist

  • The changes are covered with new or existing tests
  • Change log for changes visible by users has been added to changes/ee/(feat|perf|fix|breaking)-<PR-id>.en.md files
  • Schema changes are backward compatible or intentionally breaking (describe the changes and the reasoning in the summary)

hjianbo added a commit that referenced this pull request Sep 17, 2026
`emqx_utils:redact/1` only masks values under sensitive keys, so two
credentials reached the shared `send_packet` and `packet_received` debug logs
in plaintext: the session token issued on `POST /mqtt/connection` and returned
as the response payload, and credentials sent with the short query aliases
(`t`, `p`).

Wrap the session token in `emqx_secret' where it is produced and unwrap it in
`serialize_pkt/2' for the wire, so it can never be logged. Redact the alias
values in place, keeping the query keys the client actually sent.
Add an end-to-end assertion that a live session token never shows up in the
packet debug logs, and pin the connection flow to drop `clientid_override`
from the runtime clientinfo while still rendering the post-auth mountpoint.
@hjianbo
hjianbo force-pushed the fix/coap-session-token-and-clientid-override-r60 branch from 392f615 to 7187023 Compare September 17, 2026 13:17
@hjianbo
hjianbo marked this pull request as ready for review September 18, 2026 02:53
@hjianbo
hjianbo requested a review from a team as a code owner September 18, 2026 02:53
@hjianbo
hjianbo requested a lite review from Copilot September 18, 2026 03:27

This comment was marked as resolved.

`received_data`, `received_udp_proxy_data` and `send_data` dumped the raw
encoded datagram, which can carry credentials (e.g. CoAP query parameters and
the session token). Log only the byte size; the parsed packet is already
logged by `packet_received` / `send_packet` with frame-level redaction.
`check_auth_state/2` logged a rejected request with `emqx_utils:redact/1`,
which does not know the CoAP short query aliases (`t`, `p`). Share the
alias-aware redaction from `emqx_coap_frame` and use it there instead.

Also assert end-to-end that no debug log carries the session token, which
covers the raw datagram dumps as well.

This comment was marked as low quality.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.52%. Comparing base (15b1aef) to head (fb78ff6).
⚠️ Report is 342 commits behind head on dev-60.

Files with missing lines Patch % Lines
apps/emqx_gateway_coap/src/emqx_coap_frame.erl 90.47% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           dev-60   #19037      +/-   ##
==========================================
+ Coverage   85.08%   85.52%   +0.43%     
==========================================
  Files        1190     1174      -16     
  Lines       86646    88739    +2093     
==========================================
+ Hits        73721    75890    +2169     
+ Misses      12925    12849      -76     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

3 participants