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

Support Keychain Access Groups #2770

Closed

Conversation

ekurutepe
Copy link

@ekurutepe ekurutepe commented Feb 17, 2023

Issue #

#2508

Description

This change enables secure sharing of Cognito session between the main app and app extensions.

This is achieved through an optional awsCognitoAuthPlugin configuration parameter `KeychainAccessGroup:

{
    "auth": {
        "plugins": {
            "awsCognitoAuthPlugin": {
                "IdentityManager": {
                    "Default": {}
                },
                "CognitoUserPool": {
                    "Default": {
                        "PoolId": <Pool Id>,
                        "AppClientId": <App Client ID>,
                        "Region": <Region>
                    }
                },
                "Auth": {
                    "Default": {
                        "authenticationFlowType": "CUSTOM_AUTH",
                    }
                },
                "KeychainAccessGroup": <Access Group>
            }
        }
    }
}

General Checklist

  • Added new tests to cover change, if needed
  • Build succeeds with all target using Swift Package Manager
  • All unit tests pass
  • All integration tests pass
  • Security oriented best practices and standards are followed (e.g. using input sanitization, principle of least privilege, etc)
  • Documentation update for the change if required
  • PR title conforms to conventional commit style
  • New or updated tests include Given When Then inline code documentation and are named accordingly testThing_condition_expectation()
  • If breaking change, documentation/changelog update with migration instructions

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ekurutepe ekurutepe requested a review from a team as a code owner February 17, 2023 13:03
@harsh62
Copy link
Member

harsh62 commented Mar 7, 2023

Thank you for creating PR to support passing Keychain Groups. Since the configuration file is generated by using @aws-amplify/amplify-cli , adding KeychainAccessGroup would first need to be implemented with CLI.
Manually editing the configuration file is discouraged as could be overwritten during a change done through the CLI.
So we have two options here:

  • Propagate the right changes with @aws-amplify/amplify-cli as well to support this feature.
  • Allow customers to dynamically pass the keychain access group to the Auth Plugin.

@harsh62 harsh62 self-requested a review March 14, 2023 16:19
@harsh62 harsh62 self-assigned this Mar 14, 2023
@harsh62 harsh62 removed their request for review March 14, 2023 16:20
@harsh62
Copy link
Member

harsh62 commented Mar 16, 2023

Digging deeper into the PR and the existing implementation, I found a scenario where we also need to consider how existing items that are there in the keychain can be updated to use access group that was passed in.

Comment on lines +230 to +232
legacyKeychainStoreFactory: { service in
self.makeLegacyKeychainStore(service: service, accessGroup: accessGroup)
}
Copy link
Member

Choose a reason for hiding this comment

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

We should not be adding accessGroupto the legacy keychain. This would cause migration to fail from V1 to V2.

@ekurutepe
Copy link
Author

@harsh62 thanks for looking into this PR. We have been using this branch in production for a few weeks now. I understand that the bar to be included in a popular public SDK is very high and this probably needs to address various different use cases that I am not familiar with and did not consider. Please feel free to take over and take it to the finish line. I'd kindly suggest to prioritize this internally since this feature is required to be able to safely share cognito credentials between the main app and app extensions.

@harsh62
Copy link
Member

harsh62 commented Nov 21, 2023

I am gonna close this PR as the team will be working on this feature with a different design. Will keep the issue #2508 open for more udpates.

@harsh62 harsh62 closed this Nov 21, 2023
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