Skip to content

maintenance: version cluster message envelopes - #4294

Open
zqr10159 wants to merge 10 commits into
apache:masterfrom
zqr10159:maintenance/collector-message-envelope
Open

maintenance: version cluster message envelopes#4294
zqr10159 wants to merge 10 commits into
apache:masterfrom
zqr10159:maintenance/collector-message-envelope

Conversation

@zqr10159

@zqr10159 zqr10159 commented Jul 30, 2026

Copy link
Copy Markdown
Member

Summary

This standalone update authenticates the complete Manager/Collector message
envelope and removes network distribution of the local AES secret.

  • binds signed messages to a per-connection server challenge
  • ships required enforcement as the Java and YAML default
  • keeps optional only for an explicit, bounded mixed-version rollout
  • supports active and previous key IDs for non-disruptive rotation
  • derives a dedicated MAC key with HKDF domain separation, or accepts an
    independent cluster authentication secret
  • validates authentication and AES configuration before Netty startup
  • records bounded rejection, legacy-acceptance, and handshake-timeout metrics

The branch is based directly on current master; it does not depend on #4263.

Installation and upgrade

Manager and every standalone Collector must receive the same two independent
values:

  • COMMON_SECRET: the existing AES key, exactly 16, 24, or 32 bytes
  • CLUSTER_AUTH_ACTIVE_SECRET: an independent message-authentication secret of
    at least 32 bytes

Fresh installs can generate valid values with:

umask 077
printf 'COMMON_SECRET=%s\n' "$(openssl rand -hex 16)" > .env
printf 'CLUSTER_AUTH_ACTIVE_SECRET=%s\n' "$(openssl rand -hex 32)" >> .env

openssl rand -hex 16 produces 32 ASCII bytes and is a valid AES-256
COMMON_SECRET. The 64-character authentication value must remain independent.
Existing installations must preserve their current AES secret because replacing
it can make encrypted data unreadable.

All five Compose definitions require both variables. The Collector guide now
contains executable configuration at the real binding paths:

  • Manager: scheduler.server.authentication
  • standalone Collector: collector.dispatch.entrance.netty.authentication

The environment-variable names are deliberately identical on both sides.

Compatibility

Required mode intentionally rejects an unsigned peer. For a mixed-version
rollout, configure optional mode explicitly, upgrade Collectors first, upgrade
Manager, then restore required mode after every node has converged. Optional
mode is a transition setting, not the steady-state default.

Regression proof

  • 22 remoting authentication/compatibility tests passed.
  • 21 selected Manager/Collector/bootstrap tests plus the real forked Collector
    process test passed.
  • The documentation packaging suite now has 3 passing tests. One test parses
    the fenced YAML and verifies both real Spring binding paths, preventing a
    syntactically plausible but ignored top-level authentication block.
  • All five Docker Compose definitions render with independent valid fixtures.
  • The complete 24-module hertzbeat-startup source package passed on the latest
    master merge, including Checkstyle.
  • git diff --check passed.
  • OCR delegation resolved the applicable Java, protobuf, YAML, concurrency,
    crypto, compatibility, and packaging rules; excluded documentation/test
    changes were manually reviewed. No credible high or medium finding remains.

Operational notes

Blank, known-default, weak authentication secrets and invalid-length AES
secrets fail before the standalone Collector endpoint starts. Clock
synchronization, secret backup, and controlled key distribution remain
deployment responsibilities.

AI assistance: used for draft implementation and test iteration.
Human validation: ran focused authentication/compatibility/bootstrap tests,
rendered all Compose variants, started a real Collector process, parsed the
documented YAML paths, and completed the 24-module source package proof.
Risk notes: optional mode accepts unsigned legacy traffic and must be enabled
only for a bounded rollout window; required remains the shipped steady-state
default.

@zqr10159 zqr10159 changed the title [refactor] validate cluster message envelopes maintenance: version cluster message envelopes Jul 30, 2026
@github-actions github-actions Bot added doc Improvements or additions to documentation script labels Jul 30, 2026
@zqr10159

Copy link
Copy Markdown
Member Author

Author remediation update and remaining blocker:

The branch now has versioned, challenge-bound envelopes; configurable optional-to-required rollout modes; HKDF-separated authentication keys; key IDs and dual-key acceptance for rotation; startup validation; configurable clock skew; rejection metrics; replay handling; and an old and new manager and collector test matrix. Focused and reactor validation passed locally.

This PR is intentionally not author-approved for direct merge yet. It still targets the unmerged #4263 branch, so it must be rebased onto the final integrated master state after #4263 lands and then receive the full repository CI. GitHub currently shows only the label check. Until that dependency and CI condition are satisfied, the merge result remains blocked.

@zqr10159
zqr10159 force-pushed the maintenance/collector-message-envelope branch from c5639a2 to 7768405 Compare July 31, 2026 00:01
@zqr10159
zqr10159 changed the base branch from 0728-yuluo/feat-1 to master July 31, 2026 00:01
@zqr10159

Copy link
Copy Markdown
Member Author

The remaining review blockers are addressed in the latest head (7768405ced). The PR now targets current master directly and contains exactly one commit, so it no longer depends on #4263. Java plus all shipped YAML defaults are required; optional remains available only through an explicit rollout override. The new default regression failed on the prior head (expected REQUIRED but was OPTIONAL) and now passes. Latest local validation: 21 remoting authentication/compatibility tests, 16 targeted manager/collector tests, and the 24-module startup source package proof all passed. Full GitHub CI has now been triggered by the master retarget/push.

@zqr10159
zqr10159 force-pushed the maintenance/collector-message-envelope branch from 7768405 to 183cf6b Compare July 31, 2026 00:02
@zqr10159

Copy link
Copy Markdown
Member Author

Standalone verification is now complete on head 183cf6bb21.

  • The PR targets master and is exactly one commit above it; the former [fix](security): stop sending AES secret over unauthenticated Netty #4263 dependency is included in that standalone commit.
  • required is the shipped Java and YAML default. optional remains available only as an explicit mixed-version rollout override, with removal after convergence documented in the PR.
  • GitHub checks are green: Backend Build, Backend Maven E2E, Backend Image E2E, DOC CI, License Checker, and labeler.

For transparency, the first Maven E2E attempt stopped after 2m42s in the unrelated CommonHttpClientVirtualThreadTest.dispatchConnectionPoolCleanupClosesExpiredAndIdleConnections timing test, where a cleanup call was observed twice. This PR does not modify that client or test, and the complete class passed locally. GitHub attempt 2 passed the full Maven E2E job without any code change in 12m45s: https://github.com/apache/hertzbeat/actions/runs/30592359568

The dependency, CI coverage, and secure steady-state default blockers from the review are therefore resolved.

@zqr10159

Copy link
Copy Markdown
Member Author

Addressed the remaining default-install and upgrade blocker on head
0d850535b1.

What changed:

  • all five shipped Docker Compose definitions now require
    CLUSTER_AUTH_ACTIVE_SECRET during interpolation and return an actionable
    error before container startup when it is absent;
  • every corresponding conf/application.yml exposes the complete
    authentication block;
  • each English and Chinese distribution guide creates a private .env with
    openssl rand -hex 32, requires the value to be preserved across upgrades,
    and requires standalone Collectors to use the same value;
  • root/localized quick starts plus Docker, Compose, package, native-Collector,
    Collector-reference, and upgrade documentation now describe generation,
    sharing, backup, and optional-to-required rollout;
  • a Manager regression proves the all-in-one default replaces and persists its
    known-default AES root before Netty authentication validation, then reuses
    the same root after restart.

Regression proof:

  • on the reviewed head, the packaging contract failed because the Compose
    distributions did not require/document an install secret and the package
    upgrade path was incomplete;
  • after the change, all five definitions fail as expected when the variable is
    absent and all five render with a valid fixture under Docker Compose v5.0.2.

Validation:

  • remoting authentication/compatibility: 18 tests passed, including the
    seven-combination old/new loopback matrix;
  • Manager bootstrap and packaging contracts: 3 tests passed;
  • startup source package: 24-module reactor passed;
  • release profile: 24-module reactor passed and generated server, Docker, and
    Docker Compose archives;
  • the generated Compose archive preserves the required-secret expression in
    all five definitions;
  • Checkstyle, added-test CJK scan, and Git whitespace checks passed.

The branch remains exactly one commit above master. GitHub CI is running on
the amended head.

@zqr10159

Copy link
Copy Markdown
Member Author

Final current-head CI update for 0d850535b1: all checks passed.

  • Backend Build: 9m35s
  • Backend Maven E2E: 13m01s
  • Backend Image E2E: 1m52s
  • DOC CI: 7m26s
  • License Checker: passed
  • labeler: passed

The PR remains based on master, mergeable, and exactly one commit above the
base. No rerun or code change was needed.

@zqr10159
zqr10159 force-pushed the maintenance/collector-message-envelope branch from 0d85053 to e533b4e Compare July 31, 2026 02:16
@zqr10159

Copy link
Copy Markdown
Member Author

Addressed the remaining standalone Collector bootstrap blocker on head
e533b4e106.

The installation contract now provisions two independent values to Manager and
every standalone Collector:

  • COMMON_SECRET, generated with openssl rand -hex 16, yielding a valid
    32-byte AES key;
  • CLUSTER_AUTH_ACTIVE_SECRET, independently generated with
    openssl rand -hex 32.

The 64-character authentication output is explicitly not reused as
COMMON_SECRET. All five Compose definitions require both variables, all five
packaged application configurations map COMMON_SECRET to common.secret,
and the root/localized quick starts plus Docker, Compose, package, native
Collector, Collector-reference, and upgrade guides describe the same contract.
The upgrade path preserves an existing AES key rather than replacing it and
risking unreadable encrypted data.

Regression and runtime proof:

  • the new packaging contract failed on the reviewed head because Compose and
    documentation omitted COMMON_SECRET;
  • the updated 22-module packaging/authentication reactor passed;
  • CollectorDocumentedEnvironmentIntegrationTest launches a separate real
    Collector JVM with both documented variables and verifies stable startup;
  • a rebuilt and extracted apache-hertzbeat-collector-1.9.0-bin.tar.gz was
    started with the same environment, reached Started Collector, and remained
    alive without a common-secret validation error;
  • all five Compose definitions rendered successfully;
  • the 24-module startup package and 24-module release-profile assembly passed;
  • the generated Compose archive retains both requirements in all five
    distributions.

The temporary package extraction and source-tree test logs were removed after
verification. GitHub CI is now running on the amended head.

@zqr10159

Copy link
Copy Markdown
Member Author

Final current-head CI update for e533b4e106: all checks passed.

  • Backend Build: 9m51s
  • Backend Maven E2E: 12m57s
  • Backend Image E2E: 2m46s
  • DOC CI: 9m25s
  • License Checker: passed
  • labeler: passed

The PR remains based directly on master, mergeable, and exactly one commit
above the base. Together with the real packaged-Collector startup proof, the
standalone configuration and documentation blocker is resolved.

@zqr10159
zqr10159 marked this pull request as ready for review July 31, 2026 02:51
@Duansg

Duansg commented Aug 3, 2026

Copy link
Copy Markdown
Member

I think there's a gap in the rollout story that blocks merging as-is.

The compatibility matrix misses AES key distribution

optional mode makes a new Manager accept unsigned messages from an old Collector, so the signing side of a mixed-version rollout is covered. But on master, the GO_ONLINE response is the only place an old Collector ever obtains the AES key (GoOnlineProcessor.java:49-54):

ServerInfo serverInfo = JsonUtil.fromJson(message.getMsg().toStringUtf8(), ServerInfo.class);
if (serverInfo == null || serverInfo.getAesSecret() == null) {
log.warn("The message that server response to collector has not secret empty, please check");
} else {
AesUtil.setDefaultSecretKey(serverInfo.getAesSecret());
}

This PR deletes ServerInfo entirely and strips that branch. So with new Manager + old Collector, the collector logs one warning, keeps AesUtil.secretKey at DEFAULT_ENCODE_RULES, comes online successfully — and then every job carrying a PARAM_TYPE_PASSWORD configmap fails at WheelTimerTask.java:70-73:

if (item.getType() == CommonConstants.PARAM_TYPE_PASSWORD && item.getValue() != null) {
String decodeValue = AesUtil.aesDecode(String.valueOf(item.getValue()));
if (decodeValue == null) {
log.error("Aes Decode value {} error.", item.getValue());

The credential is set to null and collection silently breaks for every monitor with a password parameter. optional mode does not help here, because the failure is in key distribution, not message signing.

The added compatibility tests exercise heartbeat/handshake paths; as far as I can tell none dispatch a job containing a real encrypted parameter, which is why this doesn't show up. Worth adding a test at that level.

Suggestion: either keep a documented upgrade order (upgrade all collectors first, with common.secret pre-provisioned, before upgrading the manager) enforced/asserted somewhere, or have the manager detect a pre-auth collector and refuse to dispatch credentialed jobs to it rather than dispatching jobs it cannot decrypt.

Breaking change that needs a PMC decision

CollectServer.initializeAesSecret() now throws on a blank common.secret. Since the key used to arrive over Netty, every existing standalone collector will fail to start after upgrade until an operator pulls the manager's randomized AES secret out of SystemSecret and provisions it on each collector.

I think that's the correct end state, and I can see the docs were updated across the deploy guides. But it's a hard compatibility break for anyone running remote collectors, so it needs to be an explicit release decision with a prominent upgrade note — not something carried under a maintenance: title. Worth raising on dev@ before merge.

AUTH_CHALLENGE is processed before authentication and can re-pin the nonce

In processReceiveMsg, the AUTH_CHALLENGE branch returns before the verification block, and setChannelNonce has no guard against being called again. A peer that can write to the socket can therefore send a well-formed challenge after the handshake and overwrite the stored channel nonce. It can't forge a signature, so this isn't a bypass, but it does let an on-path party break an established session at will, and it makes the channel-binding invariant weaker than it reads.

Suggestion: accept a challenge only while the channel nonce is unset (mirror the APPLICATION_ACTIVE_NOTIFIED compare-and-set pattern you already use).

Replay cache can evict inside the acceptance window

maximumSize(65_536) with expireAfterWrite(2 × maxClockSkew): on a busy cluster — every heartbeat and every metrics message is signed and inserted — entries can be evicted well before the 10-minute window closes, at which point a captured message becomes replayable again within its skew window. The timestamp bound still limits the exposure, so this is a degradation rather than a hole, but it should be stated in the docs rather than implied to be absolute.

Performance — needs numbers before I'd call it a regression

Every message is signed via message.toByteArray() (a full copy), and verification does toBuilder().clearAuthSignature().build().toByteArray() — two more full serializations — on the Netty event loop. For metrics uploads, which are the large messages here, that's three extra full serializations per message plus the HMAC. I have no benchmark, so I'm flagging it as something to measure rather than asserting a regression, but given this sits on the data path it seems worth a throughput comparison before/after.

Minor

.gitignore and the three README files are unrelated to cluster message authentication. Splitting those out would make the PR easier to review and easier to revert if needed.

@zqr10159

zqr10159 commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

Addressed the actionable review findings in commit 5428958ef9.

Changes:

  • channel nonces are established with compare-and-set; a second well-formed AUTH_CHALLENGE is counted as replay, rejected, and cannot re-pin the connection;
  • the compatibility test now labels new-Manager/old-Collector as transport-only;
  • a credentialed WheelTimerTask regression proves that upgrading Manager first leaves an old Collector unable to decrypt Manager-encrypted parameters;
  • the operator and upgrade guides now make Collector-first order mandatory and explicitly state why heartbeat compatibility is insufficient;
  • the replay section now documents the 65,536-entry cache's capacity-eviction degradation inside the clock-skew window.

Human validation:

  • NettyRemotingAuthenticationTest, NettyRemotingCompatibilityTest, ClusterMessageAuthenticatorTest, and GoOnlineProcessorTest passed with reactor dependencies;
  • git diff --check;
  • a temporary local 1 MiB payload microbenchmark (100 sign+verify iterations, three fresh JVM runs) measured 499.09, 502.22, and 563.68 MiB/s, with 3.44x-6.98x cost versus plain protobuf serialization. The benchmark source was not committed.

The new-head backend, Maven E2E, license, docs, and label checks are currently queued by GitHub and have not started yet.

AI assistance: used for draft implementation and test iteration.
Risk notes: the Collector startup compatibility break is now prominent and executable in tests/docs, but accepting that release break remains a PMC decision. The README and .gitignore changes are retained because the later packaging fix now requires users to provision private install secrets safely.

@Duansg, please re-review this head when convenient.

@zqr10159
zqr10159 requested review from Duansg and tomsun28 August 4, 2026 13:20
@Duansg

Duansg commented Aug 10, 2026

Copy link
Copy Markdown
Member

This PR is a duplicate of #4263. This one involves a breaking change and will be followed up on later.

@Duansg
Duansg marked this pull request as draft August 10, 2026 16:45
Aias00
Aias00 previously approved these changes Aug 17, 2026

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review: maintenance: version cluster message envelopes (#4263 follow-up)

Verdict: ✅ APPROVED (security-focused scoped review) — well-engineered authentication hardening; no crypto flaws found.

Scope note: +3,478 / −339 across 64 files, crypto/transport security is the heart of this change. I focused the review on the security primitives rather than every line. Implementation quality is high.

Security findings (all good)

  • Constant-time MAC comparison: HMAC verification uses MessageDigest.isEqual(...) (lines ~3509, ~3530) — the correct constant-time compare. No timing side-channel.
  • CSPRNG nonces: SecureRandom generates both the 32-byte channel nonce and per-message nonce (secureRandom.nextBytes(nonce)). No predictable IVs.
  • Proper HKDF: extract-then-expand with HMAC-SHA256 over CLUSTER_AUTH_ACTIVE_SECRET, with domain separation for the MAC key.
  • Replay protection via channel binding: AUTH_CHALLENGE carries a per-connection nonce; malformed challenges are rejected and repeated challenges are rejected (covered by shouldRejectRepeatedChallengeAndKeepChannelNonce).
  • Key separation: COMMON_SECRET (AES, 32B) and CLUSTER_AUTH_ACTIVE_SECRET (≥32B, HKDF root) are independent.
  • Real hardening: the collector no longer accepts an AES key pushed from a network response — shouldIgnoreAesSecretFromNetworkResponse proves a Manager response cannot override the local key. Excellent fix.
  • Fail-closed config validation: startup rejects required mode when the server channel challenge is missing.

Non-blocking suggestions

  1. Underlying AES mode: the payload confidentiality still uses the pre-existing AesUtil.aesEncode, whose cipher mode (GCM/CBC/ECB) is not changed by this PR. The new HMAC envelope correctly authenticates the message regardless, but if AesUtil uses a weak mode it would only affect confidentiality, not integrity. Worth a follow-up to confirm/upgrade AesUtil to AES-GCM.
  2. Upgrade impact of required default: shipping required as the default is the secure choice, but an in-place upgrade from an old node (no CLUSTER_AUTH_ACTIVE_SECRET configured) to a new required node will be rejected until the secret is set. The Docker/Compose migration (openssl rand -hex 32) is documented, but operators should be told to use optional during the mixed-version rollout window (the compatibility matrix already covers this — just make sure release notes emphasize it).

No blocking issues. This is a solid, careful piece of security work.

@zqr10159
zqr10159 marked this pull request as ready for review August 24, 2026 09:44
@zqr10159
zqr10159 requested review from Duansg and removed request for Duansg August 26, 2026 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants