From 169b84bc6c85f985f5d00c5ae0ce00eaed877fc6 Mon Sep 17 00:00:00 2001 From: Joseph Vazhappilly Date: Thu, 21 Mar 2024 07:03:37 -0400 Subject: [PATCH] Restrict software commands to keystone admin role When using Keystone auth for software cli, only user with 'admin' role is allowed to run any commands. When using software cli without 'sudo', all software commands require user with 'admin' role. This review also update the exception handling and error reporting. Test Plan: PASS: A Keystone user in the 'admin' project with 'admin' role should be able to run ALL 'software' commands WITHOUT SUDO PASS: A Keystone user in the 'admin' project with only 'member' and/or 'reader' role should NOT be able to run ANY 'software' commands WITHOUT SUDO Story: 2010676 Task: 49754 Change-Id: I46653021b1a82bccded5eb870dc0907cd5c2351b Signed-off-by: Joseph Vazhappilly --- software-client/software_client/client.py | 7 +- .../software_client/common/http.py | 98 +++++++++++++- software-client/software_client/exc.py | 127 ++++++++++++++++++ .../software_client/software_client.py | 8 +- .../software/api/controllers/v1/software.py | 4 +- software/software/authapi/app.py | 2 + software/software/authapi/hooks.py | 12 +- software/software/authapi/policy.py | 4 - software/software/parsable_error.py | 119 ++++++++++++++++ software/software/utilities/utils.py | 5 +- software/software/utils.py | 8 +- 11 files changed, 358 insertions(+), 36 deletions(-) create mode 100644 software/software/parsable_error.py diff --git a/software-client/software_client/client.py b/software-client/software_client/client.py index 84e74ae5..d9b90fa7 100644 --- a/software-client/software_client/client.py +++ b/software-client/software_client/client.py @@ -111,9 +111,6 @@ def get_client(api_version, auth_mode, session=None, service_type=SERVICE_TYPE, session = _make_session(**kwargs) if not endpoint: - exception_msg = ('Either provide Keystone credentials or ' - 'user-defined endpoint and token or ' - 'execute software command as root (sudo)') if session: try: interface = kwargs.get('os_endpoint_type') @@ -122,12 +119,14 @@ def get_client(api_version, auth_mode, session=None, service_type=SERVICE_TYPE, interface=interface, region_name=region_name) except Exception as e: + msg = ('Failed to get openstack endpoint') raise exc.EndpointException( ('%(message)s, error was: %(error)s') % - {'message': exception_msg, 'error': e}) + {'message': msg, 'error': e}) elif local_root: endpoint = API_ENDPOINT else: + exception_msg = ('Missing / invalid authorization credentials') raise exc.AmbigiousAuthSystem(exception_msg) if endpoint: diff --git a/software-client/software_client/common/http.py b/software-client/software_client/common/http.py index a96d0ad5..f2de3f79 100644 --- a/software-client/software_client/common/http.py +++ b/software-client/software_client/common/http.py @@ -111,6 +111,44 @@ class ServiceCatalog(object): return matching_endpoints[0][endpoint_type] +def _extract_error_json_text(body_json): + error_json = {} + if 'error_message' in body_json: + raw_msg = body_json['error_message'] + if 'error' in raw_msg: + raw_error = jsonutils.loads(raw_msg) + error_json = {'faultstring': raw_error.get('error'), + 'debuginfo': raw_error.get('info')} + elif 'error_message' in raw_msg: + raw_error = jsonutils.loads(raw_msg) + raw_msg = raw_error['error_message'] + error_json = jsonutils.loads(raw_msg) + return error_json + + +def _extract_error_json(body, resp): + """Return error_message from the HTTP response body.""" + try: + content_type = resp.headers.get("Content-Type", "") + except AttributeError: + content_type = "" + if content_type.startswith("application/json"): + try: + body_json = resp.json() + return _extract_error_json_text(body_json) + except AttributeError: + body_json = jsonutils.loads(body) + return _extract_error_json_text(body_json) + except ValueError: + return {} + else: + try: + body_json = jsonutils.loads(body) + return _extract_error_json_text(body_json) + except ValueError: + return {} + + class SessionClient(adapter.LegacyJsonAdapter): def __init__(self, *args, **kwargs): @@ -136,8 +174,16 @@ class SessionClient(adapter.LegacyJsonAdapter): endpoint_filter.setdefault('service_type', self.service_type) endpoint_filter.setdefault('region_name', self.region_name) - return self.session.request(url, method, + resp = self.session.request(url, method, raise_exc=False, **kwargs) + if 400 <= resp.status_code < 600: + error_json = _extract_error_json(resp.content, resp) + raise exceptions.from_response( + resp, error_json.get('faultstring'), + error_json.get('debuginfo'), method, url) + elif resp.status_code in (300, 301, 302, 305): + raise exceptions.from_response(resp, method=method, url=url) + return resp def json_request(self, method, url, **kwargs): kwargs.setdefault('headers', {}) @@ -183,8 +229,6 @@ class SessionClient(adapter.LegacyJsonAdapter): body = None return resp, body - - def raw_request(self, method, url, **kwargs): kwargs.setdefault('headers', {}) kwargs['headers'].setdefault('Content-Type', @@ -222,6 +266,14 @@ class SessionClient(adapter.LegacyJsonAdapter): headers = {'Content-Type': enc.content_type, "X-Auth-Token": self.session.get_token()} response = requests.post(requests_url, data=enc, headers=headers) + if kwargs.get('check_exceptions'): + if response.status_code != 200: + err_message = _extract_error_json(response.text, response) + fault_text = ( + err_message.get("faultstring") + or "Unknown error in SessionClient while uploading request with multipart" + ) + raise exceptions.HTTPBadRequest(fault_text) return response.json() @@ -356,6 +408,26 @@ class HTTPClient(httplib2.Http): else: self.http_log_resp(_logger, resp, body) + status_code = self.get_status_code(resp) + if status_code == 401: + raise exceptions.HTTPUnauthorized(body) + elif status_code == 403: + reason = "Not allowed/Proper role is needed" + if body_str is not None: + error_json = self._extract_error_json(body_str) + reason = error_json.get('faultstring') + if reason is None: + reason = error_json.get('description') + raise exceptions.Forbidden(reason) + elif 400 <= status_code < 600: + _logger.warn("Request returned failure status: %s", status_code) # pylint: disable=deprecated-method + error_json = self._extract_error_json(body_str) + raise exceptions.from_response( + resp, error_json.get('faultstring'), + error_json.get('debuginfo'), *args) + elif status_code in (300, 301, 302, 305): + raise exceptions.from_response(resp, *args) + return resp, body def json_request(self, method, url, **kwargs): @@ -383,6 +455,9 @@ class HTTPClient(httplib2.Http): content_type = resp['content-type'] \ if resp.get('content-type', None) else None + # Add status_code attribute to make compatible with session resp + setattr(resp, 'status_code', resp.status) + if resp.status == 204 or resp.status == 205 or content_type is None: return resp, list() @@ -395,8 +470,6 @@ class HTTPClient(httplib2.Http): else: body = None - # Add status_code attribute to make compatible with session resp - setattr(resp, 'status_code', resp.status) return resp, body def multipart_request(self, method, url, **kwargs): @@ -491,6 +564,21 @@ class HTTPClient(httplib2.Http): ################# # UTILS ################# + def _extract_error_json(self, body): + error_json = {} + try: + body_json = json.loads(body) + if 'error' in body_json: + error_json = {'faultstring': body_json.get('error'), + 'debuginfo': body_json.get('info')} + elif 'error_message' in body_json: + raw_msg = body_json['error_message'] + error_json = json.loads(raw_msg) + except ValueError: + return {} + + return error_json + def _strip_credentials(self, kwargs): if kwargs.get('body') and self.password: log_kwargs = kwargs.copy() diff --git a/software-client/software_client/exc.py b/software-client/software_client/exc.py index 8345456d..0e7a81ed 100644 --- a/software-client/software_client/exc.py +++ b/software-client/software_client/exc.py @@ -43,6 +43,133 @@ class ClientException(Exception): """DEPRECATED.""" +class HTTPException(ClientException): + """Base exception for all HTTP-derived exceptions.""" + code = 'N/A' + + def __init__(self, details=None): + super(HTTPException, self).__init__() + self.details = details + + def __str__(self): + return str(self.details) or "%s (HTTP %s)" % (self.__class__.__name__, + self.code) + + +class HTTPMultipleChoices(HTTPException): + code = 300 + + def __str__(self): + self.details = ("Requested version of Software API is not" + "available.") + return "%s (HTTP %s) %s" % (self.__class__.__name__, self.code, + self.details) + + +class BadRequest(HTTPException): + """DEPRECATED.""" + code = 400 + + +class HTTPBadRequest(BadRequest): + pass + + +class Unauthorized(HTTPException): + """DEPRECATED.""" + code = 401 + + +class HTTPUnauthorized(Unauthorized): + pass + + +class Forbidden(HTTPException): + """DEPRECATED.""" + code = 403 + + +class HTTPForbidden(Forbidden): + pass + + +class NotFound(HTTPException): + """DEPRECATED.""" + code = 404 + + +class HTTPNotFound(NotFound): + pass + + +class HTTPMethodNotAllowed(HTTPException): + code = 405 + + +class Conflict(HTTPException): + """DEPRECATED.""" + code = 409 + + +class HTTPConflict(Conflict): + pass + + +class OverLimit(HTTPException): + """DEPRECATED.""" + code = 413 + + +class HTTPOverLimit(OverLimit): + pass + + +class HTTPInternalServerError(HTTPException): + code = 500 + + +class HTTPNotImplemented(HTTPException): + code = 501 + + +class HTTPBadGateway(HTTPException): + code = 502 + + +class ServiceUnavailable(HTTPException): + """DEPRECATED.""" + code = 503 + + +class HTTPServiceUnavailable(ServiceUnavailable): + pass + + +# NOTE(bcwaldon): Build a mapping of HTTP codes to corresponding exception +# classes +_code_map = {} +for obj_name in dir(sys.modules[__name__]): + if obj_name.startswith('HTTP'): + obj = getattr(sys.modules[__name__], obj_name) + _code_map[obj.code] = obj + + +def from_response(response, message=None, traceback=None, + method=None, url=None): + """Return an instance of an HTTPException based on httplib response.""" + cls = None + if hasattr(response, 'status_code'): + cls = _code_map.get(response.status_code, HTTPException) + elif hasattr(response, 'status_int'): + cls = _code_map.get(response.status_int, HTTPException) + elif hasattr(response, 'status'): + cls = _code_map.get(response.status, HTTPException) + else: + # No status code: return a generic exception + return Exception("Unexpected error in response: %s" % message) + return cls(message) + + class NoTokenLookupException(Exception): """DEPRECATED.""" pass # pylint: disable=unnecessary-pass diff --git a/software-client/software_client/software_client.py b/software-client/software_client/software_client.py index 91dac8ad..0df7ef29 100644 --- a/software-client/software_client/software_client.py +++ b/software-client/software_client/software_client.py @@ -392,17 +392,12 @@ class SoftwareClientShell(object): args.os_endpoint_type = endpoint_type client = sclient.get_client(api_version, auth_mode, **(args.__dict__)) - return args.func(client, args) - # TODO(bqian) reenable below once Exception classes are defined - """ try: - args.func(client, args) + return args.func(client, args) except exc.Unauthorized: raise exc.CommandError("Invalid Identity credentials.") except exc.HTTPForbidden: raise exc.CommandError("Error: Forbidden") - """ - def do_bash_completion(self, args): """Prints all of the commands and options to stdout. @@ -452,7 +447,6 @@ def main(): except Exception as e: print(e, file=sys.stderr) sys.exit(1) - sys.exit(0) if __name__ == "__main__": diff --git a/software/software/api/controllers/v1/software.py b/software/software/api/controllers/v1/software.py index e34ffc86..24c90bb1 100644 --- a/software/software/api/controllers/v1/software.py +++ b/software/software/api/controllers/v1/software.py @@ -6,8 +6,8 @@ SPDX-License-Identifier: Apache-2.0 """ import cgi import json +import logging import os -from oslo_log import log from pecan import expose from pecan import request from pecan import Response @@ -20,7 +20,7 @@ import software.utils as utils import software.constants as constants -LOG = log.getLogger(__name__) +LOG = logging.getLogger('main_logger') class SoftwareAPIController(object): diff --git a/software/software/authapi/app.py b/software/software/authapi/app.py index 708623e6..cb9879f1 100755 --- a/software/software/authapi/app.py +++ b/software/software/authapi/app.py @@ -12,6 +12,7 @@ from software.authapi import acl from software.authapi import config from software.authapi import hooks from software.authapi import policy +from software.parsable_error import ParsableErrorMiddleware from software.utils import ExceptionHook auth_opts = [ @@ -55,6 +56,7 @@ def setup_app(pecan_config=None, extra_hooks=None): debug=False, force_canonical=getattr(pecan_config.app, 'force_canonical', True), hooks=app_hooks, + wrap_app=ParsableErrorMiddleware, guess_content_type_from_ext=False, # Avoid mime-type lookup ) diff --git a/software/software/authapi/hooks.py b/software/software/authapi/hooks.py index 38370e4a..7a9b2350 100755 --- a/software/software/authapi/hooks.py +++ b/software/software/authapi/hooks.py @@ -121,14 +121,8 @@ class AccessPolicyHook(hooks.PecanHook): except Exception: raise exc.HTTPForbidden() else: - method = state.request.method - if method == 'GET': - has_api_access = policy.authorize( - 'reader_in_system_projects', {}, - context.to_dict(), do_raise=False) - else: - has_api_access = policy.authorize( - 'admin_in_system_projects', {}, - context.to_dict(), do_raise=False) + has_api_access = policy.authorize( + 'admin_in_system_projects', {}, + context.to_dict(), do_raise=False) if not has_api_access: raise exc.HTTPForbidden() diff --git a/software/software/authapi/policy.py b/software/software/authapi/policy.py index 6716e438..a0b1f0d4 100755 --- a/software/software/authapi/policy.py +++ b/software/software/authapi/policy.py @@ -30,10 +30,6 @@ base_rules = [ 'role:admin and (project_name:admin or ' + 'project_name:services)', description='Admin user in system projects.'), - policy.RuleDefault('reader_in_system_projects', - 'role:reader and (project_name:admin or ' + - 'project_name:services)', - description='Reader user in system projects.'), policy.RuleDefault('default', 'rule:admin_in_system_projects', description='Default rule.'), ] diff --git a/software/software/parsable_error.py b/software/software/parsable_error.py new file mode 100644 index 00000000..b526fd37 --- /dev/null +++ b/software/software/parsable_error.py @@ -0,0 +1,119 @@ +# Copyright (c) 2018-2024 Wind River Systems, Inc. +# Copyright © 2012 New Dream Network, LLC (DreamHost) +# +# Author: Doug Hellmann +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. +# + +""" +Middleware to replace the plain text message body of an error +response with one formatted so the client can parse it. + +Based on pecan.middleware.errordocument +""" +import html +import json +import logging +import six +import webob +from xml import etree as et + + +LOG = logging.getLogger('main_logger') + +# As per webob.exc code: +# https://github.com/Pylons/webob/blob/master/src/webob/exc.py +# The explanation field is added to the HTTP exception as following: +# ${explanation}

+WEBOB_EXPL_SEP = "

" + + +class ParsableErrorMiddleware(object): + """Replace error body with something the client can parse. + """ + def __init__(self, app): + self.app = app + + def __call__(self, environ, start_response): + # Request for this state, modified by replace_start_response() + # and used when an error is being reported. + state = {} + + def replacement_start_response(status, headers, exc_info=None): + """Overrides the default response to make errors parsable. + """ + try: + status_code = int(status.split(' ')[0]) + state['status_code'] = status_code + except (ValueError, TypeError): # pragma: nocover + raise Exception(( + 'ErrorDocumentMiddleware received an invalid ' + 'status %s' % status + )) + else: + if (state['status_code'] // 100) not in (2, 3): + # Remove some headers so we can replace them later + # when we have the full error message and can + # compute the length. + headers = [(h, v) + for (h, v) in headers + if h not in ('Content-Length', 'Content-Type') + ] + # Save the headers in case we need to modify them. + state['headers'] = headers + return start_response(status, headers, exc_info) + + app_iter = self.app(environ, replacement_start_response) + if (state['status_code'] // 100) not in (2, 3): + req = webob.Request(environ) + if (req.accept.best_match(['application/json', 'application/xml']) == + 'application/xml'): + + try: + # simple check xml is valid + body = [et.ElementTree.tostring( + et.ElementTree.fromstring('' + + '\n'.join(app_iter) + ''))] + except et.ElementTree.ParseError as err: + LOG.error('Error parsing HTTP response: %s' % err) + body = ['%s' % state['status_code'] + + ''] + state['headers'].append(('Content-Type', 'application/xml')) + else: + if six.PY3: + app_iter = [i.decode('utf-8') for i in app_iter] + # Parse explanation field from webob.exc and add it as + # 'faultstring' to be processed by cgts-client + fault = None + app_data = '\n'.join(app_iter) + for data in app_data.split("\n"): + if WEBOB_EXPL_SEP in str(data): + # Remove separator, trailing and leading white spaces + fault = str(data).replace(WEBOB_EXPL_SEP, "").strip() + break + if fault is None: + body = [json.dumps({'error_message': app_data})] + else: + # HTML unescape converts HTML entities back into their + # corresponding chars for human-readable text + fault = html.unescape(fault) + body = [json.dumps({'error_message': + json.dumps({'faultstring': fault})})] + if six.PY3: + body = [item.encode('utf-8') for item in body] + state['headers'].append(('Content-Type', 'application/json')) + state['headers'].append(('Content-Length', str(len(body[0])))) + else: + body = app_iter + return body diff --git a/software/software/utilities/utils.py b/software/software/utilities/utils.py index a4b9b9b5..433d652c 100644 --- a/software/software/utilities/utils.py +++ b/software/software/utilities/utils.py @@ -5,6 +5,7 @@ # import keyring +import logging import os import psycopg2 from psycopg2.extras import RealDictCursor @@ -20,9 +21,7 @@ from software.utilities.constants import KEYRING_PERMDIR from software.utilities import constants -from oslo_log import log - -LOG = log.getLogger(__name__) +LOG = logging.getLogger('main_logger') DB_CONNECTION = "postgresql://%s:%s@127.0.0.1/%s\n" KUBERNETES_CONF_PATH = "/etc/kubernetes" diff --git a/software/software/utils.py b/software/software/utils.py index ffe3aa25..b12c3320 100644 --- a/software/software/utils.py +++ b/software/software/utils.py @@ -48,10 +48,14 @@ class ExceptionHook(hooks.PecanHook): data = dict(info=e.info, warning=e.warning, error=e.error) else: err_msg = "Internal error occurred. Error signature [%s]" % signature + try: + # If exception contains error details, send that to user + if str(e): + err_msg = "Error \"%s\", Error signature [%s]" % (str(e), signature) + except Exception: + pass LOG.error(err_msg) LOG.exception(e) - # Unexpected exceptions, exception message is not sent to the user. - # Instead state as internal error data = dict(info="", warning="", error=err_msg) return webob.Response(json.dumps(data), status=status)