Provide an easy way to disable telemetry in the OpenID Connect middleware

[kevinchalet]

A (really intrusive) form of telemetry is about to be added to OpenIdConnectMessage so that all authorization and logout requests will now expose the IdentityModel assembly version to the remote OIDC server and to the end users (which has security implications, but whatever...).

Please consider adding a DisableTelemetry option in the OIDC middleware so we can easily disable that "feature" once the Wilson PR is merged.

[Tratcher]

@blowdart?

[blowdart]

Should happen, yes.

[leastprivilege]

Opt-in. Not opt-out. 

[brentschmaltz]

@blowdart @Tratcher is there time to put in a DisableTelemetry feature before 1.1 OR do you feel as
@leastprivilege telemetry should be off by default? If so the identity layer could be off by default.

[kevinchalet]

FYI, I started an email thread with Scott Guthrie, Scott Hunter and Stuart Kwan about telemetry in general and this specific PR was mentioned. @polita kindly emailed me back and shared more info about this feature:

In the unfortunate circumstance that we find a security issue in our code, our first step will be to find a patch for it and release a new package. Our next step (or perhaps in tandem) is to alert all customers who are vulnerable. This telemetry enables us to know who to call to patch vulnerabilities. Hackers don’t need to be told who has a vulnerable package; they’ll try everyone until they find someone they can target. If we can inform customers of vulnerabilities in their apps, we can help customers respond to vulnerabilities, in many cases before the vulnerability is widely known.

I understand your concern. For this reason, I’ve created a switch that allows app owners to disable telemetry altogether if they prefer. App developers can handle RedirectToIdentityProvider and set DisableTelemetryParameters on the ProtocolMessage.

[Eilon]

@brentschmaltz @polita is the IdentityModel telemetry feature even in the 1.1 release? I thought we had final builds of it already?

[polita]

@Eilon The PR AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#531 is to put it in 5.1.1, which is what we would like to have available on the day .NET 1.1 is available. We are done with our 5.1 bits.

[Tratcher]

OK, so there's no chance for us to add APIs for this in 1.1.

[polita]

I want to clarify exactly what we've added here: on authorization request and logout URIs, if DisableTelemetryParameters is false, we add "x-client-SKU=ID_NET&x-client-ver=2.1". There's no additional info in the request. If DisableTelemetryParameters is true, we don't add anything.

Since we're making private emails public :), here's my response to @PinpointTownes:

Unfortunately, the query parameters will only work if they’re turned on, and having them opt-in likely means that people won’t know to opt-in, and we’ll miss out on the data that we could have used to protect customers. I’m skeptical that people would turn it on. But I do think people should be able to turn it off easily, so the switch makes it easy to do so.

[blowdart]

As we're in escrow now we cannot add a feature to disable this, it's simply too late. @polita what's the possibility of you punting this to 5.1.2, or 5.2, whatever comes next?

[polita]

To be clear, there's a way to turn it off. App developers can handle RedirectToIdentityProvider and set DisableTelemetryParameters on the ProtocolMessage. Do we need an additional way to turn it off before we can ship it?

[Tratcher]

Most devs do not know about or hook RedirectToIdentityProvider. Adding a first class way to turn it off on the middleware options would be a much better experience.

[kevinchalet]

Since we're making private emails public :)

Your answer was of public interest and didn't contain anything confidential so I thought it wouldn't be an issue (sorry if it was).

I agree with @Tratcher: events are for advanced flow control. I'm personally not opposed to this feature being opt-out, as long as there's a trivial way to disable it.

[polita]

Ok. We will punt it to the next release. Is it worth re-releasing Katana to add a switch in the middleware options for Katana apps as well?

[Tratcher]

We can open an issue over in Katana, but I don't know that we'd wait for it. Re-releasing Katana is a much bigger task.

[brentschmaltz]

@polita @PinpointTownes @Tratcher @blowdart We could introduce a static bool DefaultAddTelemetry property on OpenIdConnectMessage. We could then have the instance pattern of initializing to the static default. This would give users a one line control that could be used in startup.cs.

Keep in mind, Identity will update before asp.net is released.

[polita]

@brentschmaltz Can you share the line of code developers would have to write to turn off telemetry in startup.cs? I assume it's...
OpenIdConnectMessage.AddTelemetryParametersToMessages = false;
or something similar...

[brentschmaltz]

I am thinking two properties to OpenIdConnectMessage.
One static: public static bool AddTelemetryParametersToMessagesDefault {get;set;} = true;
One instance: public bool AddTelemetryParametersToMessages {get; set;} = AddTelemetryParametersToMessagesDefault;

[skwan]

We definitely need this feature to be opt-out, not opt-in. With that said I'm all for making it easy to opt-out.

From the perspective of someone running an IdP, we must have a means via telemetry of identifying and notifying relying parties that are using library versions that have a serious issue.

[polita]

@brentschmaltz I like the idea of a static. Figuring out how the two settings interact might be complicated. Should we have just the static and no instance setting? You could always still handle RedirectToIdentityProvider to set it just-in-time. I can't think of a real scenario for when you'd want to turn it on and off for messages individually, really.

[brentschmaltz]

@polita I can only think of working with multiple IDP's as a scenario for the instance and static. A switch in the asp.net runtime would allow telemetry on / off on an IDP basis.

[kevinchalet]

@skwan @polita @brentschmaltz since this is an Azure AD-specific thingy (I personally don't expect any other provider to use these non-standard parameters), why not rolling your own "Azure AD OpenID Connect" extension/middleware instead of adding this feature directly in IdentityModel?

[leastprivilege]

Last time I checked, the identitymodel repo was called: "azure-activedirectory-identitymodel-extensions-for-dotnet"

[polita]

@leastprivilege We address questions wherever they come up. :)

@PinpointTownes I'm not sure a couple parameters are a good reason to spin up a completely different middleware. Extra parameters are generally ignored by providers. Is a switch to turn it off globally not adequate?

[kevinchalet]

I'm not sure a couple parameters are a good reason to spin up a completely different middleware.

I'm not suggesting using a completely different middleware, but an extension wrapping the OIDC middleware and doing exactly what you want.

That's not the first time you're (= your team) trying to add AAD-specific features that impact the OIDC middleware (the last time, it was to integrate ADAL), so maybe it's time to have your own extension, no?

[kevinchalet]

Is a switch to turn it off globally not adequate?

Well, it's fine. But OWIN/Katana users will have to explicitly disable it using ugly statics when updating their IdentityModel version (and frankly, that sucks).

[leastprivilege]

@polita you misunderstood me, my point was that identitymodel is an MS/AAD specific implementation already (hence the name of the repo, and the org it belongs to).

You always mapped to your proprietary claim types and always had first class support for your proprietary protocol parameters (and always ignored any feedback saying you shouldn't).

I am not surprised that you are continuing on that road. I am more surprised, that other people are actually surprised. Maybe it is time to realize that.

[brockallen]

Why not just code it to enable telemetry when using Azure as the IdP, and have it disabled otherwise? I think that's what we all want.

[leastprivilege]

@PinpointTownes why don't you fork System.IdentityModel and make it right - then move on the OIDC MW. We would use it!

"Make System.IdentityModel great again" ;)

[blowdart]

Please don't even suggest a static. We don't do statics, just like we don't do Context.Current. We do options. Options are sent to instances that are used.

[kevinchalet]

@leastprivilege @brockallen

[brockallen]

Actually, he was very serious.

[leastprivilege]

Brock's right.

[brentschmaltz]

@blowdart about the static. It really is just defining the default of the instance telemetry variable on OIDCMessage. The value that is acted on is the instance variable. What is the issue.

[brentschmaltz]

@PinpointTownes @leastprivilege @brockallen Now that we have 1.1 out of the box. I would really like to get some feedback on S.IM moving forward. Maybe on the other repo, perhaps.

[kevinchalet]

@leastprivilege @brockallen to be honest, it's something I've considered... but it's a huge task, as I'd also have to fork the OpenID Connect client middleware and the JWT middleware to use the forked IM bits instead of the official ones. I'm not saying this won't happen, but I clearly don't have enough spare time to do that at this moment.

[leastprivilege]

Well - up to you. Microsoft needs competition, and the community needs choices.

You would be the right guy to maintain a JWT/OIDC library. Just sayin'. We would support it.

[polita]

We (Identity and ASP.NET) talked through the goals and design for these parameters today. We need to be able to alert vulnerable apps if ever there is a security issue in Wilson, so we feel this is an important security feature. Of utmost importance was a way for the developer to turn it off easily, so the design allows for one line of code to switch it on/off.

We have no way of knowing if a request is for AAD/ADFS just by looking at the URI, unfortunately. With sovereign clouds it gets hard, and with ADFS endpoints, it’s impossible, so the feature will not have different behavior or defaults per IdP/endpoint. However, there will be a way for a middleware to universally turn it off.

We (on the Identity team) want to ship support for these features in advance of the next ASP.NET ship vehicle, so our design allows enabling and disabling the feature on the OpenIdConnectMessage class. The new members proposed (for both 4x and 5x) are:
public static bool OpenIdConnectMessage.EnableTelemetryParametersByDefault = true;
public bool OpenIdConnectMessage.EnableTelemetryParameters //initialized to the static value at message creation time

Here's how they'll work:

• EnableTelemetryParametersByDefault will be true by default. Set this to false to turn telemetry parameters off universally
• EnableTelemetryParameters will be set to the value of EnableTelemetryParametersByDefault at message creation time
• If the developer wants to change whether parameters are sent per message, they can handle RedirectToIdentityProvider directly
• The instance value of EnableTelemetryParameters will determine if telemetry is sent

For ASP.NET Core only, @Tratcher, please add an option to OpenIdConnectAuthenticationOptions that will work just like the instance switch in ASP.NET vNext. The option will set the instance variable on messages.

I will submit PRs on https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet.

[kevinchalet]

We need to be able to alert vulnerable apps if ever there is a security issue in Wilson, so we feel this is an important security feature.

There's something I don't understand: if this feature is only about security (and doesn't have statistical purposes), why not using servicing to replace vulnerable versions like any other .NET Core/ASP.NET Core library?

Also, you didn't say why an app.UseActiveDirectoryAuthentication() extension wouldn't work in this case.

Here's what I have in mind:

public static class ActiveDirectoryApplicationBuilderExtensions
{
    public static IApplicationBuilder UseActiveDirectoryAuthentication(this IApplicationBuilder app, OpenIdConnectOptions options)
    {
        if (app == null)
        {
            throw new ArgumentNullException(nameof(app));
        }

        if (options == null)
        {
            throw new ArgumentNullException(nameof(options));
        }

        // Save the events class instance provided by the user.
        var userEvents = options.Events;

        var events = new OpenIdConnectEvents
        {
            OnAuthenticationFailed = userEvents.AuthenticationFailed,
            OnAuthorizationCodeReceived = userEvents.AuthorizationCodeReceived,
            OnMessageReceived = userEvents.MessageReceived,
            OnRemoteFailure = userEvents.RemoteFailure,
            OnRemoteSignOut = userEvents.RemoteSignOut,
            OnTicketReceived = userEvents.TicketReceived,
            OnTokenResponseReceived = userEvents.TokenResponseReceived,
            OnTokenValidated = userEvents.TokenValidated,
            OnUserInformationReceived = userEvents.UserInformationReceived
        };

        events.OnRedirectToIdentityProvider = context =>
        {
            context.ProtocolMessage.SetParameter("x-client-SKU", "ID_NET");
            context.ProtocolMessage.SetParameter("x-client-ver",
                typeof(OpenIdConnectMessage).GetTypeInfo().Assembly.GetName().Version.ToString());

            return userEvents.RedirectToIdentityProvider(context);
        };

        events.OnRedirectToIdentityProviderForSignOut = context =>
        {
            context.ProtocolMessage.SetParameter("x-client-SKU", "ID_NET");
            context.ProtocolMessage.SetParameter("x-client-ver",
                typeof(OpenIdConnectMessage).GetTypeInfo().Assembly.GetName().Version.ToString());

            return userEvents.RedirectToIdentityProviderForSignOut(context);
        };

        // Replace the user-provided events in the OpenID Connect options.
        options.Events = events;

        return app.UseOpenIdConnectAuthentication(options);
    }
}

IMHO, it would be much better than adding telemetry in the IM primitives.

[polita]

if this feature is only about security (and doesn't have statistical purposes), why not using servicing to replace vulnerable versions like any other .NET Core/ASP.NET Core library?

It's certainly something I've mulled over. I see a difference between security features and security fixes. If something is broken and presents a security hole, it's appropriate to service it. If something is presenting a new security feature, it's less clear that it's appropriate for servicing. I'm open to the conversation for servicing though. That would help more people get it faster.

In the spirit of getting as many people using the security feature as possible, we don't want developers to have to change their code to use this feature. They won't know to do it, and we might as well not build the feature. We've expressed that concern several times in this thread.

[Eilon]

Thanks @PinpointTownes !