From eb98e652b6c96316a0374da08d1d2bd0e33cdd43 Mon Sep 17 00:00:00 2001 From: Tiago Melo Date: Mon, 16 Mar 2020 20:23:47 -0100 Subject: [PATCH] mgr/dashboard: Fix iSCSI's username and password validation When using regex, instead of strings, angular doesn't assume the pattern must validate the entire string. Add validation to the backend. Fixes: https://tracker.ceph.com/issues/44624 Signed-off-by: Tiago Melo (cherry picked from commit 532b271572f7b6410c099362dc046148f5a686a1) --- src/pybind/mgr/dashboard/controllers/iscsi.py | 35 ++++++++++++ ...i-target-discovery-modal.component.spec.ts | 20 ++++++- .../iscsi-target-discovery-modal.component.ts | 4 +- .../iscsi-target-form.component.spec.ts | 29 +++++++++- .../iscsi-target-form.component.ts | 4 +- .../frontend/src/testing/unit-test-helper.ts | 20 +++++++ src/pybind/mgr/dashboard/tests/test_iscsi.py | 57 ++++++++++++++++++- 7 files changed, 161 insertions(+), 8 deletions(-) diff --git a/src/pybind/mgr/dashboard/controllers/iscsi.py b/src/pybind/mgr/dashboard/controllers/iscsi.py index af5049300001d..a45a99deee3e8 100644 --- a/src/pybind/mgr/dashboard/controllers/iscsi.py +++ b/src/pybind/mgr/dashboard/controllers/iscsi.py @@ -3,6 +3,7 @@ from __future__ import absolute_import from copy import deepcopy +import re import json import cherrypy @@ -197,6 +198,13 @@ class Iscsi(BaseController): @Endpoint('PUT', 'discoveryauth') @UpdatePermission def set_discoveryauth(self, user, password, mutual_user, mutual_password): + validate_auth({ + 'user': user, + 'password': password, + 'mutual_user': mutual_user, + 'mutual_password': mutual_password + }) + gateway = get_available_gateway() config = IscsiClient.instance(gateway_name=gateway).get_config() gateway_names = list(config['gateways'].keys()) @@ -279,6 +287,10 @@ class IscsiTarget(RESTController): clients = clients or [] groups = groups or [] + validate_auth(auth) + for client in clients: + validate_auth(client['auth']) + gateway = get_available_gateway() config = IscsiClient.instance(gateway_name=gateway).get_config() if target_iqn in config['targets']: @@ -300,6 +312,10 @@ class IscsiTarget(RESTController): clients = IscsiTarget._sorted_clients(clients) groups = IscsiTarget._sorted_groups(groups) + validate_auth(auth) + for client in clients: + validate_auth(client['auth']) + gateway = get_available_gateway() config = IscsiClient.instance(gateway_name=gateway).get_config() if target_iqn not in config['targets']: @@ -938,3 +954,22 @@ def validate_rest_api(gateways): '{}'.format(gateway), code='ceph_iscsi_rest_api_not_available_for_gateway', component='iscsi') + + +def validate_auth(auth): + username_regex = re.compile(r'^[\w\.:@_-]{8,64}$') + password_regex = re.compile(r'^[\w@\-_\/]{12,16}$') + result = True + + if auth['user'] or auth['password']: + result = bool(username_regex.match(auth['user'])) and \ + bool(password_regex.match(auth['password'])) + + if auth['mutual_user'] or auth['mutual_password']: + result = result and bool(username_regex.match(auth['mutual_user'])) and \ + bool(password_regex.match(auth['mutual_password'])) and auth['user'] + + if not result: + raise DashboardException(msg='Bad authentication', + code='target_bad_auth', + component='iscsi') diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-discovery-modal/iscsi-target-discovery-modal.component.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-discovery-modal/iscsi-target-discovery-modal.component.spec.ts index ff683b9c87c8b..2f232513bed8a 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-discovery-modal/iscsi-target-discovery-modal.component.spec.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-discovery-modal/iscsi-target-discovery-modal.component.spec.ts @@ -11,7 +11,12 @@ import { RouterTestingModule } from '@angular/router/testing'; import { BsModalRef } from 'ngx-bootstrap/modal'; import { ToastrModule } from 'ngx-toastr'; -import { configureTestBed, i18nProviders } from '../../../../testing/unit-test-helper'; +import { + configureTestBed, + FormHelper, + i18nProviders, + IscsiHelper +} from '../../../../testing/unit-test-helper'; import { Permission } from '../../../shared/models/permissions'; import { SharedModule } from '../../../shared/shared.module'; import { IscsiTargetDiscoveryModalComponent } from './iscsi-target-discovery-modal.component'; @@ -117,4 +122,17 @@ describe('IscsiTargetDiscoveryModalComponent', () => { expect(elemDisabled('input#mutual_password')).toBeTruthy(); expect(elem('cd-submit-button')).toBeNull(); }); + + it('should validate authentication', () => { + component.permission = new Permission(['read', 'create', 'update', 'delete']); + fixture.detectChanges(); + const control = component.discoveryForm; + const formHelper = new FormHelper(control); + formHelper.expectValid(control); + + IscsiHelper.validateUser(formHelper, 'user'); + IscsiHelper.validatePassword(formHelper, 'password'); + IscsiHelper.validateUser(formHelper, 'mutual_user'); + IscsiHelper.validatePassword(formHelper, 'mutual_password'); + }); }); diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-discovery-modal/iscsi-target-discovery-modal.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-discovery-modal/iscsi-target-discovery-modal.component.ts index 3b25328d65319..fcf9e27c142cb 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-discovery-modal/iscsi-target-discovery-modal.component.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-discovery-modal/iscsi-target-discovery-modal.component.ts @@ -22,8 +22,8 @@ export class IscsiTargetDiscoveryModalComponent implements OnInit { permission: Permission; hasPermission: boolean; - USER_REGEX = /[\w\.:@_-]{8,64}/; - PASSWORD_REGEX = /[\w@\-_\/]{12,16}/; + USER_REGEX = /^[\w\.:@_-]{8,64}$/; + PASSWORD_REGEX = /^[\w@\-_\/]{12,16}$/; constructor( private authStorageService: AuthStorageService, diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-form/iscsi-target-form.component.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-form/iscsi-target-form.component.spec.ts index 47c61ad120ba1..eef9a8b394981 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-form/iscsi-target-form.component.spec.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-form/iscsi-target-form.component.spec.ts @@ -7,7 +7,13 @@ import { RouterTestingModule } from '@angular/router/testing'; import { ToastrModule } from 'ngx-toastr'; import { ActivatedRouteStub } from '../../../../testing/activated-route-stub'; -import { configureTestBed, i18nProviders } from '../../../../testing/unit-test-helper'; +import { + configureTestBed, + FormHelper, + i18nProviders, + IscsiHelper +} from '../../../../testing/unit-test-helper'; +import { CdFormGroup } from '../../../shared/forms/cd-form-group'; import { SharedModule } from '../../../shared/shared.module'; import { IscsiTargetFormComponent } from './iscsi-target-form.component'; @@ -247,6 +253,13 @@ describe('IscsiTargetFormComponent', () => { }); }); + it('should validate authentication', () => { + const control = component.targetForm; + const formHelper = new FormHelper(control as CdFormGroup); + formHelper.expectValid('auth'); + validateAuth(formHelper); + }); + describe('should test initiators', () => { beforeEach(() => { component.onImageSelection({ option: { name: 'rbd/disk_2', selected: true } }); @@ -363,6 +376,13 @@ describe('IscsiTargetFormComponent', () => { [{ description: '', enabled: false, name: 'iqn.initiator', selected: false }] ]); }); + + it('should validate authentication', () => { + const control = component.initiators.controls[0]; + const formHelper = new FormHelper(control as CdFormGroup); + formHelper.expectValid(control); + validateAuth(formHelper); + }); }); describe('should submit request', () => { @@ -508,4 +528,11 @@ describe('IscsiTargetFormComponent', () => { }); }); }); + + function validateAuth(formHelper: FormHelper) { + IscsiHelper.validateUser(formHelper, 'auth.user'); + IscsiHelper.validatePassword(formHelper, 'auth.password'); + IscsiHelper.validateUser(formHelper, 'auth.mutual_user'); + IscsiHelper.validatePassword(formHelper, 'auth.mutual_password'); + } }); diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-form/iscsi-target-form.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-form/iscsi-target-form.component.ts index ca2168ddd01e1..32752eb331c0f 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-form/iscsi-target-form.component.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-form/iscsi-target-form.component.ts @@ -82,8 +82,8 @@ export class IscsiTargetFormComponent implements OnInit { }; IQN_REGEX = /^iqn\.(19|20)\d\d-(0[1-9]|1[0-2])\.\D{2,3}(\.[A-Za-z0-9-]+)+(:[A-Za-z0-9-\.]+)*$/; - USER_REGEX = /[\w\.:@_-]{8,64}/; - PASSWORD_REGEX = /[\w@\-_\/]{12,16}/; + USER_REGEX = /^[\w\.:@_-]{8,64}$/; + PASSWORD_REGEX = /^[\w@\-_\/]{12,16}$/; action: string; resource: string; diff --git a/src/pybind/mgr/dashboard/frontend/src/testing/unit-test-helper.ts b/src/pybind/mgr/dashboard/frontend/src/testing/unit-test-helper.ts index c3e5750a32171..e7d8b6fdd5c10 100644 --- a/src/pybind/mgr/dashboard/frontend/src/testing/unit-test-helper.ts +++ b/src/pybind/mgr/dashboard/frontend/src/testing/unit-test-helper.ts @@ -374,3 +374,23 @@ export function expectItemTasks(item: any, executing: string, percentage?: numbe } expect(item.cdExecuting).toBe(executing); } + +export class IscsiHelper { + static validateUser(formHelper: FormHelper, fieldName: string) { + formHelper.expectErrorChange(fieldName, 'short', 'pattern'); + formHelper.expectValidChange(fieldName, 'thisIsCorrect'); + formHelper.expectErrorChange(fieldName, '##?badChars?##', 'pattern'); + formHelper.expectErrorChange( + fieldName, + 'thisUsernameIsWayyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyTooBig', + 'pattern' + ); + } + + static validatePassword(formHelper: FormHelper, fieldName: string) { + formHelper.expectErrorChange(fieldName, 'short', 'pattern'); + formHelper.expectValidChange(fieldName, 'thisIsCorrect'); + formHelper.expectErrorChange(fieldName, '##?badChars?##', 'pattern'); + formHelper.expectErrorChange(fieldName, 'thisPasswordIsWayTooBig', 'pattern'); + } +} diff --git a/src/pybind/mgr/dashboard/tests/test_iscsi.py b/src/pybind/mgr/dashboard/tests/test_iscsi.py index e37a030f244e0..5c591947e636a 100644 --- a/src/pybind/mgr/dashboard/tests/test_iscsi.py +++ b/src/pybind/mgr/dashboard/tests/test_iscsi.py @@ -105,6 +105,31 @@ class IscsiTestController(ControllerTestCase, KVStoreMockMixin): self.assertStatus(200) self.assertJsonBody(discoveryauth) + def test_bad_discoveryauth(self): + discoveryauth = { + 'user': 'myiscsiusername', + 'password': 'myiscsipasswordmyiscsipasswordmyiscsipassword', + 'mutual_user': '', + 'mutual_password': '' + } + put_response = { + 'detail': 'Bad authentication', + 'code': 'target_bad_auth', + 'component': 'iscsi' + } + get_response = { + 'user': '', + 'password': '', + 'mutual_user': '', + 'mutual_password': '' + } + self._put('/api/iscsi/discoveryauth', discoveryauth) + self.assertStatus(400) + self.assertJsonBody(put_response) + self._get('/api/iscsi/discoveryauth') + self.assertStatus(200) + self.assertJsonBody(get_response) + def test_disable_discoveryauth(self): discoveryauth = { 'user': '', @@ -199,6 +224,34 @@ class IscsiTestController(ControllerTestCase, KVStoreMockMixin): }) self._update_iscsi_target(create_request, update_request, response) + @mock.patch('dashboard.controllers.iscsi.IscsiTarget._validate_image') + def test_add_bad_client(self, _validate_image_mock): + target_iqn = "iqn.2003-01.com.redhat.iscsi-gw:iscsi-igw4" + create_request = copy.deepcopy(iscsi_target_request) + create_request['target_iqn'] = target_iqn + update_request = copy.deepcopy(create_request) + update_request['new_target_iqn'] = target_iqn + update_request['clients'].append( + { + "luns": [{"image": "lun1", "pool": "rbd"}], + "client_iqn": "iqn.1994-05.com.redhat:rh7-client4", + "auth": { + "password": "myiscsipassword7myiscsipassword7myiscsipasswo", + "user": "myiscsiusername7", + "mutual_password": "myiscsipassword8", + "mutual_user": "myiscsiusername8"} + }) + response = copy.deepcopy(iscsi_target_response) + response['target_iqn'] = target_iqn + + self._task_post('/api/iscsi/target', create_request) + self.assertStatus(201) + self._task_put('/api/iscsi/target/{}'.format(create_request['target_iqn']), update_request) + self.assertStatus(400) + self._get('/api/iscsi/target/{}'.format(update_request['new_target_iqn'])) + self.assertStatus(200) + self.assertJsonBody(response) + @mock.patch('dashboard.controllers.iscsi.IscsiTarget._validate_image') def test_change_client_password(self, _validate_image_mock): target_iqn = "iqn.2003-01.com.redhat.iscsi-gw:iscsi-igw5" @@ -206,10 +259,10 @@ class IscsiTestController(ControllerTestCase, KVStoreMockMixin): create_request['target_iqn'] = target_iqn update_request = copy.deepcopy(create_request) update_request['new_target_iqn'] = target_iqn - update_request['clients'][0]['auth']['password'] = 'mynewiscsipassword' + update_request['clients'][0]['auth']['password'] = 'MyNewPassword' response = copy.deepcopy(iscsi_target_response) response['target_iqn'] = target_iqn - response['clients'][0]['auth']['password'] = 'mynewiscsipassword' + response['clients'][0]['auth']['password'] = 'MyNewPassword' self._update_iscsi_target(create_request, update_request, response) @mock.patch('dashboard.controllers.iscsi.IscsiTarget._validate_image') -- 2.39.5