ConfChange silently converted to no-op without error notification

[SherlockShemol]

What happened?

When a ConfChange proposal is rejected (e.g., due to an existing pending config change), the proposal is silently converted to a no-op entry without returning any error. This silent failure can lead to security issues where administrators believe a configuration change succeeded when it was actually ignored.

What did you expect to happen?

The caller should receive an error notification when their ConfChange is rejected, allowing them to:

• Detect when their ConfChange was rejected
• Take appropriate action (e.g., wait and retry)
• Log warnings or alerts

How can we reproduce it (as minimally and precisely as possible)?

r := newTestRaft(1, 10, 1, newTestMemoryStorage(withPeers(1, 2)))
r.becomeCandidate()
r.becomeLeader()

// First ConfChange - accepted
r.Step(pb.Message{From: 1, To: 1, Type: pb.MsgProp, 
    Entries: []pb.Entry{{Type: pb.EntryConfChange}}})

// Second ConfChange - silently converted to no-op, err == nil!
err := r.Step(pb.Message{From: 1, To: 1, Type: pb.MsgProp, 
    Entries: []pb.Entry{{Type: pb.EntryConfChange}}})

// BUG: err is nil, but ConfChange was actually rejected!
fmt.Println(err) // <nil>

Security Impact

1. Compromised Node Cannot Be Removed: Admin tries to remove a compromised node, operation "succeeds" (no error), but node remains in cluster
2. Configuration Change DoS: Attacker can block all legitimate config changes by keeping one pending
3. Quorum Confusion: Admin thinks cluster configuration is X, but it's actually Y

Anything else we need to know?

The relevant code is in raft.go around line 1331:

if failedCheck != "" && !r.disableConfChangeValidation {
    r.logger.Infof("%x ignoring conf change %v at config %s: %s", ...)
    m.Entries[i] = pb.Entry{Type: pb.EntryNormal}  // Silent conversion!
    // No error returned!
}

etcd version

main branch (latest)

[crochee]

configure() waits indefinitely on <-ch, but a rejected ConfChange does not trigger NotifyConfChangeApplied.

[ahrtr]

My understanding is that multiple MsgProp may be batched up, so the leader can't just return error to the follower for part of them have error (e.g ignore it if there is a pending conf change).

Note normally users(e.g. etcd) are protected by mTLS, so as we assume that they run inside trusted environment. Also etcd has timeout when handling user request, so it won't be blocked forever.

Related PRs:

• rafthttp: batch MsgProp etcd#1901
• raft: proactively probe newly added followers etcd#11037

[cschanhniem]

The silent conversion at line 1331 (m.GetEntries()[i] = &pb.Entry{Type: pb.EntryNormal.Enum()}) violates the specifications expectation that Step() returns an error when the proposal cannot be processed. Here is a trace of why this is more than a convenience issue.

Protocol impact:

1. No-op entries still consume a log index. A normal entry appended in place of a ConfChange occupies a slot in the log, advancing r.raftLog.lastIndex(). When the follower processes this no-op during replication, it increments its own lastIndex — so the leader and follower agree on log length, but the follower never learns that the intended membership transition was silently dropped. If the operator retries the ConfChange, the second attempt encounters alreadyPending because r.pendingConfIndex > r.raftLog.applied — the first no-op entry established a new pending boundary.

2. Batching is not a barrier to error reporting. The concern raised about batched MsgProp is valid but containable. The stepLeader method processes entries in a loop and already calls r.appendEntry(), which returns ErrProposalDropped for the entire batch when consensus fails. The same pattern can be adapted per-entry: maintain a rejectedEntries set during the loop and, after appendEntry, drain errors for the rejected ConfChange entries while accepting the rest. The ProposeResult mechanism (already exposed in the ProposeResult channel) already gives per-entry error granularity — the missing piece is threading the rejection reason through.

Concrete implementation sketch:

In the ConfChange validation block (raft.go:L1331), instead of silently converting:

if failedCheck != "" && !r.disableConfChangeValidation {
    r.logger.Infof(...)
    m.Entries[i] = &pb.Entry{Type: pb.EntryNormal.Enum()}
    // Record the rejection
    r.recordRejectedConfChange(ctx, failedCheck)
}

Add a rejectedConfChanges map[uint64]string to the raft struct. Populate it when a ConfChange is rejected. In stepLeader, after the appendEntry call, iterate over the rejected map and send errors:

// In stepLeader, after appendEntry:
for i, reason := range r.rejectedConfChanges {
    if resp, ok := r.proposeResponses[i]; ok {
        resp.respond(ErrProposalDropped) // or a new ErrConfChangeRejected
    }
}
r.rejectedConfChanges = nil

This mirrors the existing pattern for handling proposal responses (see handleProposeResponse). The batching integration is straightforward because the entry index within the MsgProp provides a natural key.

Regression test:

Extending TestConfChangeFail (or adding a new test) to assert that the second Step(MsgProp{EntryConfChange}) returns a non-nil error, and that the cluster configuration is unchanged after the rejected proposal, would prevent reintroduction.