Merge pull request #1090 from yast/huha-open-ssh-02-sp4

Don't always Enable SSHD and Open SSH Port (V2.0) [SLE-15-SP4]

package/yast2-installation.changes

@@ -1,3 +1,9 @@
+-------------------------------------------------------------------
+Thu Jun 15 15:01:13 UTC 2023 - Stefan Hundhammer <shundhammer@suse.com>
+
+- Don't always enable sshd and open the ssh port (bsc#1211764)
+- 4.4.59
+
 -------------------------------------------------------------------
 Mon Nov  7 16:42:03 UTC 2022 - Ancor Gonzalez Sosa <ancor@suse.com>
 

package/yast2-installation.spec

@@ -16,7 +16,7 @@
 #
 
 Name:           yast2-installation
-Version:        4.4.58
+Version:        4.4.59
 Release:        0
 Summary:        YaST2 - Installation Parts
 License:        GPL-2.0-only

src/lib/installation/clients/security_proposal.rb

@@ -221,6 +221,10 @@ def firewall_proposal
       # Returns the SSH service part of the firewall proposal description
       # @return [String] proposal html text
       def sshd_proposal
+        # Check if only public key auth is configured, and if yes,
+        # enable SSHD and open the SSH port; but only now, after we are sure
+        # that the user was prompted for the root password (bsc#1211764).
+        @settings.propose
         if @settings.enable_sshd
           _(
             "SSH service will be enabled (<a href=\"%s\">disable</a>)"

src/lib/installation/security_settings.rb

@@ -78,22 +78,49 @@ def propose_lsm_config
       Yast::PackagesProposal.SetResolvables("LSM", :pattern, lsm_config.needed_patterns)
     end
 
+    # Make a one-time proposal for the security settings:
+    #
+    # If only public key authentication is configured, and no root password is set,
+    # open the SSH port and enable SSHD so at least SSH access can be used.
+    #
+    # This should be called AFTER the user was prompted for the root password, e.g.
+    # when the security proposal is made during installation.
+    #
+    # This is done only once. Use 'reset_proposal' to do do it again.
+    def propose
+      return if @proposal_done
+
+      @proposal_done = true
+      log.info("Making security settings proposal")
+      return unless only_public_key_auth?
+
+      log.info("Only public key auth")
+      open_ssh! unless @open_ssh
+      enable_sshd! unless @enable_sshd
+    end
+
+    # Reset the proposal; i.e. the next call to 'propose' will do a fresh
+    # proposal.
+    def reset_proposal
+      @proposal_done = false
+    end
+
     # Services
 
     # Add the firewall package to be installed and sets the firewalld service
     # to be enabled
     def enable_firewall!
       Yast::PackagesProposal.AddResolvables("firewall", :package, ["firewalld"])
 
-      log.info "Enabling Firewall"
+      log.info "Enabling firewall"
       self.enable_firewall = true
     end
 
     # Remove the firewalld package from being installed and sets the firewalld
     # service to be disabled
     def disable_firewall!
       Yast::PackagesProposal.RemoveResolvables("firewall", :package, ["firewalld"])
-      log.info "Disabling Firewall"
+      log.info "Disabling firewall"
       self.enable_firewall = false
     end
 
@@ -121,19 +148,19 @@ def open_ssh!
 
     # Set the ssh port to be closed
     def close_ssh!
-      log.info "Opening SSH port"
+      log.info "Closing SSH port"
       self.open_ssh = false
     end
 
     # Set the vnc port to be opened
     def open_vnc!
-      log.info "Close VNC port"
+      log.info "Opening VNC port"
       self.open_vnc = true
     end
 
     # Set the vnc port to be closed
     def close_vnc!
-      log.info "Close VNC port"
+      log.info "Closing VNC port"
       self.open_vnc = false
     end
 
@@ -144,7 +171,7 @@ def close_vnc!
     #   authentication and the system is not accesible through ssh
     def access_problem?
       # public key is not the only way
-      return false unless only_public_key_auth
+      return false unless only_public_key_auth?
 
       # without running sshd it is useless
       return true unless @enable_sshd
@@ -181,27 +208,32 @@ def global_section
     end
 
     def wanted_enable_sshd?
-      Yast::Linuxrc.usessh || only_public_key_auth || @enable_sshd
+      Yast::Linuxrc.usessh || @enable_sshd
     end
 
     def wanted_open_ssh?
-      Yast::Linuxrc.usessh || only_public_key_auth || @open_ssh
+      Yast::Linuxrc.usessh || @open_ssh
     end
 
     def wanted_open_vnc?
       Yast::Linuxrc.vnc
     end
 
-    # Determines whether only public key authentication is supported
+    # Determines whether only public key authentication is supported.
+    #
+    # Do not call this prematurely before the user was even prompted for a root password;
+    # in particular, do not call this from the constructor of this class.
     #
     # @note If the root user does not have a password, we assume that we will use a public
     #   key in order to log into the system. In such a case, we need to enable the SSH
     #   service (including opening the port).
-    def only_public_key_auth
-      return true unless root_user
+    def only_public_key_auth?
+      if root_user.nil?
+        log.warn("No root user created yet; can't check root password!")
+        return false
+      end
 
       password = root_user.password_content || ""
-
       password.empty?
     end
 

test/lib/clients/security_proposal_test.rb

@@ -26,6 +26,20 @@
   subject(:client) { described_class.new }
   let(:proposal_settings) { Installation::SecuritySettings.create_instance }
 
+  def create_target_config
+    root = Y2Users::User.create_root
+    config = Y2Users::Config.new.attach(root)
+
+    Y2Users::ConfigManager.instance.target = config
+  end
+
+  before do
+    create_target_config
+    Y2Users::ConfigManager.instance.target.users.root.password = root_password
+  end
+
+  let(:root_password) { Y2Users::Password.create_plain("s3cr3t") }
+
   describe "#initialize" do
     it "instantiates a new proposal settings" do
       expect(Installation::SecuritySettings).to receive(:instance)
@@ -154,7 +168,7 @@
       let(:ssh_open) { true }
 
       before do
-        allow(proposal_settings).to receive(:only_public_key_auth).and_return(true)
+        allow(proposal_settings).to receive(:only_public_key_auth?).and_return(true)
         proposal_settings.enable_sshd = ssh_enabled
         proposal_settings.open_ssh = ssh_open
       end
@@ -166,10 +180,17 @@
             expect(proposal["warning"]).to be_nil
           end
         end
-        context "and the SSH port is close" do
+        context "and the SSH port is closed" do
           let(:ssh_open) { false }
 
-          it "returns the proposal warning about the situation" do
+          it "returns no warning for the the original proposal" do
+            proposal = client.make_proposal({})
+            expect(proposal["warning"]).to be_nil
+          end
+
+          it "returns a warning after the user changed settings manually" do
+            client.make_proposal({})
+            proposal_settings.close_ssh!
             proposal = client.make_proposal({})
             expect(proposal["warning"]).to include("might not be allowed")
           end
@@ -179,7 +200,14 @@
       context "and the SSH is disabled" do
         let(:ssh_enabled) { false }
 
+        it "returns no warning for the the original proposal" do
+          proposal = client.make_proposal({})
+          expect(proposal["warning"]).to be_nil
+        end
+
         it "returns the proposal warning about the situation" do
+          client.make_proposal({})
+          proposal_settings.disable_sshd!
           proposal = client.make_proposal({})
           expect(proposal["warning"]).to include("might not be allowed")
         end

test/lib/security_settings_test.rb

@@ -68,6 +68,12 @@ def create_target_config
       described_class.create_instance
     end
 
+    it "does not yet check for public key auth only" do
+      expect_any_instance_of(described_class).not_to receive(:only_public_key_auth?)
+
+      described_class.create_instance
+    end
+
     context "when firewall has been enabled in the control file" do
       let(:global_section) { { "enable_firewall" => true, "enable_sshd" => false } }
 
@@ -103,19 +109,45 @@ def create_target_config
         described_class.create_instance
       end
     end
+  end
 
+  describe "#propose" do
     context "when no root password was set" do
       let(:root_password) { Y2Users::Password.create_plain("") }
 
       before do
         allow(Yast::Linuxrc).to receive(:usessh).and_return(false)
       end
 
-      it "opens SSH to allow public key authentication" do
+      it "without propose does not change the SSH settings" do
+        expect_any_instance_of(described_class).not_to receive(:enable_sshd!)
+        expect_any_instance_of(described_class).not_to receive(:open_ssh!)
+
+        described_class.create_instance
+      end
+
+      it "with propose opens SSH to allow public key authentication" do
         expect_any_instance_of(described_class).to receive(:enable_sshd!)
         expect_any_instance_of(described_class).to receive(:open_ssh!)
 
-        described_class.create_instance
+        instance = described_class.create_instance
+        instance.propose
+      end
+    end
+
+    context "when a root password was set" do
+      let(:root_password) { Y2Users::Password.create_plain("s3cr3t") }
+
+      before do
+        allow(Yast::Linuxrc).to receive(:usessh).and_return(false)
+      end
+
+      it "does not change the SSH settings" do
+        expect_any_instance_of(described_class).not_to receive(:enable_sshd!)
+        expect_any_instance_of(described_class).not_to receive(:open_ssh!)
+
+        instance = described_class.create_instance
+        instance.propose
       end
     end
   end
@@ -290,19 +322,19 @@ def create_target_config
       subject.enable_sshd = ssh_enabled
       subject.enable_firewall = firewall_enabled
       subject.open_ssh = ssh_open
-      allow(subject).to receive(:only_public_key_auth).and_return(only_ssh_key_auth)
+      allow(subject).to receive(:only_public_key_auth?).and_return(only_ssh_key_auth)
     end
 
     context "when the root user uses only SSH key based authentication" do
       context "when sshd is enabled" do
-        context "and firewall is enabled" do
+        context "and the firewall is enabled" do
           context "and the SSH port is open" do
             it "returns false" do
               expect(subject.access_problem?).to eql(false)
             end
           end
 
-          context "and the SSH port is close" do
+          context "and the SSH port is closed" do
             let(:ssh_open) { false }
 
             it "returns true" do
@@ -311,7 +343,7 @@ def create_target_config
           end
         end
 
-        context "and firewall is disabled" do
+        context "and the firewall is disabled" do
           let(:firewall_enabled) { false }
 
           it "returns false" do