Skip to content

Keep pass phrase only in memory#323

Merged
tomholub merged 31 commits intomasterfrom
feature/issue-197-pass-phrase-memory
Jun 19, 2021
Merged

Keep pass phrase only in memory#323
tomholub merged 31 commits intomasterfrom
feature/issue-197-pass-phrase-memory

Conversation

@Kharchevskyi
Copy link
Contributor

@Kharchevskyi Kharchevskyi commented May 23, 2021

This PR allows to keep pass phrase only in memory

close #197

  • created new controller to encapsulate logic in it instead of having it all in setup view controller (same as on Android)
  • move all related logic to CreatePrivateKeyViewController

Unit Tests:

  • Added test for Key Methods
  • Replaced shared singleton for BackupService which will make possible to unit test it
  • Added BackupServiceMock
  • Added extension for Promise resolveAfter which helps to create promise with expected result
  • Remove logic from shared DataService to KeyDataStorageService which can be mocked for testing purposes

Manual Tests:
Test next flows:

  • No email backups -> Create new key
  • No email backups -> Import key
  • Backups found -> loading from backup
  • Encrypt message
  • Compose message

To be filled by reviewers

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

Copy link
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

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

Just doing a checkpoint review so later I review the delta

Copy link
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

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

.

@Kharchevskyi
Copy link
Contributor Author

Simulator Screen Shot - iPhone 8 - 2021-05-26 at 15 38 43

@Kharchevskyi
Copy link
Contributor Author

Kharchevskyi commented May 31, 2021

SetupInitialViewController - responsible for fetching backups and navigate to next setup steps(Create or Import Key).
Insets will be updated to be same fo all Setup flow
s2
s1
s4

@Kharchevskyi
Copy link
Contributor Author

s11

@Kharchevskyi
Copy link
Contributor Author

@tomholub in case of saving passphrase and matching key backups locally - does it mean to save it in Keychain? or we can still save them in encrypted Realm storage but keep only for 4 hours?

@tomholub
Copy link
Collaborator

tomholub commented Jun 2, 2021 via email

@Kharchevskyi
Copy link
Contributor Author

@tomholub correct me if I'm wrong please.
Only pass phrase will be stored either locally or in memory.
Storage for key will be the same.
Currently passphrase is a part of KeyInfo object, same as it private. Should the longed be identifier to mach key and stored pass phrase?

@tomholub
Copy link
Collaborator

tomholub commented Jun 3, 2021

@tomholub correct me if I'm wrong please.
Only pass phrase will be stored either locally or in memory.
Storage for key will be the same.

This is correct

Currently passphrase is a part of KeyInfo object, same as it private.

yup

Should the longed be identifier to mach key and stored pass phrase?

I didn't understand this sentence :-) longed you meant longid? You're proposing to change longid to identifier so that you can keep track between stored keys and stored pass phrases - which makes sense to me.

You'll probably want to change the KeyInfo object then, to not contain pass phrase, or to make pass phrase nilable.

@Kharchevskyi
Copy link
Contributor Author

Kharchevskyi commented Jun 3, 2021

@tomholub Mac autocorrected from longid to longed 🙃

You're proposing to change longid to identifier so that you can keep track between stored keys and stored pass phrases - which makes sense to me.

Correct. I just didn't want to rename it to identifier)

You'll probably want to change the KeyInfo object then, to not contain pass phrase, or to make pass phrase nilable.
Exactly

thanks for clarifying!

@tomholub
Copy link
Collaborator

tomholub commented Jun 4, 2021

@Kharchevskyi for simplicity and smaller PR, on Android, we merged a PR first that only implements the setup flow, but not the message decrypt flow and other parts of app. Then we did PR review. Then we set the checkbox during setup to "disabled" and merged it. After that, you can implement individual other flows that use pass phrase in other PRs. This minimizes merge conflicts and makes it easier to review.

@Kharchevskyi
Copy link
Contributor Author

@tomholub unfortunately we didn't have split setup. SetupViewController on iOS app was responsible for almost all setup related logic. It was impossible to add additional logic to existed implementation. That's why I had to implement SetupInitialViewController(which responsible for fetching backups and farther navigation), SetupKeyViewController (responsible for setup account with generated key), SetupBackupsViewController and so on.

I think I will finish this PR before Sunday. Is it ok?

@tomholub
Copy link
Collaborator

tomholub commented Jun 4, 2021

No worries, I understand it was not very extendable. Thanks!

@Kharchevskyi
Copy link
Contributor Author

Simulator Screen Shot - iPhone 8 - 2021-06-07 at 00 11 23

@Kharchevskyi
Copy link
Contributor Author

@tomholub is it possible for you to check why https://flowcrypt.com/attester/pub/flow.test.anton@gmail.com returns 404?

@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2021

There were 1 email addresses found in the above comment. Please:

  1. click three dots -> edit to remove the email addresses
  2. click edited in the comment header, and click on the previous revision of the comment
  3. when viewing the old revision with an email in it, click options -> delete this revision from history

@tomholub
Copy link
Collaborator

tomholub commented Jun 7, 2021

No public key was submitted for that email address. During setup flow, the app should be calling the attester API to submit the public key.

@tomholub
Copy link
Collaborator

Now I was able to use the behavior the way I'd expect, except tests are failing and there is a problem when I enter a wrong pass phrase.

To reproduce:

  1. set up with pp in memory, go to inbox, kill app
  2. open app again and open a message
  3. give it a wrong pass phrase when prompted

Actual behavior:
wrong pass phrase gets remembered and now all messages are showing need_passphrase: Missing pass phrase

Expected behavior:
entered pass phrase gets tested against stored keys on this account. It must be able to decrypt at least one key. If it works for at least one, then save it temporarily for use only with that one key. If it doesn't unlock any, I should see a similar pass phrase prompt that says Wrong pass phrase. Please try again. [________] [cancel] [ok].

This way I can keep trying until I enter a pass phrase that matches or until I give up.

@Kharchevskyi Kharchevskyi marked this pull request as ready for review June 16, 2021 15:09
@tomholub
Copy link
Collaborator

@Kharchevskyi I'll test this, but could you fix tests? Without it I won't be able to merge.

@Kharchevskyi
Copy link
Contributor Author

Tests are already fixed
Screenshot 2021-06-18 at 11 45 05
@tomholub

@tomholub
Copy link
Collaborator

Ah, didn't notice - will take a look.

@tomholub
Copy link
Collaborator

I'm removing Codacy from the project, it's not very useful.

Copy link
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

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

Code looks good with some remarks. I'll have a second look at the in-memory storage.

Copy link
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

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

I understood the code better now. It's the naming that confused me. Please rename classes as suggested in comments below, and for the rest, I'll file another issue.

@tomholub tomholub merged commit 3d1dc01 into master Jun 19, 2021
@tomholub tomholub deleted the feature/issue-197-pass-phrase-memory branch June 19, 2021 13:41
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.

option to keep pass phrase only in memory

3 participants