feat: Add Windows session user #16

roblutt · 2023-10-04T02:06:57Z

What was the problem/requirement? (What/Why)

To support impersonation on Windows, we need a Windows session user class

What was the solution? (How)

Create session user class with the following properties:

User (local username, domain user's UPN, or domain user in down-level logon form)
Group (local group name or domain group in down-level)

Convert user to down-level because we cannot initiate a powershell subprocess using the domain user's UPN. This conversion uses the pywin32 package, which is a wrapper around Microsoft's win32 dlls. In particular, we use translatename, which calls TranslateNameW under the hood. Description of the enum values for the input and desired name formats can be found here.

What is the impact of this change?

User impersonation on Windows can be implemented

How was this change tested?

Unit tests pass. This is not yet covered by unit tests, but it was tested manually with a Windows machine joined to an AD.

Was this change documented?

No

Is this a breaking change?

No

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Honglichenn · 2023-10-04T03:04:15Z

src/openjd/sessions/_session_user.py

+    """
+
+    @staticmethod
+    def upn_to_down_level(upn):


Missing the type in the function definition. Please also add the docstring for the function. You could find the format by checking other functions.

I wonder if the input string is not the UPN format, will this fail? For example, Admin instead of [email protected]? I guess it will? We should check the format before translate it.

Better to have some basic unit tests in this PR, so we are more confident that this can work.

Good point, this may fail if provided Admin or even domain\\Admin, depending on how the win32 API handles this. I'll add a check and some unit tests

In terms of testing, we have some difficulty here because the translatename call is not doing a simple string conversion, but contacting the AD DC. So we can't test the translatename call yet as this is dependent on being joined to an AD and having the users in the test configured. After adding some logic to validate that we're in valid UPN format, I see 2 options:

Don't test both branches of the "if upn: convert; else: don't convert" statement. Just test that when provided user or domain\user, we don't perform any conversion or get an exception from the win32 API.

Refactor the windows session user class to inject a converter as a dependency so we can mock it out. Then we can assert on it being called and not called in cases of UPN and not UPN. This adds more complexity than it's worth, so let's avoid this for now.

we have some difficulty here because the translatename call is not doing a simple string conversion, but contacting the AD DC

To be clear, are you saying that this code path requires that the host be domain joined? Or will this work with a locally configured user as well?

Also, to second Hongli, a docstring here would be fantastic. Comments as well -- I'd like to better understand why we need to be doing this.

I've updated the code since this comment. The code path that performs the conversion does require that the instance is joined to whichever domain is specified in the UPN, but if anything other than a UPN is provided, the instance does not need to be joined to the domain for these methods.

Added docstrings

anandgo1 · 2023-10-04T18:33:43Z

src/openjd/sessions/_session_user.py

+            upn, win32api.NameUserPrincipal, win32api.NameSamCompatible
+        )
+
+    def __init__(self, user: str, *, group: str) -> None:


Question for @roblutt and @ddneilson - Do we add input validations for the session user class? For instance we have defined a regex pattern for user and group for posix on our SDK model, should a similar thing exist here? Similarly, should we add the valid formats for user and group in windows?

I haven't previously ever developed on open source modules. So looking for guidance here too.

I think the changes here are in line with what we have on Linux. Curious to hear what @ddneilson has to say though

Good thinking, @anandgo1 , but I think that we're okay without a regex here. Let's just make sure that our uses of the username/groupname are properly escaped when using them in powershell/batch scripts.

Signed-off-by: Robert Luttrell <[email protected]>

Honglichenn · 2023-10-05T04:13:04Z

src/openjd/sessions/_session_user.py

+            user (str): The user. This can be in UPN form, domain\\username, or just username
+        """
+        upn_format_regex = (
+            '^(?!\\.)[^\\s\\\\/:*?"<>|]{1,15}\\@[^\\s\\@\\\\/:*?"<>|]+\\.[^\\s\\@\\\\/:*?"<>|]+'


Would you mind sharing how you write this regex? I am worried if we miss anything here, [^\\s\\\\/:*?"<>|]{1,15} it seems we limit the length of the user name is less than 15, and I have a feeling that it may be not true in all systems. For example, this one.

Looks like the 15 is the wrong limit, it should be 256 per this doc.

Invalid characters in the suffix come from here.

^(?!\\.): negative lookahead to assert we don't start with a period

[^\\s\\\\/:*?"<>|]{1,15}: 1-15 characters which cannot be in this list. I'll update this to 256 max

\\@: @

[^\\s\\@\\\\/:*?"<>|]+: 1 or more characters which cannot be in the list

\\.: .

[^\\s\\@\\\\/:*?"<>|]+ 1 or more characters which cannot be in the list

Honglichenn · 2023-10-05T04:21:30Z

src/openjd/sessions/_session_user.py

+
+        self.group = group
+
+        self.user = self.try_convert_upn(user)


Is it simpler that we just check if the machine is:

Ad-joined by using NetGetJoinInformation

@ is in the username. Based on this, local user is not allowed to use @.

Then convert it? I am not sure if we should use a strict regex here, since user can always create some username too creative. We saw it happened several times in Nimble.

That could be simpler than what I've implemented. Are you suggesting that we use the name returned by NetGetJoinInformation and compare to the UPN suffix to ensure that we're joined to the domain in the suffix before doing the conversion, or just use the int returned by NetGetJoinInformation to ensure that we're joined to any domain before converting?

Are you considering this edge case that we discussed in the slack? If I understand correctly, you mean NetGetJoinInformation can also handle this edge case? It is a good point.
But I think we may need to consider the AD trust use case. there maybe some cross-domain user. I am not sure how does it work here.
So maybe just do a simple one to just use the int returned by NetGetJoinInformation to ensure that we're joined to any domain before converting and write a comment for the edge case which we don't support. I am open to any other opinions, since I am not an expert for the AD...

Honglichenn · 2023-10-05T04:22:10Z

src/openjd/sessions/_session_user.py

+    import win32security
+
+import re
+
 from typing import Optional

 __all__ = ("PosixSessionUser", "SessionUser")


minor: maybe you want to update this too.

By update, do you mean move re inside the condition to only import if we're on Windows? I guess this is a moot point either way based on the recommendation to just check the string for a "@" instead of using the regex, but just curious.

hmmm.. I mean L17 __all__ = ("PosixSessionUser", "SessionUser") -> __all__ = ("PosixSessionUser", "SessionUser", "WindowsSessionUser")

Honglichenn · 2023-10-05T04:32:40Z

src/openjd/sessions/_session_user.py

+        return re.search(upn_format_regex, user) is not None
+
+    @staticmethod
+    def try_convert_upn(user):


minor: I think the function name is a little bit ambiguous. Without reading the function's documentation or implementation, a developer might not be sure if the function is converting to UPN or from UPN. Maybe convert_upn_to_downlevel?

It is better to add the typing. For example, def convert_upn_to_downlevel(user:str)->str:

Will change the name next revision

Honglichenn · 2023-10-05T04:33:16Z

src/openjd/sessions/_session_user.py

+        Otherwise, returns user unmodified.
+        Arguments:
+            user (str): The user. This can be in UPN form, down-level logon, or just username
+        """


For the docstring, I think we should also include doc for the return value as well. I am not quite sure which standard we are using in the github packages. Need some clarification from @ddneilson. I see different formats of docstring in different files.

Signed-off-by: Robert Luttrell <[email protected]>

anandgo1 · 2023-10-06T18:51:04Z

src/openjd/sessions/_session_user.py

+    def __init__(self, user: str, *, group: str) -> None:
+        """
+        Arguments:
+            user (str): The user


More specifically, lets call this Windows session User

It is good to specific, but I believe we will move these two common attributes to the SessionUser later based on previous discussion.

anandgo1 · 2023-10-06T18:52:04Z

src/openjd/sessions/_session_user.py

+        """
+        Arguments:
+            user (str): The user
+            group (str): The group


Let's call this Windows session Group

src/openjd/sessions/_session_user.py

ddneilson

There's some great discussion on this one! I just have one little nitpick that I'd like to see in the __init__ docstring.

src/openjd/sessions/_session_user.py

Signed-off-by: Robert Luttrell <[email protected]>

src/openjd/sessions/_session_user.py

anandgo1 · 2023-10-11T22:09:10Z

src/openjd/sessions/_session_user.py

+
+
+class WindowsSessionUser(SessionUser):
+    __slots__ = ("user", "group")


Should we add "password" here?

Signed-off-by: Robert Luttrell <[email protected]>

roblutt force-pushed the roblutt/windows-session-user branch 2 times, most recently from 17b8542 to 36a75cf Compare October 4, 2023 02:13

roblutt changed the title ~~Add Windows session user~~ feat: Add Windows session user Oct 4, 2023

roblutt marked this pull request as ready for review October 4, 2023 02:21

roblutt requested a review from a team as a code owner October 4, 2023 02:21

Honglichenn requested changes Oct 4, 2023

View reviewed changes

Honglichenn mentioned this pull request Oct 4, 2023

feat: Change the Start-Process to Start-Job and add WindowsSessionUse… #14

Closed

anandgo1 reviewed Oct 4, 2023

View reviewed changes

roblutt force-pushed the roblutt/windows-session-user branch from 99d630e to b6fc3b2 Compare October 5, 2023 00:02

feat: Add Windows session user

1d8630d

Signed-off-by: Robert Luttrell <[email protected]>

roblutt force-pushed the roblutt/windows-session-user branch from b6fc3b2 to 1d8630d Compare October 5, 2023 00:08

Honglichenn reviewed Oct 5, 2023

View reviewed changes

Honglichenn mentioned this pull request Oct 5, 2023

feat: Change the Start-Process to Start-Job #17

Merged

roblutt added 3 commits October 6, 2023 09:53

address comments

a977816

Signed-off-by: Robert Luttrell <[email protected]>

formatting

8d6f5d0

Signed-off-by: Robert Luttrell <[email protected]>

Skip windows session user test on linux

f34d319

Signed-off-by: Robert Luttrell <[email protected]>

anandgo1 reviewed Oct 6, 2023

View reviewed changes

Honglichenn previously approved these changes Oct 6, 2023

View reviewed changes

anandgo1 previously approved these changes Oct 6, 2023

View reviewed changes

ddneilson suggested changes Oct 10, 2023

View reviewed changes

src/openjd/sessions/_session_user.py Show resolved Hide resolved

Improve WindowsSessionUser doscstring

fb07f18

Signed-off-by: Robert Luttrell <[email protected]>

roblutt dismissed stale reviews from anandgo1 and Honglichenn via fb07f18 October 10, 2023 20:42

Honglichenn approved these changes Oct 10, 2023

View reviewed changes

roblutt requested review from ddneilson and Honglichenn October 11, 2023 17:49

Honglichenn previously approved these changes Oct 11, 2023

View reviewed changes

Add password to windows session user

b2c5a67

Signed-off-by: Robert Luttrell <[email protected]>

roblutt dismissed Honglichenn’s stale review via b2c5a67 October 11, 2023 21:51

Honglichenn previously approved these changes Oct 11, 2023

View reviewed changes

anandgo1 reviewed Oct 11, 2023

View reviewed changes

Add missing password param and expand docstring

dbcdc6b

Signed-off-by: Robert Luttrell <[email protected]>

roblutt dismissed Honglichenn’s stale review via dbcdc6b October 11, 2023 22:16

anandgo1 approved these changes Oct 11, 2023

View reviewed changes

ddneilson approved these changes Oct 12, 2023

View reviewed changes

ddneilson merged commit 4e954e6 into mainline Oct 12, 2023
8 checks passed

ddneilson deleted the roblutt/windows-session-user branch October 12, 2023 16:54

ddneilson added the security Pull requests that could impact security label Jan 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Windows session user #16

feat: Add Windows session user #16

roblutt commented Oct 4, 2023 •

edited

Loading

Honglichenn Oct 4, 2023 •

edited

Loading

Honglichenn Oct 4, 2023

roblutt Oct 4, 2023

roblutt Oct 4, 2023 •

edited

Loading

ddneilson Oct 4, 2023

roblutt Oct 5, 2023

anandgo1 Oct 4, 2023

roblutt Oct 5, 2023

ddneilson Oct 10, 2023

Honglichenn Oct 5, 2023

roblutt Oct 5, 2023

Honglichenn Oct 5, 2023

roblutt Oct 5, 2023

Honglichenn Oct 5, 2023

Honglichenn Oct 5, 2023

roblutt Oct 5, 2023

Honglichenn Oct 5, 2023

Honglichenn Oct 5, 2023

roblutt Oct 5, 2023

Honglichenn Oct 5, 2023

anandgo1 Oct 6, 2023

Honglichenn Oct 6, 2023

anandgo1 Oct 6, 2023

ddneilson left a comment

anandgo1 Oct 11, 2023

roblutt Oct 11, 2023



		class WindowsSessionUser(SessionUser):
		__slots__ = ("user", "group")

feat: Add Windows session user #16

feat: Add Windows session user #16

Conversation

roblutt commented Oct 4, 2023 • edited Loading

What was the problem/requirement? (What/Why)

What was the solution? (How)

What is the impact of this change?

How was this change tested?

Was this change documented?

Is this a breaking change?

Honglichenn Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roblutt Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddneilson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roblutt commented Oct 4, 2023 •

edited

Loading

Honglichenn Oct 4, 2023 •

edited

Loading

roblutt Oct 4, 2023 •

edited

Loading