From 441d94a37d411da9795329c5dd832cc09cbb51ac Mon Sep 17 00:00:00 2001 From: Evan Baker Date: Tue, 7 Nov 2023 18:44:30 -0600 Subject: [PATCH] fix: validate that NCIDs are well-formed GUIDs (#2359) (#2364) * fix: validate that NCIDs are well-formed GUIDs * fix tests * add logs and test --------- Signed-off-by: Evan Baker --- cns/NetworkContainerContract.go | 22 +++++ cns/NetworkContainerContract_test.go | 95 +++++++++++++++++++ cns/networkcontainers/networkcontainers.go | 6 +- .../networkcontainers_linux.go | 7 +- .../networkcontainers_windows.go | 7 +- cns/restserver/api.go | 12 ++- cns/restserver/api_test.go | 28 +++--- cns/restserver/util.go | 10 +- 8 files changed, 148 insertions(+), 39 deletions(-) diff --git a/cns/NetworkContainerContract.go b/cns/NetworkContainerContract.go index fefc4f9f14..b8e9ffbfa6 100644 --- a/cns/NetworkContainerContract.go +++ b/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 diff --git a/cns/NetworkContainerContract_test.go b/cns/NetworkContainerContract_test.go index f3ff13428f..e5459ed891 100644 --- a/cns/NetworkContainerContract_test.go +++ b/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) + } + }) + } +} diff --git a/cns/networkcontainers/networkcontainers.go b/cns/networkcontainers/networkcontainers.go index 6d78bef460..19dc99f34c 100644 --- a/cns/networkcontainers/networkcontainers.go +++ b/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, diff --git a/cns/networkcontainers/networkcontainers_linux.go b/cns/networkcontainers/networkcontainers_linux.go index e53f423bc2..1d1a28e2a4 100644 --- a/cns/networkcontainers/networkcontainers_linux.go +++ b/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 } diff --git a/cns/networkcontainers/networkcontainers_windows.go b/cns/networkcontainers/networkcontainers_windows.go index 77f0c02bf2..bbb3d3a9ee 100644 --- a/cns/networkcontainers/networkcontainers_windows.go +++ b/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 diff --git a/cns/restserver/api.go b/cns/restserver/api.go index d1e7666b38..e33d3ae1db 100644 --- a/cns/restserver/api.go +++ b/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 { diff --git a/cns/restserver/api_test.go b/cns/restserver/api_test.go index a60289a5c3..1cb6534753 100644 --- a/cns/restserver/api_test.go +++ b/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,7 +86,7 @@ var ( } nc1 = createOrUpdateNetworkContainerParams{ - ncID: "ethWebApp1", + ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d479", ncIP: "11.0.0.5", ncType: cns.AzureContainerInstance, ncVersion: "0", @@ -94,7 +94,7 @@ var ( 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) diff --git a/cns/restserver/util.go b/cns/restserver/util.go index 89c797cc9b..23f59eabe3 100644 --- a/cns/restserver/util.go +++ b/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,9 +651,7 @@ 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 } @@ -663,7 +659,6 @@ func (service *HTTPRestService) isNetworkJoined(networkID string) bool { 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{