Potential for Debug.Assert if protocols.Target isn't an allocated GC handle or Target is null? Maybe actually just a null check as throwing inside call back triggered native code is never pleasant.

I can't seem to seen native asserts for these, should there not be something like this

static_assert(PAL_X509_V_ERR_CRL_SIGNATURE_FAILURE == X509_V_ERR_CRL_SIGNATURE_FAILURE, "");

To ensure that the native and managed code never get out of sync?

This is a problem, ANSI != UTF8 and if the ALPN Protocol struct is doing UTF8 default conversion you can't round trip the data.

This code doesn't exist, you're probably looking at an older commit.

This function has similar encoding issues, we are now using ASCII encoding, but Protocols struct uses UTF8, and above we are decoding with PtrToAnsi?

This code doesn't exist, you're probably looking at an older commit.

Correct you updated it after the comment so GH marks it outdated so all good now 👍

Nope, I had initially submitted the PR with this code, this code existed in the first commit, I had then changed this code after the api-proposal to use Readonlymemory in the later commits. You must have looked at this commit wise, instead of the file-changed wise.

Why are strings now being used as the backing class? This means a conversion every time ?

This code doesn't exist, you're probably looking at an older commit.

readonly for this and the above delegates?

Search/replace Apln => Alpn

There's a lot of allocation happening in these conversion routines: enumerators, byte arrays, etc. Not to block this PR, but it'd be good subsequently to see what kind of impact that has and whether there are ways to reduce it.

Why are we allocating and copying to a new array? Is the issue that ProtocolIdSize may actually be < ProtocolId.Length? If so, how about using spans?

[MarshalAs(...)]
private byte[] _protocolId;

public Span<byte> ProtocolId => new Span<byte>(_protocolId, 0, ProtocolIdSize);

?

Yeah the size can be less than 255, changed to span.

How about doing this as a fixed buffer instead of marshaling it as a byvalarray and forcing the extra allocation?

struct instead of class? Or in current usage is it always going to be boxed anyway?

Why include AllowRenegotiation now if you're not going to implement it yet?

It is part of the approved api.

The design was to fail silently when ALPN wasn't supported, correct? E.g. return a null result and let the caller decide what to do. This avoids error-prone platform light-up detection in application code.

This situation doesn't seem to be "isn't supported", but rather "should be supported, but broke". So it seems exceptional to me.

So the error message is misleading?

Why store both representations? Is ReadOnlyMemory<byte> so hard to work with that you can't use it for Equals, GetHashCode, ToString, etc.?

Equals on readonlymemory checks if the underlying array instance is the same, that's not the equality check that's needed here.

I was asking why you can't implement Equals over top of ReadOnlyMemory<byte> rather than byte[]. I don't see why you need the byte[] field.

I'm not aware how I can do that, I have to compare the values of the underlying byte[] for which i need to get the byte[] from readonlymemory, and looking at coreclr code, toarray() is expensive than just storing the byte[] here.

Probably should be a SequenceEqual extension ReadOnlyMemory; meanwhile you can do

public bool Equals(SslApplicationProtocol other)
{
    if (_readOnlyProtocol.Equals(other._readOnlyProtocol))
        return true;

    return _readOnlyProtocol.Span.SequenceEqual(other._protocol.Span);

    /*
    // Or instead of SequenceEqual
    ReadOnlySpan<byte> protocol = _readOnlyProtocol.Span; 
    ReadOnlySpan<byte> otherProtocol = other._readOnlyProtocol.Span; 
    
    if (protocol.Length != otherProtocol.Length)
        return false;
    
    for (int i = 0; i < protocol.Length; i++)
    {
        if (protocol[i] != other._protocol[i])
            return false;
    }

    return true;
    */
}

something like

if(_readonlyProtocol.Span.SequenceEquals(other._readonlyProtocol.Span))

you could drop the readonly if you didn't need the array around as well.

thus

if(_protocol.Span.SequenceEquals(other._protocol.Span))

Both work, the bonus is you get to drop all the null checking because they are structs.....

Doc comments? Preferably with the IANA link?
https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#alpn-protocol-ids

We don't include IANA links in code. Can reference the RFC number.

@Priya91 Maybe I've missed some memo somewhere, but I don't understand why the iana.org URI isn't a thing to include in comments. That does happen to (AFAIK) be the authoritative list. The RFCs all have things like "requesting IANA to register blah blah blah"

We should include IANA information in comments. This makes the source code more understandable and maintainable.

named parameters when using constants?

Any idea why this is here? I know it was in the old version, bit it's pretty strange.

I'm not sure. @davidsh ?

I'm not sure. @davidsh ?

I don't know. The code is very old. It's trying to prevent an empty string from being used as TargetHost, so it makes one up.

missing null check

Why does this take a CancellationToken? And why does this method exist if it's private?

cancellationtoken is not required, but this method takes in options bag and all other methods call this.

There's only one caller, correct? Why not collapse this method into the one above?

It's available on newer versions, correct? Will there be a way to light that up?

It should be possible to just P/Invoke into the function; though appropriate guards would need to be put in place for detecting suitability. I'd prefer it was shimmed, but sometimes needs must.

What does it mean if RemoteCertRequired is true and IsServer is false? The assert here took care of identifying that as an invalid state.

This state will not be hit with the current implementation. Previously, these internal APIs were passing the RemoteCertRequired=false for client and RemoteCertRequired=true for server. This is in direct contradiction with external meaning of RemoteCertRequired, where for client it is always true, and server is got from RemoteCertRequired parameter on the authenticate methods. Since we don't flip these values internally anymore, it doesn't make sense to have this assert.

Oh, I get it. You've changed the client to be true for this instead of false. Really the client is N/A, and that's what the assert was guarding (that it made no sense for a client to change that value during the handshake).

This seems fine, now that I understand the change. There's a mild perf gain (ditching the &&) for having the client use false, but it's not something worth adding confusion over.

This situation doesn't seem to be "isn't supported", but rather "should be supported, but broke". So it seems exceptional to me.

Why ToArray()? Using the Span property avoids the extra allocation and extra extra copy.

protocol.Protocol.Span.CopyTo(buffer.Slice(offset));

I don't know if we have a rule for (or, hopefully not, against) it, but I always end my enums with a , so that new members add one line of diff, not two.

These functions all need to use the correct type identifiers in their signatures.

• char* is only used for string. byte[] => uint8_t*.
• int (C#) => int32_t

So they might be

// If the void*'s here can't be void* then whatever type you think they are should be encoded.  I'm guessing uint8_t.
extern "C" int32_t CryptoNative_SslSelectNextProto(void** out, uint8_t* outlen, const uint8_t* server, uint32_t server_len, const void* client, uint32_t client_len);

extern "C" int32_t CryptoNative_SslCtxSetAlpnProtos(SSL_CTX* ctx, const uint8_t* protos, uint32_t protos_len);

extern "C" void CryptoNative_SslGet0AlpnSelected(SSL* ssl, const uint8_t** protocol, uint32_t* len);

I asked this question in the OpenSSL version, but I'll ask it again: why are we removing the assert? If RemoteCertRequired can only be set when IsServer is true, then the assert is good (because we can remove the && operation). If there's a state where RemoteCertRequired can be true and IsServer is false, then what do we want that behavior to be?

As explained previously.

Is there a case where NegotiationExtension isn't ALPN that isn't considered exceptional?

If I ask SChannel for the ALPN data and it gives me back a certificate, I'm certainly upset.

But maybe this is for the ALPN vs NPN thing? If so, a comment seems warranted.

new string('a', 256) seems clearer. (Or whatever number is accurate)

lol yes

This if doesn't belong. if == is true then GetHashCode() should match. And GetHashCode() should never throw.

This if doesn't belong.

I'm not sure if this is common in CoreFX tests, but shouldn't you assert the exception message to make sure this is actually failing for the reason it is expected to fail?

Fail for what reason? That should be clearer in the test name.