Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal]: Add public [Compute/Verify]IntegrityCheck methods to NegotiateAuthentication #86950

Closed
filipnavara opened this issue May 31, 2023 · 19 comments · Fixed by #96712
Closed
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Security
Milestone

Comments

@filipnavara
Copy link
Member

filipnavara commented May 31, 2023

Background and motivation

The NegotiateAuthentication API (#69920) was added in .NET 7. It exposes functionality for Negotiate, NTLM and Kerberos authentication. It's implemented as a wrapper over GSSAPI (Unix), SSPI (Windows), and there's managed NTLM implementation for other platforms (Android, tvOS) where native API is not available. The initial API shape was driven by the goal of removing reflection access to internal NTAuthentication class, with focus on meeting the needs of email protocols (SASL in context of IMAP, SMTP), SQL protocols (SQL Server and PostgreSQL), client-side HTTP, and server-side HTTP in ASP.NET.

One omission in the original proposal and its later updates are the GetMIC and VerifyMIC operations defined in GSSAPI (https://datatracker.ietf.org/doc/html/rfc2743). These APIs are not used in any of the scenarios above, but they are needed for more specialized protocols, like the NegotiateStream. It is thus desirable to expose them for public use as well.

The operations implemented by the API are generating a "message integrity code" on one side of the communication protocol and verifying it on the other side. It is usually implemented as cryptographic signing operation with a negotiated key.

API Proposal

namespace System.Net.Security;

public sealed class NegotiateAuthentication : IDisposable
{
    /// <summary>
    /// Computes the message integrity check of a given message.
    /// </summary>
    /// <param name="message">Input message for MIC calculation.</param>
    /// <param name="signature">Buffer writer where the MIC is written.</param>
    /// <remarks>
    /// Implements the GSSAPI GetMIC operation.
    ///
    /// The method modifies the internal state and may update sequence numbers depending on the
    /// selected algorithm. Two successive invocations thus don't produce the same result and
    /// it's important to carefully pair GetMIC and VerifyMIC calls on the both sides of the
    /// authenticated session.
    /// </remarks>
    public void ComputeIntegrityCheck(ReadOnlySpan<byte> message, IBufferWriter<byte> signature);

    /// <summary>
    /// Verifies the message integrity check of a given message.
    /// </summary>
    /// <param name="message">Input message for MIC calculation.</param>
    /// <param name="signature">MIC to be verified.</param>
    /// <returns>For successfully verified MIC, the method returns true.</returns>
    /// <remarks>
    /// Implements the GSSAPI VerifyMIC operation.
    ///
    /// The method modifies the internal state and may update sequence numbers depending on the
    /// selected algorithm. Two successive invocations thus don't produce the same result and
    /// it's important to carefully pair GetMIC and VerifyMIC calls on the both sides of the
    /// authenticated session.
    /// </remarks>
    public bool VerifyIntegrityCheck(ReadOnlySpan<byte> message, ReadOnlySpan<byte> signature);
}

API Usage

// Client:
var signatureBuffer = new ArrayBufferWriter<byte>();
negotiateAuthentication.ComputeIntegrityCheck(message, signatureBuffer);
// Send (message, signatureSize: signatureBuffer.WrittenBytes, signatureSpan: signatureBuffer.WrittenSpan) to server

// Server:
// Reconstruct (message, signatureSize, signatureSpan) from the wire format
if (!negotiateAuthentication.VerifyIntegrityCheck(message, signatureSpan))
    throw new FormatException("The message has been altered");
// Process the message

Alternative Designs

No response

Risks

No response

@filipnavara filipnavara added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 31, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 31, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 31, 2023
@filipnavara filipnavara added area-System.Net.Security and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 31, 2023
@ghost
Copy link

ghost commented May 31, 2023

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

Issue Details

Background and motivation

The NegotiateAuthentication API (#69920) was added in .NET 7. It exposes functionality for Negotiate, NTLM and Kerberos authentication. It's implemented as a wrapper over GSSAPI (Unix), SSPI (Windows), and there's managed NTLM implementation for other platforms (Android, tvOS) where native API is not available. The initial API shape was driven by the goal of removing reflection access to internal NTAuthentication class, with focus on meeting the needs of email protocols (SASL in context of IMAP, SMTP), SQL protocols (SQL Server and PostgreSQL), client-side HTTP, and server-side HTTP in ASP.NET.

One omission in the original proposal and its later updates are the GetMIC and VerifyMIC operations defined in GSSAPI (https://datatracker.ietf.org/doc/html/rfc2743). These APIs are not used in any of the scenarios above, but they are needed for more specialized protocols, like the NegotiateStream. It is thus desirable to expose them for public use as well.

The operations implemented by the API are generating a "message integrity code" on one side of the communication protocol and verifying it on the other side. It is usually implemented as cryptographic signing operation with a negotiated key.

API Proposal

namespace System.Net.Security;

public sealed class NegotiateAuthentication : IDisposable
{
        /// <summary>
        /// Computes the message integrity check of a given message.
        /// </summary>
        /// <param name="message">Input message for MIC calculation.</param>
        /// <param name="signature">Buffer writer where the MIC is written.</param>
        /// <remarks>
        /// The method modifies the internal state and may update sequence numbers depending on the
        /// selected algorithm. Two successive invocations thus don't produce the same result and
        /// it's important to carefully pair GetMIC and VerifyMIC calls on the both sides of the
        /// authenticated session.
        /// </remarks>
        internal void GetMIC(ReadOnlySpan<byte> message, IBufferWriter<byte> signature);

        /// <summary>
        /// Verifies the message integrity check of a given message.
        /// </summary>
        /// <param name="message">Input message for MIC calculation.</param>
        /// <param name="signature">MIC to be verified.</param>
        /// <returns>For successfully verified MIC, the method returns true.</returns>
        /// <remarks>
        /// The method modifies the internal state and may update sequence numbers depending on the
        /// selected algorithm. Two successive invocations thus don't produce the same result and
        /// it's important to carefully pair GetMIC and VerifyMIC calls on the both sides of the
        /// authenticated session.
        /// </remarks>
        internal bool VerifyMIC(ReadOnlySpan<byte> message, ReadOnlySpan<byte> signature);
}

API Usage

TBD

Alternative Designs

No response

Risks

No response

Author: filipnavara
Assignees: -
Labels:

api-suggestion, area-System.Net.Security, untriaged

Milestone: -

@filipnavara
Copy link
Member Author

Related: #86948 /cc @wfurt

@wfurt
Copy link
Member

wfurt commented May 31, 2023

I assume we have internal implementation already for NegotiateStream @filipnavara? e.g. the main focus is internal cleanup?

@filipnavara
Copy link
Member Author

I assume we have internal implementation already for NegotiateStream @filipnavara?

We have it on Unix and managed, it is somewhat trivial on Windows and introduced in #86948.

e.g. the main focus is internal cleanup?

Correct. It's primarily to expose everything through testable public APIs on NegotiateAuthentication instead of compiling the whole implementation again into the test assemblies.

@vcsjones
Copy link
Member

  • Writing to an IBufferWriter verifying with a Span seems like a somewhat odd symmetry. Should GetMIC write to a Span, or have an overload that does?
  • Should GetMIC return an int indicating how many bytes were written to the destination?
  • Input buffers that are written to are typically called "destination", should we do that here? Or rather, why have an exception here?

@filipnavara
Copy link
Member Author

  • Writing to an IBufferWriter verifying with a Span seems like a somewhat odd symmetry. Should GetMIC write to a Span, or have an overload that does?

You don't know the size of the signature upfront. That's a similar issue like in the existing Wrap/Unwrap methods on the same type.

  • Should GetMIC return an int indicating how many bytes were written to the destination?

I don't see the need if IBufferWriter is used since it already has the information.

  • Input buffers that are written to are typically called "destination", should we do that here? Or rather, why have an exception here?

I don't have strong opinion about naming.

@bartonjs
Copy link
Member

Acronyms/abbreviations/initialisms are generally discouraged.

ComputeIntegrityCheck and VerifyIntegrityCheck seem like more approachable names.

@filipnavara
Copy link
Member Author

ComputeIntegrityCheck and VerifyIntegrityCheck seem like more approachable names.

That would have been my second choice if I had to name it. 😅 The only reason going for MIC is to follow the GSSAPI specification and make it easier to search for it.

@filipnavara
Copy link
Member Author

I updated the names and added API usage.

@filipnavara filipnavara changed the title [API Proposal]: Add public GetMIC/VerifyMIC methods to NegotiateAuthentication [API Proposal]: Add public [Compute/Verify]IntegrityCheck methods to NegotiateAuthentication Jun 1, 2023
@SteveSyfuhs
Copy link

If you're going for completeness you should probably include overloads that do expose the QOS and sequence parameters here.

@filipnavara
Copy link
Member Author

filipnavara commented Jun 1, 2023

If you're going for completeness you should probably include overloads that do expose the QOS and sequence parameters here.

Fair point. We don't fully expose qop for Wrap/Unwrap either which is why I omitted it. Is there any SSPI provider that actually implements the sequence parameters? NTLM seems to maintain them internally and ignores the passed-in values (both on SSPI and NTLMSSP/GSSAPI).

@SteveSyfuhs
Copy link

Is there any SSPI provider that actually implements the sequence parameters? NTLM seems to maintain them internally and ignores the passed-in values (both on SSPI and NTLMSSP/GSSAPI).

Kerberos does and since machines can have arbitrary negotiate subpackages installed, we can't necessarily dictate that they should or should not support it at the SSPI level. NTLM is its own brand of stupid.

@filipnavara
Copy link
Member Author

Kerberos does and since machines can have arbitrary negotiate subpackages installed, we can't necessarily dictate that they should or should not support it at the SSPI level. NTLM is its own brand of stupid.

I'll update the proposal with an added sequence number parameter then.

@filipnavara
Copy link
Member Author

filipnavara commented Jun 2, 2023

On second thought, GSSAPI doesn't have the concept of user-supplied sequence numbers. That would make SSPI the odd one out, since both GSSAPI and the Managed NTLM would ignore the parameter.

Would it make sense to maintain the sequence numbers internally for SSPI? I realise that's a slippery slope (should Wrap/Unwrap also update them? do we start at zero, or other value since Negotiate exchange can also produce MIC internally?).
Re-reading the documentation makes me believe that passing 0 on Windows should behave the same way as GSSAPI. Is that correct?

@SteveSyfuhs
Copy link

Yes, correct. 0 would be equivalent. It looks like we only support user-supplied sequence numbers for datagram messages in Kerberos, so if GSSAPI doesn't expose this functionality, then it probably isn't necessary to expose this parameter until someone complains (unlikely). I retract my recommendation. :)

@filipnavara
Copy link
Member Author

It looks like we only support user-supplied sequence numbers for datagram messages...

I don't think we even added the constructors for datagram-based operations, and we certainly don't have any use cases for it. Let's address it if someone asks for a datagram API :-)

@wfurt
Copy link
Member

wfurt commented Jun 6, 2023

I assume this is not critical for 8.0 so I'll move to future even if the actual work seems reasonable small.

@wfurt wfurt removed the untriaged New issue has not been triaged by the area owner label Jun 6, 2023
@wfurt wfurt added this to the Future milestone Jun 6, 2023
@filipnavara
Copy link
Member Author

I assume this is not critical for 8.0

It is not critical but I am happy to implement it once it goes through API review.

@wfurt wfurt modified the milestones: Future, 9.0.0 Nov 13, 2023
@rzikm rzikm added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Dec 5, 2023
@bartonjs
Copy link
Member

bartonjs commented Jan 9, 2024

Video

  • ComputeIntegrityCheck's signature parameter was renamed to signatureWriter to be consistent with how the IBufferWriter parameters are named in Wrap and Unwrap.
namespace System.Net.Security;

public sealed class NegotiateAuthentication : IDisposable
{
    /// <summary>
    /// Computes the message integrity check of a given message.
    /// </summary>
    /// <param name="message">Input message for MIC calculation.</param>
    /// <param name="signatureWriter">Buffer writer where the MIC is written.</param>
    /// <remarks>
    /// Implements the GSSAPI GetMIC operation.
    ///
    /// The method modifies the internal state and may update sequence numbers depending on the
    /// selected algorithm. Two successive invocations thus don't produce the same result and
    /// it's important to carefully pair GetMIC and VerifyMIC calls on the both sides of the
    /// authenticated session.
    /// </remarks>
    public void ComputeIntegrityCheck(ReadOnlySpan<byte> message, IBufferWriter<byte> signatureWriter);

    /// <summary>
    /// Verifies the message integrity check of a given message.
    /// </summary>
    /// <param name="message">Input message for MIC calculation.</param>
    /// <param name="signature">MIC to be verified.</param>
    /// <returns>For successfully verified MIC, the method returns true.</returns>
    /// <remarks>
    /// Implements the GSSAPI VerifyMIC operation.
    ///
    /// The method modifies the internal state and may update sequence numbers depending on the
    /// selected algorithm. Two successive invocations thus don't produce the same result and
    /// it's important to carefully pair GetMIC and VerifyMIC calls on the both sides of the
    /// authenticated session.
    /// </remarks>
    public bool VerifyIntegrityCheck(ReadOnlySpan<byte> message, ReadOnlySpan<byte> signature);
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jan 9, 2024
@filipnavara filipnavara self-assigned this Jan 9, 2024
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 9, 2024
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 10, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Feb 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants