117 delay option for new set of key signing #118

TheLydonKing · 2025-02-06T10:52:15Z

Release Notes:

Added new Optional Config setting that allows for a layover period before signing new JWT's with the new keys
Fixed RefreshToken to use both available public keys

closes #117

github-actions · 2025-02-06T11:26:32Z

JaCoCo code coverage report - scala:2.12.17

File	Coverage [83.51%]	🍏
InMemoryKeyConfig.scala	100%	🍏
KeyConfig.scala	95.94%	🍏
AwsSecretsManagerKeyConfig.scala	91.51%	🍏
JWTService.scala	83.11%	🍏
AwsSecretsUtils.scala	0%	❌

Total Project Coverage	67.88%	🍏

dk1844

Most of the comments are details, but adding tests to the AwsSecretsManagerKeyConfig seem necessary

dk1844 · 2025-02-06T12:07:05Z

api/src/main/resources/example.application.yaml

        key-phase-out-time: 30min
+        key-lay-over-time: 15min


Suggested change

key-phase-out-time: 30min

key-lay-over-time: 15min

key-lay-over-time: 15min

key-phase-out-time: 30min

As these are used in a given order (first, after rotation, the layover happens, then phaseout happens), I would list these two config fields in that order, too (better readability/understandability)

Also, README.md is not up-to-date in regards to both

key-phase-out-time

key-lay-over-time

These two keys are not mentioned there (both missing from the examples and missing explanation). It would be especially important to note how these to relate to each other and how it is counted (both from rotation time). Eg.

key-lay-over-time: 15min key-phase-out-time: 30min

will mean:

t=0: keys rotation happens t=0-14m: layover period: public keys: old key+new key; old key is still used for signing t=15-29m: layover is over: public keys: old key+new key; new key is used for signing t=30m+: phase-out happens: public keys: new key; new key is used for signing

Something like that.

In other words, do I understand correctly that phaseout is always counted from the rotation, so the actual phaseout's duration is only phaseOut-layover?

If I got it right, is this a good idea? Would it be better if the time windows of phaseOut was not dependent on layover?

So: you could actually have e.g. phaseOut=15min and layover=30min, and phaseOut would only start after layover finished (so after 45min if layover was defined). This would not force phaseOut > layover, anymore, too.

However, this is just an idea -- when documented clearly, it is not a problem if it behaves as now, it is just a matter of how the configuration times are defined (just conditional addition/subtracting)

In other words, do I understand correctly that phaseout is always counted from the rotation, so the actual phaseout's duration is only phaseOut-layover?

If I got it right, is this a good idea? Would it be better if the time windows of phaseOut was not dependent on layover?

So: you could actually have e.g. phaseOut=15min and layover=30min, and phaseOut would only start after layover finished (so after 45min if layover was defined). This would not force phaseOut > layover, anymore, too.

However, this is just an idea -- when documented clearly, it is not a problem if it behaves as now, it is just a matter of how the configuration times are defined (just conditional addition/subtracting)

Hi

Yes, actually that makes a lot of sense. So will implement it like this.

dk1844 · 2025-02-06T12:08:28Z

api/src/main/resources/example.application.yaml

@@ -103,7 +104,7 @@ management:
  health:
    ldap:
      # TODO: Enable Ldap check for actuator/health when fully Implemented - issue #34
-      enabled: "false"
+      enabled: "true"


I don't see #34 being closed, so why the change?

Yeah, this was a mistake, it's changed back

dk1844 · 2025-02-06T12:08:32Z

api/src/main/scala/za/co/absa/loginsvc/rest/config/jwt/AwsSecretsManagerKeyConfig.scala

+  keyPhaseOutTime: Option[FiniteDuration],
+  keyLayOverTime: Option[FiniteDuration]


Suggested change

keyPhaseOutTime: Option[FiniteDuration],

keyLayOverTime: Option[FiniteDuration]

keyLayOverTime: Option[FiniteDuration],

keyPhaseOutTime: Option[FiniteDuration]

Same reason as above.

dk1844 · 2025-02-06T12:32:36Z

api/src/main/scala/za/co/absa/loginsvc/rest/config/jwt/AwsSecretsManagerKeyConfig.scala

@@ -79,7 +80,15 @@ case class AwsSecretsManagerKeyConfig(
        }
      }

-      (currentKeyPair, previousKeyPair)
+      previousKeyPair.fold {(currentKeyPair, previousKeyPair)} { pk =>
+        val exp = keyLayOverTime.exists(!isExpired(currentSecrets.createTime, _))


Suggested change

val exp = keyLayOverTime.exists(!isExpired(currentSecrets.createTime, _))

val layoverIsActive = keyLayOverTime.exists(!isExpired(currentSecrets.createTime, _))

I think better names denoting more closely what is meant would be welcome. Same with

val exp = keyPhaseOutTime.exists(isExpired(currentSecrets.createTime, _))

(perhaps rename to val phaseOutIsActive)?

dk1844 · 2025-02-06T12:42:27Z

api/src/main/scala/za/co/absa/loginsvc/rest/config/jwt/AwsSecretsManagerKeyConfig.scala

-      (currentKeyPair, previousKeyPair)
+      previousKeyPair.fold {(currentKeyPair, previousKeyPair)} { pk =>
+        val exp = keyLayOverTime.exists(!isExpired(currentSecrets.createTime, _))
+        if (!exp) {


I saw your last commit changing this and I believe it truly is correct now like this.
But the fact that this error was possible clearly demonstrates that this part need to be test-covered, too.

That will mean making AwsSecretsUtils a param (of the method perhaps) so that it can be mocked in tests. (to make things easier, perhaps introduce a trait for it). Perhaps MockFactory mixin can be used.

dk1844 · 2025-02-06T13:56:32Z

api/src/test/scala/za/co/absa/loginsvc/rest/service/jwt/JWTServiceTest.scala

@@ -294,11 +294,11 @@ class JWTServiceTest extends AnyFlatSpec with BeforeAndAfterEach with Matchers {
    assert(initPublicKey._1 == jwtService.publicKeys._2.orNull)
  }

-  it should "phase out older keys after 8 seconds" in {
+  it should "phase out older keys after 30 seconds" in {


These times are barely bearable, is it necessary to have such long-taking tests?

…ules

TheLydonKing added 6 commits February 4, 2025 14:58

Add KeyLayOver for inMemory use

f6e9f4b

Implement layOver for AWS SecretsManager uses

49968a3

Add Tests to InMemoryKeyConfigTest.scala

d53fb89

Add Tests to AwsSecretsManagerKeyConfigTest

a154bc2

Add Tests to JWTServiceTest

79cad95

Update refreshTokens to use both keys

0aa83f9

TheLydonKing self-assigned this Feb 6, 2025

TheLydonKing requested review from dk1844 and jakipatryk as code owners February 6, 2025 10:52

TheLydonKing linked an issue Feb 6, 2025 that may be closed by this pull request

Delay option for new set of key signing #117

Open

Edit Logging

17a0061

TheLydonKing added 2 commits February 6, 2025 14:17

Minor Change on example config

ef764e6

Fix Logic on KeyLayover in AwsSecretsManagerKeyConfig

4b48d77

dk1844 reviewed Feb 6, 2025

View reviewed changes

TheLydonKing added 4 commits February 7, 2025 13:02

Use A combination of KeyPhaseOutTime and KeyLayover time to set sched…

771504c

…ules

Change order of parameters

0387467

Refactor AwsSecretsManagerKeyConfig to accomodate testing keyPair

5b84d3d

Add Tests for fetchKeySetsFromCloud

c8ceb2c

TheLydonKing requested a review from dk1844 February 11, 2025 14:12

Updated README.md

2b8ca14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

117 delay option for new set of key signing #118

117 delay option for new set of key signing #118

TheLydonKing commented Feb 6, 2025 •

edited

Loading

github-actions bot commented Feb 6, 2025 •

edited

Loading

dk1844 left a comment

dk1844 Feb 6, 2025

dk1844 Feb 6, 2025

dk1844 Feb 6, 2025

TheLydonKing Feb 6, 2025

dk1844 Feb 6, 2025

TheLydonKing Feb 7, 2025

dk1844 Feb 6, 2025

dk1844 Feb 6, 2025

dk1844 Feb 6, 2025

dk1844 Feb 6, 2025

		keyPhaseOutTime: Option[FiniteDuration],
		keyLayOverTime: Option[FiniteDuration]

	val exp = keyLayOverTime.exists(!isExpired(currentSecrets.createTime, _))
	val layoverIsActive = keyLayOverTime.exists(!isExpired(currentSecrets.createTime, _))

117 delay option for new set of key signing #118

Are you sure you want to change the base?

117 delay option for new set of key signing #118

Conversation

TheLydonKing commented Feb 6, 2025 • edited Loading

github-actions bot commented Feb 6, 2025 • edited Loading

JaCoCo code coverage report - scala:2.12.17

dk1844 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

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

TheLydonKing commented Feb 6, 2025 •

edited

Loading

github-actions bot commented Feb 6, 2025 •

edited

Loading