Skip to content

verify signed messages(inband)#1601

Merged
DenBond7 merged 12 commits intomasterfrom
issue_1097_verify_signed_messages
Dec 15, 2021
Merged

verify signed messages(inband)#1601
DenBond7 merged 12 commits intomasterfrom
issue_1097_verify_signed_messages

Conversation

@DenBond7
Copy link
Collaborator

@DenBond7 DenBond7 commented Dec 13, 2021

This PR added verification for signed messages(inband). It means we added support for the following cases:

-----BEGIN PGP MESSAGE-----
Version: PGPainless

owGbwMvMwCXWaL/vvdiErcGMpxWSGEAgODM9LzVFoTyzJEOhJCNVoTg1OT8vRSE7
tbKjlIVBjIuBjZUpcevzDwyKnAIwzWKKLHfMHwmFMAmaprieNocJp/Uy/A/MjTzK
N9UkpXLTg0dXFFasM5wQxT93ufHj+o9hhzuO97YxMlwwsG+6lNHn4xkilcwbmmhx
/ernRzJH7rFaPLM7d/zlHj4A
=Yz2p
-----END PGP MESSAGE-----

and

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

[...]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v0.9.7 (GNU/Linux)
Comment: For info see http://www.gnupg.org

iEYEARECAAYFAjdYCQoACgkQJ9S6ULt1dqz6IwCfQ7wP6i/i8HhbcOSKF4ELyQB1
oCoAoOuqpRqEzr4kOkQqHRLE/b8/Rw2k
=y6kj
-----END PGP SIGNATURE-----

close #1097


Tests (delete all except exactly one):

  • Tests added or updated

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

@DenBond7 DenBond7 marked this pull request as ready for review December 14, 2021 11:45
@DenBond7 DenBond7 requested review from tomholub and removed request for IvanPizhenko December 14, 2021 11:46
@DenBond7
Copy link
Collaborator Author

@tomholub it's ready for review

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.

looks good - a few details + you also checked in some docker .log and .cache files by accident - should be gitignored. There is many of them

Comment on lines +1082 to 1083
ignoreMdcErrors = true
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ignoring MDC errors should only happen after user confirmed that they want to take the risk. Or do you pre-decrypt it while ignoring MDC but don't render first?

Should add a comment as this is security sensitive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tomholub
Please look at these screenshots

image

Or do you pre-decrypt it while ignoring MDC but don't render first?

It was simpler to do

Ignoring MDC errors should only happen after user confirmed that they want to take the risk.

Should I change the logic to use this way?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ref #1363

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tomholub What about this one? It seems it's the last one for the current PR.

Comment on lines -956 to +1086
content = resultWithIgnoredMDCErrors.content.toString(),
content = String(resultWithIgnoredMDCErrors.content?.toByteArray() ?: byteArrayOf()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

And here also comment

@DenBond7
Copy link
Collaborator Author

@tomholub

you also checked in some docker .log and .cache files by accident - should be gitignored. There is many of them

Do you mean docker-mailserver directory? I can't skip any files in this directory. All of them should be added to the index(git). As a solution, we can move it to a separate repo

@tomholub
Copy link
Collaborator

@tomholub

you also checked in some docker .log and .cache files by accident - should be gitignored. There is many of them

Do you mean docker-mailserver directory? I can't skip any files in this directory. All of them should be added to the index(git). As a solution, we can move it to a separate repo

You can definitely gitignore .log files in that dir - why not?

@DenBond7
Copy link
Collaborator Author

You can definitely gitignore .log files in that dir - why not?

It seems dovecot uses it. After moving to a new mailbox format manual changing any things in flowcrypt-android/docker-mailserver/maildata_source/flowcrypt.test can cause errors. I wouldn't like to investigate that issues :) (I had that experience). For me leaving all things as-is is a simpler way in this case.

https://doc.dovecot.org/developer_manual/design/indexes/mail_index_api/#view-syncing

@tomholub
Copy link
Collaborator

I think log files could definitely be gitignored, it's logs after all. Won't break anything. But I'll merge it.

@tomholub tomholub closed this Dec 15, 2021
@tomholub tomholub reopened this Dec 15, 2021
@DenBond7 DenBond7 merged commit c9bde07 into master Dec 15, 2021
@DenBond7 DenBond7 deleted the issue_1097_verify_signed_messages branch December 15, 2021 09: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.

verify signed messages(inband)

3 participants

Comments