Re: change the default action context to omit api key
We may need such a feature flag so created a PR 4334 for it. Chetan Mehrotra On Wed, Mar 6, 2019 at 10:47 PM Rodric Rabbah wrote: > There is a new comment on the issue related to this patch [1] which > suggests a deployment time parameter for enabling the feature that > restricts whisk API keys being available to all actions. I looked at the PR > and think it can be done with some changes in the controller and the tests. > We already have many deployment parameters and a pattern to replicate. > > It is worth adding this configuration parameter? I'd expect we'd remove it > at some point since the more-secure-by-default makes more sense longer > term. > > -r > > [1] > > https://github.com/apache/incubator-openwhisk/issues/4226#issuecomment-470112596 >
Re: change the default action context to omit api key
There is a new comment on the issue related to this patch [1] which suggests a deployment time parameter for enabling the feature that restricts whisk API keys being available to all actions. I looked at the PR and think it can be done with some changes in the controller and the tests. We already have many deployment parameters and a pattern to replicate. It is worth adding this configuration parameter? I'd expect we'd remove it at some point since the more-secure-by-default makes more sense longer term. -r [1] https://github.com/apache/incubator-openwhisk/issues/4226#issuecomment-470112596
Re: change the default action context to omit api key
I have addressed all the feedback to date for this pull request: https://github.com/apache/incubator-openwhisk/pull/4284 As a reminder, this patch adds an annotation such that its presence with an appropriate value will cause the API key to be added to the activation context. The absence of the annotation (on pre-existing actions) grand-fathers those actions into the old behavior, and they will receive the API key on activation. Are there any additional comments or suggestions for improvement? -r
Re: change the default action context to omit api key
> Existing demo apps which a new user might deploy and uses the SDK > won't work. Should we think about client side tooling to help? For js functions and zip files we can scan for occurrence of require(openwhisk) for example and generate a warning if the annotation is missing. This can lead to false positives when done with just a regex match but likely good enough. And of course it won’t capture all use cases where the api key is needed or work for all runtimes. -r
Re: change the default action context to omit api key
This makes sense from a security POV. Given the potential for breaking user applications[1] - we should try to document this as widely as possible. It could probably do with a blog post. I've opened an issue to add this to the JS SDK itself - https://github.com/apache/incubator-openwhisk-client-js/issues/146 [1] - Existing demo apps which a new user might deploy and uses the SDK won't work. On Tue, 19 Feb 2019 at 15:24, Rodric Rabbah wrote: > Thanks to all the input here and on the PR - I think we ended up somewhere > positive. Here's a summary: > > 1. for pre-existing actions that are already deployed, they're > grandfathered in and will continue to behave in a way where they receive > the api key on activation. This is done by detecting the absence of the new > annotation. > 2. the annotation is added on newly created actions only. > 3. on update of pre-existing actions, the annotation is not added. > > The latest code which now passes all the previous and tests (for backward > compatibility is here): > https://github.com/apache/incubator-openwhisk/pull/4284 > > -r > > > On Thu, Feb 14, 2019 at 9:37 PM Rodric Rabbah wrote: > > > I've implemented changes to the PR > > https://github.com/apache/incubator-openwhisk/pull/4284 for backward > > compatibility --- such that, actions which do not have the annotation > will > > still get the api key injected. > > > > The annotation is added by the controller when an action is created or > > updated unless already present. The default value for the annotation is > > "false", meaning no key is injected to the action context. > > > > Furthermore comments and feedback is appreciated. > > > > -r > > > -- Regards, James Thomas
Re: change the default action context to omit api key
Thanks to all the input here and on the PR - I think we ended up somewhere positive. Here's a summary: 1. for pre-existing actions that are already deployed, they're grandfathered in and will continue to behave in a way where they receive the api key on activation. This is done by detecting the absence of the new annotation. 2. the annotation is added on newly created actions only. 3. on update of pre-existing actions, the annotation is not added. The latest code which now passes all the previous and tests (for backward compatibility is here): https://github.com/apache/incubator-openwhisk/pull/4284 -r On Thu, Feb 14, 2019 at 9:37 PM Rodric Rabbah wrote: > I've implemented changes to the PR > https://github.com/apache/incubator-openwhisk/pull/4284 for backward > compatibility --- such that, actions which do not have the annotation will > still get the api key injected. > > The annotation is added by the controller when an action is created or > updated unless already present. The default value for the annotation is > "false", meaning no key is injected to the action context. > > Furthermore comments and feedback is appreciated. > > -r >
Re: change the default action context to omit api key
I've implemented changes to the PR https://github.com/apache/incubator-openwhisk/pull/4284 for backward compatibility --- such that, actions which do not have the annotation will still get the api key injected. The annotation is added by the controller when an action is created or updated unless already present. The default value for the annotation is "false", meaning no key is injected to the action context. Furthermore comments and feedback is appreciated. -r
Re: change the default action context to omit api key
If we require newly created (or updated) actions to be annotated then we also need a new composer release to produce new compositions. If we require the annotation for existing actions as well then every composition that uses async today (or parallel soon) will need to be updated (wsk action update -a ...) or redeployed. We already have async users today in prod. Olivier Rodric Rabbah wrote on 02/14/2019 10:26:08 AM: > From: Rodric Rabbah > To: dev@openwhisk.apache.org > Date: 02/14/2019 10:35 AM > Subject: Re: change the default action context to omit api key > > > If we end up making a backward incompatible change, we need to stage it: > > if we do it in a way that's backward compatible for existing actions, does > that alleviate the problem for composer? > > -r
Re: change the default action context to omit api key
> If we end up making a backward incompatible change, we need to stage it: if we do it in a way that's backward compatible for existing actions, does that alleviate the problem for composer? -r
Re: change the default action context to omit api key
Composer relies on the api key to implement asynchronous and parallel invocations. Omitting the api key by default would imply that a new composer release is required whenever the change is made. If we end up making a backward incompatible change, we need to stage it: - stage 1: fix the specification the new annotation - stage 2: rerelease composer (and give ample time to others to adjust as well) - stage 3: push the behavior change. Olivier
Re: change the default action context to omit api key
> Probably the ideal would be to replace the API key with a key with an expiry time, that can be used only within the lifespan of the action to invoke other actions. Yes - a short term key with specific rights (read, write or execute) would be superior to the current approach, for sure. -r
Re: change the default action context to omit api key
Markus posted a suggestion to the PR to make the change backward compatible:
1. treat a missing annotation as truthy (key is injected)
2. on new action create or action update, unless the annotation is already
present, add the new annotation with a false value
This would leave existing actions in a working state.
But actions that are updated must specify the parameter at deployment time.
This is perhaps OK since if you're updating the action, you're doing a new
deployment and can update your configuration.
-r
On Thu, Feb 14, 2019 at 4:33 AM Dominic Kim wrote:
> Regarding OpenWhisk SDK, do we have any way to selectively include API Key
> if an action uses the OW SDK?
>
> I think it is a useful feature to be able to omit explicit API key
> configuration if the SDK is used in the context of OpenWhisk and it is
> already widely used in my company.
>
> Is there any way to keep the backward compatibility?
>
> Best regards
> Dominic
>
> 2019년 2월 14일 (목) 오후 5:48, Michele Sciabarra 님이 작성:
>
> > My concern is that if you do not pass the API key, all the actions that
> > invoke other actions must be marked explictly as requiring another API
> key.
> > From one side I understand the fact the security risk that an action can
> be
> > fooled to leak the authorization key, from the other side I think actions
> > should still be able to invoke other actions without being marked to do
> > that.
> >
> > Probably the ideal would be to replace the API key with a key with an
> > expiry time, that can be used only within the lifespan of the action to
> > invoke other actions.
> >
> > --
> > Michele Sciabarra
> > mich...@sciabarra.com
> >
> > - Original message -
> > From: Rodric Rabbah
> > To: dev@openwhisk.apache.org
> > Subject: change the default action context to omit api key
> > Date: Wed, 13 Feb 2019 16:08:48 -0500
> >
> > Hi,
> >
> > I'm looking for feedback on the following issue:
> > https://github.com/apache/incubator-openwhisk/issues/4226
> >
> > Actions receives the API key in the environment even if it is not
> > necessary. This should not be the default behavior. With the issue I'm
> > proposing that we flip the default and provide an annotation on the
> action
> > to enable the key forwarding to preserve existing behavior.
> >
> > Additionally We currently created the following context:
> > {
> >"api_host": process.env['__OW_API_HOST'],
> >"api_key": process.env['__OW_API_KEY'],
> >"namespace": process.env['__OW_NAMESPACE'],
> >"action_name": process.env['__OW_ACTION_NAME'],
> >"activation_id": process.env['__OW_ACTIVATION_ID'],
> >"deadline": process.env['__OW_DEADLINE']
> > }
> >
> >
> >
> https://github.com/apache/incubator-openwhisk/blob/da21c9fe49b2ae72c95b6866b30d984c65253724/core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerProxy.scala#L565-L571
> >
> > Should we hide the namespace, action name and activation id as well?
> >
> > -r
> >
>
Re: change the default action context to omit api key
Regarding OpenWhisk SDK, do we have any way to selectively include API Key
if an action uses the OW SDK?
I think it is a useful feature to be able to omit explicit API key
configuration if the SDK is used in the context of OpenWhisk and it is
already widely used in my company.
Is there any way to keep the backward compatibility?
Best regards
Dominic
2019년 2월 14일 (목) 오후 5:48, Michele Sciabarra 님이 작성:
> My concern is that if you do not pass the API key, all the actions that
> invoke other actions must be marked explictly as requiring another API key.
> From one side I understand the fact the security risk that an action can be
> fooled to leak the authorization key, from the other side I think actions
> should still be able to invoke other actions without being marked to do
> that.
>
> Probably the ideal would be to replace the API key with a key with an
> expiry time, that can be used only within the lifespan of the action to
> invoke other actions.
>
> --
> Michele Sciabarra
> mich...@sciabarra.com
>
> - Original message -
> From: Rodric Rabbah
> To: dev@openwhisk.apache.org
> Subject: change the default action context to omit api key
> Date: Wed, 13 Feb 2019 16:08:48 -0500
>
> Hi,
>
> I'm looking for feedback on the following issue:
> https://github.com/apache/incubator-openwhisk/issues/4226
>
> Actions receives the API key in the environment even if it is not
> necessary. This should not be the default behavior. With the issue I'm
> proposing that we flip the default and provide an annotation on the action
> to enable the key forwarding to preserve existing behavior.
>
> Additionally We currently created the following context:
> {
>"api_host": process.env['__OW_API_HOST'],
>"api_key": process.env['__OW_API_KEY'],
>"namespace": process.env['__OW_NAMESPACE'],
>"action_name": process.env['__OW_ACTION_NAME'],
>"activation_id": process.env['__OW_ACTIVATION_ID'],
>"deadline": process.env['__OW_DEADLINE']
> }
>
>
> https://github.com/apache/incubator-openwhisk/blob/da21c9fe49b2ae72c95b6866b30d984c65253724/core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerProxy.scala#L565-L571
>
> Should we hide the namespace, action name and activation id as well?
>
> -r
>
Re: change the default action context to omit api key
My concern is that if you do not pass the API key, all the actions that invoke
other actions must be marked explictly as requiring another API key. From one
side I understand the fact the security risk that an action can be fooled to
leak the authorization key, from the other side I think actions should still be
able to invoke other actions without being marked to do that.
Probably the ideal would be to replace the API key with a key with an expiry
time, that can be used only within the lifespan of the action to invoke other
actions.
--
Michele Sciabarra
mich...@sciabarra.com
- Original message -
From: Rodric Rabbah
To: dev@openwhisk.apache.org
Subject: change the default action context to omit api key
Date: Wed, 13 Feb 2019 16:08:48 -0500
Hi,
I'm looking for feedback on the following issue:
https://github.com/apache/incubator-openwhisk/issues/4226
Actions receives the API key in the environment even if it is not
necessary. This should not be the default behavior. With the issue I'm
proposing that we flip the default and provide an annotation on the action
to enable the key forwarding to preserve existing behavior.
Additionally We currently created the following context:
{
"api_host": process.env['__OW_API_HOST'],
"api_key": process.env['__OW_API_KEY'],
"namespace": process.env['__OW_NAMESPACE'],
"action_name": process.env['__OW_ACTION_NAME'],
"activation_id": process.env['__OW_ACTIVATION_ID'],
"deadline": process.env['__OW_DEADLINE']
}
https://github.com/apache/incubator-openwhisk/blob/da21c9fe49b2ae72c95b6866b30d984c65253724/core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerProxy.scala#L565-L571
Should we hide the namespace, action name and activation id as well?
-r
Re: change the default action context to omit api key
Hi Rodric, I agree, that the key should not be passed to the action, if it is not required. But in my opinion, existing actions should continue to work without any update. But I'm OK, if all newly created actions have the default, that they don't have an API-key. Greetings Christian Am Mi., 13. Feb. 2019 um 22:53 Uhr schrieb Rodric Rabbah : > Thanks for the quick initial feedback. > I've opened a PR that excludes just the API key. > https://github.com/apache/incubator-openwhisk/pull/4284 > > This will be a breaking change - actions that are deployed already and > which need the key will need to be updated. > I added the annotation `-a provide-api-key true`. > > I think the default should be no key but we have to address the change in > behavior. > > -r > > On Wed, Feb 13, 2019 at 4:32 PM David P Grove wrote: > > > +1 my thoughts exactly. > > > > --dave > > > > Tyson Norris wrote on 02/13/2019 04:28:35 > PM: > > > > > > I agree the api_key is bad, when not using e.g. OW npm within the > > > action. +1 for using an annotation to enable this. > > > > > > activation_id is required to do the right thing for logging with > > > concurrency enabled - but I'm also not sure what risk it is to > > > include that? It will be in the response header anyways still right? > > > > > > Namespace + action - similar to activation_id, this is already > > > available to the client and may have some convenience for action > > > devs (especially with logging concurrent actiavitons __ ) > > > > > > From my perspective, I would just change the api_key to be > > > explicitly passed, and leave the rest as-is. > > > > > >
Re: change the default action context to omit api key
Thanks for the quick initial feedback. I've opened a PR that excludes just the API key. https://github.com/apache/incubator-openwhisk/pull/4284 This will be a breaking change - actions that are deployed already and which need the key will need to be updated. I added the annotation `-a provide-api-key true`. I think the default should be no key but we have to address the change in behavior. -r On Wed, Feb 13, 2019 at 4:32 PM David P Grove wrote: > +1 my thoughts exactly. > > --dave > > Tyson Norris wrote on 02/13/2019 04:28:35 PM: > > > > I agree the api_key is bad, when not using e.g. OW npm within the > > action. +1 for using an annotation to enable this. > > > > activation_id is required to do the right thing for logging with > > concurrency enabled - but I'm also not sure what risk it is to > > include that? It will be in the response header anyways still right? > > > > Namespace + action - similar to activation_id, this is already > > available to the client and may have some convenience for action > > devs (especially with logging concurrent actiavitons __ ) > > > > From my perspective, I would just change the api_key to be > > explicitly passed, and leave the rest as-is. > > >
Re: change the default action context to omit api key
I agree the api_key is bad, when not using e.g. OW npm within the action. +1
for using an annotation to enable this.
activation_id is required to do the right thing for logging with concurrency
enabled - but I'm also not sure what risk it is to include that? It will be in
the response header anyways still right?
Namespace + action - similar to activation_id, this is already available to the
client and may have some convenience for action devs (especially with logging
concurrent actiavitons __ )
From my perspective, I would just change the api_key to be explicitly passed,
and leave the rest as-is.
Thanks
Tyson
On 2/13/19, 1:09 PM, "Rodric Rabbah" wrote:
Hi,
I'm looking for feedback on the following issue:
https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fincubator-openwhisk%2Fissues%2F4226data=02%7C01%7Ctnorris%40adobe.com%7C549eb49aa3e04739078e08d691f78c5b%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C636856889729887501sdata=hcDnwNeHsOPcrmwQ3YRC1lSGmYBvre0WIpN5lGdmqFk%3Dreserved=0
Actions receives the API key in the environment even if it is not
necessary. This should not be the default behavior. With the issue I'm
proposing that we flip the default and provide an annotation on the action
to enable the key forwarding to preserve existing behavior.
Additionally We currently created the following context:
{
"api_host": process.env['__OW_API_HOST'],
"api_key": process.env['__OW_API_KEY'],
"namespace": process.env['__OW_NAMESPACE'],
"action_name": process.env['__OW_ACTION_NAME'],
"activation_id": process.env['__OW_ACTIVATION_ID'],
"deadline": process.env['__OW_DEADLINE']
}
https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fincubator-openwhisk%2Fblob%2Fda21c9fe49b2ae72c95b6866b30d984c65253724%2Fcore%2Finvoker%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fopenwhisk%2Fcore%2Fcontainerpool%2FContainerProxy.scala%23L565-L571data=02%7C01%7Ctnorris%40adobe.com%7C549eb49aa3e04739078e08d691f78c5b%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C636856889729887501sdata=qCfYzoSy%2BpAJAAC%2FDFBX%2Fu4NDkccE96eCbRvMwkvP9E%3Dreserved=0
Should we hide the namespace, action name and activation id as well?
-r
