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: APKAM keys expiry feature changes in at_commons, at_auth and at_onboarding_cli #644

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from

Conversation

sitaram-kalluri
Copy link
Member

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

- What I did

  • Introduce auto expiry feature enables APKAM keys to expire after the specified duration.

- How I did it

  • Changes in at_auth package

    • Introduce Duration apkamKeysExpiryDuration in "EnrollmentRequest" as an optional argument which represents the APKAM keys expiry duration, which accepts value from the user.
  • Changes in at_commons package

    • Introduce apkamKeysExpiryDuration in enroll_verb_builder.dart. The expiry duration from the "enrollment_request" in at_auth package is set to the enroll_verb_builder.dart to propagate it to the at_secondary_server
  • Changes in at_onboarding_cli

    • Introduce a new argument "-e" to enroll command to pass the expire duration to enroll, otp and spp verbs in human readable format.
    • In at_onboarding_service.dart, in sendEnrollRequest method, introduce apkamKeysExpiryDuration to set the expiry duration.

- How to verify it

  • Manually tested the changes. From the on-boarding cli, send an enrollment request to secondary server with expiry duration set. Once the enrollment is approved, the APKAM keys can be used for authentication. After the duration, an exception stating keys are expired is returned when trying to authenticate.

  • Test OTP with expiry duration:

~/IdeaProjects/atsign/core/at_libraries/packages/at_onboarding_cli/bin git:[2074-introducing-auto-expiry-and-time-to-birth-features-for-apkam-keys]
dart activate_cli.dart otp -a @sitaram -r vip.ve.atsign.zone -k /home/sitaram/.atsign/keys/@sitaram_key.atKeys -e 1m,10s
Connecting ... Connected
1PHZCZ
  • Test SPP with expiry duration:
~/IdeaProjects/atsign/core/at_libraries/packages/at_onboarding_cli/bin git:[2074-introducing-auto-expiry-and-time-to-birth-features-for-apkam-keys]
dart activate_cli.dart spp -a @sitaram -r vip.ve.atsign.zone -k /home/sitaram/.atsign/keys/@sitaram_key.atKeys -s ABC123 -e 1m,10s
Connecting ... Connected
Server response: data:ok
  • Test submit enrollment request:
~/IdeaProjects/atsign/core/at_libraries/packages/at_onboarding_cli/bin git:[2074-introducing-auto-expiry-and-time-to-birth-features-for-apkam-keys]
dart activate_cli.dart enroll -a @sitaram -r vip.ve.atsign.zone -s ABC123 -k /home/sitaram/.atsign/keys/@sitaram_test.atKeys -d my-device -n wavi:rw -p my-app3 -e 1d,10h,12m
Submitting enrollment request
Enrollment ID: fe56ac78-26b1-4ed7-aa6f-1370ee94f67a
Waiting for approval; will check every 10 seconds
Checking ...  not approved. Will retry in 10 seconds
Checking ...  approved.
Creating atKeys file
[Success] Your .atKeys file saved at /home/sitaram/.atsign/keys/@sitaram_test.atKeys

- Description for the changelog

  • fix: APKAM keys expiry feature changes in at_commons, at_auth and at_onboarding_cli

NOTE: For easy of review, adding all the changes in this PR. Will move the changes to the respective packages before merging to trunk.

Pending work : Add functional tests in onboarding cli once the secondary server changes are merged.

@sitaram-kalluri sitaram-kalluri marked this pull request as ready for review September 9, 2024 10:42
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 fine except for

  1. the dependency overrides which as you note we will not merge to trunk
  2. the expiry time being in granularity of minutes, which will result in duration of the functional tests being increased by a minute

On the second point above, please use the approach which is being used in the npt program now (see packages/dart/sshnoports/bin/npt.dart in the noports repo) where the timeout argument is expected to be supplied in human-readable string form e.g. 3d,1h,20s for three days, 1 hour and 20 seconds. npt parses the supplied arg using the parseDuration function from the pub.dev/packages/duration package

You could also take this opportunity to add ttl arg to the spp and otp commands in auth_cli and again use the same approach of taking input in human-readable form

@sitaram-kalluri
Copy link
Member Author

Looks fine except for

1. the dependency overrides which as you note we will not merge to trunk

2. the expiry time being in granularity of minutes, which will result in duration of the functional tests being increased by a minute

On the second point above, please use the approach which is being used in the npt program now (see packages/dart/sshnoports/bin/npt.dart in the noports repo) where the timeout argument is expected to be supplied in human-readable string form e.g. 3d,1h,20s for three days, 1 hour and 20 seconds. npt parses the supplied arg using the parseDuration function from the pub.dev/packages/duration package

You could also take this opportunity to add ttl arg to the spp and otp commands in auth_cli and again use the same approach of taking input in human-readable form

Modified code to accept expiry in human readable format. Also extended expiry feature to OTP and SPP commands. Updated the PR description accordingly.

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.

One small suggestion, otherwise LGTM @sitaram-kalluri thank you

// If apkam Keys expiry is not set, then APKAM keys should lives forever.
// Therefore set to 0ms (0 milliseconds) and TTL will not be set.
? '0ms'
: argResults[AuthCliArgs.argNameExpiry];
Copy link
Contributor

Choose a reason for hiding this comment

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

Use ?? syntax i.e. String apkamKeysExpiry = argResults[AuthCliArgs.argNameExpiry] ?? '0ms'

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.

2 participants