1.0.0 preview1 feedback: allow IServiceProvider when defining policies

[ArturDorochowicz]

It's great to see the changes in 1.0.0 preview1 around supporting multiple policies. I will now be able (and actually required, because it will become incompatible) to remove my custom code that I developed on top of earlier SecurityHeaders version to support this scenario. Thank you.

I have a question and a piece of feedback.

114 "Dynamic paths for report-uri" was closed as completed as part of these changes. I'm not finding any new api related to "report-uri". I'm guessing the suggested resolution is to use IPolicySelector and build a policy per request? Is that correct?

My feedback is basically an ask to have IServiceProvider available when building the policies. The only straightforward way to do this now would be to use IPolicySelector and build policies per request. The concrete need for this is basically about configuration (well, IOptions<> actually) and using it to build policies. This is something that I had and used in my custom code. This could be achieved if CustomHeaderOptions was public and registered as IOptions.

I started porting my code to 1.0.0 preview1 and so far came up with the following hack to use the service provider, yet not build policies per request. I hope it won't be needed ;)

var securityHeaders = builder.Services.AddSecurityHeaderPolicies();
// ...
var app = builder.Build();
// now can use app.ApplicationServices as well as securityHeaders
// which holds a reference to singleton CustomHeaderOptions

[andrewlock]

Hi @ArturDorochowicz, thanks for the feedback! Yes, you're right, my thinking was that #114 would be solved by the per-request changes, I figured you would be able to grab a policy and cache/swap it out as you see fit per request.

In terms of your request, I'm torn🤔 I really don't want to make CustomHeaderOptions public, as it's an implementation detail I would love to keep the ability to evolve (and I have a personal dislike of IOptions FWIW 😉)

Out of interest, what configuration are you using to configure the headers?

[andrewlock]

I just had a little play with this, and I think there's a relatively simple solution with an additional extension method? 🤔 https://github.com/andrewlock/NetEscapades.AspNetCore.SecurityHeaders/pull/200

e.g. something like this:

var builder = WebApplication.CreateBuilder();
// create the config
builder.Services.Configure<MyOptions>(builder.Configuration);

builder.Services
    .AddSecurityHeaderPolicies((SecurityHeaderPolicyBuilder policies, IServiceProvider provider) =>
    {
        var options = provider.GetRequiredService<IOptions<MyOptions>>().Value;
        policies.AddNamedPolicy(options.Name, new HeaderPolicyCollection());
    });

// etc

What do you think? Would that solve your issues?

[ArturDorochowicz]

I really don't want to make CustomHeaderOptions public, as it's an implementation detail I would love to keep the ability to evolve

I suspected as much :)

what configuration are you using to configure the headers?

From memory, I think it's mostly for CSP connect-src, maybe frame-src or frame-ancestors. Basically the client side talks to a number of apis. The specific endpoints come from configuration.

200 - yes, it should be ok. Thank you.

[ArturDorochowicz]

An alternative that I thought about, but more complicated, was to turn SecurityHeaderPolicyBuilder into IOptions<> and make it look like this:

public class SecurityHeaderPolicyBuilder
{
  internal CustomHeaderOptions Options { get; } = new();
  // ...
}

and then have AddSecurityHeaderPolicies:

static void AddSecurityHeaderPolicies(this IServiceCollection services, Action<SecurityHeaderPolicyBuilder> configureOptions)
{
   services.Configure(configureOptions);
   // people can separately register their own IConfigureOptions<SecurityHeaderPolicyBuilder> classes
   // library code depends on IOptions<SecurityHeaderPolicyBuilder> and
   // accesses internal Options property instead of resolving CustomHeaderOptions
   // ... 
}

static void AddSecurityHeaderPolicies(this IServiceCollection services, Action<IServiceProvider, SecurityHeaderPolicyBuilder> configureOptions)
{
  // and an extension like this can be made available as well using
  // (from memory) AddOptions().Configure() internally
}

// but I don't think an extension *returning* SecurityHeaderPolicyBuilder 
// can exist anymore with this design

Anyway, your suggestion is much simpler, and avoids IOptions.

[andrewlock]

Thanks, I guess the main questions I have about your alternatives are:

• Does it make any additional scenarios possible?
• Is it simpler?

tbh I think the answer to both is probably no, but I'm not sure.

The one thing it definitely does have going for it is it's more idiomatic to use IOptions<T> in general. For example, this is pretty much how the Cors middleware works with CorsOptions. And tbh, maybe that's a good enough reason 🤔 The main downside is that it's not very discoverable to use IConfigureOptions if you don't know about that mechanism, or if you don't dig into the implementation 🤔

I'm torn 😄

[ArturDorochowicz]

Oh, I absolutely agree with your points. I just wrote it down to show that there can still be internal representation. But I'm not pushing for anything more than #200.