Duansg commented on PR #4294:
URL: https://github.com/apache/hertzbeat/pull/4294#issuecomment-5162786458
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
