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

fix: Introduce time duration for apkam keys to auto expire #2085

Merged

Conversation

sitaram-kalluri
Copy link
Member

@sitaram-kalluri sitaram-kalluri commented Sep 9, 2024

- What I did

  • Introduce APKAM keys to auto expire after specified time duration.

- How I did it

  • In at_commons, introduced Duration apkamKeysExpiryDuration to capture the time duration set by the user. This time duration is propagated to the secondary server.
  • In the secondary server, once enrollment is approved, set the TTL value to the Duration apkamKeysExpiryDuration in milliseconds. This will ensure that the key expires and is subsequently deleted from the keystore.
  • In "AbstractVerbHandler.dart", add "_verifyIfEnrollmentIsActive", which will check if the enrollment is active on the existing connections. If the enrollment expires, closes the connection with error message.
  • For the new connections, in the "PkamVerbHandler.dart->handleApkamVerification", there is call to "getEnrollDataStoreValue" which will check if the enrollment is active. If not active, sets enrollment state to expired and authentication fails.
  • Introduce EnrollmentManager class which contains "get", "put" and "buildEnrollmentKey" methods which will return the enrollment details from the keystore, create/update enrollment details to the keystore and build the enrollment key for the enrollment id respectively.
  • Refactor the code in the verb handler which related to fetch / creating enrollment records with happen via the enrollment manager.
  • Update the unit tests to align with the EnrollmentManager class.

- How to verify it

  • Added a unit test to verify the changes.

    • A test to verify apkam expiry is set for approved enrollment
    • A test to verify pkam verb fails when apkam keys are expired
    • A test to verify update verb fails when apkam keys are expired
    • A test to verify delete verb fails when apkam keys are expired
  • Added below functional tests:

    • A test to verify apkam authentication fails with expired apkam keys
    • A test to verify connection closes when apkam keys expire

- Description for the changelog

  • fix: Introduce time duration for apkam keys to auto expire

Copy link
Contributor

@gkc gkc left a comment

Choose a reason for hiding this comment

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

A few things missing here:

  1. Enforcement to prevent a client from authenticating with an enrollment that has expired
  2. Test(s) to verify the same
  3. Enforcement in the verb handler(s) to terminate connections whose enrollment has expired
  4. Test(s) to verify the same

@sitaram-kalluri sitaram-kalluri requested a review from gkc September 19, 2024 09:04
@sitaram-kalluri
Copy link
Member Author

A few things missing here:

1. Enforcement to prevent a client from authenticating with an enrollment that has expired

2. Test(s) to verify the same

3. Enforcement in the verb handler(s) to terminate connections whose enrollment has expired

4. Test(s) to verify the same

@gkc : Addressed the review comments and updated the PR description. Please review.

Copy link
Contributor

@gkc gkc left a comment

Choose a reason for hiding this comment

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

Looks good. However, we need to be smarter in terms of dealing with enrollment records - checking every time is necessary, but means an extra fetch from the data store every time. We should create an EnrollmentManager class through which all interaction with enrollment records happens, and which can therefore make appropriate decisions regarding in-memory caching of enrollment records. Can you check the effort involved in doing that - if it's not huge then we should make those changes in this PR also.

@sitaram-kalluri sitaram-kalluri requested a review from gkc September 23, 2024 11:21
@sitaram-kalluri
Copy link
Member Author

Looks good. However, we need to be smarter in terms of dealing with enrollment records - checking every time is necessary, but means an extra fetch from the data store every time. We should create an EnrollmentManager class through which all interaction with enrollment records happens, and which can therefore make appropriate decisions regarding in-memory caching of enrollment records. Can you check the effort involved in doing that - if it's not huge then we should make those changes in this PR also.

@gkc : Introduce the "EnrollmentManager" class through which all interactions with enrollment records happens. Updated the PR description. Please review.

Copy link
Contributor

@gkc gkc left a comment

Choose a reason for hiding this comment

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

@sitaram-kalluri looks good! can you resolve the conflicts please?

…o-expiry-and-time-to-birth-features-for-apkam-keys

# Conflicts:
#	packages/at_secondary_server/lib/src/verb/handler/abstract_verb_handler.dart
#	packages/at_secondary_server/lib/src/verb/handler/enroll_verb_handler.dart
#	packages/at_secondary_server/test/enroll_verb_test.dart
#	tests/at_functional_test/test/enroll_verb_test.dart
@sitaram-kalluri sitaram-kalluri requested a review from gkc September 25, 2024 04:55
Copy link
Contributor

@gkc gkc left a comment

Choose a reason for hiding this comment

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

LGTM

@gkc gkc merged commit 997dfbc into trunk Sep 25, 2024
26 checks passed
@gkc gkc deleted the 2074-introducing-auto-expiry-and-time-to-birth-features-for-apkam-keys branch September 25, 2024 14:52
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.

Introducing Auto-Expiry and Time-to-Birth Features for APKAM Keys
2 participants