Improvements to parameter encryption to support per-namespace keys #4855
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,13 +17,13 @@ | |
|
|
||
| package org.apache.openwhisk.core.entity | ||
|
|
||
| import scala.util.{Failure, Success, Try} | ||
| import spray.json.DefaultJsonProtocol._ | ||
| import spray.json._ | ||
|
|
||
| import scala.language.postfixOps | ||
| import org.apache.openwhisk.core.entity.size.SizeInt | ||
| import org.apache.openwhisk.core.entity.size.SizeString | ||
|
|
||
| /** | ||
| * Parameters is a key-value map from parameter names to parameter values. The value of a | ||
|
|
@@ -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] (protected[entity] val params: Map[ParameterName, ParameterValue]) | ||
|
There was a problem hiding this comment. @mcdan this removes |
||
| extends AnyVal { | ||
|
|
||
| /** | ||
|
|
@@ -46,13 +46,6 @@ protected[core] class Parameters protected[entity] (private val params: Map[Para | |
| .foldLeft(0 B)(_ + _) | ||
| } | ||
|
|
||
|
|
||
| protected[entity] def +(p: ParameterName, v: ParameterValue) = { | ||
| new Parameters(params + (p -> v)) | ||
| } | ||
|
|
@@ -71,43 +64,35 @@ protected[core] class Parameters protected[entity] (private val params: Map[Para | |
| Try(new Parameters(params - new ParameterName(p))) getOrElse this | ||
| } | ||
|
|
||
| /** Gets set of all defined parameters. */ | ||
| protected[core] def definedParameters: Set[String] = { | ||
| params.keySet filter (params(_).isDefined) map (_.name) | ||
| } | ||
|
|
||
| /** Gets set of all defined parameters. */ | ||
| protected[core] def initParameters: Set[String] = { | ||
| params.keySet filter (params(_).init) map (_.name) | ||
| } | ||
|
|
||
| /** | ||
| * Gets map of all locked (encrypted) parameters, excluding parameters from given set. | ||
| */ | ||
| protected[core] def lockedParameters(exclude: Set[String] = Set.empty): Map[String, String] = { | ||
| params.collect { | ||
| case p if p._2.encryption.isDefined && !exclude.contains(p._1.name) => (p._1.name -> p._2.encryption.get) | ||
| } | ||
| } | ||
|
|
||
| protected[core] def toJsArray = { | ||
| JsArray(params map { p => | ||
| val init = if (p._2.init) Some("init" -> JsTrue) else None | ||
| val encrypt = p._2.encryption.map(e => ("encryption" -> JsString(e))) | ||
|
|
||
| JsObject(Map("key" -> p._1.name.toJson, "value" -> p._2.value) ++ init ++ encrypt) | ||
| } toSeq: _*) | ||
| } | ||
|
|
||
| protected[core] def toJsObject = JsObject(params.map(p => (p._1.name -> p._2.value.toJson))) | ||
|
|
||
| override def toString = toJsArray.compactPrint | ||
|
|
||
|
|
@@ -144,6 +129,40 @@ protected[core] class Parameters protected[entity] (private val params: Map[Para | |
| case _ => true | ||
| } getOrElse valueForNonExistent | ||
| } | ||
|
|
||
| /** | ||
| * Encrypts any parameters that are not yet encoded. | ||
| * | ||
| * @param encoder the encoder to transform parameter values with | ||
| * @return parameters with all values encrypted | ||
| */ | ||
| def lock(encoder: Option[Encrypter] = None): Parameters = { | ||
| encoder | ||
| .map { coder => | ||
| new Parameters(params.map { | ||
| case (paramName, paramValue) if paramValue.encryption.isEmpty => | ||
| paramName -> coder.encrypt(paramValue) | ||
| case p => p | ||
| }) | ||
| } | ||
| .getOrElse(this) | ||
| } | ||
|
|
||
| /** | ||
| * Decodes parameters. If the encryption scheme for a parameter is not recognized, it is not modified. | ||
| * | ||
| * @param decoder the decoder to use to transform locked values | ||
| * @return parameters will all values decoded (where scheme is known) | ||
| */ | ||
| def unlock(decoder: ParameterEncryption): Parameters = { | ||
| new Parameters(params.map { | ||
| case p @ (paramName, paramValue) => | ||
| paramValue.encryption | ||
| .map(paramName -> decoder.encryptor(_).decrypt(paramValue)) | ||
| .getOrElse(p) | ||
| }) | ||
| } | ||
|
|
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -175,21 +194,18 @@ protected[entity] class ParameterName protected[entity] (val name: String) exten | |
| * | ||
| * @param v the value of the parameter, may be null | ||
| * @param init if true, this parameter value is only offered to the action during initialization | ||
| * @param encryption the name of the encryption algorithm used to store the parameter or none (plain text) | ||
| */ | ||
| protected[entity] case class ParameterValue protected[entity] (private val v: JsValue, | ||
| val init: Boolean, | ||
| val encryption: Option[String] = None) { | ||
|
|
||
| /** @return JsValue if defined else JsNull. */ | ||
| protected[entity] def value = Option(v) getOrElse JsNull | ||
|
|
||
| /** @return true iff value is not JsNull. */ | ||
| protected[entity] def isDefined = value != JsNull | ||
|
|
||
| /** | ||
| * The size of the ParameterValue entity as ByteSize. | ||
| */ | ||
|
|
@@ -208,8 +224,8 @@ protected[core] object Parameters extends ArgNormalizer[Parameters] { | |
| * Creates a parameter tuple from a pair of strings. | ||
| * A convenience method for tests. | ||
| * | ||
| * @param p the parameter name | ||
| * @param v the parameter value | ||
| * @param init the parameter is for initialization | ||
| * @return (ParameterName, ParameterValue) | ||
| * @throws IllegalArgumentException if key is not defined | ||
|
|
@@ -224,8 +240,8 @@ protected[core] object Parameters extends ArgNormalizer[Parameters] { | |
| /** | ||
| * Creates a parameter tuple from a parameter name and JsValue. | ||
| * | ||
| * @param p the parameter name | ||
| * @param v the parameter value | ||
| * @param init the parameter is for initialization | ||
| * @return (ParameterName, ParameterValue) | ||
| * @throws IllegalArgumentException if key is not defined | ||
|
|
@@ -252,29 +268,6 @@ protected[core] object Parameters extends ArgNormalizer[Parameters] { | |
| ParameterValue(Option(v).getOrElse(JsNull), false, None)) | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. @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. There was a problem hiding this comment. Yea that's a simpler way of looking at it but that changed the wire protocol for kafka right? |
||
| override protected[core] implicit val serdes = new RootJsonFormat[Parameters] { | ||
| def write(p: Parameters) = p.toJsArray | ||
|
|
||
|
|
@@ -285,35 +278,12 @@ protected[core] object Parameters extends ArgNormalizer[Parameters] { | |
| * @param parameters the JSON representation of an parameter array | ||
| * @return Parameters instance if parameters conforms to schema | ||
| */ | ||
| def read(value: JsValue): Parameters = { | ||
| value match { | ||
| case JsArray(params) => read(params).getOrElse(deserializationError("parameters malformed!")) | ||
| case _ => deserializationError("parameters malformed!") | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Gets parameters as a Parameters instances. | ||
|
|
@@ -323,29 +293,33 @@ protected[core] object Parameters extends ArgNormalizer[Parameters] { | |
| * @return Parameters instance if parameters conforms to schema | ||
| */ | ||
| def read(params: Vector[JsValue]) = Try { | ||
|
There was a problem hiding this comment. this change is mostly white space. |
||
| new Parameters(params.map { | ||
| case o @ JsObject(fields) => | ||
| o.getFields("key", "value", "init", "encryption") match { | ||
| case Seq(JsString(k), v: JsValue) if fields.contains("value") => | ||
| val key = new ParameterName(k) | ||
| val value = ParameterValue(v, false) | ||
| (key, value) | ||
| case Seq(JsString(k), v: JsValue, JsBoolean(i)) => | ||
| val key = new ParameterName(k) | ||
| val value = ParameterValue(v, i) | ||
| (key, value) | ||
| case Seq(JsString(k), v: JsValue, JsBoolean(i), JsString(e)) => | ||
| val key = new ParameterName(k) | ||
| val value = ParameterValue(v, i, Some(e)) | ||
| (key, value) | ||
| case Seq(JsString(k), v: JsValue, JsBoolean(i), JsNull) => | ||
| val key = new ParameterName(k) | ||
| val value = ParameterValue(v, i, None) | ||
| (key, value) | ||
| case Seq(JsString(k), v: JsValue, JsString(e)) | ||
| if fields.contains("value") && fields.contains("encryption") => | ||
| val key = new ParameterName(k) | ||
| val value = ParameterValue(v, false, Some(e)) | ||
| (key, value) | ||
| } | ||
| case _ => deserializationError("invalid parameter") | ||
| }.toMap) | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also revert this PR and merge it after 1.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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