Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 12 commits into from
Oct 14, 2020
Prev Previous commit
Next Next commit
Partition params into locked and unlocked sets.
rabbah committed Oct 13, 2020

Partially verified

This commit is signed with the committer’s verified signature.
spydon’s contribution has been verified via GPG key.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
commit 31ab9353d263e46e3b6a5eb20c8fcb6c0ee0ad39
Original file line number Diff line number Diff line change
@@ -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,
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Contributor

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

cause: Option[ActivationId] = None,
traceContext: Option[Map[String, String]] = None)
extends Message {
@@ -171,7 +172,7 @@ object ActivationMessage extends DefaultJsonProtocol {
def parse(msg: String) = Try(serdes.read(msg.parseJson))

private implicit val fqnSerdes = FullyQualifiedEntityName.serdes
implicit val serdes = jsonFormat11(ActivationMessage.apply)
implicit val serdes = jsonFormat12(ActivationMessage.apply)
}

object CombinedCompletionAndResultMessage extends DefaultJsonProtocol {
Original file line number Diff line number Diff line change
@@ -21,7 +21,6 @@ import org.apache.openwhisk.core.entity.size.{SizeInt, SizeString}
import spray.json.DefaultJsonProtocol._
import spray.json._

import scala.collection.immutable.ListMap
import scala.language.postfixOps
import scala.util.{Failure, Success, Try}

@@ -74,9 +73,17 @@ protected[core] class Parameters protected[entity] (private val params: Map[Para
params.keySet filter (params(_).init) map (_.name)
}

/** Gets list all locked (encrypted) parameters. */
protected[core] def lockedParameters: Map[String, String] = {
params.collect {
case p if p._2.encryption.isDefined => (p._1.name -> p._2.encryption.get)
}
}

protected[core] def getMap = {
params
}

protected[core] def toJsArray = {
JsArray(params map { p =>
val init = if (p._2.init) Some("init" -> JsTrue) else None
@@ -86,14 +93,7 @@ protected[core] class Parameters protected[entity] (private val params: Map[Para
} toSeq: _*)
}

protected[core] def toJsObject =
JsObject(params.map(p => {
val newValue =
p._2.encryption
.map(e => JsObject("value" -> p._2.value, "init" -> p._2.init.toJson, "encryption" -> e.toJson))
.getOrElse(p._2.value)
(p._1.name, newValue)
}))
protected[core] def toJsObject = JsObject(params.map(p => (p._1.name -> p._2.value.toJson)))

override def toString = toJsArray.compactPrint

@@ -134,28 +134,27 @@ protected[core] class Parameters protected[entity] (private val params: Map[Para
/**
* 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 = {
def lock(encoder: Encrypter): Parameters = {
new Parameters(params.map {
case (paramName, paramValue) if paramValue.encryption.isEmpty =>
paramName -> encoder.getOrElse(ParameterEncryption.singleton.default).encrypt(paramValue)
paramName -> encoder.encrypt(paramValue)
case p => p
})
}

/**
* 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: Option[ParameterEncryption] = None): Parameters = {
def unlock(decoder: ParameterEncryption): Parameters = {
new Parameters(params.map {
case (paramName, paramValue) if paramValue.encryption.nonEmpty =>
paramName -> decoder
.getOrElse(ParameterEncryption.singleton)
.encryptor(paramValue.encryption)
.decrypt(paramValue)
case (paramName, paramValue) if paramValue.encryption.isDefined =>
paramName -> decoder.encryptor(paramValue.encryption).decrypt(paramValue)
case p => p
})
}
@@ -265,29 +264,6 @@ protected[core] object Parameters extends ArgNormalizer[Parameters] {
ParameterValue(Option(v).getOrElse(JsNull), false, None))
}

def readMergedList(value: JsValue): Parameters =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Try {

val JsObject(obj) = value
new Parameters(
obj
.map((tuple: (String, JsValue)) => {
val key = new ParameterName(tuple._1)
val paramVal: ParameterValue = tuple._2 match {
case o: JsObject =>
o.getFields("value", "init", "encryption") match {
case Seq(v: JsValue, JsBoolean(i), JsString(e)) =>
ParameterValue(v, i, Some(e))
case _ => ParameterValue(o, false, None)
}
case v: JsValue => ParameterValue(v, false, None)
}
(key, paramVal)
})
.toMap)
} getOrElse deserializationError(
"parameters malformed, could not get a JsObject from: " + (if (value != null) value.toString() else ""))

override protected[core] implicit val serdes = new RootJsonFormat[Parameters] {
def write(p: Parameters) = p.toJsArray

@@ -298,39 +274,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) =
Try {
val JsArray(params) = value
params
} flatMap {
read(_)
} getOrElse {
Try {
var converted = new ListMap[ParameterName, ParameterValue]()
val JsObject(o) = value
o.foreach(i =>
i._2.asJsObject.getFields("value", "init", "encryption") match {
case Seq(v: JsValue, JsBoolean(init), JsString(e)) =>
val key = new ParameterName(i._1)
val value = ParameterValue(v, init, Some(e))
converted = converted + (key -> value)
case Seq(v: JsValue, JsBoolean(init)) =>
val key = new ParameterName(i._1)
val value = ParameterValue(v, init)
converted = converted + (key -> value)
case Seq(v: JsValue, JsBoolean(init), JsNull) =>
val key = new ParameterName(i._1)
val value = ParameterValue(v, init)
converted = converted + (key -> value)
})
if (converted.size == 0) {
deserializationError("parameters malformed no parameters available: " + value.toString)
} else {
new Parameters(converted)
}
} getOrElse deserializationError(
"parameters malformed could not read directly: " + (if (value != null) value.toString else ""))
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.
@@ -348,15 +297,19 @@ protected[core] object Parameters extends ArgNormalizer[Parameters] {
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)) =>
case Seq(JsString(k), v: JsValue, JsBoolean(i), JsNull) =>
val key = new ParameterName(k)
val value = ParameterValue(v, i)
val value = ParameterValue(v, i, None)
(key, value)
case Seq(JsString(k), v: JsValue, JsString(e)) if (i.asJsObject.fields.contains("encryption")) =>
case Seq(JsString(k), v: JsValue, JsString(e)) =>
val key = new ParameterName(k)
val value = ParameterValue(v, false, Some(e))
(key, value)
Original file line number Diff line number Diff line change
@@ -46,6 +46,10 @@ protected[core] case class ParameterEncryption(default: Encrypter, encryptors: M
encryptors.get(name.getOrElse(default.name)).getOrElse(ParameterEncryption.noop)
}

def encryptor(name: String): Encrypter = {
encryptors.get(name).getOrElse(ParameterEncryption.noop)
}

}

protected[core] object ParameterEncryption {
@@ -79,6 +83,7 @@ protected[core] trait Encrypter {
val name: String
def encrypt(p: ParameterValue): ParameterValue = p
def decrypt(p: ParameterValue): ParameterValue = p
def decrypt(v: JsString): JsValue = v
}

protected[core] object Encrypter {
@@ -116,8 +121,15 @@ protected[core] trait AesEncryption extends Encrypter {
ParameterValue(JsString(Base64.getEncoder.encodeToString(cipherMessage)), value.init, Some(name))
}

override def decrypt(value: ParameterValue): ParameterValue = {
val cipherMessage = value.value.convertTo[String].getBytes(StandardCharsets.UTF_8)
override def decrypt(p: ParameterValue): ParameterValue = {
p.value match {
case s: JsString => p.copy(v = decrypt(s), encryption = None)
case _ => p
}
}

override def decrypt(value: JsString): JsValue = {
val cipherMessage = value.convertTo[String].getBytes(StandardCharsets.UTF_8)
val byteBuffer = ByteBuffer.wrap(Base64.getDecoder.decode(cipherMessage))
val ivLength = byteBuffer.getInt
if (ivLength != ivLen) {
@@ -133,7 +145,7 @@ protected[core] trait AesEncryption extends Encrypter {
cipher.init(Cipher.DECRYPT_MODE, secretKey, gcmSpec)
val plainTextBytes = cipher.doFinal(cipherText)
val plainText = new String(plainTextBytes, StandardCharsets.UTF_8)
ParameterValue(plainText.parseJson, value.init)
plainText.parseJson
}

}
Original file line number Diff line number Diff line change
@@ -384,7 +384,9 @@ object WhiskAction extends DocumentFactory[WhiskAction] with WhiskEntityQueries[
val stream = new ByteArrayInputStream(bytes)
super.putAndAttach(
db,
doc.copy(parameters = doc.parameters.lock()).revision[WhiskAction](doc.rev),
doc
.copy(parameters = doc.parameters.lock(ParameterEncryption.singleton.default))
.revision[WhiskAction](doc.rev),
attachmentUpdater,
attachmentType,
stream,
@@ -404,7 +406,12 @@ object WhiskAction extends DocumentFactory[WhiskAction] with WhiskEntityQueries[
case exec @ BlackBoxExec(_, Some(Inline(code)), _, _, binary) =>
putWithAttachment(code, binary, exec)
case _ =>
super.put(db, doc.copy(parameters = doc.parameters.lock()).revision[WhiskAction](doc.rev), old)
super.put(
db,
doc
.copy(parameters = doc.parameters.lock(ParameterEncryption.singleton.default))
.revision[WhiskAction](doc.rev),
old)
}
} match {
case Success(f) => f
Original file line number Diff line number Diff line change
@@ -205,7 +205,10 @@ object WhiskPackage
override def put[A >: WhiskPackage](db: ArtifactStore[A], doc: WhiskPackage, old: Option[WhiskPackage])(
implicit transid: TransactionId,
notifier: Option[CacheChangeNotification]): Future[DocInfo] = {
super.put(db, doc.copy(parameters = doc.parameters.lock()).revision[WhiskPackage](doc.rev), old)
super.put(
db,
doc.copy(parameters = doc.parameters.lock(ParameterEncryption.singleton.default)).revision[WhiskPackage](doc.rev),
old)
}
}

Original file line number Diff line number Diff line change
@@ -179,6 +179,7 @@ protected[actions] trait PrimitiveActions {
waitForResponse.isDefined,
args,
action.parameters.initParameters,
action.parameters.lockedParameters,
cause = cause,
WhiskTracerProvider.tracer.getTraceContext(transid))

Original file line number Diff line number Diff line change
@@ -416,7 +416,8 @@ class InvokerActor(invokerInstance: InvokerInstanceId, controllerInstance: Contr
rootControllerIndex = controllerInstance,
blocking = false,
content = None,
initArgs = Set.empty)
initArgs = Set.empty,
lockedArgs = Map.empty)

context.parent ! ActivationRequest(activationMessage, invokerInstance)
}
Original file line number Diff line number Diff line change
@@ -774,9 +774,10 @@ class ContainerProxy(factory: (TransactionId,
def initializeAndRun(container: Container, job: Run, reschedule: Boolean = false)(
implicit tid: TransactionId): Future[WhiskActivation] = {
val actionTimeout = job.action.limits.timeout.duration
val unlockedContent = job.msg.content.map(Parameters.readMergedList(_).unlock().toJsObject)
val unlockedArgs =
ContainerProxy.unlockArguments(job.msg.content, job.msg.lockedArgs, ParameterEncryption.singleton)

val (env, parameters) = ContainerProxy.partitionArguments(unlockedContent, job.msg.initArgs)
val (env, parameters) = ContainerProxy.partitionArguments(unlockedArgs, job.msg.initArgs)

val environment = Map(
"namespace" -> job.msg.user.namespace.name.toJson,
@@ -1089,6 +1090,23 @@ object ContainerProxy {
(env, JsObject(args))
}
}

def unlockArguments(content: Option[JsObject],
lockedArgs: Map[String, String],
decoder: ParameterEncryption): Option[JsObject] = {
content.map {
case JsObject(fields) =>
JsObject(fields.map {
case (k, o: JsObject) =>
o.getFields("value", "encryption") match {
case Seq(s: JsString, JsString(e)) => (k -> decoder.encryptor(e).decrypt(s))
case Seq(v: JsValue, JsNull) => (k -> v)
}

case p => p
})
}
}
}

object TCPPingClient {
Original file line number Diff line number Diff line change
@@ -87,7 +87,8 @@ class ContainerPoolTests
ControllerInstanceId("0"),
blocking = false,
content = None,
initArgs = Set.empty)
initArgs = Set.empty,
lockedArgs = Map.empty)
Run(action, message)
}

Original file line number Diff line number Diff line change
@@ -125,7 +125,8 @@ class ContainerProxyTests
ControllerInstanceId("0"),
blocking = false,
content = Some(activationArguments),
initArgs = Set("ENV_VAR"))
initArgs = Set("ENV_VAR"),
lockedArgs = Map.empty)

/*
* Helpers for assertions and actor lifecycles
Loading