Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add protect feature to avoid update or delete actions by mistake #4058

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ningyougang
Copy link
Contributor

@ningyougang ningyougang commented Oct 8, 2018

As more and more production system uses openwhisk,
Users will need some features to protect their service safe from mistake.
If we have this feature, user can protect their actions from deletion by mistake.

When users create action use like below(add annotation "lock":true)

curl  -X PUT -H "Content-type: application/json"  --user 23bc46b1-71f6-4ed5-8c54-816aa4f8c502:123zO3xZCLrMN6v2BKK1dXYFpXlPkccOFqm12CdAsMgRU4VrNZ9lyGVCGuMDGIwP 
-d '{"namespace":"guest","name":"hello","annotations":[{"key":"lock","value":true}],"exec":{"kind":"nodejs:default","code":"// Licensed to the Apache Software Foundation (ASF) under one or more contributor\r\n// license agreements; and to You under the Apache License, Version 2.0.\r\n\r\n/**\r\n * Hello, world.\r\n */\r\nfunction main() {\r\n return {\"payload\":\"Hello world\"};\r\n}\r\n"}}' 
'http://xxx.xxx.xxx.xxx:port/api/v1/namespaces/guest/actions/hello?'

The lock field will be added in couchdb's annotation, like below

...
  "annotations": [
    {
      "key": "exec",
      "value": "nodejs:6"
    },
    {
      "key": "lock",
      "value": true
    }
...

So this action can't be deleted/updated until the protection is updated to false

Description

Related issue and scope

  • I opened an issue to propose and discuss this change (#????)

My changes affect the following components

  • API
  • Controller
  • Message Bus (e.g., Kafka)
  • Loadbalancer
  • Invoker
  • Intrinsic actions (e.g., sequences, conductors)
  • Data stores (e.g., CouchDB)
  • Tests
  • Deployment
  • CLI
  • General tooling
  • Documentation

Types of changes

  • Bug fix (generally a non-breaking change which closes an issue).
  • Enhancement or new feature (adds new functionality).
  • Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

  • I signed an Apache CLA.
  • I reviewed the style guides and followed the recommendations (Travis CI will check :).
  • I added tests to cover my changes.
  • My changes require further changes to the documentation.
  • I updated the documentation where necessary.

Copy link
Contributor

@cbickel cbickel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

thanks a lot for your contribution. This PR definitely makes sense.

You are adding the protection-parameter as URL parameter. What's the reason for doing it like this?
Today: Everything that is a property of the action is within WhiskActionPut. The overwrite parameter only says, that this request should overwrite the action if it already exists.
IMHO the protection parameter is a property of the action itself and not of that request.

And a second thought that comes into my mind: If we protect the action, should it only be protected against deletion? Or would it make sense to protect it against updates as well? (The trick here would be to allow the update to unlock the action again).

I don't know what your thoughts on these two issues are. If your thoughts are completely different mybe it makes sense to ask the dev-list, if someone has a strong opinion on these two questions on the dev-list.

@rabbah
Copy link
Member

rabbah commented Oct 8, 2018

This is a good start but we should discuss and settle the design first. For example my preference is for Unix style permissions on the action (or asset more generally).

@ningyougang
Copy link
Contributor Author

ningyougang commented Oct 9, 2018

@cbickel @rabbah ,thanks for your quick reply.

Actually, i opened a pr first here: apache/openwhisk-cli#372
Solve the update/delete mistake on CLI side, after think deeply, i found, it is better to solve it in backend layer.(So don't merge that pull/372 temporarily)

I did below changes

  • Add "annotations":[{"key":"lock","value":true}] to body instead of URL request param

    So we can use below method to protect the action when created or updated

curl  -X PUT -H "Content-type: application/json"  
--user 23bc46b1-71f6-4ed5-8c54-816aa4f8c502:123zO3xZCLrMN6v2BKK1dXYFpXlPkccOFqm12CdAsMgRU4VrNZ9lyGVCGuMDGIwP 
-d '{"namespace":"guest","name":"hello","annotations":[{"key":"lock","value":true}],
"exec":{"kind":"nodejs:default","code":"// Licensed to the Apache Software Foundation (ASF) under one or more contributor\r\n// license agreements; and to You under the Apache License, Version 2.0.\r\n\r\n/**\r\n * Hello, world.\r\n */\r\nfunction main() {\r\n return {\"payload\":\"Hello world\"};\r\n}\r\n"}}' 
'/api/v1/namespaces/guest/actions/hello?overwrite=true'

Or use wsk like this wsk action create hello hello.js --annotation lock true

  • Or would it make sense to protect it against updates as well?

    I agree, already added

  • The trick here would be to allow the update to unlock the action again

    Use below method(pass -d '{"unlock:"true"}'), for example

curl  -X PUT  -H "Content-type: application/json"  
--user 23bc46b1-71f6-4ed5-8c54-816aa4f8c502:123zO3xZCLrMN6v2BKK1dXYFpXlPkccOFqm12CdAsMgRU4VrNZ9lyGVCGuMDGIwP 
-d '{"unlock":"true"}'  'http://xxx.xxx.xxx.xxx:port/api/v1/namespaces/guest/actions/hello?overwrite=true'

After executed above method, this action enabled updated or deleted operation

  • Wsk side(TODO)

    If this patch is merged into upstream, the wsk side will do some changes, add unlock feature to wsk to enable update or delete operation, for example

wsk action update hello --unlock

@ningyougang ningyougang changed the title Add protect feature to avoid delete actions by mistake Add protect feature to avoid update or delete actions by mistake Oct 11, 2018
@codecov-io
Copy link

codecov-io commented Oct 11, 2018

Codecov Report

Merging #4058 into master will decrease coverage by 7.59%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #4058     +/-   ##
=========================================
- Coverage   86.21%   78.61%   -7.6%     
=========================================
  Files         152      153      +1     
  Lines        7275     7895    +620     
  Branches      477      518     +41     
=========================================
- Hits         6272     6207     -65     
- Misses       1003     1688    +685
Impacted Files Coverage Δ
...org/apache/openwhisk/core/entity/WhiskAction.scala 86.82% <100%> (+0.07%) ⬆️
...org/apache/openwhisk/core/controller/Actions.scala 93.48% <100%> (+0.31%) ⬆️
...rg/apache/openwhisk/core/controller/ApiUtils.scala 77% <100%> (+3.43%) ⬆️
...core/database/cosmosdb/RxObservableImplicits.scala 0% <0%> (-100%) ⬇️
...n/scala/org/apache/openwhisk/common/CausedBy.scala 0% <0%> (-100%) ⬇️
...core/database/cosmosdb/CosmosDBArtifactStore.scala 0% <0%> (-95.54%) ⬇️
...sk/core/database/cosmosdb/CosmosDBViewMapper.scala 0% <0%> (-92.6%) ⬇️
...whisk/core/database/cosmosdb/CosmosDBSupport.scala 0% <0%> (-83.34%) ⬇️
...abase/cosmosdb/CosmosDBArtifactStoreProvider.scala 3.44% <0%> (-59.06%) ⬇️
...penwhisk/core/database/cosmosdb/CosmosDBUtil.scala 57.5% <0%> (-38.5%) ⬇️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb02bdb...63a2aaf. Read the comment docs.

@rabbah rabbah self-requested a review November 10, 2018 20:05
@rabbah rabbah added controller review Review for this PR has been requested and yet needs to be done. labels Nov 10, 2018
@ningyougang ningyougang reopened this Dec 24, 2018
@ningyougang ningyougang closed this Jan 7, 2019
@ningyougang ningyougang reopened this Jan 7, 2019
@ningyougang ningyougang reopened this Jan 21, 2019
@ningyougang ningyougang force-pushed the add-protection-to-action branch 3 times, most recently from 546f431 to 69602d9 Compare January 24, 2019 05:34
@rabbah
Copy link
Member

rabbah commented Jan 24, 2019

Sorry for taking so long to get through this --- taking a look now it's a good first step but i think it points to a more general permission set on an action: this is an write permission for the namespace (group in unix terminology), and one can image an execute permission as well next (disables an action from executing for a namespace).

So one of my thoughts is whether to factor the permission more generally to allow for generalization later... or leave it as is and change it later.

Thoughts?

@rabbah rabbah self-assigned this Jan 24, 2019
@ningyougang
Copy link
Contributor Author

@rabbah
Regarding factor the permission more generally
it is a good suggestion, let's leave it as is and change it later.

At the same time, we can wait other upsteam guys to give more suggestions,
if we agree on the idea, go on to do this.

@mhenke1
Copy link
Contributor

mhenke1 commented Jan 28, 2019

@ningyougang
Sorry for joining late in this discussion.
In my last year I have implemented the SPIs for plugging in alternative Authentication and Entitlement providers in Openwhisk. My strong suggestion would be to place any additional code checking for entitlement (in this case: the entitlement to delete entities) into the existing entitlement SPI implementation.
Alternative entitlement implementations (like the one I have implemented for IBM cloud functions) might prevent accidental deletion in other ways like configuring user or user group specific polices
in a policy system. They have no need of additional protection.
Of course the default entitlement provider would benefit from a mechanism like the one you proposed. By moving the functionality in the Entitelemnt-SPI the functionality could be added to the default path, but would not interfere with alternative entitlement implementations.

@rabbah
Copy link
Member

rabbah commented Jan 28, 2019

@mhenke1 Are the alternatives you've implemented something that is planned for open source contribution?

docs/actions.md Outdated
@@ -598,6 +598,28 @@ You can clean up by deleting actions that you do not want to use.
actions
/guest/mySequence private sequence
```
## Protect action updated or deleted by mistake
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will suggest some updates to this text once the API change and implementation are finalized.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated accordingly

@ningyougang
Copy link
Contributor Author

ningyougang commented May 11, 2020

Hi, guys, i push a temp commit here in this patch, just check my direction(permission unix sytle) is whether right. d77a80f

After check in my local, i found that, there has some differences between our own permission management between unix permission management.

1. user type is different

for one action, just have 2 type users.

  • the action's owner
  • the user (not the owner) who used the shared action directly(eg. get, invoke)

As we all already know, unix's permission has 3 user types, this is the different point.

2. our own permission value is restricted

  • for one action, anyway, the user(not the owner) can't modify/delete the action on any situation, just the owner has the write permission.
  • the owner's permission can affect the permission as well, e.g. if the owner is not given read permission, i think the user(not the owner) can't have read permission as well. if the owner is not give execute permission, the user(not the owner) can't have execute permission as well.

How do you guys think?

@ningyougang ningyougang force-pushed the add-protection-to-action branch 2 times, most recently from cf35509 to 1c37112 Compare May 13, 2020 09:49
// 4. permission code:r-xr--: owner:read(yes)/write(no)/execute(yes)|the shared action's user:read(yes)/write(no)/execute(no)
// 5. permission code:r--r--: owner:read(yes)/write(no)/execute(no)|the shared action's user:read(yes)/write(no)/execute(no)
// 6. permission code:rw-r--: owner:read(yes)/write(yes)/execute(no)|the shared action's user:read(yes)/write(no)/execute(no)
val permissionList = List(defaultPermissions, "rwxr--", "r-xr-x", "r-xr--", "r--r--", "rw-r--")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does r means get codes here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the shared user, the r means download the code

case t: RejectRequest =>
Future.failed(t)
case _ =>
Future.successful(())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to make some test cases successfully .e.g

 ./gradlew :tests:test --tests="org.apache.openwhisk.core.controller.test.PackageActionsApiTests"
 ./gradlew :tests:test --tests="org.apache.openwhisk.core.controller.test.ActionsApiTests"

Need to add above recoverWith logic, concrete content is

@ningyougang ningyougang closed this Sep 8, 2020
@ningyougang ningyougang reopened this Sep 8, 2020
@ningyougang
Copy link
Contributor Author

Already rebased and solved relative test cases error


val defaultPermissions = "rwxr-x"

// notes on users, just have 2 type users,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather keep this information in some markdown file and just leave the link here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated accordingly.

"rwxr--",
"r-xr-x",
"r-xr--",
"r--r--",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any case where an owner does not need to either update or execute the action?
If so, how can the user update the action permission and the action itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there has an case that the owner doesn't have permission to update or execute the action, e. g. the annotation of permission for that action is: r--r--
in this case, if the owner wants to update the action codes, need to modify the action's permissions annotation to executeable, e.g. wsk -i action update $action --annotation permissions rw-r--, then, user can update their action now.

Copy link
Member

@style95 style95 May 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So even if an owner doesn't have any permission, he can still update the permission annotations?

// 10. permission code:r-x---: owner:read(yes)/write(no)/execute(yes)|the shared action's user:download(no)/write(no)/execute(no)
// 11. permission code:r-----: owner:read(yes)/write(no)/execute(no)|the shared action's user:download(no)/write(no)/execute(no)
// 12. permission code:rw----: owner:read(yes)/write(yes)/execute(no)|the shared action's user:download(no)/write(no)/execute(no)
val permissionList = List(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd apply a regex match to confirm the permission value rather than just checking the existence of the value in a list.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree

@@ -351,6 +361,49 @@ object WhiskAction extends DocumentFactory[WhiskAction] with WhiskEntityQueries[
val execFieldName = "exec"
val requireWhiskAuthHeader = "x-require-whisk-auth"

// annotation permission key name
val permissionsFieldName = "permissions"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not quite sure it is a good idea to use annotations for permission management.
I feel like we need a new protocol.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, i also feel it is not a good idea to use annotation for permission management.
Currently, have no better idea for it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can store permission information in DB and fetch it along with Identity information.
Some proper tools to manage permission are required and the same change should be applied in CLI(wsk or at least wskadmin) as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think such permission information is applied to actions instead of Identity

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant, we can fetch the permission information in a similar way with Identity.
This information can be stored in the cache too.
Permission needs to be checked before actual operations.

get(entityStore, entityName.toDocId, DocRevision.empty, true)
.flatMap { whiskAction =>
val currentPermissions = whiskAction.annotations
.get(WhiskAction.permissionsFieldName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can depend on permission data in DB rather than relying on action fields.

} else {
val passedUpdatePermission = value.charAt(1)
if (passedUpdatePermission == 'w') { // make it to modifiable
Future.successful(())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so once passed a w of action permission in params, the update will always pass priviledge checking even old update permission is -?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right. e.g.

# wsk -i action create hello ~/hello.js --annotation permissions r-xr-x
ok: created action hello
# wsk -i action update hello ~/hello.js 
error: Unable to create action 'hello': have no permission to update this action (code iAKv0FTZLaURFtLzoSzryZhLpemxDell)
# wsk -i action update hello ~/hello.js --annotation permissions rwxr-x
ok: updated action hello

entityName: FullyQualifiedEntityName,
get: (ArtifactStore[WhiskEntity], DocId, DocRevision, Boolean) => Future[WhiskAction],
permissions: Option[String] = None)(implicit transid: TransactionId): Future[Unit] = {
if (operation == "create") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think

operation match {
  case "create" =>
...

should be better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated accordingly.

.getOrElse(WhiskAction.defaultPermissions)

val errorInfo = s"have no permission to download this shared action"
val currentDownloadPermission = currentPermissions.charAt(3)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be currentPermissions.charAt(0)? since permission string is like rwx--- and the first char indicate the "read" permission

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is for the shared user, so currentPermissions.charAt(3) is right.
If the user is owner, the owner has download permission on any situation

hm...anyway, you said problem is also a problem, it is not readable. i will optimize the codes like below

          if (user.namespace.name.asString != entityName.path.root.asString) { // the shared user who download the action
            val errorInfo = s"have no permission to download this shared action"
            val downloadPermissionOfSharedUser = currentPermissions.charAt(3)
            if (downloadPermissionOfSharedUser == '-') {
              Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
            } else {
              Future.successful(())
            }
          } else {
            // the owner has download permission on any situation
            Future.successful(())
          }

@jiangpengcheng
Copy link
Contributor

one of my thought is that there is no need to add permissions controls for action's "owner" since he/she can anyway update/remove/invoke his/her action

- As more and more production system uses openwhisk,
Users will need some features to protect their service safe from mistake.
If we have this feature, user can protect their actions from updating or
deletion by mistake.
- Apply unix style permission management
- Make the action's downloadable for the shared user
@codecov-commenter
Copy link

codecov-commenter commented May 28, 2021

Codecov Report

Merging #4058 (92b2a38) into master (b818c3b) will decrease coverage by 1.46%.
The diff coverage is 90.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4058      +/-   ##
==========================================
- Coverage   77.26%   75.80%   -1.47%     
==========================================
  Files         219      226       +7     
  Lines       11205    11578     +373     
  Branches      487      498      +11     
==========================================
+ Hits         8658     8777     +119     
- Misses       2547     2801     +254     
Impacted Files Coverage Δ
...cala/org/apache/openwhisk/http/ErrorResponse.scala 89.79% <ø> (+2.04%) ⬆️
...pache/openwhisk/core/entitlement/Entitlement.scala 83.67% <86.66%> (+1.85%) ⬆️
...org/apache/openwhisk/core/controller/Actions.scala 90.90% <94.11%> (+0.12%) ⬆️
...org/apache/openwhisk/core/entity/WhiskAction.scala 93.06% <100.00%> (+0.71%) ⬆️
...core/database/cosmosdb/RxObservableImplicits.scala 0.00% <0.00%> (-100.00%) ⬇️
...ore/database/cosmosdb/cache/CacheInvalidator.scala 0.00% <0.00%> (-100.00%) ⬇️
...e/database/cosmosdb/cache/ChangeFeedConsumer.scala 0.00% <0.00%> (-100.00%) ⬇️
...core/database/cosmosdb/CosmosDBArtifactStore.scala 0.00% <0.00%> (-95.85%) ⬇️
...sk/core/database/cosmosdb/CosmosDBViewMapper.scala 0.00% <0.00%> (-93.90%) ⬇️
...tabase/cosmosdb/cache/CacheInvalidatorConfig.scala 0.00% <0.00%> (-92.31%) ⬇️
... and 49 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b818c3b...92b2a38. Read the comment docs.

@ningyougang
Copy link
Contributor Author

ningyougang commented May 31, 2021

According to review comments

It seems there has another option to do granular permission management

1. There is no need to add permissions controls for action's "owner" since he/she can anyway update/remove/invoke his/her action

2. It is necessary to control other shared user's permission (e.g. whether has permission to read/download the codes, whether has permission to execute the codes), this separate document in db: xxx_permission may like below

{
  "_id": "whisk.system/helloAction",
  "_rev": "1-1a14818c1083bc12b6d3fd1774fcc956",
  "default": {
      {
           "read" : false
           "execute": false
      }
   }
  "permission": {
      "theSharedUserA": {
           "read" : yes
           "execute": yes
      },
      "theSharedUserB": {
           "read" : yes
           "execute": false
      }
  }

Regarding above permission document of whisk.system/helloAction

  • when create action: whisk.system/helloAction, the owner can set default read/execute permission for all shared user. e.g.
  "default": {
      {
           "read" : false
           "execute": false
      }
   }
  • the shared user will inherits the default permissions if not explicitly specified in above document. e.g.

not explicitly specified for theSharedUserC, so theSharedUserC will have default permission

  • if explicitly specified in above document for the shared user, the shared user will have specified permission, e.g.

already explicitly specified for theSharedUserA, so theSharedUserA's permission is

      "theSharedUserA": {
           "read" : yes
           "execute": yes
      }

@ningyougang
Copy link
Contributor Author

ningyougang commented Jun 10, 2021

@style95 ,

Current system permission already existed design solution:

  • the owner have any permissions(read/update/execute) for this own actions.
  • if a package is binded to a shared package, all users except the owner(we call it shared users) just have read/execute permission for that shared package's actions but do not have update permission.
  • if package is not binded to any package, all users(except the owner) do not have any permissions(e.g. read/update/execute) for that package's actions

New design solution: #4058 (comment)

The difference between the new design solution and the current system design is:

  • the new design solution can control every shared user's permission

hm..actually, for now, i don't know what's the direction of this pr.

@style95
Copy link
Member

style95 commented May 20, 2023

@ningyougang Sorry for the late response.
I want to check if we can continue this PR again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
controller review Review for this PR has been requested and yet needs to be done.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants