util/util-security: Fix Credentials fails to load valid yaml credentials

Problem

com.twitter.util.security.Credentials fails to load valid yaml credentials, and it loads invalid yaml credentials. For example, `key:!value` is accepted as a valid Map("key", "!value"), but `{"key": "value"}` is rejected as invalid.

Solution

Replace the buggy custom parser with a vetted 3rdparty one (snakeyaml)

Result

Users can load valid yaml files, and invalid yaml files are rejected.

JIRA Issues: PSEC-10357

Differential Revision: https://phabricator.twitter.biz/D617641

Author: jcrawford13

Diff for: CHANGELOG.rst

@@ -7,6 +7,18 @@ Note that ``PHAB_ID=#`` and ``RB_ID=#`` correspond to associated messages in com
 Unreleased
 ----------
 
+Breaking API Changes
+~~~~~~~~~~~~~~~~~~~~
+* util-security: Use snakeyaml to parse yaml instead of a buggy custom yaml
+  parser. This means that thrown IOExceptions have been replaced by
+  YAMLExceptions. Additionally, the parser member has been limited to private visibility. ``PHAB_ID=D617641``
+
+New Features
+~~~~~~~~~~~~
+
+* util-security: Any valid yaml / json file with string keys and values can
+  be loaded with `com.twitter.util.security.Credentials`. ``PHAB_ID=D617641``
+
 Runtime Behavior Changes
 ~~~~~~~~~~~~~~~~~~~~~~~~
 

Diff for: build.sbt

@@ -25,6 +25,8 @@ val caffeineLib = "com.github.ben-manes.caffeine" % "caffeine" % "2.9.2"
 val jsr305Lib = "com.google.code.findbugs" % "jsr305" % "2.0.1"
 val scalacheckLib = "org.scalacheck" %% "scalacheck" % "1.15.4" % "test"
 val slf4jApi = "org.slf4j" % "slf4j-api" % slf4jVersion
+val snakeyaml = "org.yaml" % "snakeyaml" % "1.24"
+
 
 def travisTestJavaOptions: Seq[String] = {
   // We have some custom configuration for the Travis environment
@@ -623,7 +625,8 @@ lazy val utilSecurity = Project(
     name := "util-security",
     libraryDependencies ++= Seq(
       scalacheckLib,
-      "org.scalatestplus" %% "scalacheck-1-14" % "3.1.2.0" % "test"
+      "org.scalatestplus" %% "scalacheck-1-14" % "3.1.2.0" % "test",
+      snakeyaml
     )
   ).dependsOn(utilCore, utilLogging, utilSecurityTestCerts % "test")
 

Diff for: util-security/src/main/scala/com/twitter/util/security/BUILD

@@ -8,6 +8,7 @@ scala_library(
     strict_deps = True,
     tags = ["bazel-compatible"],
     dependencies = [
+        "3rdparty/jvm/org/yaml:snakeyaml",
         "util/util-core:util-core-util",
         "util/util-logging/src/main/scala/com/twitter/logging",
     ],

Diff for: util-security/src/main/scala/com/twitter/util/security/Credentials.scala

@@ -16,48 +16,36 @@
 
 package com.twitter.util.security
 
-import java.io.{File, IOException}
-
+import java.io.File
+import org.yaml.snakeyaml.Yaml
+import scala.collection.JavaConverters._
 import scala.io.Source
-import scala.jdk.CollectionConverters._
-import scala.util.parsing.combinator._
-import scala.util.matching.Regex
 
 /**
  * Simple helper to read authentication credentials from a text file.
  *
- * The file's format is assumed to be trivialized yaml, containing lines of the form ``key: value``.
- * Keys can be any word character or '-' and values can be any character except new lines.
+ * The file's format is assumed to be yaml, containing string keys and values.
  */
 object Credentials {
-  object parser extends RegexParsers {
-
-    override val whiteSpace: Regex = "(?:\\s+|#.*\\r?\\n)+".r
-
-    private[this] val key = "[\\w-]+".r
-    private[this] val value = ".+".r
-
-    def auth: Parser[Tuple2[String, String]] = key ~ ":" ~ value ^^ {
-      case k ~ ":" ~ v => (k, v)
-      case _ => throw new MatchError("Failed case matching on auth parser")
-    }
-    def content: Parser[Map[String, String]] = rep(auth) ^^ { auths => Map(auths: _*) }
-
-    def apply(in: String): Map[String, String] = {
-      parseAll(content, in) match {
-        case Success(result, _) => result
-        case x: Failure => throw new IOException(x.toString)
-        case x: Error => throw new IOException(x.toString)
-      }
-    }
+  private[this] val parser: ThreadLocal[Yaml] = new ThreadLocal[Yaml] {
+    override def initialValue(): Yaml = new Yaml()
   }
 
-  def apply(file: File): Map[String, String] = parser(Source.fromFile(file).mkString)
+  def byName(name: String): Map[String, String] = {
+    apply(new File(sys.env.getOrElse("KEY_FOLDER", "/etc/keys"), name))
+  }
 
-  def apply(data: String): Map[String, String] = parser(data)
+  def apply(file: File): Map[String, String] = {
+    val fileSource = Source.fromFile(file)
+    try apply(fileSource.mkString)
+    finally fileSource.close()
+  }
 
-  def byName(name: String): Map[String, String] = {
-    apply(new File(System.getenv().asScala.getOrElse("KEY_FOLDER", "/etc/keys"), name))
+  def apply(data: String): Map[String, String] = {
+    val result: java.util.Map[String, Any] = parser.get.load(data)
+    Option(result)
+      .map(_.asScala.toMap.mapValues(v => if (v == null) "null" else v.toString)).getOrElse(
+        Map.empty)
   }
 }
 

Diff for: util-security/src/test/scala/com/twitter/util/security/CredentialsTest.scala

@@ -21,48 +21,48 @@ import org.scalatest.funsuite.AnyFunSuite
 
 class CredentialsTest extends AnyFunSuite with Checkers {
   test("parse a simple auth file") {
-    val content = "username: root\npassword: hellokitty\n"
+    val content = "username: root\npassword: not_a_password\n"
     val result = Credentials(content)
-    assert(result == Map("username" -> "root", "password" -> "hellokitty"))
+    assert(result == Map("username" -> "root", "password" -> "not_a_password"))
   }
 
   test("parse a more complex auth file") {
     val content = """# random comment
 
-username:root
-password  : last_0f-the/international:playboys
+username: root
+password: not_a-real/pr0d:password
 # more stuff
-   moar :ok
+moar: ok
 
         """
     assert(
       Credentials(content) == Map(
         "username" -> "root",
-        "password" -> "last_0f-the/international:playboys",
+        "password" -> "not_a-real/pr0d:password",
         "moar" -> "ok"
       )
     )
   }
 
   test("work for java peeps too") {
-    val content = "username: root\npassword: hellokitty\n"
+    val content = "username: root\npassword: not_a_password\n"
     val jmap = new Credentials().read(content)
     assert(jmap.size() == 2)
     assert(jmap.get("username") == "root")
-    assert(jmap.get("password") == "hellokitty")
+    assert(jmap.get("password") == "not_a_password")
   }
 
   test("handle \r\n line breaks") {
-    val content = "username: root\r\npassword: hellokitty\r\n"
-    assert(Credentials(content) == Map("username" -> "root", "password" -> "hellokitty"))
+    val content = "username: root\r\npassword: not_a_password\r\n"
+    assert(Credentials(content) == Map("username" -> "root", "password" -> "not_a_password"))
   }
 
   test("handle special chars") {
-    val pass = (0 to 127)
+    val pass = (32 to 126)
       .map(_.toChar)
-      .filter(c => c != '\r' && c != '\n')
+      .filter(c => c != '\r' && c != '\n' && c != '\'')
       .mkString
-    val content = s"username: root\npassword: $pass\n"
+    val content = s"username: root\npassword: '$pass'\n"
     assert(Credentials(content) == Map("username" -> "root", "password" -> pass))
   }
 }