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.

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

Upgrade AWS IAM v1 client to v2 #509

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
31 changes: 14 additions & 17 deletions app/aws/Federation.scala
Original file line number Diff line number Diff line change
@@ -1,18 +1,15 @@
package aws

import awscala.iam._
import com.amazonaws.auth.AWSStaticCredentialsProvider
import com.amazonaws.services.identitymanagement.model.GetRoleRequest
import com.gu.janus.model.{AwsAccount, Permission}
import data.Policies
import logic.Date
import org.joda.time.{DateTime, DateTimeZone, Duration, Period}
import play.api.libs.json.Json
import software.amazon.awssdk.auth.credentials._
import software.amazon.awssdk.services.iam.IamClient
import software.amazon.awssdk.services.iam.model.PutRolePolicyRequest
import software.amazon.awssdk.services.sts.StsClient
import software.amazon.awssdk.services.sts.model.{
AssumeRoleRequest,
Credentials
}
import software.amazon.awssdk.services.sts.model._

import java.net.{URI, URLEncoder}
import java.nio.charset.StandardCharsets.UTF_8
Expand Down Expand Up @@ -141,24 +138,24 @@ object Federation {
stsClient,
Federation.awsMinimumSessionLength
)
val sessionCredentials = awscala.Credentials(
val sessionCredentials = AwsSessionCredentials.create(
creds.accessKeyId,
creds.secretAccessKey,
creds.sessionToken
)
val provider = new AWSStaticCredentialsProvider(sessionCredentials)
val iamClient = IAM(provider)
val provider = StaticCredentialsProvider.create(sessionCredentials)
val iamClient = IamClient.builder().credentialsProvider(provider).build()

// remove access from assumed role
val roleName = getRoleName(roleArn)
val getRoleRequest = new GetRoleRequest().withRoleName(roleName)
val role = Role(iamClient.getRole(getRoleRequest).getRole)
val roleRevocationPolicy =
RolePolicy(role, "janus-role-revocation-policy", revocationPolicyDocument)
// ^
// this name should match policy in cloudformation/federation.template.yaml
val roleRevocationPolicy = PutRolePolicyRequest
.builder()
.roleName(roleName)
.policyName("janus-role-revocation-policy")
.policyDocument(revocationPolicyDocument)
.build()
iamClient.putRolePolicy(roleRevocationPolicy)
iamClient.shutdown()
iamClient.close()
}

private[aws] def getRoleName(roleArn: String): String = {
Expand Down
2 changes: 1 addition & 1 deletion app/data/Policies.scala
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package data

import awscala._
import com.gu.janus.model.{AwsAccount, Permission}
import com.gu.janus.policy.Iam._

object Policies {
val revokeAccess = Policy(
Expand Down
11 changes: 4 additions & 7 deletions build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,12 @@ import sbtversionpolicy.withsbtrelease.ReleaseVersion
ThisBuild / organization := "com.gu"
ThisBuild / licenses := Seq(License.Apache2)

val awsSdkVersion = "1.12.781"
val awsSdkV2Version = "2.30.20"
val awscalaVersion = "0.9.2"
val awsSdkVersion = "2.30.20"
Copy link
Contributor

Choose a reason for hiding this comment

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

🥳

val circeVersion = "0.14.10"
val commonDependencies = Seq(
"org.typelevel" %% "cats-core" % "2.12.0",
"joda-time" % "joda-time" % "2.13.0",
"org.joda" % "joda-convert" % "3.0.1",
"com.github.seratch" %% "awscala-iam" % awscalaVersion,
"org.scalatest" %% "scalatest" % "3.2.19" % Test,
"org.scalacheck" %% "scalacheck" % "1.18.1" % Test,
"org.scalatestplus" %% "scalacheck-1-16" % "3.2.14.0" % Test,
Expand Down Expand Up @@ -83,9 +80,9 @@ lazy val root = (project in file("."))
ws,
filters,
"com.gu.play-googleauth" %% "play-v30" % "19.0.0",
"com.amazonaws" % "aws-java-sdk-iam" % awsSdkVersion,
"software.amazon.awssdk" % "sts" % awsSdkV2Version,
"software.amazon.awssdk" % "dynamodb" % awsSdkV2Version,
"software.amazon.awssdk" % "iam" % awsSdkVersion,
"software.amazon.awssdk" % "sts" % awsSdkVersion,
"software.amazon.awssdk" % "dynamodb" % awsSdkVersion,
"net.logstash.logback" % "logstash-logback-encoder" % "7.3" // scala-steward:off
) ++ jacksonDatabindOverrides
++ jacksonOverrides
Expand Down
5 changes: 3 additions & 2 deletions configTools/src/main/scala/com/gu/janus/model/models.scala
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.gu.janus.model

import awscala.Policy
import com.gu.janus.policy.Iam.Policy
import io.circe.syntax.EncoderOps
import org.joda.time._

case class JanusData(
Expand Down Expand Up @@ -87,7 +88,7 @@ object Permission {
policy: Policy,
shortTerm: Boolean = false
): Permission = {
Permission(account, label, description, policy.asJson, shortTerm)
Permission(account, label, description, policy.asJson.noSpaces, shortTerm)
}
}

Expand Down
147 changes: 147 additions & 0 deletions configTools/src/main/scala/com/gu/janus/policy/Iam.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
package com.gu.janus.policy

import io.circe.syntax._
import io.circe.{Encoder, Json, JsonObject}

import scala.collection.immutable.ListMap

/** This models the AWS IAM policy types and subtypes.
*
* See https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies.html
* for the logic of the Json encoding.
*/
object Iam {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this Iam object wrapper? No harm, I can see (minor) arguments both ways 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

It seemed like a convenient reference to hold everything together. But can remove if you prefer - not strictly necessary.


sealed trait Effect
object Effect {
case object Allow extends Effect
case object Deny extends Effect

implicit val encoder: Encoder[Effect] = Encoder.instance {
case Allow => Json.fromString("Allow")
case Deny => Json.fromString("Deny")
}
}

case class Action(name: String)
object Action {
implicit val encoder: Encoder[Action] = Encoder[String].contramap(_.name)
}

case class Resource(id: String)
object Resource {
implicit val encoder: Encoder[Resource] = Encoder[String].contramap(_.id)
}

case class Condition(
key: String,
typeName: String,
conditionValues: Seq[String]
)
object Condition {
implicit val encoder: Encoder[Condition] = Encoder.instance { condition =>
Json.obj(
condition.typeName -> Json.obj(
condition.key -> Json.fromValues(
condition.conditionValues.map(Json.fromString)
)
)
)
}
}

case class Principal(id: String, provider: String)
object Principal {
implicit val encoder: Encoder[Principal] = Encoder.instance { principal =>
Json.obj(
principal.provider -> Json.fromString(principal.id)
)
}
}

case class Statement(
effect: Effect,
actions: Seq[Action],
resources: Seq[Resource],
id: Option[String] = None,
conditions: Seq[Condition] = Nil,
principals: Seq[Principal] = Nil
)
object Statement {
private def conditionsAsJson(conditions: Seq[Condition]): Json = {
val mergedConditions = conditions
.groupBy(_.typeName)
.view
.mapValues(conditions =>
Json.obj(
conditions.map(condition =>
condition.key -> Json.fromValues(
condition.conditionValues.map(Json.fromString)
)
): _*
)
)
.toMap
Json.fromJsonObject(JsonObject.fromMap(mergedConditions))
}

private def principalsAsJson(principals: Seq[Principal]): Json = {
val principalMap = principals
.groupBy(_.provider)
.view
.mapValues(principals =>
Json.fromValues(
principals.map(p => Json.fromString(p.id))
)
)
.toMap
Json.fromJsonObject(JsonObject.fromMap(principalMap))
}

implicit val encoder: Encoder[Statement] = Encoder.instance { statement =>
// Using ListMap to preserve order of fields, which is easier to debug
val baseFields = ListMap(
"Effect" -> statement.effect.asJson,
"Action" -> statement.actions.asJson,
"Resource" -> statement.resources.asJson
)

val withSid = statement.id.fold(baseFields)(id =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's talk about Statement IDs! The current implementation adds unique SIDs using incrementing integers, so if we're going for compatibility we should keep that behaviour. However, for inline policies (which is what Janus produces) SIDs are not required and I was already considering removing them.

Let's talk about the behaviour we want for now in this PR and in the future. However, this does tie into my other point about having a test that ensures we are maintining Janus Data config-file-compatibility during this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

On the face of it, removing the statement IDs looks sensible. I'm not sure where or how they're referenced. Let's discuss next week.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now statement IDs will be included in the output json representation if they're explicitly added to a Statement case class, otherwise they'll be omitted.

baseFields + ("Sid" -> Json.fromString(id))
)

val withConditions =
if (statement.conditions.nonEmpty)
withSid + ("Condition" -> conditionsAsJson(statement.conditions))
else withSid

val withPrincipals =
if (statement.principals.nonEmpty)
withConditions + ("Principal" -> principalsAsJson(
statement.principals
))
else withConditions

Json.fromJsonObject(JsonObject.fromMap(withPrincipals))
}
}

case class Policy(
statements: Seq[Statement],
id: Option[String] = None
)

object Policy {
private val PolicyVersion = "2012-10-17"

implicit val encoder: Encoder[Policy] = Encoder.instance { policy =>
val baseObj = JsonObject(
"Version" -> Json.fromString(PolicyVersion),
"Statement" -> policy.statements.asJson
)
Json.fromJsonObject(
policy.id.fold(baseObj)(id => baseObj.add("Id", Json.fromString(id)))
)
}
}
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package com.gu.janus.policy

import awscala._
import com.amazonaws.auth.policy.Statement.Effect
import com.gu.janus.policy.Iam._

object Statements {

Expand Down
Loading