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.

Sign up for GitHub

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

Jump to bottom

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
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5824b5c
Add protect feature to avoid update or delete actions by mistake
ningyougang Sep 28, 2018
b9a8ca8
Fix test cases error
ningyougang Sep 8, 2020
c00f5f6
Fix review points
ningyougang Oct 14, 2020
51ca755
Optimize the download code for shared user readable
ningyougang May 28, 2021
92b2a38
Change from if/else to match/case statement
ningyougang May 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,16 @@ case class WhiskActionPut(exec: Option[Exec] = None,
case _ => this
} getOrElse this
}

protected[core] def getPermissions(): Option[String] = {
annotations match {
case Some(value) =>
value
.get(WhiskAction.permissionsFieldName)
.map(value => value.convertTo[String])
case None => None
}
}
}

abstract class WhiskActionLike(override val name: EntityName) extends WhiskEntity(name, "action") {
Expand Down Expand Up @@ -352,6 +362,27 @@ 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.


val defaultPermissions = "rwxr-x"

// For more information about permission, please refer to the online manual at
// https://github.com/apache/openwhisk/blob/master/docs/actions.md#action-permission-management
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

defaultPermissions,
"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 •
edited
Loading

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?

"rw-r--",
"rwx--x",
"rwx---",
"r-x--x",
"r-x---",
"r-----",
"rw----")

override val collectionName = "actions"
override val cacheEnabled = true

Expand Down
9 changes: 1 addition & 8 deletions common/scala/src/main/scala/org/apache/openwhisk/http/ErrorResponse.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,16 @@ package org.apache.openwhisk.http
import scala.concurrent.duration.Duration
import scala.concurrent.duration.FiniteDuration
import scala.util.Try

import akka.http.scaladsl.model.StatusCode
import akka.http.scaladsl.model.StatusCodes.Forbidden
import akka.http.scaladsl.model.StatusCodes.NotFound
import akka.http.scaladsl.model.MediaType
import akka.http.scaladsl.server.Directives
import akka.http.scaladsl.marshallers.sprayjson.SprayJsonSupport.sprayJsonMarshaller
import akka.http.scaladsl.server.StandardRoute

import spray.json._

import org.apache.openwhisk.common.TransactionId
import org.apache.openwhisk.core.entity.SizeError
import org.apache.openwhisk.core.entity.ByteSize
import org.apache.openwhisk.core.entity.Exec
import org.apache.openwhisk.core.entity.ExecMetaDataBase
import org.apache.openwhisk.core.entity.ActivationId
import org.apache.openwhisk.core.entity.{ActivationId, ByteSize, Exec, ExecMetaDataBase, SizeError}

object Messages {

Expand Down
113 changes: 72 additions & 41 deletions core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,9 +218,22 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with

onComplete(checkAdditionalPrivileges) {
case Success(_) =>
putEntity(WhiskAction, entityStore, entityName.toDocId, overwrite, update(user, request) _, () => {
make(user, entityName, request)
})
val operation = if (overwrite) "update" else "create"
onComplete(
entitlementProvider
.checkActionPermissions(
operation,
user,
entityStore,
entityName,
WhiskAction.get,
content.getPermissions())) {
case Success(_) =>
putEntity(WhiskAction, entityStore, entityName.toDocId, overwrite, update(user, request) _, () => {
make(user, entityName, request)
})
case Failure(f) => super.handleEntitlementFailure(f)
}
case Failure(f) =>
super.handleEntitlementFailure(f)
}
Expand All @@ -241,37 +254,43 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
*/
override def activate(user: Identity, entityName: FullyQualifiedEntityName, env: Option[Parameters])(
implicit transid: TransactionId) = {
parameter(
'blocking ? false,
'result ? false,
'timeout.as[FiniteDuration] ? controllerActivationConfig.maxWaitForBlockingActivation) {
(blocking, result, waitOverride) =>
entity(as[Option[JsObject]]) { payload =>
getEntity(WhiskActionMetaData.resolveActionAndMergeParameters(entityStore, entityName), Some {
act: WhiskActionMetaData =>
// resolve the action --- special case for sequences that may contain components with '_' as default package
val action = act.resolve(user.namespace)
onComplete(entitleReferencedEntitiesMetaData(user, Privilege.ACTIVATE, Some(action.exec))) {
case Success(_) =>
val actionWithMergedParams = env.map(action.inherit(_)) getOrElse action

// incoming parameters may not override final parameters (i.e., parameters with already defined values)
// on an action once its parameters are resolved across package and binding
val allowInvoke = payload
.map(_.fields.keySet.forall(key => !actionWithMergedParams.immutableParameters.contains(key)))
.getOrElse(true)
onComplete(
entitlementProvider
.checkActionPermissions("invoke", user, entityStore, entityName, WhiskAction.get)) {
case Success(_) =>
parameter(
'blocking ? false,
'result ? false,
'timeout.as[FiniteDuration] ? controllerActivationConfig.maxWaitForBlockingActivation) {
(blocking, result, waitOverride) =>
entity(as[Option[JsObject]]) { payload =>
getEntity(WhiskActionMetaData.resolveActionAndMergeParameters(entityStore, entityName), Some {
act: WhiskActionMetaData =>
// resolve the action --- special case for sequences that may contain components with '_' as default package
val action = act.resolve(user.namespace)
onComplete(entitleReferencedEntitiesMetaData(user, Privilege.ACTIVATE, Some(action.exec))) {
case Success(_) =>
val actionWithMergedParams = env.map(action.inherit(_)) getOrElse action

// incoming parameters may not override final parameters (i.e., parameters with already defined values)
// on an action once its parameters are resolved across package and binding
val allowInvoke = payload
.map(_.fields.keySet.forall(key => !actionWithMergedParams.immutableParameters.contains(key)))
.getOrElse(true)

if (allowInvoke) {
doInvoke(user, actionWithMergedParams, payload, blocking, waitOverride, result)
} else {
terminate(BadRequest, Messages.parametersNotAllowed)
}

if (allowInvoke) {
doInvoke(user, actionWithMergedParams, payload, blocking, waitOverride, result)
} else {
terminate(BadRequest, Messages.parametersNotAllowed)
case Failure(f) =>
super.handleEntitlementFailure(f)
}

case Failure(f) =>
super.handleEntitlementFailure(f)
}
})
})
}
}
case Failure(f) => super.handleEntitlementFailure(f)
}
}

Expand Down Expand Up @@ -333,11 +352,17 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
* - 500 Internal Server Error
*/
override def remove(user: Identity, entityName: FullyQualifiedEntityName)(implicit transid: TransactionId) = {
deleteEntity(WhiskAction, entityStore, entityName.toDocId, (a: WhiskAction) => Future.successful({}))
onComplete(
entitlementProvider
.checkActionPermissions("remove", user, entityStore, entityName, WhiskAction.get)) {
case Success(_) =>
deleteEntity(WhiskAction, entityStore, entityName.toDocId, (a: WhiskAction) => Future.successful({}))
case Failure(f) => super.handleEntitlementFailure(f)
}
}

/** Checks for package binding case. we don't want to allow get for a package binding in shared package */
private def fetchEntity(entityName: FullyQualifiedEntityName, env: Option[Parameters], code: Boolean)(
private def fetchEntity(user: Identity, entityName: FullyQualifiedEntityName, env: Option[Parameters], code: Boolean)(
implicit transid: TransactionId) = {
val resolvedPkg: Future[Either[String, FullyQualifiedEntityName]] = if (entityName.path.defaultPackage) {
Future.successful(Right(entityName))
Expand All @@ -357,13 +382,19 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
case Left(f) => terminate(Forbidden, f)
case Right(_) =>
if (code) {
getEntity(WhiskAction.resolveActionAndMergeParameters(entityStore, entityName), Some {
action: WhiskAction =>
val mergedAction = env map {
action inherit _
} getOrElse action
complete(OK, mergedAction)
})
onComplete(
entitlementProvider
.checkActionPermissions("download", user, entityStore, entityName, WhiskAction.get)) {
case Success(_) =>
getEntity(WhiskAction.resolveActionAndMergeParameters(entityStore, entityName), Some {
action: WhiskAction =>
val mergedAction = env map {
action inherit _
} getOrElse action
complete(OK, mergedAction)
})
case Failure(f) => super.handleEntitlementFailure(f)
}
} else {
getEntity(WhiskActionMetaData.resolveActionAndMergeParameters(entityStore, entityName), Some {
action: WhiskActionMetaData =>
Expand Down Expand Up @@ -396,7 +427,7 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
if (executeOnly && user.namespace.name != entityName.namespace) {
terminate(Forbidden, forbiddenGetAction(entityName.path.asString))
} else {
fetchEntity(entityName, env, code)
fetchEntity(user, entityName, env, code)
}
}
}
Expand Down
156 changes: 156 additions & 0 deletions core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/Entitlement.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,12 @@ import org.apache.openwhisk.core.loadBalancer.{LoadBalancer, ShardingContainerPo
import org.apache.openwhisk.http.ErrorResponse
import org.apache.openwhisk.http.Messages
import org.apache.openwhisk.core.connector.MessagingProvider
import org.apache.openwhisk.core.database.ArtifactStore
import org.apache.openwhisk.spi.SpiLoader
import org.apache.openwhisk.spi.Spi

import spray.json.DefaultJsonProtocol._

object types {
type Entitlements = TrieMap[(Subject, String), Set[Privilege]]
}
Expand Down Expand Up @@ -244,6 +247,159 @@ protected[core] abstract class EntitlementProvider(
.getOrElse(Future.successful(()))
}

/**
* Checks if action operation(get/write/execute) whether feasible
*
* @param operation the action operation, e.g. get/write/execute
* @param user the user who get/write/execute the action
* @param entityStore store to write the action to
* @param entityName entityName
* @param permissions the passed permission code
* @return a promise that completes with success iff action operation is feasible
*/
protected[core] def checkActionPermissions(
operation: String,
user: Identity,
entityStore: ArtifactStore[WhiskEntity],
entityName: FullyQualifiedEntityName,
get: (ArtifactStore[WhiskEntity], DocId, DocRevision, Boolean) => Future[WhiskAction],
permissions: Option[String] = None)(implicit transid: TransactionId): Future[Unit] = {
operation match {
case "create" =>
permissions
.map { value =>
if (WhiskAction.permissionList.contains(value)) {
Future.successful(())
} else {
val errorInfo =
s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
}
}
.getOrElse(Future.successful(()))
case "update" =>
get(entityStore, entityName.toDocId, DocRevision.empty, true)
.flatMap { whiskAction =>
val currentPermissions = whiskAction.annotations
.get(WhiskAction.permissionsFieldName)
.map(value => value.convertTo[String])
.getOrElse(WhiskAction.defaultPermissions)

val errorInfo = s"have no permission to ${operation} this action"
permissions match {
case Some(value) =>
if (!WhiskAction.permissionList.contains(value)) {
val errorInfo =
s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
} else {
val passedUpdatePermission = value.charAt(1)
if (passedUpdatePermission == 'w') { // make it to modifiable
Future.successful(())
} else {
val currentUpdatePermission = currentPermissions.charAt(1)
if (currentUpdatePermission == '-') {
Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
} else {
Future.successful(())
}
}
}
case None =>
val currentUpdatePermission = currentPermissions.charAt(1)
if (currentUpdatePermission == '-') {
Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
} else {
Future.successful(())
}
}
}
.recoverWith {
case t: RejectRequest =>
Future.failed(t)
case _ =>
Future.successful(())
}
case "remove" =>
get(entityStore, entityName.toDocId, DocRevision.empty, true)
.flatMap { whiskAction =>
val currentPermissions = whiskAction.annotations
.get(WhiskAction.permissionsFieldName)
.map(value => value.convertTo[String])
.getOrElse(WhiskAction.defaultPermissions)

val currentUpdatePermission = currentPermissions.charAt(1)
if (currentUpdatePermission == '-') {
val errorInfo = s"have no permission to ${operation} this action"
Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
} else {
Future.successful(())
}
}
.recoverWith {
case t: RejectRequest =>
Future.failed(t)
case _ =>
Future.successful(())
}
case "invoke" =>
get(entityStore, entityName.toDocId, DocRevision.empty, true)
.flatMap { whiskAction =>
val currentPermissions = whiskAction.annotations
.get(WhiskAction.permissionsFieldName)
.map(value => value.convertTo[String])
.getOrElse(WhiskAction.defaultPermissions)

// the user who is owner by default
var currentExecutePermission = currentPermissions.charAt(2)
var errorInfo = s"have no permission to ${operation} this action"
if (user.namespace.name.asString != entityName.path.root.asString) { // the user who invoke the shared action
currentExecutePermission = currentPermissions.charAt(5)
errorInfo = s"have no permission to ${operation} this shared action"
}
if (currentExecutePermission == '-') { //have no permission
Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
} else {
Future.successful(())
}
}
.recoverWith {
case t: RejectRequest =>
Future.failed(t)
case _ =>
Future.successful(())
}
case _ =>
// download the code
get(entityStore, entityName.toDocId, DocRevision.empty, true)
.flatMap { whiskAction =>
val currentPermissions = whiskAction.annotations
.get(WhiskAction.permissionsFieldName)
.map(value => value.convertTo[String])
.getOrElse(WhiskAction.defaultPermissions)

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(())
}
}
.recoverWith {
case t: RejectRequest =>
Future.failed(t)
case _ =>
Future.successful(())
}
}
}

/**
* Checks if a subject has the right to access a specific resource. The entitlement may be implicit,
* that is, inferred based on namespaces that a subject belongs to and the namespace of the
Expand Down
Loading