Merge branch 'feature/KASM-3329_ssl_cert_check' into 'master'

vncserver: don't require group memebership for cert readability check

Closes KASM-3329

See merge request kasm-technologies/internal/KasmVNC!70

spec/vncserver_spec.py

@@ -0,0 +1,59 @@
+import os
+import stat
+import tempfile
+from mamba import description, context, fcontext, it, fit, before, after
+from expects import expect, equal, contain, match
+
+from helper.spec_helper import start_xvnc, kill_xvnc, run_cmd, clean_env, \
+    add_kasmvnc_user_docker, clean_kasm_users, start_xvnc_pexpect, \
+    write_config, config_filename
+
+
+def run_vncserver():
+    return start_xvnc(f'-config {config_filename}')
+
+
+def temp_file_name():
+    return f'/tmp/vncserver.{next(tempfile._get_candidate_names())}'
+
+
+with description('vncserver') as self:
+    with before.each:
+        clean_env()
+    with after.each:
+        kill_xvnc()
+
+    with context("SSL certs"):
+        with before.each:
+            add_kasmvnc_user_docker()
+
+        with it("complains if SSL certs don't exist"):
+            non_existent_file_name = temp_file_name()
+
+            write_config(f'''
+            network:
+                ssl:
+                    pem_certificate: {non_existent_file_name}
+            ''')
+            completed_process = run_vncserver()
+            expect(completed_process.returncode).to(equal(1))
+            expect(completed_process.stderr).to(
+                match(r'certificate file doesn\'t exist'))
+
+        with it("complains if SSL cert not available"):
+            cert_file_name = temp_file_name()
+            with open(cert_file_name, 'w') as f:
+                f.write('test')
+            os.chmod(cert_file_name, stat.S_IXUSR)
+
+            write_config(f'''
+            network:
+                ssl:
+                    pem_certificate: {cert_file_name}
+            ''')
+            completed_process = run_vncserver()
+            expect(completed_process.returncode).to(equal(1))
+            expect(completed_process.stderr).to(
+                match(r'certificate isn\'t readable'))
+            expect(completed_process.stderr).to(
+                match(r'addgroup \$USER'))

unix/vncserver

@@ -534,77 +534,66 @@ sub CheckRequiredDependenciesArePresent
 sub CheckSslCertReadable {
   return if IsDryRun();
 
-  CheckUserHasAccessToSslCertOnDebian();
-  CheckUserHasAccessToSslCertOnCentOS();
+  RequireSslCertsToBeReadable();
 }
 
 sub IsDebian {
   return -f "/etc/debian_version";
 }
 
-sub CheckUserHasAccessToSslCertOnDebian {
-  if (!IsDebian()) {
-    return;
-  }
-
-  if (DoesCertKeyRequireSslCertGroup()) {
-    RequireUserToHaveSslCertGroup();
-  } else {
-    RequireSslCertsToBeReadable();
-  }
-}
-
 sub RequireSslCertsToBeReadable {
   my $certFilename = DerivedValue("network.ssl.pem_certificate");
   my $certKeyFilename = DerivedValue("network.ssl.pem_key");
 
-  my @unreadableCertFiles = map { -r $_ ? () : $_ }
-    uniq($certFilename, $certKeyFilename);
+  @certs = ($certFilename, $certKeyFilename);
+  @certs = grep defined, @certs;
+  @certs = uniq @certs;
+
+  my @unreadableCertFiles = map { -r $_ ? () : $_ } @certs;
   return if (scalar @unreadableCertFiles == 0);
 
-  $unreadableCertFiles = join "\n", @unreadableCertFiles;
-  $logger->warn(<<TEXT);
-Please ensure SSL certificate files are readable by you:
-$unreadableCertFiles
-TEXT
-    exit 1;
+  foreach my $unreadableCert (@unreadableCertFiles) {
+    GuideUserToMakeCertFileReadable($unreadableCert);
+  }
+  exit 1;
 }
 
-sub DoesCertKeyRequireSslCertGroup {
-  my $certKeyFilename = ConfigValue("network.ssl.pem_key");
-  $certKeyFilename =~ m!^/etc/ssl/private!;
-}
+sub FileGroupName {
+  my $file = shift;
+  my $grpId = (stat($file))[5];
 
-sub DoesCertKeyRequireKasmvncCertGroup {
-  my $certKeyFilename = ConfigValue("network.ssl.pem_key");
-  $certKeyFilename =~ m!^/etc/pki/tls/private!;
+  getgrgid($grpId);
 }
 
-sub RequireUserToHaveSslCertGroup {
-  my $certGroup = 'ssl-cert';
-  if (system("groups | grep -qw $certGroup") != 0) {
-    $logger->warn(<<EOF);
-  Can't access TLS certificate.
-  Please add your user to $certGroup via 'addgroup \$USER $certGroup'
-EOF
-    exit(1);
-  }
-}
+sub AddUserToGroupCmd {
+  my $certGroup = shift;
 
-sub IsRpmSystem {
-  system("command -v rpm >/dev/null 2>&1") == 0;
+  if (IsRpmSystem()) {
+    "usermod -a -G $certGroup \$USER"
+  } else {
+    "addgroup \$USER $certGroup"
+  }
 }
 
-sub CheckUserHasAccessToSslCertOnCentOS {
-  if (!IsRpmSystem()) {
+sub GuideUserToMakeCertFileReadable {
+  my $certFile = shift;
+  if (! -f $certFile) {
+    $logger->warn("$certFile: certificate file doesn't exist or isn't a file");
     return;
   }
 
-  if (DoesCertKeyRequireKasmvncCertGroup()) {
-    RequireUserToHaveKasmvncCertGroup();
-  } else {
-    RequireSslCertsToBeReadable();
-  }
+  my $certGroup = FileGroupName $certFile;
+  my $addUserToGroupCmd = AddUserToGroupCmd $certGroup;
+
+  $logger->warn(<<EOF);
+$certFile: certificate isn't readable.
+Make the certificate readable by adding your user to group "$certGroup":
+  '$addUserToGroupCmd'
+EOF
+}
+
+sub IsRpmSystem {
+  system("command -v rpm >/dev/null 2>&1") == 0;
 }
 
 sub RequireUserToHaveKasmvncCertGroup {
@@ -843,12 +832,12 @@ sub ConfigureDeToRun {
 }
 
 sub AskUserToChooseDeOrManualXstartup {
+  return if IsDryRun();
+
   if (PromptingDisabled()) {
     WarnIfShouldPromptForDe();
     return;
   }
-
-  return if IsDryRun();
   return unless shouldPromptUserToSelectDe();
 
   ForgetSelectedDe();