Improvements to parameter encryption to support per-namespace keys #4855

rabbah · 2020-03-11T17:31:17Z

This PR extends @mcdan's work from #4756 to allow for per-namespace encryption keys. Each commit is an incremental refactoring which might help in understanding the progression.

There are two singleton references to the encryption configuration - these can be replaced by identity-specific encryption configurations but requires additional work on extending the subject schema. I will stage that in a subsequent PR.

The refactoring here is intended to be semantic preserving.

Description

Related issue and scope

I opened an issue to propose and discuss this change (enhance parameter encryption to support per-namespace keys #4854)

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.

rabbah · 2020-03-11T17:32:48Z

common/scala/src/main/scala/org/apache/openwhisk/core/entity/Parameter.scala

@@ -46,13 +46,6 @@ protected[core] class Parameters protected[entity] (private val params: Map[Para
      .foldLeft(0 B)(_ + _)
  }

-  protected[entity] def +(p: (ParameterName, ParameterValue)) = {


this is dead code.

rabbah · 2020-03-11T17:33:06Z

common/scala/src/main/scala/org/apache/openwhisk/core/entity/Parameter.scala

@@ -32,7 +32,7 @@ import scala.util.{Failure, Success, Try}
 * @param key the parameter name, assured to be non-null because it is a value
 * @param value the parameter value, assured to be non-null because it is a value
 */
-protected[core] class Parameters protected[entity] (private val params: Map[ParameterName, ParameterValue])
+protected[core] class Parameters protected[entity] (protected[entity] val params: Map[ParameterName, ParameterValue])


@mcdan this removes getMap and restricts access to the entity package.

rabbah · 2020-03-11T17:33:50Z

common/scala/src/main/scala/org/apache/openwhisk/core/entity/Parameter.scala

@@ -323,29 +293,30 @@ protected[core] object Parameters extends ArgNormalizer[Parameters] {
     * @return Parameters instance if parameters conforms to schema
     */
    def read(params: Vector[JsValue]) = Try {
-      new Parameters(


this change is mostly white space.

rabbah · 2020-03-11T17:34:47Z

common/scala/src/main/scala/org/apache/openwhisk/core/entity/Parameter.scala

@@ -252,29 +268,6 @@ protected[core] object Parameters extends ArgNormalizer[Parameters] {
    ParameterValue(Option(v).getOrElse(JsNull), false, None))
  }

-  def readMergedList(value: JsValue): Parameters =


@mcdan by partitioning the arguments into locked and unlocked, I think the approach is simpler. The same information is crossing the message bus, but as two separate maps: the arguments, and the list of ones that are locked.

Yea that's a simpler way of looking at it but that changed the wire protocol for kafka right?

common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala

...ontroller/src/main/scala/org/apache/openwhisk/core/controller/actions/PrimitiveActions.scala

tests/src/test/scala/org/apache/openwhisk/core/controller/test/ActionsApiTests.scala

codecov-io · 2020-03-12T03:16:30Z

Codecov Report

Merging #4855 into master will decrease coverage by 47.10%.
The diff coverage is 50.00%.

@@             Coverage Diff             @@
##           master    #4855       +/-   ##
===========================================
- Coverage   83.10%   36.00%   -47.11%     
===========================================
  Files         202      202               
  Lines        9811     9786       -25     
  Branches      426      409       -17     
===========================================
- Hits         8153     3523     -4630     
- Misses       1658     6263     +4605

Impacted Files	Coverage Δ
...org/apache/openwhisk/core/entity/WhiskAction.scala	`81.42% <ø> (-10.93%)`	⬇️
...isk/core/controller/actions/PrimitiveActions.scala	`0.00% <0.00%> (-86.17%)`	⬇️
...enwhisk/core/loadBalancer/InvokerSupervision.scala	`0.00% <0.00%> (-89.44%)`	⬇️
...a/org/apache/openwhisk/core/entity/Parameter.scala	`66.19% <42.85%> (-22.58%)`	⬇️
...he/openwhisk/core/entity/ParameterEncryption.scala	`26.31% <51.72%> (-68.52%)`	⬇️
.../org/apache/openwhisk/core/connector/Message.scala	`64.28% <100.00%> (-11.91%)`	⬇️
...rg/apache/openwhisk/core/entity/WhiskPackage.scala	`58.33% <100.00%> (-38.34%)`	⬇️
.../openwhisk/core/containerpool/ContainerProxy.scala	`69.33% <100.00%> (-25.85%)`	⬇️
...a/org/apache/openwhisk/common/ConfigMapValue.scala	`0.00% <0.00%> (-100.00%)`	⬇️
.../apache/openwhisk/core/controller/Namespaces.scala	`0.00% <0.00%> (-100.00%)`	⬇️
... and 141 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 db59f6f...4c5ffa9. Read the comment docs.

rabbah · 2020-04-11T02:44:15Z

@mcdan might you have a chance to take a look at this PR?

mcdan · 2020-05-04T15:20:12Z

common/scala/src/main/scala/org/apache/openwhisk/core/entity/Parameter.scala

@@ -252,29 +268,6 @@ protected[core] object Parameters extends ArgNormalizer[Parameters] {
    ParameterValue(Option(v).getOrElse(JsNull), false, None))
  }

-  def readMergedList(value: JsValue): Parameters =


Yea that's a simpler way of looking at it but that changed the wire protocol for kafka right?

style95

I think operators would require two different sets of controllers, Kafkas, and invokers as this would change the ActivationMessage format.

reference: Improvements to parameter encryption to support per-namespace keys #4855 (comment)

style95 · 2020-10-05T04:59:07Z

common/scala/src/main/scala/org/apache/openwhisk/core/entity/ParameterEncryption.scala

+    override val name = NO_ENCRYPTION
+  }
+
+  val singleton: ParameterEncryption = {


It's not directly related to the change.
I am just curious if there is any benefit to having this explicit singleton while an object is already a singleton.

I think you're objecting to the name singleton - we need to store the config in the object. I can rename the variable.

Never mind.
I am fine with the name as well.

rabbah

I think operators would require two different sets of controllers, Kafkas, and invokers as this would change the ActivationMessage format.

You mean for a roll out? We've changed the kafka wire format before.
If this is a concern, we can hold this change over to post 1.0.

rabbah · 2020-10-06T12:50:46Z

common/scala/src/main/scala/org/apache/openwhisk/core/entity/ParameterEncryption.scala

+    override val name = NO_ENCRYPTION
+  }
+
+  val singleton: ParameterEncryption = {


I think you're objecting to the name singleton - we need to store the config in the object. I can rename the variable.

style95 · 2020-10-12T13:06:15Z

I think operators would require two different sets of controllers, Kafkas, and invokers as this would change the ActivationMessage format.

You mean for a roll out? We've changed the kafka wire format before.
If this is a concern, we can hold this change over to post 1.0.

I am fine with this.
We already have managed this kind of change as you said.
I think so does the other downstream.
I just wanted to let them know this is the change.

Simplify expression.

Add test for unlocking args in container proxy.

bdoyle0182 · 2020-10-14T23:49:22Z

common/scala/src/main/scala/org/apache/openwhisk/core/connector/Message.scala

@@ -57,6 +57,7 @@ case class ActivationMessage(override val transid: TransactionId,
                             blocking: Boolean,
                             content: Option[JsObject],
                             initArgs: Set[String] = Set.empty,
+                             lockedArgs: Map[String, String] = Map.empty,


Could this just be an option so it doesn't mess up backwards compatibility? Then make it non-optional in a subsequent commit. I'd be fine if we had the option commit before 1.0.0 and then you just immediately have a non-option commit (this commit) following it which is what's in 1.0.0. It would make our lives a lot easier since we don't have the capability to have two openwhisk clusters set up at once to do a deployment. I'm going to have to bring this up though with my team because the project operates that these things can be done so we can't keep up with this forever and will need to figure something out.

That's OK but perhaps not necessary? I think your concern is rolling updates. If the controller is updated first it serializes the message but the invoker deserializes it without the locked args field. If you update the invokers first, they can accept these messages but the controller doesn't send any. If there are no encrypted args, the map is empty anyway.

We can also revert this PR and merge it after 1.0.

Yea that's totally fine I wasn't thinking. I just did the rolling update for the last bump. As long as we have a rolling update between controllers and invokers it's not any issue

codecov-commenter · 2024-10-15T00:01:08Z

Codecov Report

Attention: Patch coverage is 50.00000% with 37 lines in your changes missing coverage. Please review.

Project coverage is 36.00%. Comparing base (db59f6f) to head (4c5ffa9).
Report is 260 commits behind head on master.

Files with missing lines	Patch %	Lines
...a/org/apache/openwhisk/core/entity/Parameter.scala	42.85%	20 Missing ⚠️
...he/openwhisk/core/entity/ParameterEncryption.scala	51.72%	14 Missing ⚠️
...enwhisk/core/loadBalancer/InvokerSupervision.scala	0.00%	2 Missing ⚠️
...isk/core/controller/actions/PrimitiveActions.scala	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #4855       +/-   ##
===========================================
- Coverage   83.10%   36.00%   -47.11%     
===========================================
  Files         202      202               
  Lines        9811     9786       -25     
  Branches      426      409       -17     
===========================================
- Hits         8153     3523     -4630     
- Misses       1658     6263     +4605

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rabbah requested a review from mcdan March 11, 2020 17:31

rabbah commented Mar 11, 2020

View reviewed changes

rabbah force-pushed the pr-4756-v2 branch from 5ef1408 to d88da9a Compare March 11, 2020 17:39

mcdan approved these changes May 4, 2020

View reviewed changes

rabbah force-pushed the pr-4756-v2 branch from a859976 to 0b0ec15 Compare May 13, 2020 13:24

rabbah force-pushed the pr-4756-v2 branch from 0b0ec15 to bda944c Compare August 6, 2020 21:51

style95 approved these changes Oct 6, 2020

View reviewed changes

rabbah commented Oct 6, 2020

View reviewed changes

rabbah added 12 commits October 13, 2020 13:27

Review notes and refactoring. No intended semantic change.

78d2009

Remove 'strange construction' of param as json.

5342b90

Simplify expression.

Remove unnecessary cons of Encrypter class.

5a7db0b

Refactoring of encryptor names.

e70abe7

Move lock/unlock to Parameters. Refactor tests.

bf72952

Partition params into locked and unlocked sets.

31ab935

Remove getter, make field protected/accessible for test.

3705c4e

Comments.

12ba958

Revert changes to test suite.

78c4f6a

Exclude overriden parameters from decryption.

af0bc08

Tighten tests.

1020e25

Add test for unlocking args in container proxy.

Fix test.

4c5ffa9

rabbah force-pushed the pr-4756-v2 branch from bda944c to 4c5ffa9 Compare October 13, 2020 17:28

style95 merged commit a1ba987 into apache:master Oct 14, 2020

bdoyle0182 reviewed Oct 14, 2020

View reviewed changes

rabbah deleted the pr-4756-v2 branch January 25, 2021 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements to parameter encryption to support per-namespace keys #4855

Improvements to parameter encryption to support per-namespace keys #4855

rabbah commented Mar 11, 2020 •

edited

Loading

rabbah Mar 11, 2020

rabbah Mar 11, 2020

rabbah Mar 11, 2020

rabbah Mar 11, 2020

mcdan May 4, 2020

codecov-io commented Mar 12, 2020 •

edited

Loading

rabbah commented Apr 11, 2020

mcdan May 4, 2020

style95 left a comment •

edited

Loading

style95 Oct 5, 2020

rabbah Oct 6, 2020

style95 Oct 12, 2020

rabbah left a comment

rabbah Oct 6, 2020

style95 commented Oct 12, 2020

bdoyle0182 Oct 14, 2020

rabbah Oct 15, 2020

rabbah Oct 15, 2020

bdoyle0182 Oct 15, 2020

codecov-commenter commented Oct 15, 2024

Improvements to parameter encryption to support per-namespace keys #4855

Improvements to parameter encryption to support per-namespace keys #4855

Conversation

rabbah commented Mar 11, 2020 • edited Loading

Description

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

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

codecov-io commented Mar 12, 2020 • edited Loading

Codecov Report

rabbah commented Apr 11, 2020

Choose a reason for hiding this comment

style95 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

rabbah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

style95 commented Oct 12, 2020

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

codecov-commenter commented Oct 15, 2024

Codecov Report

rabbah commented Mar 11, 2020 •

edited

Loading

codecov-io commented Mar 12, 2020 •

edited

Loading

style95 left a comment •

edited

Loading