Upgrade AWS STS client to v2 #504

kelvin-chappell · 2025-02-11T09:59:19Z

What is the purpose of this change?

To remove the deprecated AWS v1 STS client and replace it with v2.
And to remove the awscala STS library which has a dependence on the v1 client.

What is the value of this change and how do we measure success?

Code is up to date and easier to patch in future.
Should still be able to:

get temporary credentials to sign in to the AWS console
get temporary credentials to access AWS locally
have temporary credentials that expire at the expected time
revoke access to an account for credentials created before a threshold datetime

Any additional notes?

This is part of a series of PRs to upgrade AWS clients to v2.
See also:

Remove v1 AWS dynamodb client and awscala dynamo lib #500

Including removal of awscala STS library.

adamnfish

I think the revocation code will be broken as-is, because the authentication isn't providing the session token. This is a super important, but irregularly used, feature of Janus! Perhaps we can test it locally towards the end of a day on our own account after checking no-one is depending on their session?

Otherwise looks great, thank you!

adamnfish · 2025-02-11T14:34:35Z

test/logic/ViewHelpersTest.scala


 class ViewHelpersTest extends AnyFreeSpec with Matchers {

+  private def mkCredentials(


Is there any reason for this name to be shortened? Would it be clearer to call it makeCredentials? I'm also not 100% convinced that it wouldn't be clearer to just use the builder in each place, but if you tried that and thought this way was clearer then 👍

Yes I'll switch to using the builder inline.

Done in 05ea600

adamnfish · 2025-02-11T14:36:09Z

app/logic/Date.scala

  def formatDateTime(date: DateTime): String =
    dateTimeFormatter.print(date)

-  def formatTime(date: DateTime): String =
-    timeFormatter.print(date)
+  def formatTime(instant: java.time.Instant): String =


We should probably switch to java.time everywhere, at some point 😬

Definitely!

adamnfish · 2025-02-11T14:42:07Z

app/aws/Federation.scala

-      Credentials(creds.accessKeyId, creds.secretAccessKey, creds.sessionToken)
-    val provider = new AWSStaticCredentialsProvider(sessionCredentials)
-    val iamClient = IAM(provider)
+    val iamClient = IAM(creds.accessKeyId(), creds.secretAccessKey())


I think this will need the session token as well, since these are temporary credentials just like any others that Janus produces.

This bit still depends on AWScala, because we're making an instance of IAM here. While that's true I think we might as well continue to use the AWSCala credentials representation. awscala's core module includes SessionCredentials, which takes the three arguments we need here, and can be passed to the IAM constructor.

I'll have another look at this code. It looked like it was actually just passing the same values around from one structure to another but I've probably missed something important here. It proves the value of code review.

I should have read the code more carefully! Fixed in f2a07b1

I've left the awscala IAM code as-is and I'll deal with that separately.

adamnfish · 2025-02-11T15:39:50Z

app/aws/Federation.scala

    autoLogoutUrl(url, autoLogout)
  }

+  private def signinToken(credentials: Credentials): String = {


This isn't necessarily the way we'd implement all this from scratch, but great for now given that this is explicitly a reimplementation of the existing code in AWScala 👍

I've tried to read this all super carefully, and it looks like it should be the same as the previous behaviour. Ultimately it comes down to verifying this by using it, which I'm sure you have done locally (and it seems to work fine for me)!

It works locally. Like the dynamo db change, I see this as the smallest change I could make and it can be built on and improved.

adamnfish · 2025-02-11T16:05:25Z

app/aws/Federation.scala


-  val awsMinimumSessionLength = 900.seconds
+  private val signInUrl = "https://signin.aws.amazon.com/federation"
+  private val consoleUrl =


Would it be clearer to apply the URLEncoding where it is used, to call this something like encodedConsoleUrl, or to inline this val?

This is only used in one place, but I can see the benefit to having it be defined up top as a "constant". However, including the encoding feels a little like mixing the constant definition with the logic of its use. I don't feel strongly, whatever you think!

You're right that would be more consistent and readable. It's a slight optimisation that's of very little value here.

Done in b598b03

kelvin-chappell · 2025-02-12T09:53:27Z

I think the revocation code will be broken as-is, because the authentication isn't providing the session token. This is a super important, but irregularly used, feature of Janus! Perhaps we can test it locally towards the end of a day on our own account after checking no-one is depending on their session?

Good point. I haven't tested that!

kelvin-chappell · 2025-02-12T15:44:44Z

Tested revocation endpoint by:

verifying that account that was signed in is signed out following revocation and can be signed in again
janus-role-revocation-policy has a conditional date that corresponds with revocation action

# Conflicts: # app/aws/Federation.scala # app/controllers/Janus.scala

Changes have been addressed, I'll re-review

adamnfish

LGTM, thank you.

Let's make sure we test it carefully, especially today while people are probably using Janus a bit more out of excitement for the multi-session feature!

adamnfish · 2025-02-14T12:00:42Z

app/aws/Federation.scala

@@ -129,8 +141,11 @@ object Federation {
      stsClient,
      Federation.awsMinimumSessionLength
    )
-    val sessionCredentials =
-      Credentials(creds.accessKeyId, creds.secretAccessKey, creds.sessionToken)
+    val sessionCredentials = awscala.Credentials(


Great stuff, nice and easy for now and we defer this to the IAM change 👍

kelvin-chappell · 2025-02-14T14:57:28Z

Tested in Prod:

Can sign into console in multiple accounts
Can sign out from console and back in again from Janus
Can revoke all credentials for an account and see policy condition has correct threshold time and console and credentials expired
Default expiry time of temporary credentials is unchanged
Explicit expiry time for temporary credentials is still honoured (will find out in a few minutes)
Elk log has no new errors

Upgrade AWS STS client to v2

cb278a4

Including removal of awscala STS library.

kelvin-chappell force-pushed the kc/sts-upgrade branch from 83e8a30 to cb278a4 Compare February 11, 2025 10:05

kelvin-chappell changed the title ~~WIP~~ Upgrade AWS STS client to v2 Feb 11, 2025

kelvin-chappell requested review from a team February 11, 2025 10:11

kelvin-chappell marked this pull request as ready for review February 11, 2025 10:11

Merge branch 'main' into kc/sts-upgrade

088f3b7

adamnfish previously requested changes Feb 11, 2025

View reviewed changes

kelvin-chappell added 5 commits February 12, 2025 10:33

Use session credentials to revoke access

f2a07b1

Inline test fixture

05ea600

Deal with URL encoding in one place

b598b03

Use session credentials to revoke access

6a14be9

Merge branch 'main' into kc/sts-upgrade

24c28b2

kelvin-chappell requested a review from adamnfish February 12, 2025 15:46

kelvin-chappell added 2 commits February 13, 2025 14:05

Merge branch 'main' into kc/sts-upgrade

01a364a

Merge branch 'main' into kc/sts-upgrade

622c7a3

# Conflicts: # app/aws/Federation.scala # app/controllers/Janus.scala

adamnfish approved these changes Feb 14, 2025

View reviewed changes

kelvin-chappell merged commit 53c4489 into main Feb 14, 2025
6 checks passed

kelvin-chappell deleted the kc/sts-upgrade branch February 14, 2025 13:51

kelvin-chappell mentioned this pull request Feb 20, 2025

Upgrade AWS IAM v1 client to v2 #509

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade AWS STS client to v2 #504

Upgrade AWS STS client to v2 #504

kelvin-chappell commented Feb 11, 2025 •

edited

Loading

adamnfish left a comment

adamnfish Feb 11, 2025

kelvin-chappell Feb 12, 2025

kelvin-chappell Feb 12, 2025

adamnfish Feb 11, 2025

kelvin-chappell Feb 12, 2025

adamnfish Feb 11, 2025

kelvin-chappell Feb 12, 2025 •

edited

Loading

kelvin-chappell Feb 12, 2025

kelvin-chappell Feb 12, 2025

adamnfish Feb 11, 2025

adamnfish Feb 11, 2025 •

edited

Loading

kelvin-chappell Feb 12, 2025

adamnfish Feb 11, 2025

kelvin-chappell Feb 12, 2025

kelvin-chappell Feb 12, 2025

kelvin-chappell commented Feb 12, 2025

kelvin-chappell commented Feb 12, 2025

adamnfish left a comment

adamnfish Feb 14, 2025

kelvin-chappell commented Feb 14, 2025


		class ViewHelpersTest extends AnyFreeSpec with Matchers {

		private def mkCredentials(

Upgrade AWS STS client to v2 #504

Upgrade AWS STS client to v2 #504

Conversation

kelvin-chappell commented Feb 11, 2025 • edited Loading

What is the purpose of this change?

What is the value of this change and how do we measure success?

Any additional notes?

adamnfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelvin-chappell Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamnfish Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelvin-chappell commented Feb 12, 2025

kelvin-chappell commented Feb 12, 2025

adamnfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelvin-chappell commented Feb 14, 2025

kelvin-chappell commented Feb 11, 2025 •

edited

Loading

kelvin-chappell Feb 12, 2025 •

edited

Loading

adamnfish Feb 11, 2025 •

edited

Loading