Create AES128 and AES256 encryption for parameters #4756

mcdan · 2019-11-26T21:31:38Z

Encrypt just before putting into the db
Decrypt only right before invoking the action

Description

This will encrypt the default params for actions and packages at rest in the DB.

Currently implemented AES128 and 256 encryption, although 256 needs some work regarding packaging.

Related issue and scope

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 · 2019-11-29T14:00:36Z

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

-protected[entity] case class ParameterValue protected[entity] (private val v: JsValue, val init: Boolean) {
+protected[entity] case class ParameterValue protected[entity] (private val v: JsValue,
+                                                               val init: Boolean,
+                                                               val e: JsValue = JsNull) {


Why not just store the encrypted value in v and a boolean to indicate it's encrypted?

Adding which scheme would allow someone later to migrate from scheme A to B if an issue surfaces in the former.

how about something better than e - secret, or encrypted or something else?

should e be an option type (which would allow omission) - similarly would it be simpler if when present this value is meaningful (a string or bool and not null).

at this point i'm not sure what values this takes on yet.

Yup, I went with your suggestion and locked down what the encryption schemes are allowed to do.

mcdan · 2019-12-04T15:06:44Z

Can I get a WIP tag on this PR?

Encrypt just before putting into the db Decrypt only right before invoking the action

Broke reading the kafka protocol into a new method to keep the strict parsing of the scheme intact. Use only base64 encoded keys.

codecov-io · 2020-01-09T22:04:34Z

Codecov Report

Merging #4756 into master will decrease coverage by 41.34%.
The diff coverage is 42.18%.

@@             Coverage Diff             @@
##           master    #4756       +/-   ##
===========================================
- Coverage    85.1%   43.75%   -41.35%     
===========================================
  Files         190      198        +8     
  Lines        8578     8987      +409     
  Branches      601      621       +20     
===========================================
- Hits         7300     3932     -3368     
- Misses       1278     5055     +3777

Impacted Files	Coverage Δ
...rg/apache/openwhisk/core/entity/WhiskPackage.scala	`30.15% <100%> (-66.62%)`	⬇️
...org/apache/openwhisk/core/entity/WhiskAction.scala	`67.75% <100%> (-23.5%)`	⬇️
.../openwhisk/core/containerpool/ContainerProxy.scala	`67.54% <100%> (-24.43%)`	⬇️
.../scala/org/apache/openwhisk/core/WhiskConfig.scala	`85.92% <100%> (-8.86%)`	⬇️
...he/openwhisk/core/entity/ParameterEncryption.scala	`29.31% <29.31%> (ø)`
...a/org/apache/openwhisk/core/entity/Parameter.scala	`59.18% <46.77%> (-31.39%)`	⬇️
...pache/openwhisk/http/LenientSprayJsonSupport.scala	`0% <0%> (-100%)`	⬇️
...core/database/cosmosdb/RxObservableImplicits.scala	`0% <0%> (-100%)`	⬇️
...ore/containerpool/kubernetes/WhiskPodBuilder.scala	`0% <0%> (-100%)`	⬇️
...nwhisk/core/database/cosmosdb/CosmosDBConfig.scala	`0% <0%> (-100%)`	⬇️
... and 151 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 d9e4c10...363e2f4. Read the comment docs.

mcdan · 2020-01-10T14:51:08Z

WIP can be removed from this it seems ready to go.

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

rabbah · 2020-01-18T17:56:05Z

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

+        case true => Some("init" -> p._2.init.toJson)
+        case _    => None
+      }
+      val encrypt = p._2.encryption match {


can you add a comment what the valid/possible values of encryption are?

I added some better tests and took your advice to tighten up this field. At first I though this might be where I stuck the IVs or other stuff the encryption scheme wanted to hold on to but I ended up just sticking that inside the raw value given that it's already a byte array.

rabbah · 2020-01-18T17:56:50Z

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

@@ -129,7 +148,7 @@ protected[core] class Parameters protected[entity] (private val params: Map[Para

 /**
 * A ParameterName is a parameter name for an action or trigger to bind to its environment.
- * It wraps a normalized string as a value type.
+ * It wraps a normalized string as a valueread type.


rabbah · 2020-01-18T17:58:05Z

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

+        case _        => Some("encryption" -> p._2.encryption.toJson)
+      }
+      // Have do use this slightly strange construction to get the json object order identical.
+      JsObject(ListMap() ++ encrypt ++ init ++ Map("key" -> p._1.name.toJson, "value" -> p._2.value.toJson))


I do not understand what's happening here.

There were tests which I did not want to alter that checked the order of the values in the JSObject. To keep them working correctly I needed to add each property in a specific way. To my mind those tests shouldn't rely on the ordering of what is really a map but with JSON that is somewhat unclear around the ordering guarantee.

Agree that ordering shouldn't matter in general - without inspecting those tests, I wonder if the tests you're referring to are actually looking at precedence order (which properties should win when there are conflicts between two maps).

I double checked by mixing up the append order and this assert fails: https://github.com/apache/openwhisk/blob/master/tests/src/test/scala/org/apache/openwhisk/core/entity/test/SchemaTests.scala#L708
As it asserts the string equality of the json output. Given that I was hesitant to alter its order.

Fixed here 845241a.

rabbah · 2020-01-18T17:59:06Z

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

-protected[entity] case class ParameterValue protected[entity] (private val v: JsValue, val init: Boolean) {
+protected[entity] case class ParameterValue protected[entity] (private val v: JsValue,
+                                                               val init: Boolean,
+                                                               val e: JsValue = JsNull) {


how about something better than e - secret, or encrypted or something else?

rabbah · 2020-01-18T18:00:53Z

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

-protected[entity] case class ParameterValue protected[entity] (private val v: JsValue, val init: Boolean) {
+protected[entity] case class ParameterValue protected[entity] (private val v: JsValue,
+                                                               val init: Boolean,
+                                                               val e: JsValue = JsNull) {


should e be an option type (which would allow omission) - similarly would it be simpler if when present this value is meaningful (a string or bool and not null).

at this point i'm not sure what values this takes on yet.

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

rabbah

Thanks @mcdan for the clarifications - looking through the rest of the PR and tests.

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

rabbah · 2020-01-26T16:05:43Z

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

+        case _        => Some("encryption" -> p._2.encryption.toJson)
+      }
+      // Have do use this slightly strange construction to get the json object order identical.
+      JsObject(ListMap() ++ encrypt ++ init ++ Map("key" -> p._1.name.toJson, "value" -> p._2.value.toJson))


Agree that ordering shouldn't matter in general - without inspecting those tests, I wonder if the tests you're referring to are actually looking at precedence order (which properties should win when there are conflicts between two maps).

…verted to strings via spray.

use compactPrint + parseJson to convert to/from JSON/String

mcdan · 2020-02-13T22:03:22Z

Not sure why travis isn't updating this but the build is green for this: https://travis-ci.org/apache/openwhisk/builds/646477660
@rabbah is this okay to get merged?

rabbah · 2020-02-13T22:49:59Z

Thanks for the nudge. Will review the newest commits.

mcdan · 2020-02-14T14:14:18Z

No worries we have this running now on our prod cluster now so it's at least working.

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

rabbah · 2020-03-08T21:26:54Z

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

+    val byteBuffer = ByteBuffer.wrap(Base64.getDecoder.decode(cipherMessage))
+    val ivLength = byteBuffer.getInt
+    if (ivLength != ivLen) {
+      throw new IllegalArgumentException("invalid iv length")


decryption occurs in the invoker - this will therefore fail in the container proxy and we should add a test to cover failure and make sure the activation cleans up cleanly.

rabbah

I have gone through this PR as careful as I can. Once this PR is merged, I have some enhancements and refactoring which I'll follow up with.

After this PR, when parameter encryption is turned on, the parameters on action update and package update are encrypted using the current scheme. When an action or package is retrieved, the parameters are not decoded

Triggers parameters are currently not locked. This should be considered as a subsequent PR.

rabbah · 2020-03-09T23:56:03Z

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

-      response.updated.toEpochMilli should be > dummyUpdated
-    }
-  }
+//  it should "ignore updated field when updating action" in {


does this test fail?

rabbah · 2020-03-09T23:59:29Z

@mcdan given that github will reattribute the commit I squash and commit, I defer to you to rebase, and fix up the commit message/merge.

rabbah reviewed Nov 29, 2019

View reviewed changes

rabbah added the wip label Dec 4, 2019

mcdan force-pushed the mcdan/param-encrypt branch from e1e6e65 to 4e910e2 Compare December 12, 2019 16:30

mcdan closed this Dec 12, 2019

mcdan reopened this Dec 12, 2019

mcdan force-pushed the mcdan/param-encrypt branch 2 times, most recently from 8726e0f to 95f7693 Compare December 12, 2019 21:03

rabbah marked this pull request as ready for review December 12, 2019 22:06

mcdan force-pushed the mcdan/param-encrypt branch 2 times, most recently from 0b37d1f to fea10b9 Compare December 13, 2019 20:53

mcdan force-pushed the mcdan/param-encrypt branch 2 times, most recently from 0b1d895 to 132fcd6 Compare January 3, 2020 21:33

Create AES128 and AES256 encryption for parameters

da7bd79

Encrypt just before putting into the db Decrypt only right before invoking the action

mcdan force-pushed the mcdan/param-encrypt branch 3 times, most recently from 5c78b26 to 94ff4bd Compare January 9, 2020 21:09

mcdan added 2 commits January 9, 2020 16:15

Tighten up handling of a read of the params.

0353b92

Broke reading the kafka protocol into a new method to keep the strict parsing of the scheme intact. Use only base64 encoded keys.

Fixes due to master changes in pureconfig versions.

a94b2a8

mcdan force-pushed the mcdan/param-encrypt branch from 94ff4bd to a94b2a8 Compare January 9, 2020 21:15

rabbah added review Review for this PR has been requested and yet needs to be done. and removed wip labels Jan 10, 2020

rabbah self-requested a review January 10, 2020 15:19

rabbah reviewed Jan 18, 2020

View reviewed changes

Scala style fixups

a68f11e

mcdan force-pushed the mcdan/param-encrypt branch from cf4eb47 to d19ed21 Compare January 24, 2020 21:07

rabbah reviewed Jan 26, 2020

View reviewed changes

mcdan force-pushed the mcdan/param-encrypt branch from d19ed21 to 60033b2 Compare January 30, 2020 22:54

mcdan force-pushed the mcdan/param-encrypt branch from 60033b2 to 115cf1e Compare January 30, 2020 23:29

Tighten up types for encryption metadata.

10355da

mcdan force-pushed the mcdan/param-encrypt branch 3 times, most recently from a046bca to 071671a Compare February 3, 2020 16:48

Fix application conf documents and improve logging from tests

5c5d4a8

mcdan force-pushed the mcdan/param-encrypt branch from 071671a to 5c5d4a8 Compare February 3, 2020 17:26

Add test to prove migration between encrypters

b60b9e9

mcdan force-pushed the mcdan/param-encrypt branch from afe184d to b60b9e9 Compare February 3, 2020 20:36

mcdan and others added 3 commits February 5, 2020 11:00

Saw some errors with deploy sequences where arrays were not being con…

363e2f4

…verted to strings via spray.

use compactPrint + parseJson to convert to/from JSON/String

a6f73a7

Merge pull request #13 from adobe-apiplatform/tnorris/param-encrypt2

af04ff9

use compactPrint + parseJson to convert to/from JSON/String

rabbah reviewed Mar 8, 2020

View reviewed changes

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

rabbah reviewed Mar 8, 2020

View reviewed changes

rabbah approved these changes Mar 9, 2020

View reviewed changes

Merge branch 'master' into mcdan/param-encrypt

aa0f85a

mcdan self-assigned this Mar 11, 2020

mcdan merged commit 75f60ae into apache:master Mar 11, 2020

This was referenced Mar 11, 2020

enhance parameter encryption to support per-namespace keys #4854

Open

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create AES128 and AES256 encryption for parameters #4756

Create AES128 and AES256 encryption for parameters #4756

mcdan commented Nov 26, 2019

rabbah Nov 29, 2019 •

edited

Loading

mcdan Dec 2, 2019

rabbah Jan 18, 2020

rabbah Jan 18, 2020

mcdan Jan 24, 2020

mcdan commented Dec 4, 2019

codecov-io commented Jan 9, 2020 •

edited

Loading

mcdan commented Jan 10, 2020

rabbah Jan 18, 2020

mcdan Jan 24, 2020

rabbah Jan 18, 2020

rabbah Jan 18, 2020

mcdan Jan 24, 2020

rabbah Jan 26, 2020

mcdan Feb 3, 2020

rabbah Mar 8, 2020

rabbah Jan 18, 2020

rabbah Jan 18, 2020

rabbah left a comment

rabbah Jan 26, 2020

mcdan commented Feb 13, 2020

rabbah commented Feb 13, 2020

mcdan commented Feb 14, 2020

rabbah Mar 8, 2020

rabbah left a comment •

edited

Loading

rabbah Mar 9, 2020

rabbah commented Mar 9, 2020

Create AES128 and AES256 encryption for parameters #4756

Create AES128 and AES256 encryption for parameters #4756

Conversation

mcdan commented Nov 26, 2019

Description

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

rabbah Nov 29, 2019 • 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

mcdan commented Dec 4, 2019

codecov-io commented Jan 9, 2020 • edited Loading

Codecov Report

mcdan commented Jan 10, 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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcdan commented Feb 13, 2020

rabbah commented Feb 13, 2020

mcdan commented Feb 14, 2020

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

rabbah commented Mar 9, 2020

rabbah Nov 29, 2019 •

edited

Loading

codecov-io commented Jan 9, 2020 •

edited

Loading

rabbah left a comment •

edited

Loading