Protect Package Bindings from containing circular references #4122

asteed · 2018-11-18T05:55:22Z

Description

We ran across an issue where a package binding can contain a reference to itself. When this happens, any attempt at reading the package results in an infinite loop, eventually leading to the controller falling over. This can occur by simply performing the following commands using the wsk tool.

$ wsk package create target
$ wsk package bind target binding
$ wsk package bind binding binding

There are two changes contained in this PR

Prevent allowing an incoming binding update to point to itself as a reference, which previously created an invalid data record.
Prevent following a package's binding reference when it points to itself which causes the infinite loop.

Related issue and scope

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

My changes affect the following components

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.

codecov-io · 2018-11-18T06:30:16Z

Codecov Report

Merging #4122 into master will decrease coverage by 5.38%.
The diff coverage is 57.14%.

@@            Coverage Diff            @@
##           master   #4122      +/-   ##
=========================================
- Coverage   86.48%   81.1%   -5.39%     
=========================================
  Files         151     151              
  Lines        7272    7276       +4     
  Branches      468     467       -1     
=========================================
- Hits         6289    5901     -388     
- Misses        983    1375     +392

Impacted Files	Coverage Δ
...openwhisk/core/entitlement/PackageCollection.scala	`100% <ø> (ø)`	⬆️
...cala/org/apache/openwhisk/http/ErrorResponse.scala	`92.22% <0%> (-1.04%)`	⬇️
...rg/apache/openwhisk/core/controller/Packages.scala	`88.57% <66.66%> (-0.65%)`	⬇️
...core/database/cosmosdb/RxObservableImplicits.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	`0% <0%> (-62.5%)`	⬇️
...whisk/connector/kafka/KafkaConsumerConnector.scala	`59.7% <0%> (-25.38%)`	⬇️
...in/scala/org/apache/openwhisk/common/Counter.scala	`40% <0%> (-20%)`	⬇️
... and 3 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 83de20e...b40c898. Read the comment docs.

asteed · 2018-11-18T06:41:11Z

core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala

@@ -243,7 +243,7 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities {
      wp.binding map {
        // pre-existing entity is a binding, check that new binding is valid
        b =>
-          checkBinding(b.fullyQualifiedName)
+          checkBinding(binding.fullyQualifiedName)


this was previously checking the existing binding, and not the incoming binding from content.binding

indeed - this is a bug.
i would change the line to _ => checkBinding(binding.fullyQualifiedName)

This also suggests that we need a test along these lines:

wsk package create p wsk package bind p b wsk package delete p wsk package create p2 wsk package bind p2 b # will fail without your patch, and should succeed with

the error message for fetching a binding missing its references package is confusing - we should open an issue to also fix that.

currently returns: The requested resource 'guest/b' does not exist. instead of Binding references a package that does not exist.

@rabbah I don't see how the last line (wsk package update p2 b) will succeed currently. As b is a binding already, and updating a binding to another binding was already not allowed

Packages.scala:L206

// trying to create a new package binding that refers to another binding case provider if provider.binding.nonEmpty => Future.failed(RejectRequest(BadRequest, Messages.bindingCannotReferenceBinding))

While attempting to run your suggested test, I noticed that the command wsk update p2 b isn't accepted, and I'm wondering whether you intended to use wsk bind p2 b or another operation? Since p2 is created as a package it shouldn't allow it to be updated as a binding (from my understanding). My apologies if my inference is incorrect here.

$ wsk -i package create p ok: created package p $ wsk -i package bind p b ok: created binding b $ wsk -i package delete p ok: deleted package p $ wsk -i package create p2 ok: created package p2 $ wsk -i package update p2 b error: Invalid argument(s): b. A package name is required. Run 'wsk --help' for usage. $ wsk -i package bind p2 b error: Binding creation failed: resource already exists (code MjyhYzbZlRWsxjpmNToG4enhbK1j6EYz)

the error message for fetching a binding missing its references package is confusing - we should open an issue to also fix that.

Do you mean that after p is deleted, attempting to fetch b is failing with that error?

@rabbah note that your example (with ?overwrite=true) returns 400 as expected today as pointed out in the source.

PUT /api/v1/namespaces/guest/packages/b?overwrite=true HTTP/1.1 Content-Type: application/json {"binding":{"namespace":"guest","name":"p2"}}

HTTP/1.1 400 Bad Request Content-Type: application/json {"error":"Binding references a package that does not exist.","code":"HWkcauMms0QCAFNs4mhDCrtx7EbqHBWF"}

Do you still think there is another failure case we should resolve in this PR?

the error message for fetching a binding missing its references package is confusing - we should open an issue to also fix that.

currently returns: The requested resource 'guest/b' does not exist. instead of Binding references a package that does not exist.

I've created issue #4130 to track this

@asteed the error "Binding references a package that does not exist" is wrong since p2 does exist. The controller tried to look up p which was deleted and hence the reason the error is wrong. If the error contained the missing package, it would be:

> wsk package bind p2 b error: Binding references a package 'p' that does not exist.

which is nonsensical since we're requesting to bind p2 and p is no longer relevant.

@rabbah Thanks, I didn't catch the incorrect error originally, as I took your comments to suggest another area where we could encounter the looping scenario.

Do you think this should be addressed in this PR or filed as a separate bug similar to (or included in) #4130 ?

i think your patch may fix/hide this bug.
i'm ok with a separate pr if the test can be written against the new code.

rabbah

Thanks for catching and fixing this.

rabbah · 2018-11-18T14:44:54Z

core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala

-        })
+        if (docid == wp.docid) {
+          logging.error(this, "unexpected package binding refers to itself: $docid")
+          terminate(InternalServerError)


this should be a 40x bad request/user error not a server error; in any case, i don't see a test for this response (which may not be possible after your fix i think but we can unit test this method separately).

Two points about this:

I'm not quite sure I 100% agree with a 400 Bad Request in this situation as there is nothing in fact wrong with the request, however there is entirely something wrong with the stored state of the server. While I also don't agree with a 500 Internal Server Error I don't see a better alternative for either. While it's use is intended for WebDAV, it almost appears that 508 Loop Detected would be the most accurate description :)

There is the attempt at performing the get on the object afterward, and that the test checks it is a 200 response, however you're correct that with the fix for ingest in place it would only be possible to manually craft this bad record (and I'm unable to determine how to do that given the current tests)

@rabbah WDYT?

We've avoided using some of the WebDAV codes (207 was another I used at one point and we removed although it was applicable for us in practice). I agree with you that if the data record is corrupted and now circular, it's not the client's fault but it's also not entirely out of their control (deleting the record should restore a proper invariant and the client can resume their operations).

If we keep the 500 error, we could at least provide a more meaningful error message.

Seems like a 422 is pretty spot on "422 UNPROCESSABLE ENTITY"

https://tools.ietf.org/html/rfc4918#section-11.2

I can agree on 422 Unprocessable Entity - I'll make those changes.

rabbah · 2018-11-18T14:52:59Z

core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala

@@ -243,7 +243,7 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities {
      wp.binding map {
        // pre-existing entity is a binding, check that new binding is valid
        b =>
-          checkBinding(b.fullyQualifiedName)
+          checkBinding(binding.fullyQualifiedName)


indeed - this is a bug.
i would change the line to _ => checkBinding(binding.fullyQualifiedName)

rabbah · 2018-11-18T15:02:10Z

core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala

@@ -243,7 +243,7 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities {
      wp.binding map {
        // pre-existing entity is a binding, check that new binding is valid
        b =>
-          checkBinding(b.fullyQualifiedName)
+          checkBinding(binding.fullyQualifiedName)


This also suggests that we need a test along these lines:

wsk package create p wsk package bind p b wsk package delete p wsk package create p2 wsk package bind p2 b # will fail without your patch, and should succeed with

rabbah · 2018-11-18T15:05:31Z

core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala

@@ -243,7 +243,7 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities {
      wp.binding map {
        // pre-existing entity is a binding, check that new binding is valid
        b =>
-          checkBinding(b.fullyQualifiedName)
+          checkBinding(binding.fullyQualifiedName)


the error message for fetching a binding missing its references package is confusing - we should open an issue to also fix that.

currently returns: The requested resource 'guest/b' does not exist. instead of Binding references a package that does not exist.

rabbah · 2018-11-18T15:12:33Z

core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/PackageCollection.scala

@@ -98,7 +98,10 @@ class PackageCollection(entityStore: EntityStore)(implicit logging: Logging) ext
          val pkgOwner = namespaces.contains(binding.namespace.asString)
          val pkgDocid = binding.docid
          logging.debug(this, s"checking subject has privilege '$right' for bound package '$pkgDocid'")
-          checkPackageReadPermission(namespaces, pkgOwner, pkgDocid)
+          if (doc == pkgDocid)


should this be treated as an error also?

The way the flow works here, this is a precursor to the call into mergePackageWithBinding (where we now will fail). My rationalization here was that this is merely a permissions check, and if we're checking the children of self, we've already checked self and that passed or we'd not be here.

I'm up to suggestions on whether this should fail, however. WDYT?

I'm inclined to fail early since this too is a circular reference that indicates a corrupted or buggy record. Your point is valid though 🤔

rabbah

one small bug to fix otherwise LGTM.

core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala

core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/PackageCollection.scala

… check validity

…who is bound to itself

…sions check

chetanmeh · 2018-11-29T03:51:22Z

@asteed Should we also update swagger definition for 422 status code response in package api?

…4122) Use incoming package binding for update when calling checkBinding and add protection to prevent following circular package bindings.

asteed commented Nov 18, 2018

View reviewed changes

rabbah reviewed Nov 18, 2018

View reviewed changes

asteed mentioned this pull request Nov 21, 2018

Inconsistent error when retrieving a package bound to deleted package #4130

Open

rabbah self-assigned this Nov 23, 2018

rabbah added bug controller review Review for this PR has been requested and yet needs to be done. awaits-contributor The contributor needs to respond to comments from reviewer. labels Nov 23, 2018

asteed force-pushed the andsteed/binding-protection branch from c029c83 to 5db5977 Compare November 28, 2018 15:21

rabbah approved these changes Nov 28, 2018

View reviewed changes

core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala Outdated Show resolved Hide resolved

core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/PackageCollection.scala Outdated Show resolved Hide resolved

Andy Steed added 3 commits November 28, 2018 12:40

Use incoming binding instead of existing when calling checkBinding to…

1ae2919

… check validity

Add protection to prevent following circular references to a package …

f208719

…who is bound to itself

Review feedback - Switch to 422 response code and error during permis…

14e2011

…sions check

rabbah force-pushed the andsteed/binding-protection branch from 5db5977 to 14e2011 Compare November 28, 2018 17:41

rabbah added ready and removed awaits-contributor The contributor needs to respond to comments from reviewer. labels Nov 28, 2018

Fix log messages.

b40c898

rabbah force-pushed the andsteed/binding-protection branch from 6cf7aea to b40c898 Compare November 28, 2018 18:00

tysonnorris merged commit cba3b7b into apache:master Nov 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protect Package Bindings from containing circular references #4122

Protect Package Bindings from containing circular references #4122

asteed commented Nov 18, 2018

codecov-io commented Nov 18, 2018 •

edited

Loading

asteed Nov 18, 2018

rabbah Nov 18, 2018

rabbah Nov 18, 2018 •

edited

Loading

rabbah Nov 18, 2018

asteed Nov 19, 2018 •

edited

Loading

asteed Nov 21, 2018

asteed Nov 21, 2018

rabbah Nov 22, 2018

asteed Nov 26, 2018

rabbah Nov 26, 2018 •

edited

Loading

rabbah left a comment •

edited

Loading

rabbah Nov 18, 2018

asteed Nov 19, 2018

asteed Nov 21, 2018

rabbah Nov 22, 2018

mcdan Nov 26, 2018

asteed Nov 26, 2018

rabbah Nov 18, 2018

rabbah Nov 18, 2018 •

edited

Loading

rabbah Nov 18, 2018

rabbah Nov 18, 2018

asteed Nov 19, 2018 •

edited

Loading

rabbah Nov 22, 2018

rabbah left a comment

chetanmeh commented Nov 29, 2018

Protect Package Bindings from containing circular references #4122

Protect Package Bindings from containing circular references #4122

Conversation

asteed commented Nov 18, 2018

Description

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

codecov-io commented Nov 18, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rabbah Nov 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asteed Nov 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rabbah Nov 26, 2018 • edited Loading

Choose a reason for hiding this comment

rabbah left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rabbah Nov 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asteed Nov 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rabbah left a comment

Choose a reason for hiding this comment

chetanmeh commented Nov 29, 2018

codecov-io commented Nov 18, 2018 •

edited

Loading

rabbah Nov 18, 2018 •

edited

Loading

asteed Nov 19, 2018 •

edited

Loading

rabbah Nov 26, 2018 •

edited

Loading

rabbah left a comment •

edited

Loading

rabbah Nov 18, 2018 •

edited

Loading

asteed Nov 19, 2018 •

edited

Loading