search

antiduh / nsspi

A C# / .Net interface to the Win32 SSPI authentication API
BSD 2-Clause "Simplified" License
64 stars 34 forks source link

Fixed AcquireCredentialsHandle call for PasswordCredential and added Thread.CurrentPrincipal support. #11

Closed SteveSyfuhs closed 6 years ago

SteveSyfuhs commented 6 years ago

Fixed enum value. Any actual unicode strings passed in will only use the first character because the 0 byte after the first character is treated as a terminator.

Added ability to set the current thread principal so it's inline with the actual security context.

antiduh commented 6 years ago

Hi, thanks for contributing. I'll try to have this merged sometime this afternoon. I'm probably going to make a couple changes (and fix a possible bug of mine). The ImpersonationHandle really should only have one method, the Dispose method, because it's just a handle to the caller and a hook back to the server; I don't see any reason yet why the server couldn't just set the Thread.CurrentPrinciple right in the ImpersonateClient implementation.

Also, I think I spotted a bug in the ImpersonateClient constrained execution region that would relate to your changes, but I have to think about it some more. One, I've noticed that I'm setting 'impersonating' in the finally block to true regardless of the outcome of the call. Perhaps that should be changed, but I need to think about that still. Relatedly, we should probably be calling SetThreadIdentity() in that same finally block (after checking status == OK) to make sure that both (the impersonation and the SetThreadIdentity) happen or neither happen.

There's also a couple stray whitespace changes I'm going to edit out. Also, thanks for fixing that bad enum. I've been meaning to fix that :)

SteveSyfuhs commented 6 years ago

That sounds like a good idea. I debated about where to put the SetThreadIdentity(). I originally had it in ServerContext when trying to use WindowsIdentity.Impersonate(), and decided that wasn't a good idea because you needed to track that impersonation handle too, so it got moved into the handle class. Then Impersonate() didn't actually acquire the right level, so I fell back to just manually setting it and never moved it back to the ServerContext.

antiduh commented 6 years ago

I should ask, is there a circumstance where someone would ever want to not do this? Was this an oversight on my part? I'm wondering if the library should make it compulsory because it would seem to be an integral part of the correct operation of this feature.

antiduh commented 6 years ago

After doing some digging, I think leaving this as an option is a better choice. Thread.CurrentPrinciple's use seems to be something that an application developer would want to have strict control over, because the user may be relying on it being set to a specific object of their creation that they wouldn't want overwritten. On the other hand, it would seems that a lot of code handles principle acquisition through Thread.CurrentPrinciple (as opposed to, for instance, some sort of custom DI setup), so many folks would find this necessary for the correct operation of their project.

SteveSyfuhs commented 6 years ago

I think either way is actually reasonable. If it were automatic you could just collect the existing principal, impersonate, and then set back, since you have to manually call it. Personally I do prefer the option though.