[GitHub] rabbah commented on a change in pull request #3388: Update require-whisk-auth behavior to secure web action
rabbah commented on a change in pull request #3388: Update require-whisk-auth behavior to secure web action URL: https://github.com/apache/incubator-openwhisk/pull/3388#discussion_r172546821 ## File path: common/scala/src/main/scala/whisk/core/entity/WhiskAction.scala ## @@ -313,6 +313,9 @@ object WhiskAction extends DocumentFactory[WhiskAction] with WhiskEntityQueries[ override val cacheEnabled = true + val requireWhiskAuthAnnotation = "require-whisk-auth" Review comment: I also suggested below that we should instead consolidate all the annotations into a singleton - they're scattered right now. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rabbah commented on a change in pull request #3388: Update require-whisk-auth behavior to secure web action
rabbah commented on a change in pull request #3388: Update require-whisk-auth behavior to secure web action URL: https://github.com/apache/incubator-openwhisk/pull/3388#discussion_r172499506 ## File path: docs/annotations.md ## @@ -48,7 +48,7 @@ and must be present and explicitly set to `true` to have an affect. The annotati * `final`: Makes all of the action parameters that are already defined immutable. A parameter of an action carrying the annotation may not be overridden by invoke-time parameters once the parameter has a value defined through its enclosing package or the action definition. * `raw-http`: When set, the HTTP request query and body parameters are passed to the action as reserved properties. * `web-custom-options`: When set, this annotation enables a web action to respond to OPTIONS requests with customized headers, otherwise a [default CORS response](webactions.md#options-requests) applies. -* `require-whisk-auth`: This annotation protects the web action so that it is only accessible to an authenticated subject. It is important to note that the _owner_ of the web action will still incur the cost of running them in the system (i.e., the _owner_ of the action also owns the activations record). +* `require-whisk-auth`: This annotation protects the web action so that it is only invoked by requests that provide appropriate authentication credentials. When set to a boolean value, it controls whether or not the request's Basic Authentication subject will be authenticated - a value of `true` will authenticate the subject, a value of `false` will invoke the action without any authentication. When set to an integer or a string, this value must match the request's `X-Require-Whisk-Auth` header value. In both cases, it is important to note that the _owner_ of the web action will still incur the cost of running them in the system (i.e., the _owner_ of the action also owns the activations record). Review comment: can you further elaborate that the basic auth credentials would be valid WHISK API keys. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rabbah commented on a change in pull request #3388: Update require-whisk-auth behavior to secure web action
rabbah commented on a change in pull request #3388: Update require-whisk-auth
behavior to secure web action
URL:
https://github.com/apache/incubator-openwhisk/pull/3388#discussion_r172500051
##
File path:
tests/src/test/scala/whisk/core/controller/test/WebActionsApiTests.scala
##
@@ -379,21 +386,51 @@ trait WebActionsApiBaseTests extends
ControllerTestCommon with BeforeAndAfterEac
Seq(s"$systemId/proxy/export_auth").foreach { path =>
allowedMethods.foreach { m =>
- if (creds.isDefined)
-invocationsAllowed += 1
requireAuthentication = true
+ Seq(true, false).foreach { useReqWhiskAuthBool =>
+requireAuthenticationAsBoolean = useReqWhiskAuthBool
+ }
- m(s"$testRoutePath/${path}.json") ~> Route.seal(routes(creds)) ~>
check {
-creds match {
- case None => status should be(Unauthorized)
- case Some(user) =>
+ if (requireAuthenticationAsBoolean) {
+if (creds.isDefined) {
+ val user = creds.get
+ invocationsAllowed += 1
+ m(s"$testRoutePath/${path}.json") ~> Route
+.seal(routes(creds)) ~> check {
status should be(OK)
val response = responseAs[JsObject]
response shouldBe JsObject(
"pkg" -> s"$systemId/proxy".toJson,
"action" -> "export_auth".toJson,
"content" -> metaPayload(m.method.name.toLowerCase,
JsObject(), creds, pkgName = "proxy"))
response.fields("content").asJsObject.fields(webApiDirectives.namespace)
shouldBe user.namespace.toJson
+ }
+} else {
+ m(s"$testRoutePath/${path}.json") ~> Route.seal(routes(creds))
~> check {
+status should be(Unauthorized)
+ }
+}
+ } else if (creds.isDefined) {
+val user = creds.get
+invocationsAllowed += 1
+m(s"$testRoutePath/${path}.json") ~>
addHeader("X-Require-Whisk-Auth", requireAuthenticationKey) ~> Route
+ .seal(routes(creds)) ~> check {
+ status should be(OK)
+ val response = responseAs[JsObject]
+ response shouldBe JsObject(
+"pkg" -> s"$systemId/proxy".toJson,
+"action" -> "export_auth".toJson,
+"content" -> metaPayload(
+ m.method.name.toLowerCase,
+ JsObject(),
+ creds,
+ pkgName = "proxy",
+ headers = List(RawHeader("X-Require-Whisk-Auth",
requireAuthenticationKey
+
response.fields("content").asJsObject.fields(webApiDirectives.namespace)
shouldBe user.namespace.toJson
+}
+ } else {
+m(s"$testRoutePath/${path}.json") ~> Route.seal(routes(creds)) ~>
check {
Review comment:
can you add the expected header but with the wrong value as well.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rabbah commented on a change in pull request #3388: Update require-whisk-auth behavior to secure web action
rabbah commented on a change in pull request #3388: Update require-whisk-auth
behavior to secure web action
URL:
https://github.com/apache/incubator-openwhisk/pull/3388#discussion_r172499869
##
File path:
tests/src/test/scala/whisk/core/controller/test/WebActionsApiTests.scala
##
@@ -197,7 +200,9 @@ trait WebActionsApiBaseTests extends ControllerTestCommon
with BeforeAndAfterEac
annotations ++
Parameters("web-export", JsBoolean(true)) ++ {
if (requireAuthentication) {
- Parameters("require-whisk-auth", JsBoolean(true))
+ Parameters(
+"require-whisk-auth",
Review comment:
we should probably start pulling all the annotations to an Annotations
singleton.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rabbah commented on a change in pull request #3388: Update require-whisk-auth behavior to secure web action
rabbah commented on a change in pull request #3388: Update require-whisk-auth
behavior to secure web action
URL:
https://github.com/apache/incubator-openwhisk/pull/3388#discussion_r172498719
##
File path:
core/controller/src/main/scala/whisk/core/controller/WebActions.scala
##
@@ -483,7 +483,29 @@ trait WhiskWebActionsApi extends Directives with
ValidateRequestSize with PostAc
provide(fullyQualifiedActionName(actionName)) { fullActionName =>
onComplete(verifyWebAction(fullActionName,
onBehalfOf.isDefined)) {
case Success((actionOwnerIdentity, action)) =>
-if
(!action.annotations.getAs[Boolean]("web-custom-options").exists(identity)) {
+// If the require-whisk-auth annotation is either an
integer or a string, secure the web action by enforcing
+// require-whisk-auth annotation value == request header
x-require-whisk-auth value
+// If the require-whisk-auth annotation is a boolean, skip
the request header x-require-whisk-auth check
+val requireWhiskHeaderAuthenticationFailed =
action.annotations
+ .get(WhiskAction.requireWhiskAuthAnnotation)
+ .flatMap {
+case JsString(authStr) => Some(authStr)
+case JsNumber(authNum) => Some(authNum.toInt.toString)
Review comment:
do you need .toInt?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rabbah commented on a change in pull request #3388: Update require-whisk-auth behavior to secure web action
rabbah commented on a change in pull request #3388: Update require-whisk-auth
behavior to secure web action
URL:
https://github.com/apache/incubator-openwhisk/pull/3388#discussion_r172499335
##
File path:
core/controller/src/main/scala/whisk/core/controller/WebActions.scala
##
@@ -719,7 +741,8 @@ trait WhiskWebActionsApi extends Directives with
ValidateRequestSize with PostAc
private def confirmExportedAction(actionLookup: Future[WhiskActionMetaData],
authenticated: Boolean)(
implicit transid: TransactionId): Future[WhiskActionMetaData] = {
actionLookup flatMap { action =>
- val requiresAuthenticatedUser =
action.annotations.getAs[Boolean]("require-whisk-auth").exists(identity)
+ val requiresAuthenticatedUser =
+
action.annotations.getAs[Boolean](WhiskAction.requireWhiskAuthAnnotation).exists(identity)
Review comment:
I was thinking... not sure if it works... to make the auth check a truthy,
so that then you can consolidate the authentication failure. The
`authenticated` bit could be computed by the earlier check of the header (or
the whisk key as done now).
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rabbah commented on a change in pull request #3388: Update require-whisk-auth behavior to secure web action
rabbah commented on a change in pull request #3388: Update require-whisk-auth
behavior to secure web action
URL:
https://github.com/apache/incubator-openwhisk/pull/3388#discussion_r172498942
##
File path:
core/controller/src/main/scala/whisk/core/controller/WebActions.scala
##
@@ -483,7 +483,29 @@ trait WhiskWebActionsApi extends Directives with
ValidateRequestSize with PostAc
provide(fullyQualifiedActionName(actionName)) { fullActionName =>
onComplete(verifyWebAction(fullActionName,
onBehalfOf.isDefined)) {
case Success((actionOwnerIdentity, action)) =>
-if
(!action.annotations.getAs[Boolean]("web-custom-options").exists(identity)) {
+// If the require-whisk-auth annotation is either an
integer or a string, secure the web action by enforcing
+// require-whisk-auth annotation value == request header
x-require-whisk-auth value
+// If the require-whisk-auth annotation is a boolean, skip
the request header x-require-whisk-auth check
+val requireWhiskHeaderAuthenticationFailed =
action.annotations
Review comment:
can you refactor the handing of the auth string to a new method?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rabbah commented on a change in pull request #3388: Update require-whisk-auth behavior to secure web action
rabbah commented on a change in pull request #3388: Update require-whisk-auth
behavior to secure web action
URL:
https://github.com/apache/incubator-openwhisk/pull/3388#discussion_r172498687
##
File path:
core/controller/src/main/scala/whisk/core/controller/WebActions.scala
##
@@ -483,7 +483,29 @@ trait WhiskWebActionsApi extends Directives with
ValidateRequestSize with PostAc
provide(fullyQualifiedActionName(actionName)) { fullActionName =>
onComplete(verifyWebAction(fullActionName,
onBehalfOf.isDefined)) {
case Success((actionOwnerIdentity, action)) =>
-if
(!action.annotations.getAs[Boolean]("web-custom-options").exists(identity)) {
+// If the require-whisk-auth annotation is either an
integer or a string, secure the web action by enforcing
+// require-whisk-auth annotation value == request header
x-require-whisk-auth value
+// If the require-whisk-auth annotation is a boolean, skip
the request header x-require-whisk-auth check
+val requireWhiskHeaderAuthenticationFailed =
action.annotations
+ .get(WhiskAction.requireWhiskAuthAnnotation)
+ .flatMap {
+case JsString(authStr) => Some(authStr)
+case JsNumber(authNum) => Some(authNum.toInt.toString)
Review comment:
do you need .toInt?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rabbah commented on a change in pull request #3388: Update require-whisk-auth behavior to secure web action
rabbah commented on a change in pull request #3388: Update require-whisk-auth
behavior to secure web action
URL:
https://github.com/apache/incubator-openwhisk/pull/3388#discussion_r172498687
##
File path:
core/controller/src/main/scala/whisk/core/controller/WebActions.scala
##
@@ -483,7 +483,29 @@ trait WhiskWebActionsApi extends Directives with
ValidateRequestSize with PostAc
provide(fullyQualifiedActionName(actionName)) { fullActionName =>
onComplete(verifyWebAction(fullActionName,
onBehalfOf.isDefined)) {
case Success((actionOwnerIdentity, action)) =>
-if
(!action.annotations.getAs[Boolean]("web-custom-options").exists(identity)) {
+// If the require-whisk-auth annotation is either an
integer or a string, secure the web action by enforcing
+// require-whisk-auth annotation value == request header
x-require-whisk-auth value
+// If the require-whisk-auth annotation is a boolean, skip
the request header x-require-whisk-auth check
+val requireWhiskHeaderAuthenticationFailed =
action.annotations
+ .get(WhiskAction.requireWhiskAuthAnnotation)
+ .flatMap {
+case JsString(authStr) => Some(authStr)
+case JsNumber(authNum) => Some(authNum.toInt.toString)
Review comment:
do you need .toInt?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rabbah commented on a change in pull request #3388: Update require-whisk-auth behavior to secure web action
rabbah commented on a change in pull request #3388: Update require-whisk-auth
behavior to secure web action
URL:
https://github.com/apache/incubator-openwhisk/pull/3388#discussion_r172023991
##
File path:
core/controller/src/main/scala/whisk/core/controller/WebActions.scala
##
@@ -483,7 +483,24 @@ trait WhiskWebActionsApi extends Directives with
ValidateRequestSize with PostAc
provide(fullyQualifiedActionName(actionName)) { fullActionName =>
onComplete(verifyWebAction(fullActionName,
onBehalfOf.isDefined)) {
case Success((actionOwnerIdentity, action)) =>
-if
(!action.annotations.getAs[Boolean]("web-custom-options").exists(identity)) {
+val requireWebAuthIsBool =
(action.annotations.getAs[Boolean]("require-whisk-auth") != None)
Review comment:
use `.isTruthy` on annotation instead.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
