From e255126b87eabb508b57caef78eba2a130a2e4df Mon Sep 17 00:00:00 2001 From: bkemburu <66023647+bkemburu@users.noreply.github.com> Date: Tue, 25 Aug 2020 15:45:39 -0400 Subject: [PATCH] Execute Only for Shared Actions (#4935) * Initial Commit of Execute Only --- .../scala/src/main/resources/application.conf | 1 + .../apache/openwhisk/core/WhiskConfig.scala | 1 + .../apache/openwhisk/http/ErrorResponse.scala | 9 +++ .../openwhisk/core/controller/Actions.scala | 72 ++++++++++++++----- .../openwhisk/core/controller/Packages.scala | 32 +++++++-- .../test/PackageActionsApiTests.scala | 34 ++++++++- .../controller/test/PackagesApiTests.scala | 54 ++++++++++++++ 7 files changed, 180 insertions(+), 23 deletions(-) diff --git a/common/scala/src/main/resources/application.conf b/common/scala/src/main/resources/application.conf index aa9fa08eea6..22a9b575843 100644 --- a/common/scala/src/main/resources/application.conf +++ b/common/scala/src/main/resources/application.conf @@ -99,6 +99,7 @@ kamon { } whisk { + shared-packages-execute-only = false metrics { # Enable/disable Prometheus support. If enabled then metrics would be exposed at `/metrics` endpoint # If Prometheus is enabled then please review `kamon.metric.tick-interval` (set to 1 sec by default above). diff --git a/common/scala/src/main/scala/org/apache/openwhisk/core/WhiskConfig.scala b/common/scala/src/main/scala/org/apache/openwhisk/core/WhiskConfig.scala index 0dd1c160a50..7aea661e599 100644 --- a/common/scala/src/main/scala/org/apache/openwhisk/core/WhiskConfig.scala +++ b/common/scala/src/main/scala/org/apache/openwhisk/core/WhiskConfig.scala @@ -264,6 +264,7 @@ object ConfigKeys { val featureFlags = "whisk.feature-flags" val whiskConfig = "whisk.config" + val sharedPackageExecuteOnly = s"whisk.shared-packages-execute-only" val swaggerUi = "whisk.swagger-ui" val disableStoreResult = s"$activation.disable-store-result" diff --git a/common/scala/src/main/scala/org/apache/openwhisk/http/ErrorResponse.scala b/common/scala/src/main/scala/org/apache/openwhisk/http/ErrorResponse.scala index 1198cdc9141..5424dd309f7 100644 --- a/common/scala/src/main/scala/org/apache/openwhisk/http/ErrorResponse.scala +++ b/common/scala/src/main/scala/org/apache/openwhisk/http/ErrorResponse.scala @@ -229,6 +229,15 @@ object Messages { /** Indicates that the container for the action could not be started. */ val resourceProvisionError = "Failed to provision resources to run the action." + + def forbiddenGetActionBinding(entityDocId: String) = + s"GET not permitted for '$entityDocId'. Resource does not exist or is an action in a shared package binding." + def forbiddenGetAction(entityPath: String) = + s"GET not permitted for '$entityPath' since it's an action in a shared package" + def forbiddenGetPackageBinding(packageName: String) = + s"GET not permitted since $packageName is a binding of a shared package" + def forbiddenGetPackage(packageName: String) = + s"GET not permitted for '$packageName' since it's a shared package" } /** Replaces rejections with Json object containing cause and transaction id. */ diff --git a/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala b/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala index 4fc53121d6f..a6e4554b071 100644 --- a/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala +++ b/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala @@ -43,6 +43,8 @@ import org.apache.openwhisk.http.Messages._ import org.apache.openwhisk.core.entitlement.Resource import org.apache.openwhisk.core.entitlement.Collection import org.apache.openwhisk.core.loadBalancer.LoadBalancerException +import pureconfig._ +import org.apache.openwhisk.core.ConfigKeys /** * A singleton object which defines the properties that must be present in a configuration @@ -102,6 +104,10 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with /** Database service to get activations. */ protected val activationStore: ActivationStore + /** Config flag for Execute Only for Actions in Shared Packages */ + protected def executeOnly = + loadConfigOrThrow[Boolean](ConfigKeys.sharedPackageExecuteOnly) + /** Entity normalizer to JSON object. */ import RestApiCommons.emptyEntityToJsObject @@ -330,6 +336,50 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with deleteEntity(WhiskAction, entityStore, entityName.toDocId, (a: WhiskAction) => Future.successful({})) } + /** 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)( + implicit transid: TransactionId) = { + val resolvedPkg: Future[Either[String, FullyQualifiedEntityName]] = if (entityName.path.defaultPackage) { + Future.successful(Right(entityName)) + } else { + WhiskPackage.resolveBinding(entityStore, entityName.path.toDocId, mergeParameters = true).map { pkg => + val originalPackageLocation = pkg.fullyQualifiedName(withVersion = false).namespace + if (executeOnly && originalPackageLocation != entityName.namespace) { + Left(forbiddenGetActionBinding(entityName.toDocId.asString)) + } else { + Right(entityName) + } + } + } + onComplete(resolvedPkg) { + case Success(pkgFuture) => + pkgFuture match { + 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) + }) + } else { + getEntity(WhiskActionMetaData.resolveActionAndMergeParameters(entityStore, entityName), Some { + action: WhiskActionMetaData => + val mergedAction = env map { + action inherit _ + } getOrElse action + complete(OK, mergedAction) + }) + } + } + case Failure(t: Throwable) => + logging.error(this, s"[GET] package ${entityName.path.toDocId} failed: ${t.getMessage}") + terminate(InternalServerError) + } + } + /** * Gets action. The action name is prefixed with the namespace to create the primary index key. * @@ -341,22 +391,12 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with override def fetch(user: Identity, entityName: FullyQualifiedEntityName, env: Option[Parameters])( implicit transid: TransactionId) = { parameter('code ? true) { code => - code match { - case true => - getEntity(WhiskAction.resolveActionAndMergeParameters(entityStore, entityName), Some { action: WhiskAction => - val mergedAction = env map { - action inherit _ - } getOrElse action - complete(OK, mergedAction) - }) - case false => - getEntity(WhiskActionMetaData.resolveActionAndMergeParameters(entityStore, entityName), Some { - action: WhiskActionMetaData => - val mergedAction = env map { - action inherit _ - } getOrElse action - complete(OK, mergedAction) - }) + //check if execute only is enabled, and if there is a discrepancy between the current user's namespace + //and that of the entity we are trying to fetch + if (executeOnly && user.namespace.name != entityName.namespace) { + terminate(Forbidden, forbiddenGetAction(entityName.path.asString)) + } else { + fetchEntity(entityName, env, code) } } } diff --git a/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala b/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala index b1e555bbbef..ffc992f3235 100644 --- a/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala +++ b/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala @@ -19,12 +19,10 @@ package org.apache.openwhisk.core.controller import scala.concurrent.Future import scala.util.{Failure, Success} - import akka.http.scaladsl.marshallers.sprayjson.SprayJsonSupport._ import akka.http.scaladsl.model.StatusCodes._ import akka.http.scaladsl.server.{RequestContext, RouteResult} import akka.http.scaladsl.unmarshalling.Unmarshaller - import org.apache.openwhisk.common.TransactionId import org.apache.openwhisk.core.controller.RestApiCommons.{ListLimit, ListSkip} import org.apache.openwhisk.core.database.{CacheChangeNotification, DocumentTypeMismatchException, NoDocumentException} @@ -33,6 +31,9 @@ import org.apache.openwhisk.core.entity._ import org.apache.openwhisk.core.entity.types.EntityStore import org.apache.openwhisk.http.ErrorResponse.terminate import org.apache.openwhisk.http.Messages +import org.apache.openwhisk.http.Messages._ +import pureconfig._ +import org.apache.openwhisk.core.ConfigKeys trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { services: WhiskServices => @@ -42,6 +43,10 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { /** Database service to CRUD packages. */ protected val entityStore: EntityStore + /** Config flag for Execute Only for Shared Packages */ + protected def executeOnly = + loadConfigOrThrow[Boolean](ConfigKeys.sharedPackageExecuteOnly) + /** Notification service for cache invalidation. */ protected implicit val cacheChangeNotification: Some[CacheChangeNotification] @@ -157,7 +162,14 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { */ override def fetch(user: Identity, entityName: FullyQualifiedEntityName, env: Option[Parameters])( implicit transid: TransactionId) = { - getEntity(WhiskPackage.get(entityStore, entityName.toDocId), Some { mergePackageWithBinding() _ }) + if (executeOnly && user.namespace.name != entityName.namespace) { + val value = entityName.toString + terminate(Forbidden, forbiddenGetPackage(entityName.asString)) + } else { + getEntity(WhiskPackage.get(entityStore, entityName.toDocId), Some { + mergePackageWithBinding() _ + }) + } } /** @@ -303,9 +315,17 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { logging.error(this, s"unexpected package binding refers to itself: $docid") terminate(UnprocessableEntity, Messages.packageBindingCircularReference(b.fullyQualifiedName.toString)) } else { - getEntity(WhiskPackage.get(entityStore, docid), Some { - mergePackageWithBinding(Some { wp }) _ - }) + + /** Here's where I check package execute only case with package binding. */ + if (executeOnly && wp.namespace.asString != b.namespace.asString) { + terminate(Forbidden, forbiddenGetPackageBinding(wp.name.asString)) + } else { + getEntity(WhiskPackage.get(entityStore, docid), Some { + mergePackageWithBinding(Some { + wp + }) _ + }) + } } } getOrElse { val pkg = ref map { _ inherit wp.parameters } getOrElse wp diff --git a/tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackageActionsApiTests.scala b/tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackageActionsApiTests.scala index a645e87c3ee..71d821c13c4 100644 --- a/tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackageActionsApiTests.scala +++ b/tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackageActionsApiTests.scala @@ -187,7 +187,6 @@ class PackageActionsApiTests extends ControllerTestCommon with WhiskActionsApi { status should be(BadRequest) } } - it should "reject put action in package binding" in { implicit val tid = transid() val provider = WhiskPackage(namespace, aname(), None, publish = true) @@ -636,4 +635,37 @@ class PackageActionsApiTests extends ControllerTestCommon with WhiskActionsApi { responseAs[ErrorResponse].error shouldBe Messages.corruptedEntity } } + + var testExecuteOnly = false + override def executeOnly = testExecuteOnly + + it should ("allow access to get of action in binding of shared package when config option is disabled") in { + testExecuteOnly = false + implicit val tid = transid() + val auser = WhiskAuthHelpers.newIdentity() + val provider = WhiskPackage(namespace, aname(), None, Parameters("p", "P"), publish = true) + val binding = WhiskPackage(EntityPath(auser.subject.asString), aname(), provider.bind, Parameters("b", "B")) + val action = WhiskAction(provider.fullPath, aname(), jsDefault("??"), Parameters("a", "A")) + put(entityStore, provider) + put(entityStore, binding) + put(entityStore, action) + Get(s"$collectionPath/${binding.name}/${action.name}") ~> Route.seal(routes(auser)) ~> check { + status should be(OK) + } + } + + it should ("deny access to get of action in binding of shared package when config option is enabled") in { + testExecuteOnly = true + implicit val tid = transid() + val auser = WhiskAuthHelpers.newIdentity() + val provider = WhiskPackage(namespace, aname(), None, Parameters("p", "P"), publish = true) + val binding = WhiskPackage(EntityPath(auser.subject.asString), aname(), provider.bind, Parameters("b", "B")) + val action = WhiskAction(provider.fullPath, aname(), jsDefault("??"), Parameters("a", "A")) + put(entityStore, provider) + put(entityStore, binding) + put(entityStore, action) + Get(s"$collectionPath/${binding.name}/${action.name}") ~> Route.seal(routes(auser)) ~> check { + status should be(Forbidden) + } + } } diff --git a/tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackagesApiTests.scala b/tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackagesApiTests.scala index e59121f4373..c52ba07500d 100644 --- a/tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackagesApiTests.scala +++ b/tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackagesApiTests.scala @@ -884,4 +884,58 @@ class PackagesApiTests extends ControllerTestCommon with WhiskPackagesApi { responseAs[ErrorResponse].error shouldBe Messages.corruptedEntity } } + + var testExecuteOnly = false + override def executeOnly = testExecuteOnly + + it should ("allow access to get of shared package binding when config option is disabled") in { + testExecuteOnly = false + implicit val tid = transid() + val auser = WhiskAuthHelpers.newIdentity() + val provider = WhiskPackage(namespace, aname(), None, Parameters("p", "P"), publish = true) + val binding = WhiskPackage(EntityPath(auser.subject.asString), aname(), provider.bind, Parameters("b", "B")) + put(entityStore, provider) + put(entityStore, binding) + Get(s"/$namespace/${collection.path}/${provider.name}") ~> Route.seal(routes(auser)) ~> check { + status should be(OK) + } + } + + it should ("allow access to get of shared package when config option is disabled") in { + testExecuteOnly = false + implicit val tid = transid() + val auser = WhiskAuthHelpers.newIdentity() + val provider = WhiskPackage(namespace, aname(), None, publish = true) + put(entityStore, provider) + + Get(s"/$namespace/${collection.path}/${provider.name}") ~> Route.seal(routes(auser)) ~> check { + status should be(OK) + } + } + + it should ("deny access to get of shared package binding when config option is enabled") in { + testExecuteOnly = true + implicit val tid = transid() + val auser = WhiskAuthHelpers.newIdentity() + val provider = WhiskPackage(namespace, aname(), None, Parameters("p", "P"), publish = true) + val binding = WhiskPackage(EntityPath(auser.subject.asString), aname(), provider.bind, Parameters("b", "B")) + put(entityStore, provider) + put(entityStore, binding) + Get(s"/$namespace/${collection.path}/${provider.name}") ~> Route.seal(routes(auser)) ~> check { + status should be(Forbidden) + } + + } + + it should ("deny access to get of shared package when config option is enabled") in { + testExecuteOnly = true + implicit val tid = transid() + val auser = WhiskAuthHelpers.newIdentity() + val provider = WhiskPackage(namespace, aname(), None, publish = true) + put(entityStore, provider) + + Get(s"/$namespace/${collection.path}/${provider.name}") ~> Route.seal(routes(auser)) ~> check { + status should be(Forbidden) + } + } }