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

Add an encrypted version of the CredentialsManager #115

Merged
merged 18 commits into from
Oct 5, 2017

Conversation

lbalmaceda
Copy link
Contributor

@lbalmaceda lbalmaceda commented Aug 29, 2017

This PR adds a new class, similar but separate to the existing CredentialsManager one, that encrypts the data before persisting it and can also request the user authentication before accessing the stored credentials. This authentication behavior is disabled by default. By using the @RequiresAPI annotation I can make the compiler show an error if the developer is using this class on lower APIs. It requires minimum API 21 because that's when the "Confirm credentials" intent is first available.

Security discussion

  • Encryption is done with an AES key of 256 bits of size. (We can gain some performance if I lower it to 128 I guess, but it's fast enough).
  • The AES key is saved in the Storage encrypted using an RSA key pair of 2048 bits of size.
  • The RSA key pair is saved in the Android KeyStore. I can generate keys and ask the OS to use them in the encryption/decryption process, but I can never access the actual key values.
  • Detailed implementation can be seen in the CryptoUtil class.

Pending stuff

  • CryptoUtil tests. White-box tests using PowerMock and Mockito. Not many alternatives here... The coverage report will show "zero coverage" because of Jacoco lack of support for the PowerMock test runner.
  • CryptoManager tests. Including the "requires authentication" flow.
  • Readme. Make clear which manager to use depending on the Android API level and how to support the authentication feature (i.e. add an example).
  • Add some exception catching tests
  • Try this manager in a sample like "session-handling" on API 21 and 23
  • Rename CryptoManager to something cooler, like "EnhancedCredentialsManager". What do you think?

@lbalmaceda lbalmaceda force-pushed the add-encrypted-manager branch 2 times, most recently from 7d19726 to 1fb3f34 Compare August 30, 2017 19:57
README.md Outdated

## Credentials Manager

This library ships with two additional classes that help you store and retrieve the `Credentials` received in authentication calls. Depending on the minimum API level that your application is targeting you'd like to use a different implementation.
Copy link
Member

Choose a reason for hiding this comment

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

you store and retrieve the Credentials received in authentication calls. ->
you manage the Credentials received during authentication.

README.md Outdated
1. **Instantiate the manager**
You'll need an `AuthenticationAPIClient` instance used to renew the credentials when they expire and a `Storage`. The Storage implementation is up to you. We provide a `SharedPreferencesStorage` that uses `SharedPreferences` to create a file in the application's directory with Context.MODE_PRIVATE mode. This implementation is thread safe and can either be obtained through a Singleton like method or be created every time it's needed.
1. **Instantiate the manager:**
You'll need an `AuthenticationAPIClient` instance used to renew the credentials when they expire and a `Storage`. The Storage implementation is up to you. We provide a `SharedPreferencesStorage` that uses `SharedPreferences` to create a file in the application's directory with **Context.MODE_PRIVATE** mode. This implementation is thread safe and can either be obtained through a Singleton method or be created every time it's needed.
Copy link
Member

Choose a reason for hiding this comment

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

You'll need an AuthenticationAPIClient instance used to renew the credentials when they expire and a Storage instance. The Storage implementation is up to you.

This implementation is thread safe and can either be obtained through a Singleton method or be created every time it's needed. ->
This implementation is thread safe and can either be obtained through a shared method or on demand.

README.md Outdated

### Encryption enforced (Min API 21)

The enhanced version contains the same methods as the _Basic_ manager but encrypts the data before storing it, and in those devices where a Secure LockScreen has been configured it can require the user authentication before letting them obtain the stored credentials. The class is called `CryptoManager`.
Copy link
Member

Choose a reason for hiding this comment

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

The enhanced version contains the same methods as the Basic manager but encrypts the data before storing it, and in ->
This version expands the minimum version and adds encryption to the data storage. In

README.md Outdated


#### Usage
The usage is the same as in the _Basic_ version. What changes is the way you instantiate the manager as it now requires a valid `Context`.
Copy link
Member

Choose a reason for hiding this comment

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

Change this, don't like calling them basic etc don't need to say it.

In this version the manager requires a valid `context as shown below:

Copy link
Member

@cocojoe cocojoe left a comment

Choose a reason for hiding this comment

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

Few language changes to consider.

@lbalmaceda lbalmaceda force-pushed the add-encrypted-manager branch 3 times, most recently from b55a333 to c21a265 Compare September 28, 2017 15:48
@auth0 auth0 deleted a comment from codecov-io Sep 28, 2017
@lbalmaceda lbalmaceda force-pushed the add-encrypted-manager branch 2 times, most recently from ad2df66 to aad51ae Compare September 29, 2017 17:17
@cocojoe
Copy link
Member

cocojoe commented Oct 4, 2017

Modify your codecov.yml to allow for a little variance in overall and patch. For me -0.26% is acceptable so is 80% coverage in patch as sometimes it's just not possible. See threshold

https://github.com/auth0/Auth0.swift/blob/master/codecov.yml

Copy link
Member

@cocojoe cocojoe left a comment

Choose a reason for hiding this comment

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

Update your codecov to pass.

@lbalmaceda lbalmaceda added this to the v1-Next milestone Oct 5, 2017
@lbalmaceda lbalmaceda merged commit dac05f1 into master Oct 5, 2017
@lbalmaceda lbalmaceda deleted the add-encrypted-manager branch October 5, 2017 17:34
@lbalmaceda lbalmaceda modified the milestones: v1-Next, 1.11.0 Oct 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants