fix: validate that NCIDs are well-formed GUIDs (#2359)

cns/NetworkContainerContract.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/Azure/azure-container-networking/cns/types"
 	"github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha"
+	"github.com/google/uuid"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 )
@@ -75,6 +76,8 @@ const (
 	MultiTenantCRD = "MultiTenantCRD"
 )
 
+var ErrInvalidNCID = errors.New("invalid NetworkContainerID")
+
 // CreateNetworkContainerRequest specifies request to create a network container or network isolation boundary.
 type CreateNetworkContainerRequest struct {
 	HostPrimaryIP              string
@@ -96,6 +99,16 @@ type CreateNetworkContainerRequest struct {
 	NCStatus                   v1alpha.NCStatus
 }
 
+func (req *CreateNetworkContainerRequest) Validate() error {
+	if req.NetworkContainerid == "" {
+		return errors.Wrap(ErrInvalidNCID, "NetworkContainerID is empty")
+	}
+	if _, err := uuid.Parse(strings.TrimPrefix(req.NetworkContainerid, SwiftPrefix)); err != nil {
+		return errors.Wrapf(ErrInvalidNCID, "NetworkContainerID %s is not a valid UUID: %s", req.NetworkContainerid, err.Error())
+	}
+	return nil
+}
+
 // CreateNetworkContainerRequest implements fmt.Stringer for logging
 func (req *CreateNetworkContainerRequest) String() string {
 	return fmt.Sprintf("CreateNetworkContainerRequest"+
@@ -374,6 +387,15 @@ type PostNetworkContainersRequest struct {
 	CreateNetworkContainerRequests []CreateNetworkContainerRequest
 }
 
+func (req *PostNetworkContainersRequest) Validate() error {
+	for i := range req.CreateNetworkContainerRequests {
+		if err := req.CreateNetworkContainerRequests[i].Validate(); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 // PostNetworkContainersResponse specifies response of creating all NCs that are sent from DNC.
 type PostNetworkContainersResponse struct {
 	Response Response

cns/NetworkContainerContract_test.go

@@ -105,3 +105,98 @@ func TestNewPodInfoFromIPConfigRequest(t *testing.T) {
 		})
 	}
 }
+
+func TestCreateNetworkContainerRequestValidate(t *testing.T) {
+	tests := []struct {
+		name    string
+		req     CreateNetworkContainerRequest
+		wantErr bool
+	}{
+		{
+			name: "valid",
+			req: CreateNetworkContainerRequest{
+				NetworkContainerid: "f47ac10b-58cc-0372-8567-0e02b2c3d479",
+			},
+			wantErr: false,
+		},
+		{
+			name: "valid",
+			req: CreateNetworkContainerRequest{
+				NetworkContainerid: SwiftPrefix + "f47ac10b-58cc-0372-8567-0e02b2c3d479",
+			},
+			wantErr: false,
+		},
+		{
+			name: "invalid",
+			req: CreateNetworkContainerRequest{
+				NetworkContainerid: "-f47ac10b-58cc-0372-8567-0e02b2c3d479",
+			},
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if err := tt.req.Validate(); (err != nil) != tt.wantErr {
+				t.Errorf("CreateNetworkContainerRequest.Validate() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}
+
+func TestPostNetworkContainersRequest_Validate(t *testing.T) {
+	tests := []struct {
+		name    string
+		req     PostNetworkContainersRequest
+		wantErr bool
+	}{
+		{
+			name: "valid",
+			req: PostNetworkContainersRequest{
+				CreateNetworkContainerRequests: []CreateNetworkContainerRequest{
+					{
+						NetworkContainerid: "f47ac10b-58cc-0372-8567-0e02b2c3d479",
+					},
+					{
+						NetworkContainerid: "f47ac10b-58cc-0372-8567-0e02b2c3d478",
+					},
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "valid",
+			req: PostNetworkContainersRequest{
+				CreateNetworkContainerRequests: []CreateNetworkContainerRequest{
+					{
+						NetworkContainerid: "f47ac10b-58cc-0372-8567-0e02b2c3d479",
+					},
+					{
+						NetworkContainerid: SwiftPrefix + "f47ac10b-58cc-0372-8567-0e02b2c3d478",
+					},
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "invalid",
+			req: PostNetworkContainersRequest{
+				CreateNetworkContainerRequests: []CreateNetworkContainerRequest{
+					{
+						NetworkContainerid: "f47ac10b-58cc-0372-8567-0e02b2c3d479",
+					},
+					{
+						NetworkContainerid: "-f47ac10b-58cc-0372-8567-0e02b2c3d478",
+					},
+				},
+			},
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if err := tt.req.Validate(); (err != nil) != tt.wantErr {
+				t.Errorf("PostNetworkContainersRequest.Validate() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}

cns/networkcontainers/networkcontainers.go

@@ -91,11 +91,7 @@ func (cn *NetworkContainers) Delete(networkContainerID string) error {
 }
 
 // CreateLoopbackAdapter creates a loopback adapter with the specified settings
-func CreateLoopbackAdapter(
-	adapterName string,
-	ipConfig cns.IPConfiguration,
-	setWeakHostOnInterface bool,
-	primaryInterfaceIdentifier string) error {
+func CreateLoopbackAdapter(adapterName string, ipConfig cns.IPConfiguration, setWeakHostOnInterface bool, primaryInterfaceIdentifier string) error {
 	return createOrUpdateWithOperation(
 		adapterName,
 		ipConfig,

cns/networkcontainers/networkcontainers_linux.go

@@ -89,11 +89,6 @@ func configureNetworkContainerNetworking(operation, podName, podNamespace, docke
 	return fmt.Errorf("[Azure CNS] Operation is not supported in linux.")
 }
 
-func createOrUpdateWithOperation(
-	adapterName string,
-	ipConfig cns.IPConfiguration,
-	setWeakHost bool,
-	primaryInterfaceIdentifier string,
-	operation string) error {
+func createOrUpdateWithOperation(string, cns.IPConfiguration, bool, string, string) error {
 	return nil
 }

cns/networkcontainers/networkcontainers_windows.go

@@ -126,12 +126,7 @@ func setWeakHostOnInterface(ipAddress, ncID string) error {
 	return nil
 }
 
-func createOrUpdateWithOperation(
-	adapterName string,
-	ipConfig cns.IPConfiguration,
-	setWeakHost bool,
-	primaryInterfaceIdentifier string,
-	operation string) error {
+func createOrUpdateWithOperation(adapterName string, ipConfig cns.IPConfiguration, setWeakHost bool, primaryInterfaceIdentifier, operation string) error {
 	acnBinaryPath, err := getAzureNetworkContainerBinaryPath()
 	if err != nil {
 		return err

cns/restserver/api.go

@@ -797,14 +797,20 @@ func (service *HTTPRestService) createOrUpdateNetworkContainer(w http.ResponseWr
 	logger.Printf("[Azure CNS] createOrUpdateNetworkContainer")
 
 	var req cns.CreateNetworkContainerRequest
-	err := service.Listener.Decode(w, r, &req)
-	logger.Request(service.Name, req.String(), err)
-	if err != nil {
+	if err := service.Listener.Decode(w, r, &req); err != nil {
+		w.WriteHeader(http.StatusBadRequest)
+		return
+	}
+	if err := req.Validate(); err != nil {
+		logger.Errorf("[Azure CNS] invalid request %+v: %s", req, err)
+		w.WriteHeader(http.StatusBadRequest)
 		return
 	}
 
+	logger.Request(service.Name, req.String(), nil)
 	var returnCode types.ResponseCode
 	var returnMessage string
+	var err error
 	switch r.Method {
 	case http.MethodPost:
 		if req.NetworkContainerType == cns.WebApps {

cns/restserver/api_test.go

@@ -8,7 +8,6 @@ import (
 	"context"
 	"encoding/json"
 	"encoding/xml"
-	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -28,6 +27,7 @@ import (
 	"github.com/Azure/azure-container-networking/nmagent"
 	"github.com/Azure/azure-container-networking/processlock"
 	"github.com/Azure/azure-container-networking/store"
+	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -86,15 +86,15 @@ var (
 	}
 
 	nc1 = createOrUpdateNetworkContainerParams{
-		ncID:         "ethWebApp1",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d479",
 		ncIP:         "11.0.0.5",
 		ncType:       cns.AzureContainerInstance,
 		ncVersion:    "0",
 		podName:      "testpod",
 		podNamespace: "testpodnamespace",
 	}
 	nc2 = createOrUpdateNetworkContainerParams{
-		ncID:         "ethWebApp2",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d478",
 		ncIP:         "11.0.0.5",
 		ncType:       cns.AzureContainerInstance,
 		ncVersion:    "0",
@@ -323,7 +323,7 @@ func TestCreateNetworkContainer(t *testing.T) {
 	fmt.Println("TestCreateNetworkContainer: JobObject")
 
 	params := createOrUpdateNetworkContainerParams{
-		ncID:         "testJobObject",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d476",
 		ncIP:         "10.1.0.5",
 		ncType:       "JobObject",
 		ncVersion:    "0",
@@ -348,7 +348,7 @@ func TestCreateNetworkContainer(t *testing.T) {
 	// Test create network container of type WebApps
 	fmt.Println("TestCreateNetworkContainer: WebApps")
 	params = createOrUpdateNetworkContainerParams{
-		ncID:         "ethWebApp",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d475",
 		ncIP:         "192.0.0.5",
 		ncType:       "WebApps",
 		ncVersion:    "0",
@@ -363,7 +363,7 @@ func TestCreateNetworkContainer(t *testing.T) {
 	}
 
 	params = createOrUpdateNetworkContainerParams{
-		ncID:         "ethWebApp",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d475",
 		ncIP:         "192.0.0.6",
 		ncType:       "WebApps",
 		ncVersion:    "0",
@@ -387,7 +387,7 @@ func TestCreateNetworkContainer(t *testing.T) {
 
 	// Test create network container of type COW
 	params = createOrUpdateNetworkContainerParams{
-		ncID:         "testCOWContainer",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d474",
 		ncIP:         "10.0.0.5",
 		ncType:       "COW",
 		ncVersion:    "0",
@@ -418,7 +418,7 @@ func TestGetNetworkContainerByOrchestratorContext(t *testing.T) {
 	setOrchestratorType(t, cns.Kubernetes)
 
 	params := createOrUpdateNetworkContainerParams{
-		ncID:         "ethWebApp",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d471",
 		ncIP:         "11.0.0.5",
 		ncType:       cns.AzureContainerInstance,
 		ncVersion:    "0",
@@ -480,7 +480,7 @@ func TestGetInterfaceForNetworkContainer(t *testing.T) {
 	setOrchestratorType(t, cns.Kubernetes)
 
 	params := createOrUpdateNetworkContainerParams{
-		ncID:         "ethWebApp",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d479",
 		ncIP:         "11.0.0.5",
 		ncType:       "WebApps",
 		ncVersion:    "0",
@@ -548,7 +548,7 @@ func TestGetNetworkContainerVersionStatus(t *testing.T) {
 	defer cleanupWSP()
 
 	params := createOrUpdateNetworkContainerParams{
-		ncID:         "nc-nma-success",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d475",
 		ncIP:         "11.0.0.5",
 		ncType:       cns.AzureContainerInstance,
 		ncVersion:    "0",
@@ -596,7 +596,7 @@ func TestGetNetworkContainerVersionStatus(t *testing.T) {
 	// Testing the path where the NC version with CNS is higher than the one with NMAgent.
 	// This indicates that the NMAgent is yet to program the NC version.
 	params = createOrUpdateNetworkContainerParams{
-		ncID:         "nc-nma-fail-version-mismatch",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d474",
 		ncIP:         "11.0.0.5",
 		ncType:       cns.AzureContainerInstance,
 		ncVersion:    "1",
@@ -635,7 +635,7 @@ func TestGetNetworkContainerVersionStatus(t *testing.T) {
 
 	// Testing the path where NMAgent response status code is not 200.
 	params = createOrUpdateNetworkContainerParams{
-		ncID:         "nc-nma-fail-500",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d473",
 		ncIP:         "11.0.0.5",
 		ncType:       cns.AzureContainerInstance,
 		ncVersion:    "0",
@@ -677,7 +677,7 @@ func TestGetNetworkContainerVersionStatus(t *testing.T) {
 
 	// Testing the path where NMAgent response status code is 200 but embedded response is 401
 	params = createOrUpdateNetworkContainerParams{
-		ncID:         "nc-nma-fail-unavailable",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d472",
 		ncIP:         "11.0.0.5",
 		ncType:       cns.AzureContainerInstance,
 		ncVersion:    "0",
@@ -1290,7 +1290,7 @@ func postAllNetworkContainers(t *testing.T, ncParams []createOrUpdateNetworkCont
 	err = decodeResponse(w, &resp)
 
 	if err != nil || resp.Response.ReturnCode != types.Success {
-		return fmt.Errorf("post Network Containers failed with response %+v Err:  %w", resp, err)
+		return fmt.Errorf("post Network Containers failed with response %+v: %w", resp, err)
 	}
 	t.Logf("Post Network Containers succeeded with response %+v\n", resp)
 

cns/restserver/util.go

@@ -635,9 +635,7 @@ func (service *HTTPRestService) getNetPluginDetails() *networkcontainers.NetPlug
 func (service *HTTPRestService) getNetworkContainerDetails(networkContainerID string) (containerstatus, bool) {
 	service.RLock()
 	defer service.RUnlock()
-
 	containerDetails, containerExists := service.state.ContainerStatus[networkContainerID]
-
 	return containerDetails, containerExists
 }
 
@@ -653,17 +651,14 @@ func (service *HTTPRestService) areNCsPresent() bool {
 func (service *HTTPRestService) isNetworkJoined(networkID string) bool {
 	namedLock.LockAcquire(stateJoinedNetworks)
 	defer namedLock.LockRelease(stateJoinedNetworks)
-
 	_, exists := service.state.joinedNetworks[networkID]
-
 	return exists
 }
 
 // Set the network as joined
 func (service *HTTPRestService) setNetworkStateJoined(networkID string) {
 	namedLock.LockAcquire(stateJoinedNetworks)
 	defer namedLock.LockRelease(stateJoinedNetworks)
-
 	service.state.joinedNetworks[networkID] = struct{}{}
 }
 
@@ -886,6 +881,11 @@ func (service *HTTPRestService) handlePostNetworkContainers(w http.ResponseWrite
 		logger.Response(service.Name, response, response.Response.ReturnCode, err)
 		return
 	}
+	if err := req.Validate(); err != nil { //nolint:govet // shadow okay
+		logger.Errorf("[Azure CNS] handlePostNetworkContainers failed with error: %s", err.Error())
+		w.WriteHeader(http.StatusBadRequest)
+		return
+	}
 
 	createNCsResp := service.createNetworkContainers(req.CreateNetworkContainerRequests)
 	response := cns.PostNetworkContainersResponse{