http.sys: Allow configuring HTTP_AUTH_EX_FLAGs as options #51833

evgenykotkov · 2023-11-02T19:31:21Z

http.sys: Allow configuring HTTP_AUTH_EX_FLAGs as options

Description

The native HTTP.sys API offers two extended authentication flags:

that can be used by users to fine-tune their Windows authentication setups.

For instance, the HTTP_AUTH_EX_FLAG_ENABLE_KERBEROS_CREDENTIAL_CACHING flag can be used to avoid having to authenticate every request and make the authentication session-based, thus reducing the overall number of requests and improving the high-latency scenarios. Enabling it can be used to achieve the same behavior as with the authPersistNonNTLM option in IIS.

Change details:

The groundwork change c50346d fixes an existing issue in the definition/layout of the HTTP_SERVER_AUTHENTICATION_INFO interop structure.
The core change 7489786 exposes both flags as options in the authentication manager. Because setting the extended flags requires a different property type (HttpServerExtendedAuthenticationProperty), we take additional precaution to only use that property type if we actually have the extended flags to set, and use the original HttpServerAuthenticationProperty otherwise.

Fixes #13634

API proposal: #51990

ghost · 2023-11-02T19:31:31Z

Thanks for your PR, @evgenykotkov. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

Tratcher · 2023-11-02T19:54:33Z

FYI https://github.com/dotnet/aspnetcore/pull/50685/files#diff-9ef7d6eddeed6c08a6095b72475b87bc6b8b84b2b657ad07d407f9feb44f232c removes the need for pre-defined interop structs.

Tratcher · 2023-11-02T19:55:00Z

@blowdart

blowdart

Cached in the comments is vague.

Cached for how long?

ghost · 2023-11-02T19:57:21Z

Thank you for your API proposal. I'm removing the api-ready-for-review label. API Proposals should be submitted for review through Issues based on this template.

Tratcher · 2023-11-02T20:12:11Z

I think this is the same intent as

aspnetcore/src/Security/Authentication/Negotiate/src/NegotiateOptions.cs

Lines 22 to 27 in 0fe51bd

    
           /// <summary> 
        
           /// Indicates if Kerberos credentials should be persisted and re-used for subsquent anonymous requests. 
        
           /// This option must not be used if connections may be shared by requests from different users. 
        
           /// </summary> 
        
           /// <value>Defaults to <see langword="false"/>.</value> 
        
           public bool PersistKerberosCredentials { get; set; }

amcasey

Please have a look at the merge conflicts.

src/Servers/HttpSys/src/AuthenticationManager.cs

amcasey · 2023-11-08T23:49:06Z

src/Servers/HttpSys/src/AuthenticationManager.cs

+    /// or Negotiate authentication. Kerberos or Negotiate authentication must be enabled.
+    /// The default is false.
+    /// </summary>
+    public bool CaptureCredential { get; set; }


I know we're mirroring an underlying API, but it seems like this should be "CaptureCredentials".

CaptureProcessCredentials

In 39edd84, I went with the CaptureCredentials name for this option. Please let me know if it works for you.

src/Servers/HttpSys/src/AuthenticationManager.cs

src/Servers/HttpSys/src/PublicAPI.Unshipped.txt

src/Servers/HttpSys/test/FunctionalTests/AuthenticationTests.cs

amcasey · 2023-11-08T23:56:37Z

src/Shared/HttpSys/NativeInterop/HttpApiTypes.cs

-        internal bool ReceiveContextHandle;
-        internal bool DisableNTLMCredentialCaching;
-        internal ulong ExFlags;
+        internal byte ReceiveMutualAuth;


I'd prefer to retain at least a comment indicating that it's boolean.

I'd prefer to retain at least a comment indicating that it's boolean.

With 7f18f8f, this change seems to be no longer necessary for main.

However, since c50346d seems to fix a rather significant issue with an invalid structure layout/size, is it something that should be backported to stable branches?

Please let me know if that is the case, and whether you'd want me to handle that with a separate PR or in any other convenient way.

Can you demonstrate any negative effects from the old structure? We've not had any complaints here.

Can you demonstrate any negative effects from the old structure? We've not had any complaints here.

In my x64 environment, the native size of the structure is:

sizeof(HTTP_SERVER_AUTHENTICATION_INFO) == 64

In the same environment, the size of the interop structure before the fix is:

Marshal.SizeOf<HttpApiTypes.HTTP_SERVER_AUTHENTICATION_INFO> == 80

In turn, this means that:

The field layout is wrong after the AuthSchemes field

An invalid (larger) size is passed during the call to HttpSetUrlGroupProperty()

Maybe 1. hasn't been causing any practical issues because the current version of the code didn't change any structure fields after the AuthSchemes. But I would still say that 2. is quite significant just by itself: the behavior when passing an invalid size to the HttpSetUrlGroupProperty() function seems to be unspecified, and could have easily resulted in an error or even a crash.

In my environment the call seems to succeed, so from that perspective this remains a theoretical issue, but, to be precise, there is a chance that this behavior may change in the future or may differ depending on the OS version — I haven't checked that in detail.

For reference, here is the native definition of the related typedefs and the structure itself:

typedef BYTE BOOLEAN; typedef unsigned char UCHAR; typedef struct _HTTP_SERVER_AUTHENTICATION_INFO { HTTP_PROPERTY_FLAGS Flags; ULONG AuthSchemes; BOOLEAN ReceiveMutualAuth; BOOLEAN ReceiveContextHandle; BOOLEAN DisableNTLMCredentialCaching; UCHAR ExFlags; HTTP_SERVER_AUTHENTICATION_DIGEST_PARAMS DigestParams; HTTP_SERVER_AUTHENTICATION_BASIC_PARAMS BasicParams; } HTTP_SERVER_AUTHENTICATION_INFO, *PHTTP_SERVER_AUTHENTICATION_INFO;

Ok, we won't consider backporting it without a repro of a functional issue. For now it's enough that the issue is gone in main.

The native HTTP.sys API offers two extended authentication flags: - `HTTP_AUTH_EX_FLAG_ENABLE_KERBEROS_CREDENTIAL_CACHING` and - `HTTP_AUTH_EX_FLAG_CAPTURE_CREDENTIAL` that can be used by users to fine-tune their Windows authentication setups. For instance, the `HTTP_AUTH_EX_FLAG_ENABLE_KERBEROS_CREDENTIAL_CACHING` flag can be used to avoid having to authenticate every request and make the authentication session-based, thus reducing the overall number of requests and improving the high-latency scenarios. Enabling it can be used to achieve the same behavior as with the `authPersistNonNTLM` option in IIS. (See dotnet#13634) This commit exposes both flags as options in the authentication manager. Because setting the extended flags requires a different property type (`HttpServerExtendedAuthenticationProperty`), we take additional precaution to only use that property type if we actually have the extended flags to set, and use the original `HttpServerAuthenticationProperty` otherwise.

evgenykotkov · 2023-11-10T20:40:12Z

Please have a look at the merge conflicts.

Done.

(With 7f18f8f, the groundwork change c50346d is no longer required, so I rebased the core change as bf884dc.)

…ching`

ghost · 2023-11-18T03:00:36Z

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

amcasey · 2023-11-27T20:47:11Z

I think we're just waiting for an API review.

amcasey · 2023-11-28T20:56:09Z

API review asked for a rename. Otherwise, we should be good to go.

Based on the discussion in dotnet#51990

evgenykotkov · 2024-02-05T18:03:17Z

API review asked for a rename. Otherwise, we should be good to go.

Based on the discussion in #51990, I kept the original names and made a couple of tweaks to the docstrings.

src/Servers/HttpSys/src/AuthenticationManager.cs

* http.sys: Allow configuring HTTP_AUTH_EX_FLAGs as options The native HTTP.sys API offers two extended authentication flags: - `HTTP_AUTH_EX_FLAG_ENABLE_KERBEROS_CREDENTIAL_CACHING` and - `HTTP_AUTH_EX_FLAG_CAPTURE_CREDENTIAL` that can be used by users to fine-tune their Windows authentication setups. For instance, the `HTTP_AUTH_EX_FLAG_ENABLE_KERBEROS_CREDENTIAL_CACHING` flag can be used to avoid having to authenticate every request and make the authentication session-based, thus reducing the overall number of requests and improving the high-latency scenarios. Enabling it can be used to achieve the same behavior as with the `authPersistNonNTLM` option in IIS. (See dotnet#13634) This commit exposes both flags as options in the authentication manager. Because setting the extended flags requires a different property type (`HttpServerExtendedAuthenticationProperty`), we take additional precaution to only use that property type if we actually have the extended flags to set, and use the original `HttpServerAuthenticationProperty` otherwise. * http.sys: Rename: CaptureCredential → CaptureCredentials * http.sys: Clarify description for `EnableKerberosCredentialCaching` * http.sys: Further clarify description for `EnableKerberosCredentialCaching` * http.sys: Further clarify documentation for new options Based on the discussion in dotnet#51990 * http.sys: Tweak documentation for `CaptureCredentials`

evgenykotkov requested review from Tratcher, halter73, BrennanConroy, JamesNK and mgravell as code owners November 2, 2023 19:31

ghost added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member labels Nov 2, 2023

This comment was marked as resolved.

Sign in to view

blowdart approved these changes Nov 2, 2023

View reviewed changes

Tratcher added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Nov 2, 2023

ghost removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Nov 2, 2023

mkArtakMSFT assigned adityamandaleeka Nov 7, 2023

amcasey reviewed Nov 8, 2023

View reviewed changes

evgenykotkov requested review from wtgodbe, a team, captainsafia, javiercn and mkArtakMSFT as code owners November 10, 2023 19:55

evgenykotkov force-pushed the httpsys-auth-ex-flags branch from aeb2178 to bf884dc Compare November 10, 2023 20:16

evgenykotkov added 2 commits November 10, 2023 21:21

http.sys: Rename: CaptureCredential → CaptureCredentials

39edd84

http.sys: Clarify description for EnableKerberosCredentialCaching

db5b580

http.sys: Further clarify description for `EnableKerberosCredentialCa…

e2a327a

…ching`

evgenykotkov mentioned this pull request Nov 11, 2023

http.sys: Allow configuring extended authentication flags (HTTP_AUTH_EX_FLAG) as options #51990

Open

ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Nov 18, 2023

evgenykotkov added 2 commits February 5, 2024 18:03

http.sys: Further clarify documentation for new options

5db6406

Based on the discussion in dotnet#51990

Merge branch 'dotnet:main' into httpsys-auth-ex-flags

dac4840

evgenykotkov requested a review from amcasey February 6, 2024 16:40

amcasey reviewed Feb 7, 2024

View reviewed changes

src/Servers/HttpSys/src/AuthenticationManager.cs Outdated Show resolved Hide resolved

http.sys: Tweak documentation for CaptureCredentials

a2885d9

amcasey approved these changes Feb 7, 2024

View reviewed changes

amcasey enabled auto-merge (squash) February 7, 2024 22:43

amcasey merged commit 4deb60b into dotnet:main Feb 9, 2024

dotnet-policy-service bot added this to the 9.0-preview2 milestone Feb 9, 2024

mgravell mentioned this pull request May 3, 2024

Experimental revert of P/Invoke layer #55519

Closed

http.sys: Allow configuring HTTP_AUTH_EX_FLAGs as options #51833

http.sys: Allow configuring HTTP_AUTH_EX_FLAGs as options #51833

Uh oh!

Conversation

evgenykotkov commented Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

http.sys: Allow configuring HTTP_AUTH_EX_FLAGs as options

Description

Change details:

Uh oh!

ghost commented Nov 2, 2023

Uh oh!

This comment was marked as resolved.

Tratcher commented Nov 2, 2023

Uh oh!

Tratcher commented Nov 2, 2023

Uh oh!

blowdart left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Nov 2, 2023

Uh oh!

Tratcher commented Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amcasey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evgenykotkov commented Nov 10, 2023

Uh oh!

ghost commented Nov 18, 2023

Uh oh!

amcasey commented Nov 27, 2023

Uh oh!

amcasey commented Nov 28, 2023

Uh oh!

evgenykotkov commented Feb 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

evgenykotkov commented Nov 2, 2023 •

edited

Loading

Tratcher commented Nov 2, 2023 •

edited

Loading

evgenykotkov commented Feb 5, 2024 •

edited

Loading