Closed ScottChapman closed 6 years ago
You can pass a parameter:
ignore_certs. Turns off server SSL/TLS certificate verification. This allows the client to be used against local deployments of OpenWhisk with a self-signed certificate. Defaults to false.
If you’re running locally you can also set NODE_TLS_REJECT_UNAUTHORIZED
Yes, I am currently doing that in order to run my test code. In my particular case I have an action that is using this module to be able to invoke other actions.
I don't want to have to change my production action code to always ignore certificates.
If this env var was available then I could include the variable in my travis config, and when tests are run certs would be ignored. But they would be enforced when running in production on IBM Cloud
We could improve the library by detecting if apihost its an IP address then ignore ssl if it’s a hostname then do not ignore.
It’s a heuristic that would be prone to error. I use an alias “openwhisk” as my api host for my local deployment.
I’d rather have an explicit toggle (param, env var), or use the http port instead (10001).
i think scott is simply suggesting that the openwhisk npm be updated to accept an env var for ignore_certs. it accepts env vars for everything else, already:
https://github.com/apache/incubator-openwhisk-client-js/blob/master/lib/client.js#L64-L72
note the conspicuous anomaly of ignore_certs
i'd like to go further, and have the npm parse out process.env.WSK_PROPS_FILE || ~/.wskprops
Why not just set NODE_TLS_REJECT_UNAUTHORIZED
Maybe users want to be more selective.
The wsk prop file doesn’t help. We rejected the pr which records the certificate validity.
I’m just pointing out we have at least three ways to achieve the workaround today: parameter, node’s own env var, or use an http port.
i do see what scott is after. he doesn't want to have to modify his code for various setups. i certainly wouldn't feel comfortable disabling cert checking globally (for all sites). he could introduce the env var support in his code (ignore_certs: process.env.IGNORE_CERTS
), but why not just have this support in the npm directly?
going further: openwhisk already passes __OW_API_HOST, etc. why not (also) inform the code that it (openwhisk) is using insecure certs, so that the code can configure itself accordingly?
still, your suggestion to use http:10001 seems the easiest for now.
FWIW I had same problem on swift4 sdk testing and use an override baseURL action Param and use controller host and port 10001
Scott can you clarify how you’re developing and running?
Are you writing js/node code and running it with node against a local openwhisk deployment, or are you creating actions and running them using a local openwhisk deployment?
If it’s the former, your best option is to use the node tls setting or change the api host to use http://...:10001. If it’s the latter you will need to pass the ignore cert parameter or again configure the deployment to use the http port 10001.
Anything more if you’re running against a local openwhisk deployment requires backend support and changes in more than just the node runtime to support a specific toggle that disables the tls check for openwhisk.
I would rather spend the effort and automate a signed certificate or enable port 80 in the edge host and eschew the problem entirely. The rest are all hacks, some easier than others to realize.
I would rather spend the effort and automate a signed certificate or enable port 80 in the edge host and eschew the problem entirely. The rest are all hacks, some easier that others to realize.
I really like this better to deploy in Travis and configure ssl cert and local hostname But this brings two difficult problems to solve how to configure the dns for the action runtime and how to insert the certs to trust the certificate. I think is doable by pulling the runtime and then modifying the image or rebuildin with the dns and certs inside the image.
Enabling port 80 and using http on the edge can be quick solution also.
I just realized I’m an idiot I should have done this for swift4 ssl sdk problem 🤦♂️
@rabbah - I am building a "Template" for IBM Cloud Functions. Part of that is to implement automated tests that run in a travis build. Nearly all the actions in my Template leverage other actions in the same package (using this node module).
I can imagine it would be a useful feature during dev/test to be able to run against essentially an insecure openwhisk deployment.
My request is simply to make it consistent with the other constructor parameters that can be set via env vars.
I prefer explicit behavior over implicit as it is less likely to violate the rule of least astonishment.
Thanks for the clarification. So these are actions. The constructors accept the namespace and api host optionally from the environment. These are provided by the backend. So we would need back end support, part of a deployment, to enable a new environment variable to toggle the TLS verification.
The way I’ve done this in my own code is to set the ignore cert parameter in the constructor using args.ignore_cert || false.
You can modify Travis to then pass this flag through in the tests.
To enable this in the backend we’d need a deployment parameter, and modifications in the invoker which generates the environment for the action.
Yes, I've similarly implemented a work-around by altering my code, and modified my travis. I'm doing it as a package param, but still my code needs to leverage it.
I do understand that there would need to be some backend support. Sort of surprised it doesn't exist already so that actions running in a dev OW deployment can leverage actions in the same environment in a similar fashion.
But yea, I realize now it is not as simple as supporting the env var in the node code... But I can imagine this being a common use-case (dev/test against insecure OW deployment)
@rabbah
Just to be sure we want to disable tls only for whisk api but we should not disable for other external url (ie watson url, weather api url, etc)
I think having ignore_certs
as an env param is a valid suggestion. It'll be consistent with setting other properties using env params and doesn't require you to disable cert checking for all hosts using the global node env params.
This should be __OW_IGNORE_CERTS
to be consistent with the other params.
@ScottChapman would you be comfortable opening a PR for this? It should be a simple change at https://github.com/apache/incubator-openwhisk-client-js/blob/master/lib/client.js#L64-L72 along with adding this flag to the README.
@jthomas this would be ok if running locally with node. We do not know in a deployment if the certificate is trusted or not. This is the aforementioned backend support which would be required.
@rabbah - shouldn't that be defined as part of the configuration of the deployment? I would think you would have the same issue with any of the client libraries you are making available for action development. I would expect that any deployment setup and configures the defaults to be used by the clients.
@jthomas - I think you probably want someone more familiar with doing development on this package to make the change. For example you probably should have a test, and run some run on the PR.
We don’t have such a setting today - so we’d need to think about how to add it, thread it through the invoker, and into the runtimes containers.
I would think you would have the same issue with any of the client libraries you are making available for action development
Why? This is specific to openwhisk actions making an openwhisk api call. Any other client issue would be beyond scope.
This is specific to openwhisk actions making an openwhisk api call.
@rabbah - right, exactly; that's what all the client libraries do. They shouldn't require any customizations to be able to run in a deployment. They should be picking that up from the deployment environment itself. I understand that what you are saying is that this isn't what is happening today. I am just saying as a developer that's how I would expect it to work. IMHO...
When running test code for openwhisk actions it is often necessary to ignore certificates since it is running openwhisk in travis. It looks like the other constructor parameters are already handled this way.