diff --git a/CHANGELOG.md b/CHANGELOG.md index df1b7395ff..c5fff9cdd9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,9 +11,14 @@ This file is used to list changes made in each version of the AWS ParallelCluste - Further reduce transient build-image failures on RHEL and Rocky caused by out-of-sync repo mirrors by resetting metadata upon retry. - Improve cluster update resiliency on login nodes by reusing the head-node-driven orchestration already in place on compute nodes, removing the dependency on cfn-hup and cfn-init. +- Support usernames longer than 8 characters and usernames with `.` in DCV authenticator. **CHANGES** +- Enforce NFSv4-only on the ParallelCluster-managed NFS server (head node). NFSv3 can be + re-enabled on the server by overriding the `nfs/v3` attribute to `'yes'`. The NFSv3 client + stack (rpcbind, rpc-statd, lockd) and its statically pinned ports are unchanged, so cluster + nodes can still mount external NFSv3 servers through a restricted firewall. - In GPU Health Check, skip DCGM diagnostics when NVIDIA MIG is enabled because dcgmi diag does not support MIG. **CHANGES** diff --git a/cookbooks/aws-parallelcluster-environment/attributes/environment.rb b/cookbooks/aws-parallelcluster-environment/attributes/environment.rb index ea0e82bf3e..bb217879f3 100644 --- a/cookbooks/aws-parallelcluster-environment/attributes/environment.rb +++ b/cookbooks/aws-parallelcluster-environment/attributes/environment.rb @@ -5,6 +5,11 @@ # For performance, set NFS threads to min(256, max(8, num_cores * 4)) default['cluster']['nfs']['threads'] = [[node['cpu']['cores'].to_i * 4, 8].max, 256].min +# Enforce NFSv4-only on the ParallelCluster-managed NFS server. +default['nfs']['v4'] = 'yes' +default['nfs']['v3'] = 'no' +default['nfs']['v2'] = 'no' + # Kernel release version used to select Lustre version # This is a mechanism used to mock kernel release on docker system-tests, see kitchen.docker.yml: # when kernel_release is defined, it will be used, otherwise the release version will be taken from ohai. diff --git a/cookbooks/aws-parallelcluster-environment/resources/nfs/nfs_alinux2023.rb b/cookbooks/aws-parallelcluster-environment/resources/nfs/nfs_alinux2023.rb index 163c331251..f4d9a65673 100644 --- a/cookbooks/aws-parallelcluster-environment/resources/nfs/nfs_alinux2023.rb +++ b/cookbooks/aws-parallelcluster-environment/resources/nfs/nfs_alinux2023.rb @@ -27,12 +27,3 @@ action_install_nfs4 action_disable_start_at_boot end - -action_class do - def override_server_template - edit_resource(:template, node['nfs']['config']['server_template']) do - source 'nfs/default-nfs-kernel-server.conf.erb' - cookbook 'aws-parallelcluster-environment' - end - end -end diff --git a/cookbooks/aws-parallelcluster-environment/resources/nfs/nfs_redhat8.rb b/cookbooks/aws-parallelcluster-environment/resources/nfs/nfs_redhat8.rb index 34f7addc37..51d6851b80 100644 --- a/cookbooks/aws-parallelcluster-environment/resources/nfs/nfs_redhat8.rb +++ b/cookbooks/aws-parallelcluster-environment/resources/nfs/nfs_redhat8.rb @@ -27,11 +27,3 @@ action_install_nfs4 action_disable_start_at_boot end - -action_class do - def override_server_template - edit_resource(:template, node['nfs']['config']['server_template']) do - cookbook 'nfs' - end - end -end diff --git a/cookbooks/aws-parallelcluster-environment/resources/nfs/nfs_rocky8.rb b/cookbooks/aws-parallelcluster-environment/resources/nfs/nfs_rocky8.rb index 4019715e88..90b3d2228c 100644 --- a/cookbooks/aws-parallelcluster-environment/resources/nfs/nfs_rocky8.rb +++ b/cookbooks/aws-parallelcluster-environment/resources/nfs/nfs_rocky8.rb @@ -27,11 +27,3 @@ action_install_nfs4 action_disable_start_at_boot end - -action_class do - def override_server_template - edit_resource(:template, node['nfs']['config']['server_template']) do - cookbook 'nfs' - end - end -end diff --git a/cookbooks/aws-parallelcluster-environment/resources/nfs/nfs_ubuntu22+.rb b/cookbooks/aws-parallelcluster-environment/resources/nfs/nfs_ubuntu22+.rb index 792b0fec18..24bf94ba93 100644 --- a/cookbooks/aws-parallelcluster-environment/resources/nfs/nfs_ubuntu22+.rb +++ b/cookbooks/aws-parallelcluster-environment/resources/nfs/nfs_ubuntu22+.rb @@ -30,13 +30,3 @@ action_install_nfs4 action_disable_start_at_boot end - -action_class do - def override_server_template - node.default['nfs']['config']['server_template'] = '/etc/nfs.conf.d/parallelcluster-nfs.conf' - edit_resource(:template, node['nfs']['config']['server_template']) do - source 'nfs/nfs-ubuntu22+.conf.erb' - cookbook 'aws-parallelcluster-environment' - end - end -end diff --git a/cookbooks/aws-parallelcluster-environment/resources/nfs/partial/_configure.rb b/cookbooks/aws-parallelcluster-environment/resources/nfs/partial/_configure.rb index 785604be1f..0e81a5f97f 100644 --- a/cookbooks/aws-parallelcluster-environment/resources/nfs/partial/_configure.rb +++ b/cookbooks/aws-parallelcluster-environment/resources/nfs/partial/_configure.rb @@ -16,13 +16,10 @@ action :configure do if node['cluster']['node_type'] == "HeadNode" node.force_override['nfs']['threads'] = node['cluster']['nfs']['threads'] + render_server_config - override_server_template - - # Explicitly restart NFS server for thread setting to take effect - # and enable it to start at boot service node['nfs']['service']['server'] do - action %i(restart enable) + action %i(enable start) supports restart: true retries 5 retry_delay 10 @@ -33,3 +30,35 @@ end unless on_docker? end end + +action_class do + def render_server_config + server_service = node['nfs']['service']['server'] + + if conf_d_supported? + directory '/etc/nfs.conf.d' do + mode '0755' + end + + template '/etc/nfs.conf.d/parallelcluster-nfs.conf' do + source 'nfs/parallelcluster-nfs.conf.erb' + cookbook 'aws-parallelcluster-environment' + mode '0644' + notifies :restart, "service[#{server_service}]", :delayed + end + else + template '/etc/nfs.conf' do + source 'nfs/nfs.conf.erb' + cookbook 'aws-parallelcluster-environment' + mode '0644' + notifies :restart, "service[#{server_service}]", :delayed + end + end + end + + # /etc/nfs.conf.d/*.conf auto-include requires nfs-utils >= ~2.4.1. The only supported platform + # older than that is RHEL/Rocky 8 (nfs-utils 2.3.3), where we render /etc/nfs.conf directly. + def conf_d_supported? + !(platform_family?('rhel') && node['platform_version'].to_i == 8) + end +end diff --git a/cookbooks/aws-parallelcluster-environment/spec/unit/resources/nfs_spec.rb b/cookbooks/aws-parallelcluster-environment/spec/unit/resources/nfs_spec.rb index 816c487793..302c0cc47a 100644 --- a/cookbooks/aws-parallelcluster-environment/spec/unit/resources/nfs_spec.rb +++ b/cookbooks/aws-parallelcluster-environment/spec/unit/resources/nfs_spec.rb @@ -21,10 +21,10 @@ def self.configure(chef_run) describe 'nfs:setup' do for_all_oses do |platform, version| context "on #{platform}#{version}" do - cached(:disabled_service) { 'disable_service' } + cached(:server_service) { 'nfs_server_service' } cached(:chef_run) do runner = runner(platform: platform, version: version, step_into: ['nfs']) do |node| - node.override['nfs']['service']['server'] = disabled_service + node.override['nfs']['service']['server'] = server_service end ConvergeNfs.setup(runner) end @@ -33,25 +33,20 @@ def self.configure(chef_run) is_expected.to setup_nfs('setup') end - if %w(amazon centos redhat).include?(platform) - it 'installs nfs::server4' do - expect(chef_run).to include_recipe('nfs::server4') - chef_run - end + it 'installs the full NFS stack (client + server) via nfs::server4' do + expect(chef_run).to include_recipe('nfs::server4') + chef_run + end - elsif platform == 'ubuntu' - it 'installs nfs::server and nfs:server4' do + if %w(ubuntu debian).include?(platform) + it 'also includes nfs::server on Debian (sous-chefs/nfs#93 workaround)' do expect(chef_run).to include_recipe('nfs::server') - expect(chef_run).to include_recipe('nfs::server4') chef_run end - - else - pending "to be implemented" end - it 'disables service at boot' do - is_expected.to disable_service(disabled_service) + it 'does not start the nfs server at boot' do + is_expected.to disable_service(server_service) end end end @@ -59,16 +54,17 @@ def self.configure(chef_run) describe 'nfs:configure' do for_all_oses do |platform, version| + cached(:server_service) { 'nfs_server_service' } + cached(:nfs_conf) { '/etc/nfs.conf' } + cached(:nfs_conf_dropin) { '/etc/nfs.conf.d/parallelcluster-nfs.conf' } + context "on #{platform}#{version} on node type HeadNode" do cached(:threads) { 10 } - cached(:server_template) { 'server_template' } - cached(:nfs_service) { 'nfs_service' } cached(:chef_run) do runner = runner(platform: platform, version: version, step_into: ['nfs']) do |node| + node.override['nfs']['service']['server'] = server_service node.override['cluster']['nfs']['threads'] = threads - node.override['nfs']['config']['server_template'] = server_template - node.override['nfs']['service']['server'] = nfs_service - node.override['cluster']['node_type'] = "HeadNode" + node.override['cluster']['node_type'] = 'HeadNode' end ConvergeNfs.configure(runner) end @@ -77,50 +73,74 @@ def self.configure(chef_run) is_expected.to configure_nfs('configure') end - if %w(amazon centos).include?(platform) - it 'overrides nfs config with custom template' do - is_expected.to create_template(server_template) - .with(source: 'nfs/default-nfs-kernel-server.conf.erb') + if %w(redhat rocky centos).include?(platform) && version.to_i == 8 + it 'renders /etc/nfs.conf with the NFSv4-only template (no conf.d on el8)' do + is_expected.to create_template(nfs_conf) + .with(source: 'nfs/nfs.conf.erb') .with(cookbook: 'aws-parallelcluster-environment') + is_expected.to_not create_template(nfs_conf_dropin) end - elsif %w(ubuntu).include?(platform) - it 'overrides nfs config with custom template' do - if version.to_i >= 22 - is_expected.to create_template(server_template) - .with(source: 'nfs/nfs-ubuntu22+.conf.erb') - .with(cookbook: 'aws-parallelcluster-environment') - else - is_expected.to create_template(server_template) - .with(source: 'nfs/default-nfs-kernel-server.conf.erb') - .with(cookbook: 'aws-parallelcluster-environment') - end + it 'disables NFSv3 and enables NFSv4 in /etc/nfs.conf' do + expect(chef_run).to render_file(nfs_conf).with_content(/vers3=no/) + expect(chef_run).to render_file(nfs_conf).with_content(/vers4=yes/) end - elsif %(redhat rocky).include?(platform) - it 'uses nfs config template shipped with nfs cookbook' do - is_expected.to create_template(server_template) - .with(source: "#{server_template}.erb") - .with(cookbook: 'nfs') + it 'restart of the server is notified when /etc/nfs.conf changes' do + expect(chef_run.template(nfs_conf)).to notify("service[#{server_service}]").to(:restart).delayed end - else - pending "to be implemented" + it 'ships an NFSv4-only drop-in and leaves /etc/nfs.conf untouched' do + is_expected.to create_template(nfs_conf_dropin) + .with(source: 'nfs/parallelcluster-nfs.conf.erb') + .with(cookbook: 'aws-parallelcluster-environment') + is_expected.to_not create_template(nfs_conf) + end + + it 'disables NFSv3 and enables NFSv4 in the drop-in' do + expect(chef_run).to render_file(nfs_conf_dropin).with_content(/vers3=no/) + expect(chef_run).to render_file(nfs_conf_dropin).with_content(/vers4=yes/) + end + + it 'pins the ancillary v3 client ports in the drop-in (unpinned by nfs::server4 on AL2023)' do + expect(chef_run).to render_file(nfs_conf_dropin).with_content(/\[statd\]\nport=32765\noutgoing-port=32766/) + expect(chef_run).to render_file(nfs_conf_dropin).with_content(/\[mountd\]\nport=32767/) + expect(chef_run).to render_file(nfs_conf_dropin).with_content(/\[lockd\]\nport=32768\nudp-port=32768/) + end + + it 'restart of the server is notified when the drop-in changes' do + expect(chef_run.template(nfs_conf_dropin)).to notify("service[#{server_service}]").to(:restart).delayed + end end - it 'enables and restarts service' do - is_expected.to restart_service(nfs_service) - .with(action: %i(restart enable)) - .with(supports: { restart: true }) + it 'enables and starts the server' do + is_expected.to enable_service(server_service) + is_expected.to start_service(server_service) + end + + context 'when v3 is re-enabled via node attribute' do + cached(:chef_run) do + runner = runner(platform: platform, version: version, step_into: ['nfs']) do |node| + node.override['nfs']['service']['server'] = server_service + node.override['cluster']['nfs']['threads'] = threads + node.override['nfs']['v3'] = 'yes' + node.override['cluster']['node_type'] = 'HeadNode' + end + ConvergeNfs.configure(runner) + end + + it 'advertises vers3 in the rendered config' do + target = (%w(redhat rocky centos).include?(platform) && version.to_i == 8) ? nfs_conf : nfs_conf_dropin + expect(chef_run).to render_file(target).with_content(/vers3=yes/) + end end end context "on #{platform}#{version} on node type ComputeFleet" do - cached(:server_template) { 'server_template' } - cached(:nfs_service) { 'nfs_service' } cached(:chef_run) do runner = runner(platform: platform, version: version, step_into: ['nfs']) do |node| - node.override['cluster']['node_type'] = "ComputeFleet" + node.override['nfs']['service']['server'] = server_service + node.override['cluster']['node_type'] = 'ComputeFleet' end ConvergeNfs.configure(runner) end @@ -129,12 +149,14 @@ def self.configure(chef_run) is_expected.to configure_nfs('configure') end - it 'not overrides nfs config with custom template' do - is_expected.to_not create_template(server_template) + it 'does not manage the NFS server config (client-only node)' do + is_expected.to_not create_template(nfs_conf) + is_expected.to_not create_template(nfs_conf_dropin) end - it 'not enables and restarts service' do - is_expected.to_not restart_service(nfs_service) + it 'stops and disables the server (client-only node)' do + is_expected.to stop_service(server_service) + is_expected.to disable_service(server_service) end end end diff --git a/cookbooks/aws-parallelcluster-environment/templates/nfs/default-nfs-kernel-server.conf.erb b/cookbooks/aws-parallelcluster-environment/templates/nfs/default-nfs-kernel-server.conf.erb deleted file mode 100644 index 12eb6ff28b..0000000000 --- a/cookbooks/aws-parallelcluster-environment/templates/nfs/default-nfs-kernel-server.conf.erb +++ /dev/null @@ -1,21 +0,0 @@ -# Generated by Chef for <%= node['fqdn'] %> -# Local modifications will be overwritten. - -STATD_PORT="<%= node['nfs']['port']['statd'] -%>" -STATD_OUTGOING_PORT="<%= node['nfs']['port']['statd_out'] -%>" -STATDARG="-p <%= node['nfs']['port']['statd'] -%> -o <%= node['nfs']['port']['statd_out'] -%>" -MOUNTD_PORT="<%= node['nfs']['port']['mountd'] -%>" -RPCMOUNTDOPTS="-p <%= node['nfs']['port']['mountd'] -%>" -LOCKD_UDPPORT="<%= node['nfs']['port']['lockd'] -%>" -LOCKD_TCPPORT="<%= node['nfs']['port']['lockd'] -%>" -RQUOTAD_PORT="<%= node['nfs']['port']['rquotad'] -%>" -<% unless node['nfs']['v2'].nil? -%> -MOUNTD_NFS_V2="<%= node['nfs']['v2'] -%>" -<% end -%> -<% unless node['nfs']['v3'].nil? -%> -MOUNTD_NFS_V3="<%= node['nfs']['v3'] -%>" -<% end -%> -RQUOTAD="<%= node['nfs']['rquotad'] -%>" -<% unless node['nfs']['threads'] == 0 -%> -RPCNFSDCOUNT="<%= node['nfs']['threads'] -%>" -<% end -%> \ No newline at end of file diff --git a/cookbooks/aws-parallelcluster-environment/templates/nfs/nfs-ubuntu22+.conf.erb b/cookbooks/aws-parallelcluster-environment/templates/nfs/nfs-ubuntu22+.conf.erb deleted file mode 100644 index 71472a6d2a..0000000000 --- a/cookbooks/aws-parallelcluster-environment/templates/nfs/nfs-ubuntu22+.conf.erb +++ /dev/null @@ -1,25 +0,0 @@ -# Generated by Chef for <%= node['fqdn'] %> -# Local modifications will be overwritten. - -[lockd] -port = <%= node['nfs']['port']['lockd'] %> -udp-port = <%= node['nfs']['port']['lockd'] %> - -[mountd] -port = <%= node['nfs']['port']['mountd'] %> - -[statd] -port = <%= node['nfs']['port']['statd'] %> -outgoing-port = <%= node['nfs']['port']['statd_out'] %> - -[nfsd] -threads = <%= node['nfs']['threads'] %> -<% unless node['nfs']['v2'].nil? -%> -vers2 = <%= node['nfs']['v2'] %> -<% end -%> -<% unless node['nfs']['v3'].nil? -%> -vers3 = <%= node['nfs']['v3'] %> -<% end -%> -<% unless node['nfs']['v4'].nil? -%> -vers4 = <%= node['nfs']['v4'] %> -<% end -%> diff --git a/cookbooks/aws-parallelcluster-environment/templates/nfs/nfs.conf.erb b/cookbooks/aws-parallelcluster-environment/templates/nfs/nfs.conf.erb new file mode 100644 index 0000000000..0f98722a51 --- /dev/null +++ b/cookbooks/aws-parallelcluster-environment/templates/nfs/nfs.conf.erb @@ -0,0 +1,33 @@ +# Generated by Chef for <%= node['fqdn'] %> +# Local modifications will be overwritten. +# +# Used ONLY on RHEL/Rocky 8, whose nfs-utils (2.3.3) does not auto-read /etc/nfs.conf.d, so we own +# /etc/nfs.conf directly. This mirrors the upstream nfs cookbook's nfs.conf.erb (so node['nfs'] +# attributes keep working the same way — e.g. set a port to 0 for dynamic allocation) and only +# adds NFSv4-only enforcement (https://github.com/aws/aws-parallelcluster/issues/6622) via the +# node['nfs']['v3'/'v4'] attributes. +[general] +pipefs-directory=<%= node['nfs']['idmap']['pipefs_directory'] %> + +[gssd] +use-gss-proxy=1 + +[lockd] +port=<%= node['nfs']['port']['lockd'] %> +udp-port=<%= node['nfs']['port']['lockd'] %> + +[mountd] +port=<%= node['nfs']['port']['mountd'] %> + +[nfsd] +threads=<%= node['nfs']['threads'] %> +<% unless node['nfs']['v3'].nil? -%> +vers3=<%= node['nfs']['v3'] %> +<% end -%> +<% unless node['nfs']['v4'].nil? -%> +vers4=<%= node['nfs']['v4'] %> +<% end -%> + +[statd] +port=<%= node['nfs']['port']['statd'] %> +outgoing-port=<%= node['nfs']['port']['statd_out'] %> diff --git a/cookbooks/aws-parallelcluster-environment/templates/nfs/parallelcluster-nfs.conf.erb b/cookbooks/aws-parallelcluster-environment/templates/nfs/parallelcluster-nfs.conf.erb new file mode 100644 index 0000000000..d01f1173f8 --- /dev/null +++ b/cookbooks/aws-parallelcluster-environment/templates/nfs/parallelcluster-nfs.conf.erb @@ -0,0 +1,31 @@ +# Generated by Chef for <%= node['fqdn'] %> +# Local modifications will be overwritten. +# +# ParallelCluster NFS drop-in. +# Read after /etc/nfs.conf, so it overrides only the keys below and leaves the rest of the +# distro's /etc/nfs.conf (e.g. [general] pipefs-directory, [gssd]) untouched. +# +# We render the pinned ancillary ports here rather than relying on nfs::server4 to write them to +# /etc/nfs.conf, because the upstream nfs.erb has no 'amazon' platform_family branch and so leaves +# the ports unpinned on Amazon Linux 2023. Pinning them here keeps the NFSv3 client able to mount +# external v3 servers through a restricted firewall on every conf.d OS. All values come from the +# node['nfs'][...] attributes, so the upstream knobs still work (e.g. set a port to 0 for dynamic). +[lockd] +port=<%= node['nfs']['port']['lockd'] %> +udp-port=<%= node['nfs']['port']['lockd'] %> + +[mountd] +port=<%= node['nfs']['port']['mountd'] %> + +[nfsd] +threads=<%= node['nfs']['threads'] %> +<% unless node['nfs']['v3'].nil? -%> +vers3=<%= node['nfs']['v3'] %> +<% end -%> +<% unless node['nfs']['v4'].nil? -%> +vers4=<%= node['nfs']['v4'] %> +<% end -%> + +[statd] +port=<%= node['nfs']['port']['statd'] %> +outgoing-port=<%= node['nfs']['port']['statd_out'] %> diff --git a/cookbooks/aws-parallelcluster-platform/files/dcv/pcluster_dcv_authenticator.py b/cookbooks/aws-parallelcluster-platform/files/dcv/pcluster_dcv_authenticator.py index c67494dab5..108a8c7474 100644 --- a/cookbooks/aws-parallelcluster-platform/files/dcv/pcluster_dcv_authenticator.py +++ b/cookbooks/aws-parallelcluster-platform/files/dcv/pcluster_dcv_authenticator.py @@ -29,7 +29,7 @@ from datetime import datetime, timedelta from http.server import BaseHTTPRequestHandler, HTTPServer from logging.handlers import RotatingFileHandler -from pwd import getpwuid +from pwd import getpwnam, getpwuid from socketserver import ThreadingMixIn from urllib.parse import parse_qsl, urlparse @@ -133,7 +133,7 @@ class IncorrectRequestError(Exception): pass - USER_REGEX = r"^[a-z_]([a-z0-9_-]{0,31}|[a-z0-9_-]{0,30}\$)$" + USER_REGEX = r"^[a-z_]([a-z0-9_.-]{0,31}|[a-z0-9_.-]{0,30}\$)$" SESSION_ID_REGEX = r"^([a-zA-Z0-9_-]{0,128})$" # A nosec comment is appended to the following line in order to disable the B105 check. # Since the TOKEN_REGEX is not a hardcoded password @@ -356,18 +356,24 @@ def _is_session_valid(user, session_id): """ Verify if the DCV session exists and the ownership. - # We are using ps aux to retrieve the list of sessions - # because currently DCV doesn't allow list-session to list all session even for non-root user. - # TODO change this method if DCV updates his behaviour. + We use `ps -eo uid,args` rather than `ps aux` because the latter truncates the user + column to 8 characters, which breaks ownership checks for usernames longer than 8 + characters. The username is resolved to its numeric UID and matched against the UID + column instead. """ logger.info("Verifying Amazon DCV session validity..") - # Remove the first and the last because they are the heading and empty, respectively - # All commands and arguments in this subprocess call are built as literals - processes = subprocess.check_output(["/bin/ps", "aux"]).decode("utf-8").split("\n")[1:-1] # nosec B603 - # Check the filter is empty + try: + uid = getpwnam(user).pw_uid + except KeyError: + raise DCVAuthenticator.IncorrectRequestError("The given user does not exist on this system") + + # The first and last entries are the heading and the trailing empty line, respectively. + # All commands and arguments in this subprocess call are built as literals. + processes = subprocess.check_output(["/bin/ps", "-eo", "uid,args"]).decode("utf-8").split("\n")[1:-1] # nosec B603 + if not next( - filter(lambda process: DCVAuthenticator.check_dcv_process(process, user, session_id), processes), None + filter(lambda process: DCVAuthenticator.check_dcv_process(process, uid, session_id), processes), None ): raise DCVAuthenticator.IncorrectRequestError("The given session does not exists") logger.info("The Amazon DCV session is valid.") @@ -377,23 +383,19 @@ def _verify_session_existence(user, session_id): retry(DCVAuthenticator._is_session_valid, func_args=[user, session_id], attempts=20, wait=1) @staticmethod - def check_dcv_process(row, user, session_id): - """Check if there is a dcvagent process running for the given user and for the given session_id.""" - # row example: - # centos 63 0.0 0.0 4348844 3108 ?? Ss 23Jul19 2:32.46 /usr/libexec/dcv/dcvagent --mode full \ - # --session-id mysession - # ubuntu 2949 0.3 0.4 860568 34328 ? Sl 20:10 0:18 /usr/lib/x86_64-linux-gnu/dcv/dcvagent --mode full \ - # --session-id mysession + def check_dcv_process(row, uid, session_id): + """Check if there is a dcvagent process running for the given UID and for the given session_id.""" + # row example (from `ps -eo uid,args`): + # 1000 /usr/libexec/dcv/dcvagent --mode full --session-id mysession fields = row.split() - command_index = 10 - session_name_index = 14 - user_index = 0 - - return ( - fields[command_index].endswith("/dcv/dcvagent") - and fields[user_index] == user - and fields[session_name_index] == session_id - ) + try: + return ( + fields[1].endswith("/dcv/dcvagent") + and fields[0] == str(uid) + and fields[fields.index("--session-id") + 1] == session_id + ) + except (ValueError, IndexError): + return False class ThreadedHTTPServer(ThreadingMixIn, HTTPServer): diff --git a/test/unit/dcv/test_dcv_authenticator.py b/test/unit/dcv/test_dcv_authenticator.py index 1d0d3f6f7c..2890e65ecc 100644 --- a/test/unit/dcv/test_dcv_authenticator.py +++ b/test/unit/dcv/test_dcv_authenticator.py @@ -92,29 +92,48 @@ def test_sha_generator(): @pytest.mark.parametrize( - "user, command, session_id, result", + "uid, command, session_id, result", [ - ("user1", "/usr/libexec/dcv/dcvagent", "mysession", True), - ("user1", "/usr/libexec/dcv/dcvagent2", "mysession", False), - ("wrong", "/usr/libexec/dcv/dcvagent", "mysession", False), - ("user1", "/usr/libexec/dcv/dcvagent", "wrong", False), - ("user1", "/usr/lib/x86_64-linux-gnu/dcv/dcvagent", "mysession", True), - ("wrong", "/usr/lib/x86_64-linux-gnu/dcv/dcvagent", "mysession", False), - ("user1", "/usr/lib/x86_64-linux-gnu/dcv/dcvagent", "wrong", False), + (1000, "/usr/libexec/dcv/dcvagent", "mysession", True), + (1000, "/usr/libexec/dcv/dcvagent2", "mysession", False), + (4242, "/usr/libexec/dcv/dcvagent", "mysession", False), + (1000, "/usr/libexec/dcv/dcvagent", "wrong", False), + (1000, "/usr/lib/x86_64-linux-gnu/dcv/dcvagent", "mysession", True), + (4242, "/usr/lib/x86_64-linux-gnu/dcv/dcvagent", "mysession", False), + (1000, "/usr/lib/x86_64-linux-gnu/dcv/dcvagent", "wrong", False), ], ) -def test_is_process_valid(user, command, session_id, result): +def test_is_process_valid(uid, command, session_id, result): expected_session_id = "mysession" - expected_user = "user1" + expected_uid = 1000 - ps_aux_output = ( - f"{user} 63 0.0 0.0 4348844 3108 ?? Ss 23Jul19 2:32.46 {command} --mode full " - f"--session-id {session_id}" - ) + # Row format produced by `ps -eo uid,args`: " ". + # The numeric UID avoids the username truncation that `ps aux` introduces for names > 8 chars. + ps_output = f"{uid} {command} --mode full --session-id {session_id}" + + assert_that(DCVAuthenticator.check_dcv_process(ps_output, expected_uid, expected_session_id)).is_equal_to(result) + + +@pytest.mark.parametrize( + "row", + [ + "", # empty line + " ", # blank line + "1000", # uid only, no command + "1000 /usr/libexec/dcv/dcvagent --mode full", # no --session-id argument + "1000 /usr/libexec/dcv/dcvagent --mode full --session-id", # --session-id with no value + ], +) +def test_is_process_valid_malformed_rows(row): + # Malformed or non-dcvagent rows must never be considered a valid session. + assert_that(DCVAuthenticator.check_dcv_process(row, 1000, "mysession")).is_false() - assert_that(DCVAuthenticator.check_dcv_process(ps_aux_output, expected_user, expected_session_id)).is_equal_to( - result - ) + +def test_is_process_valid_minimal_row(): + # A dcvagent process invoked without --mode must still be matched (regression for an + # overly strict field-count guard). + row = "1000 /usr/libexec/dcv/dcvagent --session-id mysession" + assert_that(DCVAuthenticator.check_dcv_process(row, 1000, "mysession")).is_true() def mock_generate_random_token(mocker, value): @@ -204,6 +223,36 @@ def test_get_request_token_regex(user, session_id): DCVAuthenticator._get_request_token(user, session_id) +@pytest.mark.parametrize( + "user", + [ + "first.last", # dot is now allowed (e.g. AD/LDAP style names) + "first.last.middle", + "averylongusernamebeyond8chars", # names longer than 8 chars must be accepted + "a" * 32, # 1 leading char + 31 = max length + "_service.account", + "first.last$", # trailing $ (machine account style) + ], +) +def test_user_regex_allows_valid_names(user): + # Should not raise for valid usernames, including those with dots and long names. + DCVAuthenticator._validate_param(user, DCVAuthenticator.USER_REGEX, "authUser") + + +@pytest.mark.parametrize( + "user", + [ + ".first.last", # cannot start with a dot + "First.Last", # uppercase not allowed + "a" * 33, # exceeds max length + "first.last space", # whitespace not allowed + ], +) +def test_user_regex_rejects_invalid_names(user): + with pytest.raises(DCVAuthenticator.IncorrectRequestError): + DCVAuthenticator._validate_param(user, DCVAuthenticator.USER_REGEX, "authUser") + + @pytest.mark.parametrize("token", ["assvbsd", "?" + "".join(("c" for _ in range(255)))]) def test_get_session_token_regex(token): with pytest.raises(DCVAuthenticator.IncorrectRequestError):