Avoid echoing vault value on a decoding error (#136)

The library currently will output the data from Vault if decoding fails, which will print out secret data, and will likely have it written to logs. This should hide that value by changing the message of the resulting `InvalidMessageBodyFailure` error.
Banno · Dec 14, 2020 · a85cc21 · a85cc21
1 parent b457394
commit a85cc21
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 6 deletions.
diff --git a/core/src/main/scala/com/banno/vault/Vault.scala b/core/src/main/scala/com/banno/vault/Vault.scala
@@ -78,7 +78,10 @@ object Vault {
       uri = vaultUri.withPath(s"/v1/$newSecretPath"),
       headers = Headers.of(Header("X-Vault-Token", token))
     )
-    F.handleErrorWith(client.expect[VaultSecret[A]](request)(jsonOf[F, VaultSecret[A]])) { e =>
+    F.adaptError(client.expect[VaultSecret[A]](request)(jsonOf[F, VaultSecret[A]])) {
+      case InvalidMessageBodyFailure(_, Some(cause: DecodingFailure)) =>
+        InvalidMessageBodyFailure("Could not decode secret key value", cause.some)
+    }.handleErrorWith { e =>
       F.raiseError(VaultRequestError(request = request, cause = e.some, extra = s"tokenLength=${token.length}".some))
     }
   }
@@ -194,7 +197,7 @@ object Vault {
   }
 
   def loginAndKeepSecretLeased[F[_]: Concurrent, A: Decoder](client: Client[F], vaultUri: Uri)
-                                                (roleId: String, secretPath: String, duration: FiniteDuration, waitInterval: FiniteDuration)(implicit T: Timer[F]): Stream[F, A] = 
+                                                (roleId: String, secretPath: String, duration: FiniteDuration, waitInterval: FiniteDuration)(implicit T: Timer[F]): Stream[F, A] =
     Stream.eval(login(client, vaultUri)(roleId)).flatMap(token => keepLoginAndSecretLeased[F, A](client, vaultUri)(token, secretPath, duration, waitInterval))
 
   /**
@@ -232,7 +235,7 @@ object Vault {
 
   def loginAndKeep[F[_]: Concurrent](client: Client[F], vaultUri: Uri)
                                 (roleId: String, tokenLeaseExtension: FiniteDuration)
-                                (implicit T: Timer[F]): Stream[F, String] = 
+                                (implicit T: Timer[F]): Stream[F, String] =
     Stream.eval(login(client, vaultUri)(roleId)).flatMap(token => keepLoginRenewed[F](client, vaultUri)(token, tokenLeaseExtension))
 
 
@@ -253,7 +256,7 @@ object Vault {
     }
 
     def keep(secret: VaultSecret[A]): Stream[F, Unit] =
-      secret.renewal.fold[Stream[F, Unit]](Stream.empty)(initRenewal => 
+      secret.renewal.fold[Stream[F, Unit]](Stream.empty)(initRenewal =>
         Stream
           .iterateEval(initRenewal)(renewOnDuration)
           .takeThrough(_.renewable)
@@ -267,10 +270,10 @@ object Vault {
     def cleanup(secret: VaultSecret[A]): F[Unit] =
       secret.renewal.fold[F[Unit]](
         Applicative[F].unit
-      ){renewal => 
+      ){renewal =>
         Vault.revokeLease[F](client, vaultUri)(clientToken, renewal.leaseId).handleError(_ => ())
       }
-      
+
 
     Stream.bracket(read)(cleanup).flatMap { secret =>
       Stream.emit(secret.data).concurrently(keep(secret))

diff --git a/core/src/test/scala/com/banno/vault/VaultSpec.scala b/core/src/test/scala/com/banno/vault/VaultSpec.scala
@@ -59,6 +59,7 @@ object VaultSpec extends Spec with ScalaCheck {
         |readSecret works as expected when requesting the private key with a valid token $readSecretPrivateKeyProp
         |readSecret works as expected when requesting the postgres password with an invalid token $readSecretPostgresBadTokenProp
         |readSecret works as expected when requesting the private key with an invalid token $readSecretPrivateKeyBadTokenProp
+        |readSecret suppresses echoing the data when JSON decoding fails $readSecretDecodingFailProp
         |renewToken works as expected when sending a valid token $renewAValidToken
         |revokeToken works as expected when revoking a valid token $revokeAValidToken
         |renewLease works as expected when sending valid input arguments $renewLeaseValidAddressProp
@@ -399,6 +400,19 @@ object VaultSpec extends Spec with ScalaCheck {
       .isLeft
   }
 
+  val readSecretDecodingFailProp : Prop = Prop.forAll(VaultArbitraries.validVaultUri){uri =>
+    Vault.readSecret[IO, TokenValue](mockClient, uri)(clientToken, secretPrivateKeyPath)
+      .attempt
+      .unsafeRunSync()
+      .fold(
+        { error =>
+          if (error.getMessage.contains(privateKey)) Prop.falsified :| "Secret data in the error message"
+          else Prop.passed :| "Secret data redacted"
+        },
+        _ => Prop.falsified :| "Data should not be parseable"
+      )
+  }
+
   val renewAValidToken : Prop = Prop.forAll(VaultArbitraries.validVaultUri){uri =>
     Vault.renewSelfToken[IO](mockClient, uri)(VaultToken(clientToken, 3600, true), 1.hour)
       .unsafeRunSync() === VaultToken(clientToken, 3600, renewable)