From 5e0ce4eea9a58d00b920fc0da0065beca1ab25ad Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 8 Jan 2015 13:26:24 -0500 Subject: [PATCH] Add validation of github to the config tool --- endpoints/api/suconfig.py | 2 +- .../directives/config/config-setup-tool.html | 16 +++++--- static/js/core-config-setup.js | 10 ++++- util/config/validator.py | 36 ++++++++++++++--- util/oauth.py | 40 ++++++++++++++----- 5 files changed, 80 insertions(+), 24 deletions(-) diff --git a/endpoints/api/suconfig.py b/endpoints/api/suconfig.py index 25b91380d..d403b617b 100644 --- a/endpoints/api/suconfig.py +++ b/endpoints/api/suconfig.py @@ -53,7 +53,7 @@ class SuperUserRegistryStatus(ApiResource): 'file_exists': os.path.exists(OVERRIDE_CONFIG_YAML_FILENAME), 'is_testing': app.config['TESTING'], 'valid_db': database_is_valid(), - 'ready': not app.config['TESTING'] and file_exists and bool(app.config['SUPER_USERS']) + 'ready': not app.config['TESTING'] and config_file_exists() and bool(app.config['SUPER_USERS']) } diff --git a/static/directives/config/config-setup-tool.html b/static/directives/config/config-setup-tool.html index 3480a1104..63f9701e5 100644 --- a/static/directives/config/config-setup-tool.html +++ b/static/directives/config/config-setup-tool.html @@ -369,11 +369,13 @@ Github Endpoint: - +
- The Github Enterprise endpoint. + The Github Enterprise endpoint. Must start with http:// or https://.
@@ -499,11 +501,13 @@ Github Endpoint: - +
- The Github Enterprise endpoint. + The Github Enterprise endpoint. Must start with http:// or https://.
diff --git a/static/js/core-config-setup.js b/static/js/core-config-setup.js index a57e74b37..6950ebe68 100644 --- a/static/js/core-config-setup.js +++ b/static/js/core-config-setup.js @@ -29,6 +29,10 @@ angular.module("core-config-setup", ['angularFileUpload']) {'id': 'github-login', 'title': 'Github (Enterprise) Authentication', 'condition': function(config) { return config.FEATURE_GITHUB_LOGIN; + }}, + + {'id': 'github-trigger', 'title': 'Github (Enterprise) Build Triggers', 'condition': function(config) { + return config.FEATURE_GITHUB_BUILD; }} ]; @@ -151,8 +155,10 @@ angular.module("core-config-setup", ['angularFileUpload']) } if (value == 'enterprise') { - $scope.config[key]['GITHUB_ENDPOINT'] = ''; - $scope.config[key]['API_ENDPOINT'] = ''; + if ($scope.config[key]['GITHUB_ENDPOINT'] == 'https://github.com/') { + $scope.config[key]['GITHUB_ENDPOINT'] = ''; + } + delete $scope.config[key]['API_ENDPOINT']; } else if (value == 'hosted') { $scope.config[key]['GITHUB_ENDPOINT'] = 'https://github.com/'; $scope.config[key]['API_ENDPOINT'] = 'https://api.github.com/'; diff --git a/util/config/validator.py b/util/config/validator.py index 16211c254..7be8657df 100644 --- a/util/config/validator.py +++ b/util/config/validator.py @@ -32,16 +32,19 @@ def validate_service_for_config(service, config): 'reason': str(ex) } + def _validate_database(config): """ Validates connecting to the database. """ validate_database_url(config['DB_URI']) + def _validate_redis(config): """ Validates connecting to redis. """ redis_config = config['BUILDLOGS_REDIS'] client = redis.StrictRedis(socket_connect_timeout=5, **redis_config) client.ping() + def _validate_registry_storage(config): """ Validates registry storage. """ parameters = config.get('DISTRIBUTED_STORAGE_CONFIG', {}).get('local', ['LocalStorage', {}]) @@ -54,6 +57,7 @@ def _validate_registry_storage(config): driver.put_content('_verify', 'testing 123') driver.remove('_verify') + def _validate_mailing(config): """ Validates sending email. """ test_app = Flask("mail-test-app") @@ -68,12 +72,31 @@ def _validate_mailing(config): test_msg.add_recipient(get_authenticated_user().email) test_mail.send(test_msg) -def _validate_github_login(config): - """ Validates the OAuth credentials and API endpoint for Github Login. """ + +def _validate_github(config_key): + return lambda config: _validate_github_with_key(config_key, config) + + +def _validate_github_with_key(config_key, config): + """ Validates the OAuth credentials and API endpoint for a Github service. """ + endpoint = config[config_key].get('GITHUB_ENDPOINT') + if not endpoint: + raise Exception('Missing Github Endpoint') + + if endpoint.find('http://') != 0 and endpoint.find('https://') != 0: + raise Exception('Github Endpoint must start with http:// or https://') + + if not config[config_key].get('CLIENT_ID'): + raise Exception('Missing Client ID') + + if not config[config_key].get('CLIENT_SECRET'): + raise Exception('Missing Client Secret') + client = app.config['HTTPCLIENT'] - oauth = GithubOAuthConfig(config, 'GITHUB_LOGIN_CONFIG') - endpoint = oauth.authorize_endpoint() - # TODO: this + oauth = GithubOAuthConfig(config, config_key) + result = oauth.validate_client_id_and_secret(client) + if not result: + raise Exception('Invalid client id or client secret') def _validate_ssl(config): @@ -116,7 +139,8 @@ _VALIDATORS = { 'redis': _validate_redis, 'registry-storage': _validate_registry_storage, 'mail': _validate_mailing, - 'github-login': _validate_github_login, + 'github-login': _validate_github('GITHUB_LOGIN_CONFIG'), + 'github-trigger': _validate_github('GITHUB_TRIGGER_CONFIG'), 'ssl': _validate_ssl, 'ldap': _validate_ldap, } \ No newline at end of file diff --git a/util/oauth.py b/util/oauth.py index 65af5cd13..2405423ea 100644 --- a/util/oauth.py +++ b/util/oauth.py @@ -17,15 +17,15 @@ class OAuthConfig(object): def login_endpoint(self): raise NotImplementedError + def validate_client_id_and_secret(self, http_client): + raise NotImplementedError + def client_id(self): return self.config.get('CLIENT_ID') def client_secret(self): return self.config.get('CLIENT_SECRET') - def basic_scope(self): - raise NotImplementedError - def _get_url(self, endpoint, *args): for arg in args: endpoint = urlparse.urljoin(endpoint, arg) @@ -46,9 +46,6 @@ class GithubOAuthConfig(OAuthConfig): endpoint = endpoint + '/' return endpoint - def basic_scope(self): - return 'user:email' - def authorize_endpoint(self): return self._get_url(self._endpoint(), '/login/oauth/authorize') + '?' @@ -69,6 +66,30 @@ class GithubOAuthConfig(OAuthConfig): api_endpoint = self._api_endpoint() return self._get_url(api_endpoint, 'user/emails') + def validate_client_id_and_secret(self, http_client): + # First: Verify that the github endpoint is actually Github by checking for the + # X-GitHub-Request-Id here. + api_endpoint = self._api_endpoint() + result = http_client.get(api_endpoint, auth=(self.client_id(), self.client_secret())) + if not 'X-GitHub-Request-Id' in result.headers: + raise Exception('Endpoint is not a Github (Enterprise) installation') + + # Next: Verify the client ID and secret. + # Note: The following code is a hack until such time as Github officially adds an API endpoint + # for verifying a {client_id, client_secret} pair. That being said, this hack was given to us + # *by a Github Engineer*, so I think it is okay for the time being :) + # + # TODO(jschorr): Replace with the real API call once added. + # + # Hitting the endpoint applications/{client_id}/tokens/foo will result in the following + # behavior IF the client_id is given as the HTTP username and the client_secret as the HTTP + # password: + # - If the {client_id, client_secret} pair is invalid in some way, we get a 401 error. + # - If the pair is valid, then we get a 404 because the 'foo' token does not exists. + validate_endpoint = self._get_url(api_endpoint, 'applications/%s/tokens/foo' % self.client_id()) + result = http_client.get(validate_endpoint, auth=(self.client_id(), self.client_secret())) + return result.status_code == 404 + def get_public_config(self): return { 'CLIENT_ID': self.client_id(), @@ -85,9 +106,6 @@ class GoogleOAuthConfig(OAuthConfig): def service_name(self): return 'Google' - def basic_scope(self): - return 'openid email' - def authorize_endpoint(self): return 'https://accounts.google.com/o/oauth2/auth?response_type=code&' @@ -97,6 +115,10 @@ class GoogleOAuthConfig(OAuthConfig): def user_endpoint(self): return 'https://www.googleapis.com/oauth2/v1/userinfo' + def validate_client_id_and_secret(self, http_client): + # No validation supported at this time. + return None + def get_public_config(self): return { 'CLIENT_ID': self.client_id(),