add support for Impersonation and Delegation to HttpWebRequest #102038
Conversation
wfurt
changed the title
| { | ||
| // This is legacy hack. We want to deal with it only if explicitly set. | ||
| var setting = typeof(SocketsHttpHandler).InvokeMember("_settings", BindingFlags.GetField | BindingFlags.NonPublic | BindingFlags.Instance, null, handler, null); | ||
| setting?.GetType().InvokeMember("_impersonationLevel", BindingFlags.SetField | BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance, null, setting, new object[] { request!.ImpersonationLevel }); | ||
| } |
There was a problem hiding this comment.
It would be nice to structure it to be usable with UnsafeAccessor attribute. That's not possible here since HttpConnectionSettings is internal.
I assume InvokeMember is known reflection pattern in ILLink/ILC, right? (/cc @sbomer @MichalStrehovsky)
There was a problem hiding this comment.
InvokeMember is not handled. It will keep everything on SocketsHttpHandler because of the annotation:
runtime/src/libraries/System.Private.CoreLib/src/System/Type.cs
Lines 571 to 572 in de4fa45
It would be preferable to call GetField instead so that we just keep a single field.
The subsequent setting?.GetType().InvokeMember is not trim safe at all, this will break under trimming because we have no clue what type is this reflecting on, like the suppressed warning says. Please don't merge this like that, the warning suppression is a bug.
There was a problem hiding this comment.
updated. However, it is not clear to me how to proceed on the trimming @MichalStrehovsky. Should I annotate the HttpConnectionSettings somehow?
There was a problem hiding this comment.
Is HttpConnectionSettings the type we expect as the result of setting?.GetType()? If so replacing that with a typeof (or Type.GetType("TypeName, AssemblyName") if it's inaccessible) followed by GetField("_impersonationLevel")) should work without trimming warnings.
There was a problem hiding this comment.
Yes. But it is not public type. As @filipnavara mentioned it is internal to SocketsHttpHandler. It is generally container the handler use to pass the setting around to connection pools.
There was a problem hiding this comment.
But it is not public type.
That doesn't matter for Type.GetType. You can use Type.GetType("System.Net.Http.HttpConnectionSettings, System.Net.Http") and it resolves the type just fine.
| { | ||
| // This is legacy hack. We want to deal with it only if explicitly set. | ||
| var setting = typeof(SocketsHttpHandler).InvokeMember("_settings", BindingFlags.GetField | BindingFlags.NonPublic | BindingFlags.Instance, null, handler, null); | ||
| setting?.GetType().InvokeMember("_impersonationLevel", BindingFlags.SetField | BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance, null, setting, new object[] { request!.ImpersonationLevel }); |
There was a problem hiding this comment.
Should we be add something public to SocketsHttpHandler instead of using private reflection?
There was a problem hiding this comment.
This is what #100895 is asking for. My hesitation comes from
- it works only on Windows
- It is somewhat deprecated and it no longer works by default in new browsers e.g. IE was fine and Edge is not.
So my primary concern was support for legacy app migration.
If we see value, adding public property would not be too difficult.
Let me know @stephentoub and we can push #100895 forward for SocketsHttpHandler.
We still may proceed with this so testing is possible and make updates once public API is available.
There was a problem hiding this comment.
We still may proceed with this so testing is possible and make updates once public API is available.
Ok. I don't love the private reflection, but as the components are both in the shared framework and versioned together, I can tolerate it.
it works only on Windows, It is somewhat deprecated and it no longer works by default in new browsers e.g. IE was fine and Edge is not.
Ok, then I'd agree it's not desirable to expose.
There was a problem hiding this comment.
we can revisit public API if more desirable for whatever reason.
| @@ -29,6 +30,7 @@ internal sealed class HttpConnectionSettings | |||
|
|
|||
| internal bool _preAuthenticate = HttpHandlerDefaults.DefaultPreAuthenticate; | |||
| internal ICredentials? _credentials; | |||
| internal TokenImpersonationLevel _impersonationLevel = HttpHandlerDefaults.DefaultImpersonationLevel; | |||
There was a problem hiding this comment.
It'd be worth a comment that this is used via reflection from WebRequest.
|
all test failures are known errors. |
…t#102038) * initial drop * update * feedback * feedbcak * comment * spacing
fixes #29587, contributes to #100895.
HttpWebRequesthasImpersonationLevelproperty but we simply silently ignore it since .NET Core beginning.That creates migration issue for certain customers who still depend on different levels.
This brings the functionality on par with .NET Framework.
SocketsHttpHandlerdoes not have API to control it but the underlyingNegotiateAuthenticationwe use can do it all. So I added plumbing for the property and little Reflection to access it. It is still probably way cheaper than anything else we do there. I wanted new code to be used only when somebody explicitly sets it so I changed the default - I feel it is OK since it was ignored anyway.The test only verifies that the reflection does not throw. I did functional testing on setup @sainath-reddy-gnv kindly provided.