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

Make generateSecret more robust #393

Merged
merged 1 commit into from
Nov 7, 2023
Merged

Make generateSecret more robust #393

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
33 changes: 26 additions & 7 deletions core/src/main/scala/com/banno/vault/Vault.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
morgen-peschke marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down
147 changes: 137 additions & 10 deletions core/src/test/scala/com/banno/vault/VaultSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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 { _ =>
Expand Down Expand Up @@ -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"
) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm lost what made that test unparsable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The third type argument to generateSecret on line 697 doesn't match the second, so it's putting in a SelfManaged and trying to pull it back out as a VaultKeys (which has an incompatible serialization).

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"
) {
Expand Down