Add a SecretProvider that can be used for Docker Secrets

[fgheysels]

When using Docker Swarm & Docker secrets, secrets are mounted into the docker container as files.
This KeyPerFile secret provider (which is based on the KeyPerFile configuration provider) allows easy access to those secrets.

(Maybe the secretprovider should be renamed to DockerSecrets ? I've choosen KeyPerFile since it is based on that package ... )

This is related to #149

[arcus-automation]

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

You can pull it locally via the CLI:

PM> Install-Package Arcus.Security.All -Version 20200827.0.0-PR-171 -Source https://www.myget.org/F/arcus/api/v3/index.json

[tomkerkhove]

@stijnmoreels Do you want to pick this review up?

Looks good from a first glance, only thing I'm not sure about is the naming indeed. I'm more on the DockerSecrets front as it's more straight forward for what it's built as per the docs but can also see why you'd not want to do it 🤔

[stijnmoreels]

@stijnmoreels Do you want to pick this review up?

Yes, of course, will do it first thing monday (or some today if I got the time), though if that's OK. I'll assign myself.

[stijnmoreels]

Reason why this is called inside the constructor and not in the extension? Because it allows you to make sure that the configuration provider was loaded?

[fgheysels]

It makes sure that if I instantiate an instance, the secrets are loaded as well.
It might make sense to move it outside the ctor, but then users will to call the Load method themselves. I can change that if you want

[tomkerkhove]

This is ok for me! But does this mean it will not reflect changes to the file later on?

[stijnmoreels]

The UserSecrets is different in that way. It's begin called there before the ctor. Seems like they both should have the same setup but not sure which one.
Not sure also if it should be the responsibility of the secret provider that the configuration was loaded. 🤔 If this could be checked or so... Also, with 'reloading' this should maybe looked further into.

Should we call .Reload upon ICachedSecretProvider.InvalidateSecretAsync ?

[fgheysels]

Well, the ´Load´ method is part of the KeyPerFileConfigurationProvider whereas the parameter of the ctor is a configuration source.
I could change that we're injecting a configurationprovider instead of a configuration source though.

[fgheysels]

What do you mean ? Call load when the first secret is retrieved ?

[tomkerkhove]

Yes or will it work automatically with ConfigurationProvider? Or does that depend on the IConfiguration of the user? Sorry I'm lost here I think.

[fgheysels]

Calling Load on the provider loads all files that are present in the configured directory, and creates 'secrets' from them. So, if you create the secret provider, all secrets will be loaded only once.
In contrast of the KeyVaultSecretProviderwhich requests the secret every time from KeyVault itself (when no caching is configured), this provider retrieves the secrets only once.

This can be changed offcourse, but then we should not rely on the KeyPerFile configuration provider which is now used internally, but implement this functionality ourselves. In that case, we would need to load the correct file, read and return its contents on every call to 'GetSecretAsync'.

Question is, is this necessary ? When using docker and docker secrets, the secrets are mounted as files inside the container. When the secret changes, I guess that this also means that the container needs to be restarted ?

[fgheysels]

@tomkerkhove what is your view on this ?

[tomkerkhove]

Question is, is this necessary ? When using docker and docker secrets, the secrets are mounted as files inside the container. When the secret changes, I guess that this also means that the container needs to be restarted ?

That is not the case actually, often hot-swapping of configs are done without restarting the container.

Maybe odd question, but what if we call Load now and then to refresh things?

[arcus-automation]

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

You can pull it locally via the CLI:

PM> Install-Package Arcus.Security.All -Version 20200828.0.0-PR-171 -Source https://www.myget.org/F/arcus/api/v3/index.json

[stijnmoreels]

Should we use the optional here? It was supposed to be used (I think) to return null instead of throwing, but because we use the secret store, this feature is resulting in the same result bc the secret store will eventuall throw if the secret was not found in any of the registered providers.

[fgheysels]

If optional is set to false, an exception will be thrown when the directory-path (where the secrets are located inside the container) doesn't exist.
If set to true, no exception will be thrown at that point, however, an exception will be thrown later (which says that the secret could not be found). However, if we have multiple secretstores configured, the secret might as well be found by another secret provider ....
Don't know what the best approach is here, therefore I've just taken over the functionality that exists in the 'KeyPerFile' config provider.

[tomkerkhove]

If it's just for the file then I would use false internally because otherwise there is a "setup issue", but I might be too extreme?

[stijnmoreels]

Yes, I agree with @tomkerkhove , our other providers are also throwing on 'setup' things and if we would want to have such an optional functionality, I think we should look into it on a broader level and how we can use it for every provider. So, for now, maybe set it on false and we can maybe look at it in an issue later.

[fgheysels]

I've changed and pushed this, but would like to do some additional testing.

[tomkerkhove]

No problem, let us know when you are ready!

[fgheysels]

I've changed this, but now this means that if you specify a non-existing directory, the container will fail to start. That is a good example of failing fast but I don't know if you really want that.

What if you have a program which has multiple secret providers configured. In some cases, your program might not be running in a container and needs to retrieve secrets from another location. In that case, you still need to have that directory present.

[stijnmoreels]

Thanks @fgheysels for the implementation, clearly specified docs and tests! Very clear, and it shows that you try to follow the Arcus standards. Thanks for this!

[stijnmoreels]

When my last comments are resolved, I think this is OK. Thanks for keeping up with this and the complete and correct addition! Much appriciated!

[fgheysels]

I see there are some failing tests; I can have a look at it tonight

[fgheysels]

I see there are some failing tests; I can have a look at it tonight

Should be fixed now.

[arcus-automation]

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

You can pull it locally via the CLI:

PM> Install-Package Arcus.Security.All -Version 20201015.0.0-PR-171 -Source https://www.myget.org/F/arcus/api/v3/index.json

[fgheysels]

Are there any issues left, or are we good to go ?

[stijnmoreels]

Are there any issues left, or are we good to go ?

I think not (as for my part, I guess). Let's see if @tomkerkhove has any remaining remarks.

[tomkerkhove]

LGTM, just a few remarks for the docs

[arcus-automation]

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

You can pull it locally via the CLI:

PM> Install-Package Arcus.Security.All -Version 20201016.0.0-PR-171 -Source https://www.myget.org/F/arcus/api/v3/index.json