From 19ed9dd183e05bf1a138a879c7c22c2893aecf97 Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Thu, 1 Sep 2022 13:15:23 +0300 Subject: [PATCH] FIX: Keep private theme key secret from user (#18106) The generate RSA key and import theme routes worked separate from each other. The RSA key returned both the public and private key and it was the frontend which posted the private key back to the server. With this commit, only the public key is necessary as the server keeps a map of public and private keys that is used to get the private key back from a public key. --- .../controllers/modals/admin-install-theme.js | 12 ++++------ app/controllers/admin/themes_controller.rb | 14 +++++------ config/locales/server.en.yml | 1 + spec/requests/admin/themes_controller_spec.rb | 23 ++++++++++++++++++- 4 files changed, 34 insertions(+), 16 deletions(-) diff --git a/app/assets/javascripts/admin/addon/controllers/modals/admin-install-theme.js b/app/assets/javascripts/admin/addon/controllers/modals/admin-install-theme.js index a168583bd42..d42446ddbb1 100644 --- a/app/assets/javascripts/admin/addon/controllers/modals/admin-install-theme.js +++ b/app/assets/javascripts/admin/addon/controllers/modals/admin-install-theme.js @@ -93,10 +93,7 @@ export default Controller.extend(ModalFunctionality, { this._keyLoading = true; ajax(this.keyGenUrl, { type: "POST" }) .then((pair) => { - this.setProperties({ - privateKey: pair.private_key, - publicKey: pair.public_key, - }); + this.set("publicKey", pair.public_key); }) .catch(popupAjaxError) .finally(() => { @@ -139,7 +136,6 @@ export default Controller.extend(ModalFunctionality, { this.setProperties({ duplicateRemoteThemeWarning: null, privateChecked: false, - privateKey: null, localFile: null, uploadUrl: null, publicKey: null, @@ -216,7 +212,7 @@ export default Controller.extend(ModalFunctionality, { }; if (this.privateChecked) { - options.data.private_key = this.privateKey; + options.data.public_key = this.publicKey; } } @@ -239,10 +235,10 @@ export default Controller.extend(ModalFunctionality, { this.send("closeModal"); }) .then(() => { - this.setProperties({ privateKey: null, publicKey: null }); + this.set("publicKey", null); }) .catch((error) => { - if (!this.privateKey || this.themeCannotBeInstalled) { + if (!this.publicKey || this.themeCannotBeInstalled) { return popupAjaxError(error); } diff --git a/app/controllers/admin/themes_controller.rb b/app/controllers/admin/themes_controller.rb index 50749db4777..c22228ab6fe 100644 --- a/app/controllers/admin/themes_controller.rb +++ b/app/controllers/admin/themes_controller.rb @@ -35,11 +35,8 @@ class Admin::ThemesController < Admin::AdminController def generate_key_pair require 'sshkey' k = SSHKey.generate - - render json: { - private_key: k.private_key, - public_key: k.ssh_public_key - } + Discourse.redis.setex("ssh_key_#{k.ssh_public_key}", 1.hour, k.private_key) + render json: { public_key: k.ssh_public_key } end THEME_CONTENT_TYPES ||= %w{ @@ -101,14 +98,17 @@ class Admin::ThemesController < Admin::AdminController begin branch = params[:branch] ? params[:branch] : nil - @theme = RemoteTheme.import_theme(remote, theme_user, private_key: params[:private_key], branch: branch) + private_key = params[:public_key] ? Discourse.redis.get("ssh_key_#{params[:public_key]}") : nil + return render_json_error I18n.t("themes.import_error.ssh_key_gone") if params[:public_key].present? && private_key.blank? + + @theme = RemoteTheme.import_theme(remote, theme_user, private_key: private_key, branch: branch) render json: @theme, status: :created rescue RemoteTheme::ImportError => e if params[:force] theme_name = params[:remote].gsub(/.git$/, "").split("/").last remote_theme = RemoteTheme.new - remote_theme.private_key = params[:private_key] + remote_theme.private_key = private_key remote_theme.branch = params[:branch] ? params[:branch] : nil remote_theme.remote_url = params[:remote] remote_theme.save! diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 12d2eb6b9b5..8ccc2236a86 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -82,6 +82,7 @@ en: file_too_big: "The uncompressed file is too big." unknown_file_type: "The file you uploaded does not appear to be a valid Discourse theme." not_allowed_theme: "`%{repo}` is not in the list of allowed themes (check `allowed_theme_repos` global setting)." + ssh_key_gone: "You waited too long to install the theme and SSH key expired. Please try again." errors: component_no_user_selectable: "Theme components can't be user-selectable" component_no_default: "Theme components can't be default theme" diff --git a/spec/requests/admin/themes_controller_spec.rb b/spec/requests/admin/themes_controller_spec.rb index e42e67787a0..70e06609e4b 100644 --- a/spec/requests/admin/themes_controller_spec.rb +++ b/spec/requests/admin/themes_controller_spec.rb @@ -16,8 +16,9 @@ RSpec.describe Admin::ThemesController do post "/admin/themes/generate_key_pair.json" expect(response.status).to eq(200) json = response.parsed_body - expect(json["private_key"]).to include("RSA PRIVATE KEY") + expect(json["private_key"]).to eq(nil) expect(json["public_key"]).to include("ssh-rsa ") + expect(Discourse.redis.get("ssh_key_#{json["public_key"]}")).not_to eq(nil) end end @@ -139,6 +140,26 @@ RSpec.describe Admin::ThemesController do expect(response.status).to eq(201) end + it 'can lookup a private key by public key' do + Discourse.redis.setex('ssh_key_abcdef', 1.hour, 'rsa private key') + + ThemeStore::GitImporter.any_instance.stubs(:import!) + RemoteTheme.stubs(:extract_theme_info).returns( + 'name' => 'discourse-brand-header', + 'component' => true + ) + RemoteTheme.any_instance.stubs(:update_from_remote) + + post '/admin/themes/import.json', params: { + remote: ' https://github.com/discourse/discourse-brand-header.git ', + public_key: 'abcdef', + } + + expect(RemoteTheme.last.private_key).to eq('rsa private key') + + expect(response.status).to eq(201) + end + it 'imports a theme' do post "/admin/themes/import.json", params: { theme: theme_json_file } expect(response.status).to eq(201)