Skip to content

Feature/issue 278 verify signed messages#998

Merged
tomholub merged 24 commits intomasterfrom
feature/issue-278-verify-signed-messages
Nov 15, 2021
Merged

Feature/issue 278 verify signed messages#998
tomholub merged 24 commits intomasterfrom
feature/issue-278-verify-signed-messages

Conversation

@sosnovsky
Copy link
Collaborator

@sosnovsky sosnovsky commented Nov 11, 2021

This PR adds verification for signed messages

close #278
close #1021


Tests (delete all except exactly one):

  • Difficult to test (explain why) - currently we don't have any tests for MessageService

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • 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.

Thanks! See details about how to understand the signature result.

@tomholub
Copy link
Collaborator

tomholub commented Nov 11, 2021

Rendering is well done - thank you. I gave you vague instructions but it turned out well 👍 just please put the green bubble below the date.

tom@flowcrypt.com       <-   ^
Nov 08
[🔒 signed]

Please have it say just signed instead of Signed by xxx in the good case. Since we already check for email match.

Then please make sure it collapses together with the text when I collapse that message. Unless you think it shouldn't?

@tomholub
Copy link
Collaborator

tomholub commented Nov 11, 2021

And also let's do what you suggested:

tom@flowcrypt.com                <-   ^
Nov 08
[🔒 encrypted] [🔒 signed]

Hello

For plain emails it will show:

tom@flowcrypt.com                <-   ^
Nov 08
[🔒 not encrypted] [🔒 not signed]

Hello

for signed-only message it will show:

tom@flowcrypt.com                <-   ^
Nov 08
[🔒 not encrypted] [🔒signed]

Hello

etc

@sosnovsky
Copy link
Collaborator Author

Then please make sure it collapses together with the text when I collapse that message. Unless you think it shouldn't?

Yes, I missed collapsing part, it definitely should collapse along with other message parts.

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.

Looking better 👍

@sosnovsky
Copy link
Collaborator Author

@tomholub I found an issue with non-encrypted messages here https://github.com/FlowCrypt/flowcrypt-ios/blob/feature/issue-278-verify-signed-messages/FlowCrypt/Functionality/Mail%20Provider/Message%20Provider/MessageService.swift#L168

In case when user receives plain non-encrypted mail then app tries to decrypt it, but can't find local pub keys for mail sender, so it sends request to https://flowcrypt.com/attester/pub/ and receives 404 error. But this request takes some time and delays showing message to user.

Is there any method to find if message needs decryption or it's just plain text?
I thought about checking if message starts with \r\n\r\n-----BEGIN PGP MESSAGE-----\r\n, but it doesn't seem to be a reliable solution, or it should work well?

@tomholub
Copy link
Collaborator

tomholub commented Nov 12, 2021

@tomholub I found an issue with non-encrypted messages here https://github.com/FlowCrypt/flowcrypt-ios/blob/feature/issue-278-verify-signed-messages/FlowCrypt/Functionality/Mail%20Provider/Message%20Provider/MessageService.swift#L168

In case when user receives plain non-encrypted mail then app tries to decrypt it, but can't find local pub keys for mail sender, so it sends request to https://flowcrypt.com/attester/pub/ and receives 404 error. But this request takes some time and delays showing message to user.

Is there any method to find if message needs decryption or it's just plain text? I thought about checking if message starts with \r\n\r\n-----BEGIN PGP MESSAGE-----\r\n, but it doesn't seem to be a reliable solution, or it should work well?

Not reliably. This is why I was thinking to fetch the message from Gmail, and fetch the pubkey, in parallel.

But fetching a public key from Attester can be even slower, especially when it's a 404 (and Attester itself is trying to fetch them from remote places).

The proper way to do all of this, I think, is not to pre-fetch at all, but post-fetch and re-verify.

  1. user taps a message
  2. message gets downloaded from Gmail API
  3. message gets processed / decrypted / verified with whatever public keys we have locally
  4. result gets rendered including signature verification, and if we are not missing any public keys, then this is the last step
  5. if we are missing a public key, the verification "badge" will show gray 🕒 verifying signature..
  6. at this point we fetch remote public keys, but user is no longer waiting for it
  7. after remote public keys are fetched, we check if any of the keys now contain the signer longid. If they don't, now we render missing pub key badge
  8. if one of the keys now contain the signer longid, we re-process the whole message with that key. But only in background. The badge still says 🕒 verifying signature. Once we've done re-processing the message, we only look at verifyRes and re-render the badge with the final result, without re-rendering the rest of the message.

(btw - the chrome extension doesn't quite go as far at the moment. It renders the message, then realizes it's missing public key, fetches them, and if it finds any, renders "found a public key to verify this message, click here to verify). Clicking there just does window.reload() so whole message gets re-downloaded, and this time it verifies with the already imported keys. But one day, we'll make it work the way I've just described above)

@tomholub
Copy link
Collaborator

sometimes it gets confused when a message is signed but not encrypted (see email for how to reproduce):

image

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.

Looking good from code - see comments

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.

This looks good by code. What is the status of the PR - did you want to do more testing, or something left to do?

@sosnovsky
Copy link
Collaborator Author

I want to test it with flowcrypt.compatibility@gmail.com account, as you mentioned there is an issue with signed but not encrypted messages. But as it's quite rare case we can leave it for another issue, is it okay?

@tomholub
Copy link
Collaborator

I want to test it with flowcrypt.compatibility@gmail.com account, as you mentioned there is an issue with signed but not encrypted messages. But as it's quite rare case we can leave it for another issue, is it okay?

I'll file another issue for it, not necessary to do now.

@FlowCrypt FlowCrypt deleted a comment from github-actions bot Nov 15, 2021
@FlowCrypt FlowCrypt deleted a comment from github-actions bot Nov 15, 2021
@sosnovsky sosnovsky marked this pull request as ready for review November 15, 2021 20:48
tomholub
tomholub previously approved these changes Nov 15, 2021
Comment on lines +14 to +19
let signature: String
if let processedMessage = threadMessage.processedMessage, processedMessage.messageType == .encrypted {
signature = processedMessage.signature.message
} else {
signature = "message_not_signed".localized
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is likely the reason why signed-only messages are not recognized as signed - per above code, if they are not encrypted, they are always rendered as "not signed".

Would it be possible to always use processedMessage.signature.message regardless if encrypted or not? If that is missing, sounds like an error as well (since when signature is missing, processedMessage.signature.message is supposed to be there with notSigned case or similar)

Can address later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, initially I thought that all signed messages are encrypted, and haven't implemented signature state check for this case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Check the browser extension - next to the send button, there is a chevron. That's how a signed-only message could be sent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, didn't know about such feature 👍

@tomholub tomholub mentioned this pull request Nov 15, 2021
@tomholub tomholub enabled auto-merge (squash) November 15, 2021 21:13
@tomholub tomholub merged commit 8f69c60 into master Nov 15, 2021
@tomholub tomholub deleted the feature/issue-278-verify-signed-messages branch November 15, 2021 21:52
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.

findBy:longid unused? verify signed messages

2 participants