diff --git a/services/eap-noob/guards_test.go b/services/eap-noob/guards_test.go new file mode 100644 index 000000000..cf9375c82 --- /dev/null +++ b/services/eap-noob/guards_test.go @@ -0,0 +1,112 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package eapnoob + +import "testing" + +// expectStateMismatch asserts that a message was refused as out of order: a +// terminal ErrStateMismatch, no success, and no transport error. +func expectStateMismatch(t *testing.T, out Outcome, err error) { + t.Helper() + if err != nil { + t.Fatalf("Receive returned err %v; an out-of-order message must surface in out.Err", err) + } + if out.Err == nil || out.Err.Code != ErrStateMismatch { + t.Fatalf("out.Err = %v, want code %d (state mismatch)", out.Err, ErrStateMismatch) + } + if !out.Done || out.Success { + t.Fatalf("out Done=%v Success=%v, want a terminal failure", out.Done, out.Success) + } +} + +// A completion message can arrive while the server is waiting for the PIN, +// before its authentication keys exist. Before PIN verification the transcript +// is public, so an empty-key MAC is computable without the PIN and must not +// grant trust. +func TestServerRejectsCompletionBeforeKeysAreDerived(t *testing.T) { + for _, suite := range []int{1, 2} { + srv := NewServer(ServerConfig{Cryptosuites: []int{suite}, Dirs: 2}, nil) + peer := NewPeer(PeerConfig{PreferDir: 2}, nil) + driveConversation(t, srv, peer) + if srv.State() != StateWaiting || len(srv.keyM.Kmp) != 0 { + t.Fatalf("suite %d: server state=%s Kmp=%d, want Waiting with no keys", suite, srv.State(), len(srv.keyM.Kmp)) + } + + macp := srv.suite.computeMAC(nil, 1, srv.in) + msg, _, err := encode(&wireMessage{ + Type: intp(typeCompletion), + PeerId: rawString(srv.peerId), + MACp: rawString(b64Encode(macp)), + }) + if err != nil { + t.Fatalf("encode: %v", err) + } + + out, err := srv.Receive(msg) + expectStateMismatch(t, out, err) + if srv.State() == StateRegistered { + t.Fatalf("suite %d: server registered on an empty-key MAC", suite) + } + } +} + +// A sender can answer discovery with a key-exchange response before +// negotiation selects an algorithm and creates the server's private key. +func TestServerRejectsKeyExchangeBeforeNegotiation(t *testing.T) { + srv := NewServer(ServerConfig{}, nil) + if _, err := srv.Start(); err != nil { + t.Fatalf("server Start: %v", err) + } + out, err := srv.Receive([]byte(`{"Type":3,"PKp":{},"Np":""}`)) + expectStateMismatch(t, out, err) + if srv.State() != StateUnregistered { + t.Fatalf("server state %s, want Unregistered", srv.State()) + } +} + +// A sender can skip negotiation after discovery. The peer must reject its +// key-exchange message before touching the algorithm negotiation selects. +func TestPeerRejectsKeyExchangeBeforeNegotiation(t *testing.T) { + peer := NewPeer(PeerConfig{}, nil) + discovery, err := peer.Receive([]byte(`{"Type":1}`)) + if err != nil || discovery.Err != nil { + t.Fatalf("discovery: err=%v out.Err=%v", err, discovery.Err) + } + out, err := peer.Receive([]byte(`{"Type":3,"PKs":{},"Ns":""}`)) + expectStateMismatch(t, out, err) + if peer.State() != StateUnregistered { + t.Fatalf("peer state %s, want Unregistered", peer.State()) + } +} + +// EAP-Success only means something once the peer has verified the server's +// Completion MAC. Earlier, it must not register the association. +func TestPeerRejectsSuccessBeforeVerifiedCompletion(t *testing.T) { + srv := NewServer(ServerConfig{Dirs: 2}, nil) + peer := NewPeer(PeerConfig{PreferDir: 2}, nil) + driveConversation(t, srv, peer) + + out, err := peer.Receive(resultBytes(eapSuccess)) + expectStateMismatch(t, out, err) + if peer.State() == StateRegistered { + t.Fatal("peer registered on an unverified EAP-Success") + } + if _, ok, _ := peer.store.Load(peer.peerId); ok { + t.Fatal("peer stored an association on an unverified EAP-Success") + } +} + +// Any message between the verified Completion and EAP-Success clears the +// verification, a malformed one included. +func TestPeerSuccessNeedsTheCompletionDirectlyBeforeIt(t *testing.T) { + srv := NewServer(ServerConfig{Dirs: 2}, nil) + peer := NewPeer(PeerConfig{PreferDir: 2}, nil) + driveConversation(t, srv, peer) + peer.completionVerified = true // as after a verified Completion request + if _, err := peer.Receive([]byte("not json")); err != nil { + t.Fatalf("malformed input: %v", err) + } + out, err := peer.Receive(resultBytes(eapSuccess)) + expectStateMismatch(t, out, err) +} diff --git a/services/eap-noob/peer.go b/services/eap-noob/peer.go index d1d5042ba..42a74230d 100644 --- a/services/eap-noob/peer.go +++ b/services/eap-noob/peer.go @@ -35,6 +35,10 @@ type Peer struct { cfg PeerConfig store Store state State + // completionVerified is set only while the last message the peer handled + // was a Completion request whose MACs verified. EAP-Success is accepted + // only then: anything else would register on the server's word alone. + completionVerified bool methodState @@ -86,11 +90,13 @@ func (p *Peer) Export(label string, context []byte, length int) ([]byte, error) func (p *Peer) Receive(in []byte) (Outcome, error) { wm, err := decode(in) if err != nil { + p.completionVerified = false return p.fail(ErrInvalidMessageStructure, "malformed JSON") } if wm.EAP != "" { return p.onResult(wm) } + p.completionVerified = false if wm.ErrorCode != nil { return Outcome{State: p.state, Done: true, Err: &ProtocolError{Code: *wm.ErrorCode, Info: wm.ErrorInfo}}, nil } @@ -116,7 +122,12 @@ func (p *Peer) Receive(in []byte) (Outcome, error) { } func (p *Peer) onResult(wm *wireMessage) (Outcome, error) { + verified := p.completionVerified + p.completionVerified = false if wm.EAP == eapSuccess { + if !verified { + return p.fail(ErrStateMismatch, "success before verified Completion") + } assoc := &Association{ PeerId: p.peerId, NAI: p.nai, @@ -230,6 +241,9 @@ func (p *Peer) onNegotiationRequest(wm *wireMessage) (Outcome, error) { } func (p *Peer) onKeyExchangeRequest(wm *wireMessage) (Outcome, error) { + if p.state != StateUnregistered || p.suite == nil { + return p.fail(ErrStateMismatch, "key exchange requires negotiation during the initial exchange") + } jwk, err := rawToJWK(wm.PKs) if err != nil { return p.fail(ErrInvalidData, "bad PKs") @@ -342,6 +356,7 @@ func (p *Peer) onCompletionRequest(wm *wireMessage) (Outcome, error) { if err != nil { return Outcome{}, err } + p.completionVerified = true return Outcome{Send: b, State: p.state}, nil } diff --git a/services/eap-noob/server.go b/services/eap-noob/server.go index cd95d35e4..6c763cb1a 100644 --- a/services/eap-noob/server.go +++ b/services/eap-noob/server.go @@ -264,6 +264,9 @@ func (s *Server) buildKeyExchangeRequest() (Outcome, error) { } func (s *Server) onKeyExchange(wm *wireMessage) (Outcome, error) { + if s.state != StateUnregistered || s.suite == nil || s.priv == nil { + return s.fail(ErrStateMismatch, "key exchange requires negotiation during the initial exchange") + } if err := s.checkPeerId(wm); err != nil { return s.fail(ErrUnexpectedPeerIdentier, err.Error()) } @@ -365,6 +368,12 @@ func (s *Server) buildCompletionRequest() (Outcome, error) { } func (s *Server) onCompletion(wm *wireMessage) (Outcome, error) { + // Completion follows the OOB step: Waiting in the server-to-peer direction, + // OOBReceived in the peer-to-server direction. Either way the keys must + // already be derived, or MACp would be checked against an empty key. + if (s.state != StateWaiting && s.state != StateOOBReceived) || s.suite == nil || len(s.keyM.Kmp) == 0 { + return s.fail(ErrStateMismatch, "completion before authentication keys are ready") + } if err := s.checkPeerId(wm); err != nil { return s.fail(ErrUnexpectedPeerIdentier, err.Error()) } diff --git a/services/nvpair-cluster-manager/httpserver.go b/services/nvpair-cluster-manager/httpserver.go index f010e58bd..3165215de 100644 --- a/services/nvpair-cluster-manager/httpserver.go +++ b/services/nvpair-cluster-manager/httpserver.go @@ -195,19 +195,28 @@ func (m *Manager) handlePairingInitial(w http.ResponseWriter, env *pairingEnvelo return } - sess.mu.Lock() - out, err := sess.peer.Receive(msg) + var completed *Invite + // Scope the lock so a panic releases it and notifications run after + // unlocking. A terminal protocol error releases the session too, or a + // sender repeating malformed exchanges would hold pending-session slots + // until they expire. + out, err := func() (eapnoob.Outcome, error) { + sess.mu.Lock() + defer sess.mu.Unlock() + out, err := sess.peer.Receive(msg) + if err != nil || out.Err != nil { + m.deleteSession(env.InviteID) + return out, err + } + if out.State == eapnoob.StateWaiting && !sess.invitedEmitted { + completed = m.prepareJoinerInitialComplete(env.InviteID, sess) + } + return out, nil + }() if err != nil { - m.deleteSession(env.InviteID) - sess.mu.Unlock() http.Error(w, "eap-noob: "+err.Error(), http.StatusBadRequest) return } - var completed *Invite - if out.State == eapnoob.StateWaiting && !sess.invitedEmitted { - completed = m.prepareJoinerInitialComplete(env.InviteID, sess) - } - sess.mu.Unlock() if completed != nil { m.onJoinerInitialComplete(completed, sess) } diff --git a/services/nvpair-cluster-manager/pairing_order_test.go b/services/nvpair-cluster-manager/pairing_order_test.go new file mode 100644 index 000000000..de7a9731c --- /dev/null +++ b/services/nvpair-cluster-manager/pairing_order_test.go @@ -0,0 +1,57 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "encoding/base64" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "eapnoob" +) + +// Skipping negotiation used to panic while holding the session lock, leaving +// a session expiry could never reclaim. It must now fail as a protocol error +// and release the pending-session slot. +func TestPairingInitialRejectsKeyExchangeBeforeNegotiation(t *testing.T) { + m := testManagerAt(t, t.TempDir(), 15211) + env := &pairingEnvelope{InviteID: "skipped-negotiation", Phase: "initial"} + + discovery := httptest.NewRecorder() + m.handlePairingInitial(discovery, env, []byte(`{"Type":1}`), "127.0.0.1") + if discovery.Code != http.StatusOK { + t.Fatalf("discovery status %d, want 200", discovery.Code) + } + if _, ok := m.getSession(env.InviteID); !ok { + t.Fatal("discovery did not register a joiner session") + } + + response := httptest.NewRecorder() + m.handlePairingInitial(response, env, []byte(`{"Type":3,"PKs":{},"Ns":""}`), "127.0.0.1") + if response.Code != http.StatusOK { + t.Fatalf("key-exchange status %d, want 200 carrying an EAP-NOOB error", response.Code) + } + var reply pairingEnvelope + if err := json.Unmarshal(response.Body.Bytes(), &reply); err != nil { + t.Fatalf("decode envelope: %v", err) + } + message, err := base64.StdEncoding.DecodeString(reply.Msg) + if err != nil { + t.Fatalf("decode message: %v", err) + } + var failure struct { + ErrorCode int `json:"ErrorCode"` + } + if err := json.Unmarshal(message, &failure); err != nil { + t.Fatalf("decode failure: %v", err) + } + if failure.ErrorCode != eapnoob.ErrStateMismatch { + t.Fatalf("ErrorCode %d, want %d (state mismatch)", failure.ErrorCode, eapnoob.ErrStateMismatch) + } + if _, ok := m.getSession(env.InviteID); ok { + t.Fatal("a terminal protocol error must release the pending session slot") + } +} diff --git a/services/versions.json b/services/versions.json index 722be4d4b..bedf23f14 100644 --- a/services/versions.json +++ b/services/versions.json @@ -13,7 +13,7 @@ "nvpair-node-settings": "3.1.0", "nvpair-ui-broker": "2.4.1", "nvpair-engine-manager": "1.3.0", - "nvpair-cluster-manager": "2.1.0", + "nvpair-cluster-manager": "2.1.1", "nvpair-job-scheduler": "1.1.0", "nvpair-tui": "1.2.0", "omlx-proxy": "1.4.1"