use swagger for api security

[justinggrant]

I recommend a few changes to the way API key is handled by WPA. Swagger has built in capability to handle API keys, so I updated the swagger to use that and protect a few specific routes. I recommend that the NLU route be protected as it contains credentials for the skills WCS space that could be used for bad intent.

I also recommend that the API_KEY be stored in a environmental variable as opposed to a text file within the res folder. This will allow the skill developer to set the key how they wish. For example, they could add it to a .env file or store it in their CF or use a secret within Kubernetes.

[justinggrant]

Code below line 62 in server.js is not needed anymore too.

[erezbi]

@justinggrant Thanks for the help!
The Skills are supposed to support multi-tenancy. This means that they can have a list of keys that are allowed access. Currently your code supports only one key.

@offerakrabi - is /nlu removed in the release that removes support for NLU eval by core?

[offerakrabi]

@erezbi
You can, but this could also be used by the developer...we need to think if we want to remove it.

[justinggrant]

@erezbi - understood. that would be a fairly easy change to make and just store keys in JSON or a comma separated list. Is there a preferred format?

[erezbi]

@justinggrant I would go with the simplest. it just needs to be a list of keys, that's all.
Also, note that this branch has conflicts, and that we'll need to test this with our end to end tests to verify this, and this might take time.

We would eventually like to support adding a key to the list, but this has an even more complicated authentication story. Such an endpoint will have to be only accessible to the skill maintainer/admin, and not to any customer that is using the skill and has a key to the other endpoints.

[justinggrant]

@erezbi - I made a small change to support either a single key or a comma separated list of keys. Both are kept in the environmental variable API_KEY.

[erezbi]

@justinggrant thanks!
We'll try to take this sooner, rather than later.
I'll need to get someone in my team to work on this to make sure that it is getting as input the same input file that can currently be used, otherwise this is a breaking change. We can enable new ways of defining keys, and we are requested by customers to do this, but we shouldn't break old interface without deprecation period.
Also, we'll need to put effort into testing this. I'll prioritize accordingly.
Again, thanks for you help, you are always welcome to help with more tasks needed for accepting this fix, or with any other fix!

[justinggrant]

@erezbi - any progress on this update? thanks.

[erezbi]

@justinggrant sorry, nothing yet, we need to do quite some work to get this into master. This is waiting currently for higher priority issues.