Consider changing or adding to the MessagePackProtocol options

[BrennanConroy]

In MessagePack-CSharp v2 there is a MessagePackSerializerOptions class that can be passed to the serializer. Currently we expose a list of IFormatterResolvers for the user to configure, but there are other options like compression and IMessagePackFormatter that we don't expose.

Options: https://github.com/neuecc/MessagePack-CSharp/blob/8eeb7b8ab71ff7d70f8b2013589423eadbba90b7/src/MessagePack.UnityClient/Assets/Scripts/MessagePack/MessagePackSerializerOptions.cs#L15

Discussion: #18133 (comment)

[tebeco]

hello @BrennanConroy
Sorry to beother you about it again, i remember you wrote a comment ... somewhere ... containing the ApiReview + link to the UnitTest validation custom MessagePack Options/Formater/Resolvers

i can't seem to find your comment here or in my mail or on aspnet/aspnetcore ...
As I'm still not sure if i have the background to submit an idea of these new "options", i supposed that having the equivalent of this "lost message" would be nice here

I suppose it was pointing on this :

aspnetcore/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs

Lines 2363 to 2400 in 7def102

	[Fact]
	public async Task HubOptionsCanUseCustomMessagePackSettings()
	{
	using (StartVerifiableLog())
	{
	var serviceProvider = HubConnectionHandlerTestUtils.CreateServiceProvider(services =>
	{
	services.AddSignalR()
	.AddMessagePackProtocol(options =>
	{
	options.FormatterResolvers.Insert(0, new CustomFormatter());
	});
	}, LoggerFactory);
	var connectionHandler = serviceProvider.GetService<HubConnectionHandler<MethodHub>>();
	var msgPackOptions = serviceProvider.GetRequiredService<IOptions<MessagePackHubProtocolOptions>>();
	using (var client = new TestClient(protocol: new MessagePackHubProtocol(msgPackOptions)))
	{
	client.SupportedFormats = TransferFormat.Binary;
	var connectionHandlerTask = await client.ConnectAsync(connectionHandler);
	await client.Connected.OrTimeout();
	await client.SendInvocationAsync(nameof(MethodHub.BroadcastItem)).OrTimeout();
	var message = Assert.IsType<InvocationMessage>(await client.ReadAsync().OrTimeout());
	var result = message.Arguments[0] as Dictionary<object, object>;
	Assert.Equal("formattedString", result["Message"]);
	Assert.Equal("formattedString", result["paramName"]);
	client.Dispose();
	await connectionHandlerTask.OrTimeout();
	}
	}
	}

[tebeco]

So after trying to understand what could be the various ideas, i'm not sure what/how to change ^^

If i describe what i understand from the "code flow" :

• The current MessagePackHubProtocolOptions class from SignalR that "kinda wrap" the message pack one expose an IList<IFormatterResolver>
• MessagePackOptions exposes only a SINGLE IFormatResovler

This transition from "Multiple" to a "Single" is done in a call to SetupResolver from the MessagePackHubProtocol constructor,

• It checks if the consumer changed the default one (either by count or ref check)
• then uses static method from MessagePack : MessagePackSerializerOptions.Standard.WithResolver(resolver)

The Cache<T> is kinda like "smart Dictionary<Type, Resolver> leveraging on the fact that static ctor being threadsafe i guess ?

Questions :

• Why does SignalR exposes an IList<>, since at the end it results in a single FormatterResolver ?
  I suppose that if it's designed like this there's a good reason for that, i'm curious here ^^

• It "could" directly resolves an IOptions<MessagePackSerializerOptions>
  It would look like the same idea behind JsonOption for Mvc => 1 Formater, and you just need to properly .Configure it as a consumer ?

There's probably other things / way i did not though about ^^

[tebeco]

would love @AArnott point of view here, as you probably have a way better insight than me here

[AArnott]

Exposing MessagePackSerializerOptions will give the app owner more control. They could turn on compression, fiddle with security settings, etc.

Does aspnetcore require any resolvers to be included in the list, for its own types? If not, then exposing MessagePackSerializerOptions to the user and using their exact instance internally makes sense, IMO.
But if aspnetcore needs to ensure that some of its own types can be serialized via some particular resolver, it gets more complicated. You can check out our MessagePackFormatter class from StreamJsonRpc as an example where we let the user supply their own MessagePackSerializerOptions instance (see the SetMessagePackSerializerOptions method), but we then alter it slightly in order to 'wrap' the user-supplied resolvers with our own. We take very special care to only use the user's selected resolvers for user data, and only our own resolvers for our types that tend to 'wrap' that user data.

[tebeco]

I forgot to mention that by default the resolvers are :

            public static readonly IList<IFormatterResolver> Resolvers = new IFormatterResolver[]
            {
                DynamicEnumAsStringResolver.Instance,
                ContractlessStandardResolver.Instance,
            };

[BrennanConroy]

But if aspnetcore needs to ensure that some of its own types can be serialized via some particular resolver

We don't let users customize how the signalr framing is handled, the options are only for their types. So we don't need to set anything on top of the custom settings they provide.

then exposing MessagePackSerializerOptions to the user and using their exact instance internally makes sense

Yes, that would be the ideal situation. It is slightly inconvenient that we currently provide a list as our options, but maybe we can mark it obsolete and provide the new options type.

[AArnott]

We don't let users customize how the signalr framing is handled

@BrennanConroy Does your code ever call into the MessagePackSerializer, or are you always working directly with MessagePackWriter and MessagePackReader?
How do customers get their types serialized? Do they (or you) call directly into the resolver/formatter to serialize them or do customer types go through MessagePackSerializer?

I'm curious about how signalr framing relates to the customer types that those frames may carry, as that can impact what you do with their MessagePackSerializerOptions.

[BrennanConroy]

Customers types go through the serializer, our framing goes through the reader/writer.

Arbitrary example using json since it's readable

{
  "signalrstuff":"blah",
  "arguments": [ "this", "is", "user", "stuff" ], // they only impact what is inside this array
  "moreSignalR": "stuff"
}

[tebeco]

@BrennanConroy Does your code ever call into the MessagePackSerializer

remember this morning PR ?
that's a perfect sample (also one VERY RARE place of the code doing a direct call), it's not exposed to the client thought IIRC

(Just forget the RED THINGS, just the code)

[AArnott]

OK, sounds good.

[tebeco]

@BrennanConroy
do you mean that the ctor of MessagePackHubProtocol can be changed from :
public MessagePackHubProtocol(IOptions<MessagePackHubProtocolOptions> options)
to
public MessagePackHubProtocol(IOptions<MessagePackSerializerOptions> options) ?

==============
here is my very dumb commit, to see the impact
https://github.com/tebeco/AspNetCore/commit/56c457739c54677626fad11f2821f3c101d8e04c

Problem 1 : No parameter less ctor => no Options

i just made an attempt and out of the box it's not possible since services.AddOptions<T> needs a parameter less ctor which is not the case for MessagePackSerializerOptions
~980 test KO

Problem 2 : Immutable Options does not seems to be Option compliant

Also even if there was a public parameter-less ctor, there's another issue, since the design of that Options look like a Fluent/Immutable api.
my attempt was to do this :

builder
    .Services.AddOptions<MessagePackSerializerOptions>()
    .Configure(options => options.WithResolver(SignalRResolver.Instance))
    .PostConfigure(options => options.WithSecurity(MessagePackSecurity.UntrustedData));

This issue here is that i supposed in fact i would have to do something like :

builder
    .Services.AddOptions<MessagePackSerializerOptions>()
    .Configure(options => options = options.WithResolver(SignalRResolver.Instance))
    .PostConfigure(options => options = options.WithSecurity(MessagePackSecurity.UntrustedData));

But that won't work since the "refType" being registered in the DI would be the one instanciated by the first call to AddOptions<T>, mean i would just have a "local ref to a refType" and have literally 0 effect on the "later to be resolved Options"

[tebeco]

@AArnott do you know any alternatives to the preview points ?
It seems to end up with a "SignalR Options wrapping MessagePack Options again" :s

[AArnott]

I'm not too familiar with ASP.NET Core or SignalR's typical patterns here. IOptions<T> in particular I don't yet understand. But if IOptions<T> puts certain constraints on T and MessagePackSerializerOptions doesn't fit, I see two options:

1. drop the IOptions<T> wrapper and just use MessagePackSerializerOptions directly
2. Use IOptions<MessagePackHubProtocolOptions> and simply change MessagePackHubProtocolOptions to have a MessagePackSerializerOptions property instead of the list of resolvers that it took before.

[tebeco]

ok so we end up with the same 2 possible designs ;)

[AArnott]

If you were hoping I'd recommend one, I appreciate the opportunity, but I just don't know your API design and what customers expect enough to feel I have a well informed opinion.

[tebeco]

i just wanted to be sure i did not missed other possible design :)

(edit : that was confusing)

[tebeco]

I think this is the definition of irony ...

(yes i did a feedback, and i'm stupid enought to have pressed Esc so i don't know where to find that stacktrace again ...)

[AArnott]

I'm not sure I see the correlation. SignalR doesn't use StreamJsonRpc, does it?

Just FYI, that is an NRE thrown from another process, and JSON-RPC is just the messenger, bringing it back in-proc and informing VS that a failure occurred somewhere else.

[tebeco]

SignalR doesn't use StreamJsonRpc

Visual Studio does use it i guess, and it crashed while opening signalr repo when i tried to work on this issue ;)
And this thread spoke about that lib as an example, this is pure coincidence ^^

[tebeco]

@BrennanConroy

As of today, the code ALWAYS does this :
_msgPackSerializerOptions = options.WithSecurity(MessagePackSecurity.UntrustedData);

So if we open MessagePackSerializerOptions open to the consumer :

• do we still enforce this ?
• should it log a Warn ? (or does MessagePack already log a warning if the serializer is used with unsecured options @AArnott ?)
• do we let the consumers fully responsible of what they do if they customize it ?

[AArnott]

No warning is produced when using the default TrustedData setting.

I assume you have customers that build web apps for intranets, or for authenticated users where the trust level is higher. There are some small perf gains to be had from using UntrustedData, and some functionality is more likely to work by default (like serializing dictionaries where the key isn't known to be collision resistant). So yes, I think you should allow customers to use TrustedData.

But to help users be on the safe path by default, what you might do is add a MessagePackHubProtocolOptions.UseUntrustedData boolean property that is set to true by default, and use options.WithSecurity(MessagePackSecurity.UntrustedData); when that's set to true. This way it's secure by default, and someone has to explicitly opt out of that protection to get to TrustedData behavior.

[tebeco]

Current design + IOptions seems to support such scenario :

Default value in MessagePackHubProtocolOptions.cs :

// pseudo equivalent code (the `setter` is public and defaulted to a secured one) :
public MessagePackSerializerOptions SerializerOptions {get;set;} = MessagePackHubProtocol.CreateDefaultMessagePackSerializerOptions();

...

internal static MessagePackSerializerOptions CreateDefaultMessagePackSerializerOptions() =>
  MessagePackSerializerOptions
    .Standard
    .WithResolver(SignalRResolver.Instance)
    .WithSecurity(MessagePackSecurity.UntrustedData); <===== secured by default

Constructor of HubProtocol.cs :

public MessagePackHubProtocol() : this(Options.Create(new MessagePackHubProtocolOptions())) {} // <=== use a the default values of the `Options`
public MessagePackHubProtocol(IOptions<MessagePackHubProtocolOptions> options) // Will look in the DI if there's a registration for `IOptions<MessagePackHubProtocolOptions>`

If a customer want to change it i suppose they would just do :

Either :

services
  .AddSignalR()
  .AddMessagePackProtocol(options =>
{
  // Trusting Data :
  options.SerializerOptions = options.SerializerOptions.WithSecurity(MessagePackSecurity.TrustedData);

  // full new options :
  options.SerializerOptions = theirOwnMessagePackSerializer;

  // combining Formatter (looks weird to my eyes)
  options.SerializerOptions = MessagePackSerializerOptions.Standard.WithResolver(CompositeResolver.Create(new CustomFormatter(), options.SerializerOptions.Resolver));
});

Or :

services
  .AddSignalR()
  .AddMessagePackProtocol()

service.Configure<MessagePackHubProtocolOptions>(options => /* Same code block as above */);
// or
service.PostConfigure<MessagePackHubProtocolOptions>(options => /* Same code block as above */);

The only "weird" thing i see is that it would basically do this :

MessagePackSerializerOptions
  .Standard
  .WithSecurity(MessagePackSecurity.UntrustedData)
  .WithSecurity(MessagePackSecurity.TrustedData);

can it have any undesirable impact on message pack side ? (the Clone + Copy Constructor look like it would only have an impact at create, so only one) but I may have missed something