[
https://issues.apache.org/jira/browse/YARN-1063?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14007485#comment-14007485
]
Ivan Mitic commented on YARN-1063:
----------------------------------
I have already reviewed a version of the initial patch (YARN-1063.patch). Copy
pasting the full list of comments for documentation purposes on the Jira.
First round:
1. winutils.h: You have a duplicate EnablePrivilege declaration. Please remove
the one with BOOL.
2. winutils.h: Convention in the file is to use CamelCased function names.
Please name your functions appropriately. Also, a nit, no space between '(' and
function args. Same comment across the board.
3. libwinutils.c#EnablePrivilege:
{code}
{
ReportErrorCode(L"LookupPrivilegeValue", GetLastError());
CloseHandle(hToken);
return GetLastError();
}
{code}
You shouldn't be calling GetLastError() twice above. CloseHandle() might reset
it to 0 or some other value. Can you change all error code paths in this
function to first assign GetLastError() to a local variable, and then log it
and do other things. E.g.
{code}
{
dwErrorCode = GetLastError();
ReportErrorCode(L"LookupPrivilegeValue", dwErrorCode);
CloseHandle(hToken);
return dwErrorCode;
}
{code}
3. Something is wrong with this comment
{code}
// Function: assignLsaString
//
// Description:
// fills in values of LSA_STRING struct to point to a string buffer
{code}
4. void assignLsaString( __in LSA_STRING * target, __in const char *strBuf )
Is target an __inout parameter?
5. libwinutils.c: Mixed tabs and spaces. Please use 2 space ident across the
board.
6. libwinutils.c: authentication pacakage - typo
7. libwinutils.c: Should the constant be named MICROSOFT_KERBEROS_NAME or there
is something else more appropriate?
8. libwinutils.c: GetNameFromLogonToken: You can assert that first
GetTokenInformation returns false. Don't do assert(GetTokenInformation() ==
FALSE) :)
9. I don't believe that calloc() sets the last error. If the return value is
NULL, you should assume/error-with ERROR_NOT_ENOUGH_MEMORY. Applies to all
places.
10. libwinutils.c: LookupAccountSid: Do you need to allocate userNameSize+1 and
domainNameSize+1 buffer sizes, or is this already accounted for?
11. task.c: You are not checking result of GetCurrentDirectory(). If you expect
the method to always succeed, you can assert that the result > 0.
12. task.c: Please keep CreateProcessAsUser and old CreateProcess codepaths
separate. I don't think it is trivial to prove that the new code with
CreateProcessAsUser has exactly the same sematic as the old code.
13. task.c: I don't understand the need to TerminateJobObject when
CreateProcessAsUser failed? Why wouldn't the regular return code path exit the
process with non zero code?
14. Consider using dwErrorCode in your functions to track status of the win
error codes.
15. task.c: createTaskAsUser: Do you need to check if lsaHandle is valid to be
sent to unregisterWithLsa. You can initialize to INVALID_HANDLE_VALUE
16. task.c: Task: Can you please rename size to cmdLineSize. argLen to
crtArgLen. Btw, is size needed?
17. task.c: Task: Can you please rename ARGC_GROUPID to ARGC_JOBOBJECTNAME or
something alike.
18. task.c: TaskUsage: Your command line format does not seem to be valid. You
are missing jobobject and pidfile.
19. task.c:
{code}
cmdLine = argv[ARGC_COMMAND];
if (argc > ARGC_COMMAND_ARGS) {
crtArg = ARGC_COMMAND;
insertHere = buffer;
while (crtArg < argc) {
argLen = wcslen(argv[crtArg]);
wcscat(insertHere, argv[crtArg]);
insertHere += argLen;
size -= argLen;
insertHere[0] = L' ';
insertHere += 1;
size -= 1;
insertHere[0] = 0;
++crtArg;
}
cmdLine = buffer;
}
{code}
Do you mind adding a short comment on what you're doing.
20.
bq. add PIDFILE argument to task createAsUser. Pidfile must be created by the
task controller just before launching the process.
Can you comment on the rationale?
21.
bq. accept arbitrary arguments to pass to the process launched. We use the
launcher for both container and localizer and that requires variable arguments.
How this works today? Is the localizer using something else?
22. We haven't written a whole lot of unittests for winutils so far (Check
testwinutils.java). I'll let you make the call on whether this could/should be
unittested and if yes, what is appropriate. I will sign off anyhow.
Second round:
1. I still see GetLastError() being called twice in EnablePrivilege. Can you
please use dwErrorCode instead of the second call.
{code}
dwErrCode = GetLastError();
ReportErrorCode(L"OpenProcessToken", GetLastError());
{code}
2. You missed changing the SAL annoation in the header file
{code}
void AssignLsaString(__inout LSA_STRING * target, __in const char *strBuf)
{code}
3. libwinutils.c: LoadUserProfileForLogon and UnloadProfileForLogon(): Might
make sense to assert that pi != NULL. Also, is the right annotation __inout in
this scenario?
4. task.c:
{code}
// We have to explictly Terminate, passing in the error code
// simply closing the job would kill our own process with success exit
status
TerminateJobObject(jobObject, dwErrorCode);
return dwErrorCode;
{code}
You are using winerror code to terminate the process with. I don't think this
is OK. Process exit codes usually have a different range, right? Please correct
me if you think differently. You can define your own special exit code/codes.
5. task.c: Nit: I see that you're still using space between braces and
condition in if
{code}
if( err != ERROR_SUCCESS ) {
{code}
6. task.c:
{code}
// This case is not MSDN documented.
// Strictly speaking E_FAIL is a COM HRESULT
// But there isn't any Win32 system equivalent and everybody will
recognize it
dwErrorCode = E_FAIL;
goto TaskExit;
{code}
I personally don't like the idea to mixing COM HRESULT and winerror codes.
Please use ERROR_GEN_FAILURE or some other winerror code you feel is ok.
7. task.c: Are you sure that GetCurrentDirectory() cannot return a legit path
longer then MAX_PATH in our scenarios (e.g. Yarn + Java7)?
8. task.c:
{quote}
task.c: Please keep CreateProcessAsUser and old CreateProcess codepaths
separate. I don't think it is trivial to prove that the new code with
CreateProcessAsUser has exactly the same sematic as the old code.
RemusR: Not sure about that. For instance, the existing createTask code in
task.c would return success if SetEnvironmentVariable or CreateProcess fails
(line 141 or line 152) because of the CloseHandle(jobObject) issue (item 11).
CreateProcess is quite a dangerous failure not to properly report.
{quote}
Main reason to keep the two separate (just condition the
CreateProcess/CreateProcessAsUser calls) was to avoid possible hidden problems.
e.g. CreateProcess assumes some different flags or has a different behavior.
To clarify a bit more, I meant do the following:
{code}
if (logognHandle == null) {
// Call original CreateProcess
CreateProcess...
} else {
CreateProcessAsUser...
}
{code}
You should keep all other bugfixes you made.
> Winutils needs ability to create task as domain user
> ----------------------------------------------------
>
> Key: YARN-1063
> URL: https://issues.apache.org/jira/browse/YARN-1063
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: nodemanager
> Environment: Windows
> Reporter: Kyle Leckie
> Assignee: Remus Rusanu
> Labels: security, windows
> Attachments: YARN-1063.2.patch, YARN-1063.3.patch, YARN-1063.4.patch,
> YARN-1063.patch
>
>
> h1. Summary:
> Securing a Hadoop cluster requires constructing some form of security
> boundary around the processes executed in YARN containers. Isolation based on
> Windows user isolation seems most feasible. This approach is similar to the
> approach taken by the existing LinuxContainerExecutor. The current patch to
> winutils.exe adds the ability to create a process as a domain user.
> h1. Alternative Methods considered:
> h2. Process rights limited by security token restriction:
> On Windows access decisions are made by examining the security token of a
> process. It is possible to spawn a process with a restricted security token.
> Any of the rights granted by SIDs of the default token may be restricted. It
> is possible to see this in action by examining the security tone of a
> sandboxed process launch be a web browser. Typically the launched process
> will have a fully restricted token and need to access machine resources
> through a dedicated broker process that enforces a custom security policy.
> This broker process mechanism would break compatibility with the typical
> Hadoop container process. The Container process must be able to utilize
> standard function calls for disk and network IO. I performed some work
> looking at ways to ACL the local files to the specific launched without
> granting rights to other processes launched on the same machine but found
> this to be an overly complex solution.
> h2. Relying on APP containers:
> Recent versions of windows have the ability to launch processes within an
> isolated container. Application containers are supported for execution of
> WinRT based executables. This method was ruled out due to the lack of
> official support for standard windows APIs. At some point in the future
> windows may support functionality similar to BSD jails or Linux containers,
> at that point support for containers should be added.
> h1. Create As User Feature Description:
> h2. Usage:
> A new sub command was added to the set of task commands. Here is the syntax:
> winutils task createAsUser [TASKNAME] [USERNAME] [COMMAND_LINE]
> Some notes:
> * The username specified is in the format of "user@domain"
> * The machine executing this command must be joined to the domain of the user
> specified
> * The domain controller must allow the account executing the command access
> to the user information. For this join the account to the predefined group
> labeled "Pre-Windows 2000 Compatible Access"
> * The account running the command must have several rights on the local
> machine. These can be managed manually using secpol.msc:
> ** "Act as part of the operating system" - SE_TCB_NAME
> ** "Replace a process-level token" - SE_ASSIGNPRIMARYTOKEN_NAME
> ** "Adjust memory quotas for a process" - SE_INCREASE_QUOTA_NAME
> * The launched process will not have rights to the desktop so will not be
> able to display any information or create UI.
> * The launched process will have no network credentials. Any access of
> network resources that requires domain authentication will fail.
> h2. Implementation:
> Winutils performs the following steps:
> # Enable the required privileges for the current process.
> # Register as a trusted process with the Local Security Authority (LSA).
> # Create a new logon for the user passed on the command line.
> # Load/Create a profile on the local machine for the new logon.
> # Create a new environment for the new logon.
> # Launch the new process in a job with the task name specified and using the
> created logon.
> # Wait for the JOB to exit.
> h2. Future work:
> The following work was scoped out of this check in:
> * Support for non-domain users or machine that are not domain joined.
> * Support for privilege isolation by running the task launcher in a high
> privilege service with access over an ACLed named pipe.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
