dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.07k stars 4.69k forks source link

NegotiateAuthentication - Provide method to get error messages #103536

Open jborean93 opened 3 months ago

jborean93 commented 3 months ago

Description

Currently the NegotiateAuthentication class reports errors behind a status code enum NegotiateAuthenticationStatusCode. This might be ok for common errors like InvalidCredential but in the case of unknown errors it becomes GenericFailure.

A specific example is when there is a clock skew that is too great for Kerberos, the call to GetOutgoingBlob returned a null byte array with a status code of GenericFailure. I had to resort to using Python to replicate the same call to retrieve the GSSAPI error details and figure out why it was failing

Major (851968):  Miscellaneous failure (see text), Minor (2529638949): Clock skew too great

Being able to get the major/minor error codes as well as their messages can provide a lot of context as to why authentication failed. Considering how opaque Kerberos can be when something goes wrong this would be very helpful for people trying to use it.

I'm unsure what this would look like as we cannot change GetOutgoingBlob to raise an exception normally but maybe a GetLastError() or a boolean overload that can be used to change it to raise an exception? Some thought into what the exception structure or error details would look like is needed as SSPI only has the 1 error code vs GSSAPI with a major and minor code.

Reproduction Steps

I don't have a generic reproducer as this isn't a bug in the code, just what I see as something missing from the public API.

Expected behavior

See description

Actual behavior

See description

Regression?

No

Known Workarounds

Nothing I know off, maybe some hooking into the event source that logs the internal exceptions that are fired.

Configuration

This specific error is from macOS' GSS.Framework, I'm unsure if Heimdal actual or MIT krb5 has another error code for the clock skew issue.

Other information

No response

dotnet-policy-service[bot] commented 3 months ago

Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones See info in area-owners.md if you want to be subscribed.

filipnavara commented 3 months ago

Two thoughts:

The current status codes roughly map to the major error codes in GSSAPI. The API design was driven by the idea that you would recognize some of the codes and possibly take a different code path. They are not very well suited for logging. Logging was intentionally done through tracing APIs because the current uses of NegotiateAuthentication are hidden behind other API layers (eg. HttpClient, SmtpClient, NegotiateStream).

jborean93 commented 3 months ago

Can the event details to track be documented on the class? I can understand that this might be low level for the typical use case but for people wanting to use it directly, not knowing the underlying error messages reported by the underlying library can be quite challenging when trying to figure out why.

SteveSyfuhs commented 3 months ago

This is more of an FYI to inform any API decisions, but Windows usage of GetLastError in SSPI code is explicitly undefined so there's no guarantee it returns anything meaningful. I bring this up as a warning that exposing a similar API here may be confusing. I recommend any error code be part of the call itself, as maybe an overloaded out param GetOutgoingBlob(..., out int substatus) and if absolutely necessary maybe keep a stack of the returned error tuples stack<(int result, int substatus)> or some similar mapping. Trying to reason about what "last" is, is super complicated out of context.

Edit: and while I'm here, the substatus is often something protocol-specific so it would be super helpful to tell what protocol negotiated that error in the first place.

jborean93 commented 3 months ago

Trying to reason about what "last" is, is super complicated out of context.

That is a good point, this request was more of a request to get the native status code and message from the underlying native call. Which on SSPI would be the return value from AcquireCredentialsHandle/InitializeSecurityContext/AcceptSecurityContext/etc. For GSSAPI it would be the major and minor code with the GSSAPI provided message for those status codes. I'm not the biggest fan of the get last model personally I was more spitballing a way to get that information considering the current code doesn't throw an exception but returns a generic enum value rather than the native details.

rzikm commented 3 months ago

Triage: there is a room for improvement, not critical for 9.0