From 513f55c7d2a9d26f969501715f5c9360eeb385ba Mon Sep 17 00:00:00 2001 From: Morgen Peschke Date: Tue, 24 Oct 2023 12:19:50 -0700 Subject: [PATCH] Make generateSecret more robust 1. Handle non-200 successes 2. Attempt recovery if the body is malformed/omitted --- .../main/scala/com/banno/vault/Vault.scala | 33 +++- .../scala/com/banno/vault/VaultSpec.scala | 147 ++++++++++++++++-- 2 files changed, 163 insertions(+), 17 deletions(-) diff --git a/core/src/main/scala/com/banno/vault/Vault.scala b/core/src/main/scala/com/banno/vault/Vault.scala index c3df52c..bf47f5d 100644 --- 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 index b1966cf..7c0cda3 100644 --- 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" ) {