Skip to content

Conversation

rrrooommmaaa
Copy link
Contributor

@rrrooommmaaa rrrooommmaaa commented Jul 9, 2021

This PR uses as many encryption keys as it can find per each recipient

close #3769


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

👍

Comment on lines 248 to 252
if (encryptionKeys.some(k => k.usableForEncryption)) {
return [{ email, keys: encryptionKeys.filter(k => k.usableForEncryption) }];
} else {
return [{ email, keys: encryptionKeys }];
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks good, to preserve existing notification that user key has expired. I think this may be the last step in facilitating smooth public key rotations / expirations as well - the old key will naturally expire, and both will be used in the in-between period.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, this could cause a little bit of trouble: we have a mechanism to keep public keys fresh, but it works higher up in the stack and probably needs all of the keys to work properly.

I can't come up with exact scenario when it would cause user-noticeable issues, but there possibly are some.

Ideally, the storage method would be returning all public keys for that recipient, and this logic would happen somewhere in composer view code / one of its modules. But the point is that the composer should have a chance to update a newer version of an expired key, even if some other valid key is available.

As it is now, it appears maybe the updated version of the expired key would not be noticed. (actually, depending on implementation, maybe on WKD it would be noticed since we pull by email and get back an array - can process the whole array which is good. But when pulling from Attester by fingerprint, that maybe doesn't work so well, because we don't ha ve a chance to pull updated version of the expired key)

@rrrooommmaaa rrrooommmaaa marked this pull request as ready for review July 12, 2021 15:16
@rrrooommmaaa rrrooommmaaa requested a review from tomholub July 13, 2021 13:38
@tomholub tomholub merged commit bcbd844 into master Jul 13, 2021
@tomholub tomholub deleted the issue-3769-encrypt-with-all-available-keys branch July 13, 2021 19:56
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.

encrypt for all of recipient's keys when sending a message

2 participants