Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow UID to be returned by the mapper script. #3795

Merged
merged 9 commits into from
Sep 25, 2024
Merged

Allow UID to be returned by the mapper script. #3795

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fcd2187
Allow UID to be returned by the mapper script. Using a UID is useful …
guruevi Sep 13, 2024
e57ce02
Use POSIX call, should be faster than running a command.
guruevi Sep 13, 2024
740bcfe
Fix various side cases (see comments)
guruevi Sep 13, 2024
9196987
Benchmarked and optimized.
guruevi Sep 14, 2024
330e174
Negative condition in control flow statement
guruevi Sep 20, 2024
f9dbc36
Merge branch 'master' of https://github.com/OSC/ondemand
guruevi Sep 20, 2024
e4fa9ff
Merge branch 'OSC:master' into master
guruevi Sep 23, 2024
f8a4bdf
Merge branch 'master' of https://github.com/OSC/ondemand
guruevi Sep 23, 2024
06a37d4
Merge branch 'master' of https://github.com/guruevi/ondemand
guruevi Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions mod_ood_proxy/lib/ood/user_map.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,12 @@ function map(r, user_map_match, user_map_cmd, remote_user)
handle:close()
end

-- if sys_user is a number, then it's the uid, so convert to username
if tonumber(sys_user) then
local pwd = require "posix.pwd"
sys_user = pwd.getpwuid(tonumber(sys_user)).pw_name
end

time_user_map = (r:clock() - now)/1000.0
r:debug("Mapped '" .. remote_user .. "' => '" .. (sys_user or "") .. "' [" .. time_user_map .. " ms]")

Expand All @@ -25,6 +31,7 @@ function map(r, user_map_match, user_map_cmd, remote_user)
return nil
end


r.subprocess_env['MAPPED_USER'] = sys_user -- set as CGI variable for later hooks (i.e., analytics)
r.subprocess_env['OOD_TIME_USER_MAP'] = time_user_map -- set as CGI variable for later hooks (i.e., analytics)
return sys_user
Expand Down
5 changes: 3 additions & 2 deletions nginx_stage/lib/nginx_stage/user.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,12 @@ class User
# @param user [String] the user name defining this object
# @raise [ArgumentError] if user or primary group doesn't exist on local system
def initialize(user)
@passwd = Etc.getpwnam user.to_s
user_is_uid = (user[0].to_i and user.to_i > 0)
Copy link
Contributor

@johrstrom johrstrom Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why check the first character here - user[0]? "123jeff".to_i returns 123 and "jeff123".to_i returns 0.

Maybe we need a regex to account for the case when user is a proper username that just happens to start with a number. Maybe user.to_s.chomp.match?(/\A\d+\Z/)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usernames are illegal to start with numbers per the POSIX standard (^[a-z][-a-z0-9]*$").

I was of the impression there were cases that certain strings could return numbers (I am not a Ruby expert), but you are correct, just user.to_i > 0 is shorter. I am away from the desk, but I will change that soon. I didn't want to do a regexp because of the overhead of parsing regexp.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.Usernames are illegal to start with numbers per the POSIX standard (^[a-z][-a-z0-9]*$").

If only this were true! Or at least, if only all Linux distros adhered to it. Here I just hopped into a rocky 9 container and made the 123jeff user without any issue.

image

And just for kicks - there's the --badname flag (lol)

image

I just went through this with #3753 having to account for @ in usernames.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put a new version up that works in various test I did and this will 'fall through' if someone out there has all-number login as their user name.

I also found a bug in the group conversion. Eg "domain users" is a group name that SSSD can return. So again, I used number id and then map them to an array of strings using Etc.

@passwd = (user_is_uid) ? Etc.getpwuid(user.to_i) : Etc.getpwnam(user)
@group = Etc.getgrgid gid
@groups = get_groups

if name.to_s != user.to_s
if not user_is_uid and (name.to_s != user.to_s)
err_msg = <<~HEREDOC
Username '#{user}' is being mapped to '#{name}' in SSSD and they don't match.
Users with domain names cannot be mapped correctly. If '#{name}' still has the
Expand Down
Loading