Merge pull request #393 from Banno/ISSUE-392-increase-generate-secret…

…-robustness Make generateSecret more robust
Banno · Nov 7, 2023 · 76b7aa8 · 76b7aa8
2 parents ec20d9f + 513f55c
commit 76b7aa8
Show file tree

Hide file tree

Showing 2 changed files with 163 additions and 17 deletions.
diff --git a/core/src/main/scala/com/banno/vault/Vault.scala b/core/src/main/scala/com/banno/vault/Vault.scala
@@ -308,19 +308,38 @@ object Vault {
       headers = Headers(Header.Raw(CIString("X-Vault-Token"), token))
     )
     val withBody = request.withEntity(payload.asJson)
-    for {
-      secret <- F.handleErrorWith(
-        client.expect[VaultSecret[B]](withBody)(jsonOf[F, VaultSecret[B]])
-      ) { e =>
+    client.run(withBody).use {
+      case resp @ Status.Successful(_) =>
+        resp
+          .attemptAs[VaultSecret[B]](jsonOf[F, VaultSecret[B]])
+          .adaptError {
+            case InvalidMessageBodyFailure(_, Some(cause: DecodingFailure)) =>
+              InvalidMessageBodyFailure(
+                "Could not decode secret key value",
+                cause.some
+              )
+          }
+          .valueOrF { decodeError =>
+            readSecret[F, B](client, vaultUri)(token, secretPath)
+              .adaptError { case readError =>
+                decodeError.addSuppressed(readError)
+                VaultRequestError(
+                  request = withBody,
+                  cause = decodeError.some,
+                  extra = s"tokenLength=${token.length}".some
+                )
+              }
+          }
+      case resp =>
         F.raiseError(
           VaultRequestError(
             request = withBody,
-            cause = e.some,
+            cause =
+              UnexpectedStatus(resp.status, request.method, request.uri).some,
             extra = s"tokenLength=${token.length}".some
           )
         )
-      }
-    } yield secret
+    }
   }
 
   /** This function logs in, requests a secret and then continually asks for a

diff --git a/core/src/test/scala/com/banno/vault/VaultSpec.scala b/core/src/test/scala/com/banno/vault/VaultSpec.scala
@@ -18,28 +18,28 @@ package com.banno.vault
 
 import java.util.UUID
 import java.util.concurrent.TimeUnit
-
 import cats.effect.{Concurrent, IO}
-import cats.implicits._
+import cats.implicits.*
 import com.banno.vault.models.{
   CertificateData,
   CertificateRequest,
+  VaultKeys,
   VaultSecret,
   VaultSecretRenewal,
-  VaultToken,
-  VaultKeys
+  VaultToken
 }
-import io.circe.Decoder
-import org.http4s._
-import org.http4s.implicits._
+import io.circe.{Codec, Decoder}
+import org.http4s.*
+import org.http4s.implicits.*
 import org.http4s.dsl.Http4sDsl
 import org.http4s.dsl.impl.QueryParamDecoderMatcher
-import org.http4s.circe._
+import org.http4s.circe.*
 import org.http4s.client.Client
 
-import scala.concurrent.duration._
+import scala.concurrent.duration.*
 import munit.{CatsEffectSuite, ScalaCheckEffectSuite}
-import org.scalacheck._
+import org.scalacheck.*
+
 import scala.util.Random
 import org.scalacheck.effect.PropF
 import org.typelevel.ci.CIString
@@ -101,6 +101,12 @@ class VaultSpec
       Decoder.forProduct1("lease_id")(Lease.apply)
   }
 
+  case class SelfManaged(key: String, kid: String)
+  object SelfManaged {
+    implicit val codec: Codec[SelfManaged] =
+      Codec.forProduct2("key", "kid")(SelfManaged.apply)(sm => (sm.key, sm.kid))
+  }
+
   implicit val certificateRequestDecoder: Decoder[CertificateRequest] =
     Decoder.forProduct7(
       "common_name",
@@ -286,6 +292,41 @@ class VaultSpec
                 | "renewable": $renewable
                 |}""".stripMargin)
         }
+
+      case req @ POST -> Root / "v1" / "secret" / "selfmanaged" / "200" / kid =>
+        checkVaultToken(req) {
+          Ok(s"""
+               |{
+               | "data": {
+               |   "key": "$privateKey",
+               |   "kid": "$kid"
+               | },
+               | "lease_duration": $leaseDuration,
+               | "lease_id": "$leaseId",
+               | "renewable": $renewable
+               |}""".stripMargin)
+        }
+
+      case req @ GET -> Root / "v1" / "secret" / "selfmanaged" / "204" / kid =>
+        checkVaultToken(req) {
+          Ok(s"""
+               |{
+               | "data": {
+               |   "key": "$privateKey",
+               |   "kid": "$kid"
+               | },
+               | "lease_duration": $leaseDuration,
+               | "lease_id": "$leaseId",
+               | "renewable": $renewable
+               |}""".stripMargin)
+        }
+
+      case req @ POST -> Root / "v1" / "secret" / "selfmanaged" / "204" / _ =>
+        checkVaultToken(req)(NoContent())
+
+      case req @ POST -> Root / "v1" / "secret" / "selfmanaged" / "4xx" / _ =>
+        checkVaultToken(req)(Forbidden())
+
       case req @ PUT -> Root / "v1" / "sys" / "leases" / "renew" =>
         checkVaultToken(req) {
           req.decodeJson[IncrementLease].flatMap { _ =>
@@ -627,6 +668,92 @@ class VaultSpec
     }
   }
 
+  test("generateSecret works as expected when receiving a 200 Ok response") {
+    PropF.forAllF(VaultArbitraries.validVaultUri, Gen.identifier) {
+      (uri, kid) =>
+        Vault
+          .generateSecret[IO, SelfManaged, SelfManaged](mockClient, uri)(
+            clientToken,
+            s"secret/selfmanaged/200/$kid",
+            SelfManaged(privateKey, kid)
+          )
+          .assertEquals(
+            VaultSecret(
+              SelfManaged(privateKey, kid),
+              leaseDuration.some,
+              leaseId.some,
+              renewable.some
+            )
+          )
+    }
+  }
+
+  test(
+    "generateSecret works as expected when receiving a 200 Ok with an unparsable response"
+  ) {
+    PropF.forAllF(VaultArbitraries.validVaultUri, Gen.identifier) {
+      (uri, kid) =>
+        Vault
+          .generateSecret[IO, SelfManaged, VaultKeys](mockClient, uri)(
+            clientToken,
+            s"secret/selfmanaged/200/$kid",
+            SelfManaged(privateKey, kid)
+          )
+          .redeem(
+            error =>
+              if (error.getMessage.contains(privateKey))
+                PropF
+                  .falsified[IO]
+                  .label(s"Secret data in: ${error.getMessage}")
+              else PropF.passed[IO].label("Secret data redacted"),
+            _ => PropF.falsified[IO].label("Data should not be parseable")
+          )
+    }
+  }
+
+  test(
+    "generateSecret works as expected when receiving a 204 No Content response"
+  ) {
+    PropF.forAllF(VaultArbitraries.validVaultUri, Gen.identifier) {
+      (uri, kid) =>
+        Vault
+          .generateSecret[IO, SelfManaged, SelfManaged](mockClient, uri)(
+            clientToken,
+            s"secret/selfmanaged/204/$kid",
+            SelfManaged(privateKey, kid)
+          )
+          .assertEquals(
+            VaultSecret(
+              SelfManaged(privateKey, kid),
+              leaseDuration.some,
+              leaseId.some,
+              renewable.some
+            )
+          )
+    }
+  }
+
+  test("generateSecret works as expected when receiving a 4xx response") {
+    PropF.forAllF(VaultArbitraries.validVaultUri, Gen.identifier) {
+      (uri, kid) =>
+        Vault
+          .generateSecret[IO, SelfManaged, SelfManaged](mockClient, uri)(
+            clientToken,
+            s"secret/selfmanaged/4xx/$kid",
+            SelfManaged(privateKey, kid)
+          )
+          .redeem(
+            error =>
+              if (error.getMessage.contains(privateKey))
+                PropF
+                  .falsified[IO]
+                  .label(s"Secret data in: ${error.getMessage}")
+              else PropF.passed[IO].label("Secret data redacted"),
+            _ => PropF.falsified[IO].label("Data should not be parseable")
+          )
+    }
+  }
+
   test(
     "loginAndKeepSecretLeased fails when wait duration is longer than lease duration"
   ) {