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

#45 have a set of keys instead of just one #111

Merged
merged 15 commits into from
Jan 16, 2025

Conversation

TheLydonKing
Copy link
Collaborator

@TheLydonKing TheLydonKing commented Jan 8, 2025

Release Notes:

  • Made the Previously rotated public keys available for clients to fetch and allows for clients to parse JWT's that haven't expired but are using an older keyset.

closes #45

@TheLydonKing TheLydonKing linked an issue Jan 8, 2025 that may be closed by this pull request
Copy link

github-actions bot commented Jan 8, 2025

JaCoCo code coverage report - scala:2.12.17

File Coverage [67.77%]
InMemoryKeyConfig.scala 100% 🍏
PublicKeySet.scala 100% 🍏
PublicKey.scala 100% 🍏
SecurityConfig.scala 85.45% 🍏
TokenController.scala 84.15% 🍏
JWTService.scala 83.28% 🍏
KeyConfig.scala 82.07% 🍏
AwsSecretsManagerKeyConfig.scala 42.39%
ServiceAccountConfig.scala 37.57%
AwsSecretsUtils.scala 0%
AwsSecret.scala 0%
Total Project Coverage 63.58% 🍏

Copy link
Collaborator

@dk1844 dk1844 left a comment

Choose a reason for hiding this comment

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

  • I am wondering if it makes sense to distinguish current and old publicKey for the clients -- whether just not give them a set/list (then we can be generic of what we give them in the future and they just have to trust the public keys as a set -- one of them should be the signing one).
    -> so I would rather keep the PublicKey class intact for the current endpoint and created a new class PublicKeySet that would serve the new purpose.

  • I don't see the functionality where the previous publicKey is phased out after some time has passed after the rotation - and I don't see a configuration for these timeouts either

Comment on lines 93 to 96
if(secretsOption.isEmpty)
throw new Exception("Error retrieving username and password from from AWS Secrets Manager")

val secrets = secretsOption.get
Copy link
Collaborator

@dk1844 dk1844 Jan 9, 2025

Choose a reason for hiding this comment

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

I think option processing can be done better without calling Option.get e.g. via

      secretsOption.fold(
        throw new Exception("Error retrieving username and password from from AWS Secrets Manager")
      ){ secrets =>
        (secrets(usernameFieldName), secrets(passwordFieldName))  
      }

or more explicitly via

      secretsOption match {
        case None => throw new Exception("Error retrieving username and password from from AWS Secrets Manager")
        case Some(secrets) =>
          (secrets(usernameFieldName), secrets(passwordFieldName))
      }

@TheLydonKing
Copy link
Collaborator Author

  • I am wondering if it makes sense to distinguish current and old publicKey for the clients -- whether just not give them a set/list (then we can be generic of what we give them in the future and they just have to trust the public keys as a set -- one of them should be the signing one).
    -> so I would rather keep the PublicKey class intact for the current endpoint and created a new class PublicKeySet that would serve the new purpose.
  • I don't see the functionality where the previous publicKey is phased out after some time has passed after the rotation - and I don't see a configuration for these timeouts either
  • Will do on the PublicKeySet
  • Ok, I was under the impression that the previous key would be phased out on rotation but I can add another schedule that phases it out after a set time until rotation occurs.

@TheLydonKing TheLydonKing changed the title Feature/45 have a set of keys instead of just one #45 have a set of keys instead of just one Jan 14, 2025
@TheLydonKing TheLydonKing requested a review from dk1844 January 14, 2025 13:19
Copy link
Collaborator

@dk1844 dk1844 left a comment

Choose a reason for hiding this comment

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

See the comments, otherwise LGTM 😃

@TheLydonKing TheLydonKing requested a review from dk1844 January 15, 2025 15:39
@TheLydonKing TheLydonKing requested a review from dk1844 January 16, 2025 11:03
Copy link
Collaborator

@dk1844 dk1844 left a comment

Choose a reason for hiding this comment

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

LGTM

@TheLydonKing TheLydonKing merged commit 57e13a2 into master Jan 16, 2025
3 of 4 checks passed
@TheLydonKing TheLydonKing deleted the feature/45-have-a-set-of-keys-instead-of-just-one branch January 21, 2025 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Have a set of keys instead of just one
3 participants