FEAT: add certificate based authentication filter & attribute

[stijnmoreels]

One requires a easy way to allow only a certain amount of client certificates on global and local level. This commit provides this way by using a global filter and a local attribute.

#31
#39

[arcus-automation]

A new preview package for Arcus.WebApi.All is available on MyGet.

You can pull it locally via the CLI:

PM> Install-Package Arcus.WebApi.All -Version 20190612.0.0-PR-43 -Source https://www.myget.org/F/arcus/api/v3/index.json

[tomkerkhove]

I've had a quick look and looks very good! Before I drill deeper I'd like to see some test for the global enforcement and see how that feels from a consumer side.

[stijnmoreels]

I've had a quick look and looks very good! Before I drill deeper I'd like to see some test for the global enforcement and see how that feels from a consumer side.

Yes, of course.
Thanks!

[arcus-automation]

A new preview package for Arcus.WebApi.All is available on MyGet.

You can pull it locally via the CLI:

PM> Install-Package Arcus.WebApi.All -Version 20190613.0.0-PR-43 -Source https://www.myget.org/F/arcus/api/v3/index.json

[tomkerkhove]

Looking very good, I have some restructuring suggestions and a couple of doubts that I'd like to address. If you want to discuss this in person, just let me know.

[tomkerkhove]

I have also noticed that our ExceptionHandlingMiddleware is using the following approach to enable it:

public void Configure(IApplicationBuilder app, IHostingEnvironment env)
{
   app.UseMiddleware<Arcus.WebApi.Logging.ExceptionHandlingMiddleware>();

   ...
   app.UseMvc();
}

While the SharedAccessKeyAuthenticationFilter & this approach is the following:

public void ConfigureServices(IServiceCollections services)
{
    services.AddScoped<ICachedSecretProvider>(serviceProvider => new MyCachedSecretProvider());
    services.AddMvc(options => options.Filters.Add(new SharedAccessKeyAuthenticationFilter(headerName: "http-request-header-name", secretName: "shared-access-key-name")));
}

Is the latter approach different given the authentication nature of it or what was the reason again? Please refresh my memory 😄

/cc @fgheysels

[fgheysels]

I have also noticed that our ExceptionHandlingMiddleware is using the following approach to enable it:

Is the latter approach different given the authentication nature of it or what was the reason again? Please refresh my memory 😄

/cc @fgheysels

There is a difference, and you can see it in the code already:
The filters operate within the MVC context whereas the middleware operates before the MVC thing kicks in.
The ExceptionHandling functionality has been implemented as middleware since it doesn't need to access routing data etc...

I think the Authentication functionality has been implemented as a filter because you might want to apply it on a specific set of operations or exclude certain operations. Middleware will be executed on each request since it is registered in the asp.net pipeline. The 'Filters' will only be executed on operations that are decorated with that attribute.
/correct-me-if-im-wrong :P

[tomkerkhove]

That sound pretty clear, thanks for the update!

[arcus-automation]

A new preview package for Arcus.WebApi.All is available on MyGet.

You can pull it locally via the CLI:

PM> Install-Package Arcus.WebApi.All -Version 20190619.0.0-PR-43 -Source https://www.myget.org/F/arcus/api/v3/index.json

[stijnmoreels]

Question @tomkerkhove : I worked locally on this PR-branch to pass keys to the filter/attribute instead of the actual value but I'm not sure if the configuration should be an IConfiguration instance from ASP.NET or our own ISecretProvder?

[arcus-automation]

A new preview package for Arcus.WebApi.All is available on MyGet.

You can pull it locally via the CLI:

PM> Install-Package Arcus.WebApi.All -Version 20190620.0.0-PR-43 -Source https://www.myget.org/F/arcus/api/v3/index.json

[tomkerkhove]

What is this docs/.ionide/symbolCache.db file? Can we gitignore it please?

Woops, my bad.

[arcus-automation]

A new preview package for Arcus.WebApi.All is available on MyGet.

You can pull it locally via the CLI:

PM> Install-Package Arcus.WebApi.All -Version 20190704.0.0-PR-43 -Source https://www.myget.org/F/arcus/api/v3/index.json

[arcus-automation]

A new preview package for Arcus.WebApi.All is available on MyGet.

You can pull it locally via the CLI:

PM> Install-Package Arcus.WebApi.All -Version 20190708.0.0-PR-43 -Source https://www.myget.org/F/arcus/api/v3/index.json

[tomkerkhove]

I've added the final remarks and once these are resolved we are good to go, I think!

Thanks for all the effort and certainly for the nice docs!

[tomkerkhove]

One more thing - In terms of builder-pattern, the object is only created after the first call or at the end of it.

Currently we use the following approach:

new CertificateAuthenticationConfig()
     .WithSubject(X509ValidationLocation.SecretProvider, subjectKey)
     .WithIssuer(X509ValidationLocation.SecretProvider, issuerKey)

I'd suggest to go to a similar approach to align with the usual pattern:

CertificateAuthenticationConfigBuilder()
     .WithSubject(X509ValidationLocation.SecretProvider, subjectKey)
     .WithIssuer(X509ValidationLocation.SecretProvider, issuerKey)
     .Build() // Create CertificateAuthenticationConfig here

Thoughts?

[stijnmoreels]

One more thing - In terms of builder-pattern, the object is only created after the first call or at the end of it.

Currently we use the following approach:

new CertificateAuthenticationConfig()
     .WithSubject(X509ValidationLocation.SecretProvider, subjectKey)
     .WithIssuer(X509ValidationLocation.SecretProvider, issuerKey)

I'd suggest to go to a similar approach to align with the usual pattern:

CertificateAuthenticationConfigBuilder()
     .WithSubject(X509ValidationLocation.SecretProvider, subjectKey)
     .WithIssuer(X509ValidationLocation.SecretProvider, issuerKey)
     .Build() // Create CertificateAuthenticationConfig here

Thoughts?

It would be more in line with the syntax of the 'textbook' builder-pattern; but since the .Build() would only pass allong the created config dictionary to another class; I don't think there is any 'building logic' left.

Mayybe it's me, but I always try to come-up with reasonable creation approaches with sometimes the use of patterns but not litterly duplication of patterns.

If you say that we should also here use the 'basic-approach'; than OK, I'll change this.

[tomkerkhove]

The reason I'd prefer a builder here is that we can create a new instance which will not contain any configuration, which is ok, but that might be done by accident.

For me this would be more explicit:

var config = CertificateAuthenticationConfigBuilder()
               .WithSubject(X509ValidationLocation.SecretProvider, subjectKey)
               .WithIssuer(X509ValidationLocation.SecretProvider, issuerKey)
               .Build();

Or to create an empty one:

var config = CertificateAuthenticationConfigBuilder()
               .Build();

Anyhow, don't have a strong opinion, just figured it would align with the other frameworks in .NET.

Pulling in @fgheysels to get quorum on this.

[arcus-automation]

A new preview package for Arcus.WebApi.All is available on MyGet.

You can pull it locally via the CLI:

PM> Install-Package Arcus.WebApi.All -Version 20190709.0.0-PR-43 -Source https://www.myget.org/F/arcus/api/v3/index.json

[tomkerkhove]

Thank you very much @stijnmoreels 🙌

[fgheysels]

The reason I'd prefer a builder here is that we can create a new instance which will not contain any configuration, which is ok, but that might be done by accident.

For me this would be more explicit:

var config = CertificateAuthenticationConfigBuilder()
               .WithSubject(X509ValidationLocation.SecretProvider, subjectKey)
               .WithIssuer(X509ValidationLocation.SecretProvider, issuerKey)
               .Build();

Or to create an empty one:

var config = CertificateAuthenticationConfigBuilder()
               .Build();

Anyhow, don't have a strong opinion, just figured it would align with the other frameworks in .NET.

Pulling in @fgheysels to get quorum on this.

I would agree with Tom's opinion, since it's an explicit way of creating an object, and the object is only created when you call Build.

With the approach that is currently in the PR:

new CertificateAuthenticationConfig()
     .WithSubject(X509ValidationLocation.SecretProvider, subjectKey)
     .WithIssuer(X509ValidationLocation.SecretProvider, issuerKey)

you're creating an object and changing it's state, and you allow others to change its state as well, whereas with the explicit builder pattern, you can make the CertificateAuthenticationConfig class immutable I think.

edit: ow, seems like I was too late to the party :)

[tomkerkhove]

Agreed! We've switched to a builder now, only exception is that you need to create an instance of it which is ok.

So now you can build config which are fully ready. Only thing I didn't check was if the config constructor was internal.

[tomkerkhove]

It's internal, all is well!