Remus Rusanu commented on YARN-2198:

1. nativeio.c: Should we return null here?
RR: Fixed

2.Nit: nativeio code uses different naming convention for local variables. 
Please try to be consistent with the rest of the file.
RR: Fixed

3. nativeio.c: Nit: I would move throw_ioe if check before done:, the code flow 
will be less error prone 
RR: fixed

4. winutils_process_stub.c: Can {{env->NewGlobalRef())) return null/throw? 
Should we handle this?
RR: Fixed

5. winutils_process_stub.c: You should properly handle the GetExitCodeProcess() 
failure case.
RR: fixed

6. winutils_process_stub.c:Init to INVALID_HANDLE_VALUE?
RR: Fixed

7. client.c: Are RPC_STATUS error codes compatible with winerror codes? 
(semantic around checking for error)
RR: From my experiments they are compatible. FormatMessage gets the right 
message for RPC statuses

8. config.cpp: Wondering if there is a way to get to config files without 
adding a dependency on env variables?
RR: config location is now ../etc/hadoop/wsce-site.xml relative to exe. It is 
defined in pom.xml

9. config.cpp: This error check is unintuitive. Can you please be more explicit?
RR: fixed (no longer applies because only one file is checked)

10. config.cpp: Are SAL annotations correct? For strings one would usually use 
RR: Fixed, and it was  broken all over, thanks for catching it

11. config.cpp: SAL annotation __out_bcount? Also outLen->len in the annotation.
RR: fixed

11. config.cpp: This should be before StringCbPrintf to guarantee that CoInit 
and CoUninit are balanced. 
RR: fixed

12. hdpwinutilsvc.idl: Name does not seem appropriate for apache... possibly 
name it just winutilsvc.idl. Should we use spaces in this file for consistency? 
RR: fixes all names as "hadoopwinutilsvc"

13. winutils.h:__in_bcount(len) -> __in_ecount(len)
RR: fixed

14. libwinutils.c: I'm wondering if this is good opportunity to introduce 
unittests for our C code, as the complexity started increasing beyond just 
windows OS calls, where there is little value in unittesting. 
RR: Not fixed. I will come back later and add units here, but the core work 
(LRPC, SCM, logon user and create process) are basically untestable from C unit 

15. libwinutils.c: Should we deallocate this when BuildSecurityDescriptor fails?
RR: is alloca, so it doesn't need dealloc. 

I don't think it is required to do this now, just wanted to bring it up: if our 
native codebase continues to grow at this pace we should consider introducing 
smart pointers. It is becoming impossibly hard to properly manage the memory in 
all success/failure cases. This becomes more important now that we have long 
running NM native client and winutils service. 
RR: the whole winutils/libwinutils code style is early 90's Petzold Windows 
code style. I'm not a fan of it, but I kept all new code consistent with this 
style. Moving to C++ RAI would be better, but I don;t want to do it piecemeal. 
Some other time.

16. What is the behaviour of calling winutils service. Will this command 
install and start a winutils.exe service under SYSTEM account, and exit?
RR: no. SCM instalation/config is left to SCM tools (eg. sc.exe). "winutils 
service" is the command line to start the service (it starts, register entry 
point with SCM, waits for SCM commands).

> Remove the need to run NodeManager as privileged account for Windows Secure 
> Container Executor
> ----------------------------------------------------------------------------------------------
>                 Key: YARN-2198
>                 URL: https://issues.apache.org/jira/browse/YARN-2198
>             Project: Hadoop YARN
>          Issue Type: Improvement
>            Reporter: Remus Rusanu
>            Assignee: Remus Rusanu
>              Labels: security, windows
>         Attachments: YARN-2198.1.patch, YARN-2198.2.patch, YARN-2198.3.patch, 
> YARN-2198.separation.patch
> YARN-1972 introduces a Secure Windows Container Executor. However this 
> executor requires a the process launching the container to be LocalSystem or 
> a member of the a local Administrators group. Since the process in question 
> is the NodeManager, the requirement translates to the entire NM to run as a 
> privileged account, a very large surface area to review and protect.
> This proposal is to move the privileged operations into a dedicated NT 
> service. The NM can run as a low privilege account and communicate with the 
> privileged NT service when it needs to launch a container. This would reduce 
> the surface exposed to the high privileges. 
> There has to exist a secure, authenticated and authorized channel of 
> communication between the NM and the privileged NT service. Possible 
> alternatives are a new TCP endpoint, Java RPC etc. My proposal though would 
> be to use Windows LPC (Local Procedure Calls), which is a Windows platform 
> specific inter-process communication channel that satisfies all requirements 
> and is easy to deploy. The privileged NT service would register and listen on 
> an LPC port (NtCreatePort, NtListenPort). The NM would use JNI to interop 
> with libwinutils which would host the LPC client code. The client would 
> connect to the LPC port (NtConnectPort) and send a message requesting a 
> container launch (NtRequestWaitReplyPort). LPC provides authentication and 
> the privileged NT service can use authorization API (AuthZ) to validate the 
> caller.

This message was sent by Atlassian JIRA

Reply via email to