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

feat: Apkam onboarding changes #399

Closed
wants to merge 38 commits into from
Closed

Conversation

murali-shris
Copy link
Member

@murali-shris murali-shris commented Aug 29, 2023

- What I did

  • apkam changes for at_onboarding_cli
    - How I did it

  • Introduced enroll method in AtOnboardingService. Clients/apps can used this method to place an enrollment request

  • Added enrollmentId as an optional param to authenticate method. After enrollment, clients must pass enrollmentId for apkam auth

  • onboarding changes.

    • onboard method will thrown an exception if appName or deviceName is not set in OnboardingPreference. Both the params are required for enrolling the app on server
  • enroll method impl

    • generate new apkam key pair and apkam symmetric key
    • retrieve encryption private key from server and use it to encrypt apkam symmetric key
    • send enroll request to server with appName, deviceName,otp, encrypted apkam symmetric key and apkam public key from apkam key pair
    • asynchronously attempt apkam to server until enrollment is approved or denied
    • once apkam auth is success then add enrollmentId to _pkamSuccessController. This wil trigger the method _listenToPkamSuccessStream which will save the security keys to .atKeys file
  • _activateAtsign changes

    • encrypt default encryption key and self encryption key with first client's apkam symmetric key.
    • send enroll request to server which gets auto approved
    • if enroll request is approved, attempt pkam with enrollmentId. if pkam is success then update encryption public key to server, delete cram secret, generate keys file, persist keys to local secondary

- How to verify it

  • run the functional tests in at_onboarding_cli

@murali-shris murali-shris marked this pull request as ready for review September 14, 2023 07:53
@murali-shris
Copy link
Member Author

murali-shris commented Sep 14, 2023

@cconstab @gkc Purnima has tested the onboarding_cli changes with at_talk and sshnp. With existing atKeys file and also new atKeys file that get generated as a part of new APKAM onboarding flow.
New clients onboarding using onboarding_cli have to specify appName and deviceName for apkam using onboarding preference. Without these params, onboarding will not continue.Is this behavior fine?
Once review comments in this PR are addressed, need someone else also to test sshnp before we merge the changes and publish at_onboarding_cli

@gkc
Copy link
Contributor

gkc commented Sep 14, 2023

@cconstab @gkc Purnima has tested the onboarding_cli changes with at_talk and sshnp. With existing atKeys file and also new atKeys file that get generated as a part of new APKAM onboarding flow.

Great!

New clients onboarding using onboarding_cli have to specify appName and deviceName for apkam using onboarding preference. Without these params, onboarding will not continue. Is this behavior fine?

It's fine, but of course it's a breaking change, so will need to be a new major version of at_onboarding_cli package... is there a way of making it optional - i.e. it's required only for APKAM enrollments?

Once review comments in this PR are addressed, need someone else also to test sshnp before we merge the changes and publish at_onboarding_cli

I will be happy to do that

@murali-shris
Copy link
Member Author

@cconstab @gkc Purnima has tested the onboarding_cli changes with at_talk and sshnp. With existing atKeys file and also new atKeys file that get generated as a part of new APKAM onboarding flow.

Great!

New clients onboarding using onboarding_cli have to specify appName and deviceName for apkam using onboarding preference. Without these params, onboarding will not continue. Is this behavior fine?

It's fine, but of course it's a breaking change, so will need to be a new major version of at_onboarding_cli package... is there a way of making it optional - i.e. it's required only for APKAM enrollments?

Once review comments in this PR are addressed, need someone else also to test sshnp before we merge the changes and publish at_onboarding_cli

I will be happy to do that

@gkc i have completed the backwards compatibility changes

@gkc
Copy link
Contributor

gkc commented Sep 17, 2023

@murali-shris Looks good, although the example/apkam_authenticate.dart is confusing to me as it implies that I need to supply the enrollmentId as well as the atKeys file - but isn't the enrollmentId stored in the atKeys file?

I'd like to test this myself

  • step 1: Enrollment: let's say I want to do an enrollment of a second app for an atSign I've already onboarded - would I use something like the example/apkam_enroll.dart?
  • step 2: Authentication: Let's say I want to run at_talk and use the atKeys file I generated in step 1 : do I need to make any code changes to at_talk?

@murali-shris
Copy link
Member Author

murali-shris commented Sep 18, 2023

@murali-shris Looks good, although the example/apkam_authenticate.dart is confusing to me as it implies that I need to supply the enrollmentId as well as the atKeys file - but isn't the enrollmentId stored in the atKeys file?

I'd like to test this myself

  • step 1: Enrollment: let's say I want to do an enrollment of a second app for an atSign I've already onboarded - would I use something like the example/apkam_enroll.dart?
  • step 2: Authentication: Let's say I want to run at_talk and use the atKeys file I generated in step 1 : do I need to make any code changes to at_talk?
    Step 1: Yes
    I have to make a minor change in at_onboarding_service_impl.dart. If enrollmentId is not passed to authenticate method from the client, then the enrollmentId has to be read from the atKeysFile.
    Once I make these changes, no changes will be required to run at_talk with new atKeysFile.

@gkc
Copy link
Contributor

gkc commented Oct 4, 2023

@murali-shris Looks good, although the example/apkam_authenticate.dart is confusing to me as it implies that I need to supply the enrollmentId as well as the atKeys file - but isn't the enrollmentId stored in the atKeys file?
I'd like to test this myself

  • step 1: Enrollment: let's say I want to do an enrollment of a second app for an atSign I've already onboarded - would I use something like the example/apkam_enroll.dart?
  • step 2: Authentication: Let's say I want to run at_talk and use the atKeys file I generated in step 1 : do I need to make any code changes to at_talk?

Step 1: Yes
I have to make a minor change in at_onboarding_service_impl.dart. If enrollmentId is not passed to authenticate method from the client, then the enrollmentId has to be read from the atKeysFile.
Once I make these changes, no changes will be required to run at_talk with new atKeysFile.

Have you made those changes, @murali-shris ? And are the examples (apkam_authenticate and apkam_enroll) up to date ?

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.

@murali-shris Have you made the changes mentioned in this comment?

@murali-shris
Copy link
Member Author

murali-shris commented Oct 5, 2023

@murali-shris Have you made the changes mentioned in this comment?

yes Gary. I added the change to read enrollmentId from keys file. But I am unable to recollect whether I re-tested the examples after my change.
I have to replace duplicate code with call to at_auth package which I am currently implementing. I will re-test the examples along with this change.

@murali-shris murali-shris marked this pull request as draft October 10, 2023 06:31
@srieteja srieteja force-pushed the apkam_onboarding_changes branch from 7bb4eff to 87a0efb Compare October 20, 2023 11:28
@murali-shris
Copy link
Member Author

raised a new PR with squashed commits
#441

@murali-shris murali-shris deleted the apkam_onboarding_changes branch October 26, 2023 18:13
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.

3 participants