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.

Sign up for GitHub

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

Jump to bottom

Fixes #8782: Fix details on script (remove $ from token and fix double password prefix) #1153

Merged
merged 1 commit into from
Jul 29, 2016
Merged

Fixes #8782: Fix details on script (remove $ from token and fix double password prefix) #1153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 44 additions & 42 deletions rudder-core/src/main/scala/com/normation/rudder/services/policies/JavascriptEngine.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,6 @@ final object HashOsType {
def all = sealerate.values[HashOsType]
}



/*
* Define implicit bytes from string methods
*/
Expand Down Expand Up @@ -307,7 +305,6 @@ object JsRudderLibBinding {
}
}


/**
* This class provides the Rhino (java 7) or Longhorn (Java 8 & up)
* java script engine.
Expand All @@ -318,7 +315,6 @@ object JsRudderLibBinding {
*/
final object JsEngineProvider {


/**
* Initialize a new JsEngine with the correct bindings.
*
Expand Down Expand Up @@ -358,48 +354,54 @@ sealed trait JsEngine {
*/
final object JsEngine {
// Several evals: one default and one JS (in the future, we may have several language)
final val DEFAULT_EVAL = "$eval:"
final val EVALJS = "$evaljs:"
final val DEFAULT_EVAL = "eval:"
final val EVALJS = "evaljs:"

final val PASSWORD_PREFIX = "plain:"
final val DEFAULT_EVAL_PASSWORD = PASSWORD_PREFIX+"$eval:"
final val EVALJS_PASSWORD = PASSWORD_PREFIX+"$evaljs:"
final val DEFAULT_EVAL_PASSWORD = PASSWORD_PREFIX+DEFAULT_EVAL
final val EVALJS_PASSWORD = PASSWORD_PREFIX+EVALJS

// From a variable, returns the two string we should check at the beginning of the variable value
// For a password, check if it's a plain text or not, otherwise check simply the eval keywords
def getEvaluatorTuple(variable: Variable) : (String, String, Boolean) = {
variable.spec.constraint.typeName match {
case t: AbstactPassword => (DEFAULT_EVAL_PASSWORD, EVALJS_PASSWORD, true)
case t: AbstactPassword => (DEFAULT_EVAL_PASSWORD, EVALJS_PASSWORD, true)
case _ => (DEFAULT_EVAL, EVALJS, false)
}
}

final object DisabledEngine extends JsEngine {
/*
* Eval does nothing on variable without the EVAL keyword, and
* fails on variable with the keyword.
*/
def eval(variable: Variable, lib: JsRudderLibBinding): Box[Variable] = {
val (default, js, _) = getEvaluatorTuple(variable)
variable.values.find( x => x.startsWith(default) || x.startsWith(js) ) match {
case None => Full(variable)
variable.values.find( x => x.startsWith(default)) match {
Copy link
Member

Choose a reason for hiding this comment

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

i think it's less readable to have two separated tests. Or maybe add a comment before (i guess it's for performance reason), and move the comme "Here, we nned to chose" just before this line

Copy link
Member Author

Choose a reason for hiding this comment

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

I find it less readable too, but it was not able to differentiate which EVAL was found,

Séparate them allow a much more précise error message

Le 29 juillet 2016 07:53:43 GMT+02:00, Nicolas Charles notifications@github.com a écrit :

final object DisabledEngine extends JsEngine {
/*
* Eval does nothing on variable without the EVAL keyword, and
* fails on variable with the keyword.
*/
def eval(variable: Variable, lib: JsRudderLibBinding):
Box[Variable] = {
val (default, js, _) = getEvaluatorTuple(variable)

  •  variable.values.find( x => x.startsWith(default) ||
    x.startsWith(js) ) match {
  •    case None    => Full(variable)

  •  variable.values.find( x => x.startsWith(default)) match {

i think it's less readable to have two separated tests. Or maybe add a
comment before (i guess it's for performance reason), and move the
comme "Here, we nned to chose" just before this line

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
https://github.com/Normation/rudder/pull/1153/files/150059137b7d15c19d9169ad39ce59aea863c83d#r72744578

Envoyé de mon appareil Android avec K-9 Mail. Veuillez excuser ma brièveté.

Copy link
Member

Choose a reason for hiding this comment

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

having the "Here, we need to chose betwwen" in between masks that they are related and do the same

/*
* Here, we need to chose between:
* - fails when the feature is disabled, but the string starts with $eval,
* meaning that maybe the user wanted to use it anyway.
* But that means that we are changing GENERATION behavior on existing prod,
* for a feature the user don't know anything.
* - not fails, because perhaps the user had that in its parameter. But in
* that case, it will fails when feature is enabled by default.
* And we risk to let the user spread sensitive information into nodes
* (because he thought the will be hashed, but in fact no).
*
* For now, failing because it seems to be the safe bet.
*/
case None =>
variable.values.find( x => x.startsWith(js)) match {
case None =>
Full(variable)
case Some(v) =>
Failure(s"Value '${v}' starts with the ${js} keyword, but the 'parameter evaluation feature' "
+"is disabled. Please, either don't use the keyword or enable the feature")
}
case Some(v) =>
/*
* Here, we need to chose between:
* - fails when the feature is disabled, but the string starts with $eval,
* meaning that maybe the user wanted to use it anyway.
* But that means that we are changing GENERATION behavior on existing prod,
* for a feature the user don't know anything.
* - not fails, because perhaps the user had that in its parameter. But in
* that case, it will fails when feature is enabled by default.
* And we risk to let the user spread sensitive information into nodes
* (because he thought the will be hashed, but in fact no).
*
* For now, failing because it seems to be the safe bet.
*/
Failure(s"Value '${v}' starts with the ${DEFAULT_EVAL} keyword, but the 'parameter evaluation feature' "
+"is disables. Please, either don't use the keyword or enable the feature")

Failure(s"Value '${v}' starts with the ${default} keyword, but the 'parameter evaluation feature' "
+"is disabled. Please, either don't use the keyword or enable the feature")
}
}
}
Expand Down Expand Up @@ -492,6 +494,15 @@ final object JsEngine {
}
}

// If it's a password, we need to reconstruct the correct password structure
def reconstructPassword(value: String, isPassword: Boolean) : String = {
if (isPassword) {
(PASSWORD_PREFIX + value)
} else {
value
}
}

/**
* This is the user-accessible eval.
* It is expected to throws, and should always be used in
Expand All @@ -502,29 +513,22 @@ final object JsEngine {
def eval(variable: Variable, lib: JsRudderLibBinding): Box[Variable] = {

val (default, js, isPassword) = getEvaluatorTuple(variable)

for {
values <- sequence(variable.values) { value =>
(if (value.startsWith(default)) {
val script = value.substring(default.length())
//do something with script
singleEval(script, lib.bindings)
singleEval(script, lib.bindings).map(reconstructPassword(_, isPassword))
} else {
if (value.startsWith(js)) {
val script = value.substring(js.length())
//do something with script
singleEval(script, lib.bindings)
singleEval(script, lib.bindings).map(reconstructPassword(_, isPassword))
} else {
Full(value)
}
}).map{ x =>
// If it's a password, we need to reconstruct the correct password structure
if (isPassword) {
(PASSWORD_PREFIX + x)
} else {
x
}
} ?~! s"Invalid script '${value}' for Variable ${variable.spec.name} - please check method call and/or syntax"
}) ?~! s"Invalid script '${value}' for Variable ${variable.spec.name} - please check method call and/or syntax"
}
} yield {
variable.copyWithSavedValues(values)
Expand Down Expand Up @@ -708,7 +712,6 @@ final object JsEngine {
override def newThread(r: Runnable): Thread = {
val t = new SandboxedThread(group, r, RUDDER_JSENGINE_THREAD + "-" + threadNumber.getAndIncrement(), 0)


if(t.isDaemon) {
t.setDaemon(false)
}
Expand All @@ -722,4 +725,3 @@ final object JsEngine {
}

}

39 changes: 16 additions & 23 deletions rudder-core/src/test/scala/com/normation/rudder/services/policies/TestJsEngine.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@

package com.normation.rudder.services.policies


import org.junit.runner._
import org.specs2.mutable._
import org.specs2.runner._
Expand Down Expand Up @@ -67,23 +66,20 @@ class TestJsEngine extends Specification {
val hashPrefix = "test"
val variableSpec = InputVariableSpec(hashPrefix, "")


val noscriptVariable = variableSpec.toVariable(Seq("simple ${rudder} value"))
val get4scriptVariable = variableSpec.toVariable(Seq("$evaljs: 2+2"))
val infiniteloopVariable = variableSpec.toVariable(Seq("$evaljs:while(true){}"))

val setFooVariable = variableSpec.toVariable(Seq("$evaljs:var foo = 'some value'; foo"))
val getFooVariable = variableSpec.toVariable(Seq("$eval:foo"))
val get4scriptVariable = variableSpec.toVariable(Seq(s"${JsEngine.EVALJS} 2+2"))
val infiniteloopVariable = variableSpec.toVariable(Seq(s"${JsEngine.EVALJS}while(true){}"))

val setFooVariable = variableSpec.toVariable(Seq(s"${JsEngine.EVALJS}var foo = 'some value'; foo"))
val getFooVariable = variableSpec.toVariable(Seq(s"${JsEngine.DEFAULT_EVAL}foo"))

/*
* For some tests, we need to know if we are on rhino or nashorn
*/
val isNashorn = {
// val javaVersionElements = System.getProperty("java.version").split(""".""")
// val major = Integer.parseInt(javaVersionElements(1))
// major >= 8 // else rhino, because we don't support java 1.5 and before in all case
true
val javaVersionElements = System.getProperty("java.version").split('.')
val major = Integer.parseInt(javaVersionElements(1))
major >= 8 // else rhino, because we don't support java 1.5 and before in all case
}

/**
Expand Down Expand Up @@ -131,14 +127,12 @@ class TestJsEngine extends Specification {
}):MatchResult[Any]
}


"let test have direct access to the engine" in {
val engine = JsEngine.SandboxedJsEngine.getJsEngine().openOrThrowException("Missing jsengine")
engine.eval("1 + 1") === 2.0
}
}


"When feature is disabled, one " should {

def context[T] = JsEngineProvider.withNewEngine[T](FeatureSwitch.Disabled) _
Expand All @@ -152,11 +146,10 @@ class TestJsEngine extends Specification {
}

"failed with a message when the variable is a script" in {
context( _.eval(get4scriptVariable, JsRudderLibBinding.Crypt)) must beFailure(".*starts with the $eval.*".r)
context( _.eval(get4scriptVariable, JsRudderLibBinding.Crypt)) must beFailure(".*starts with the evaljs:.*".r)
}
}


"When getting the sandboxed environement, one " should {

"still be able to do dangerous things, because it's only the JsEngine which is sandboxed" in {
Expand Down Expand Up @@ -250,10 +243,10 @@ class TestJsEngine extends Specification {

"When using the Rudder JS Library, one" should {

val sha256Variable = variableSpec.toVariable(Seq("$evaljs:rudder.password.sha256('secret')"))
val sha512Variable = variableSpec.toVariable(Seq("$eval:rudder.password.sha512('secret', '01234567')"))
val sha256Variable = variableSpec.toVariable(Seq(s"${JsEngine.EVALJS}rudder.password.sha256('secret')"))
val sha512Variable = variableSpec.toVariable(Seq(s"${JsEngine.DEFAULT_EVAL}rudder.password.sha512('secret', '01234567')"))

val md5VariableAIX = variableSpec.toVariable(Seq("$evaljs:rudder.password.aixMd5('secret')"))
val md5VariableAIX = variableSpec.toVariable(Seq(s"${JsEngine.EVALJS}rudder.password.aixMd5('secret')"))

def context[T] = JsEngineProvider.withNewEngine[T](FeatureSwitch.Enabled) _

Expand Down Expand Up @@ -290,10 +283,10 @@ class TestJsEngine extends Specification {

"When using the Rudder JS Library and advertised method, one" should {

val sha256Variable = variableSpec.toVariable(Seq("$evaljs:rudder.password.auto('sha256', 'secret')"))
val sha512Variable = variableSpec.toVariable(Seq("$eval:rudder.password.auto('SHA-512', 'secret', '01234567')"))
val md5VariableAIX = variableSpec.toVariable(Seq("$evaljs:rudder.password.aix('MD5', 'secret')"))
val invalidAlgo = variableSpec.toVariable(Seq("$evaljs:rudder.password.auto('foo', 'secret')"))
val sha256Variable = variableSpec.toVariable(Seq(s"${JsEngine.EVALJS}rudder.password.auto('sha256', 'secret')"))
val sha512Variable = variableSpec.toVariable(Seq(s"${JsEngine.DEFAULT_EVAL}rudder.password.auto('SHA-512', 'secret', '01234567')"))
val md5VariableAIX = variableSpec.toVariable(Seq(s"${JsEngine.EVALJS}rudder.password.aix('MD5', 'secret')"))
val invalidAlgo = variableSpec.toVariable(Seq(s"${JsEngine.EVALJS}rudder.password.auto('foo', 'secret')"))

def context[T] = JsEngineProvider.withNewEngine[T](FeatureSwitch.Enabled) _

Expand Down Expand Up @@ -326,7 +319,7 @@ class TestJsEngine extends Specification {
engine.eval(md5VariableAIX, JsRudderLibBinding.Aix)
} must beVariableValue( _.startsWith("{smd5}") )
}

"fail when we ask for a wrong password" in {
context { engine =>
engine.eval(invalidAlgo, JsRudderLibBinding.Aix)
Expand Down