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

Implement ID store in a file #81

Merged
merged 36 commits into from
Mar 7, 2022
Merged

Implement ID store in a file #81

merged 36 commits into from
Mar 7, 2022

Conversation

covert-encryption
Copy link
Owner

@covert-encryption covert-encryption commented Feb 3, 2022

Covert needs persistent keystore to avoid always typing keys on command line and more importantly to enable forward secrecy in conversations (because temporary keys need to be stored somewhere).

This is a draft implementation that still contains bugs and does not implement anything but static keys.

covert enc -I alice:bob -i ~/.ssh/id_ed25519 -R github:bob   # Encrypt a message, store keys in database
covert enc -I alice:bob   # Encrypt another message using the stored keys

If the sender profile (alice) does not exist, it will be created. If no -i is provided, a new keypair is created. Keys may be replaced later by providing another recipient or -i arguments. It is planned that bob won't need to add alice's key when replying. In near future this should also enable a conversation mode where replies have forward secrecy and post-breach secrecy. The profile names are strictly local and are never sent to peers.

The ID store is protected by a longer than normal 5-word passphares, which is automatically created the first time the keystore is used. All profiles share the same master passphrase.

Breaking change: Signatures of upcoming 0.7 will be incompatible with prior versions.

TODO:

  • Public ID key storage and automatic generation
  • Use ID-based keys, display IDs in authentication and signatures instead of raw keys (CLI)
  • ID management (key export, changing of passphrase etc) covert id subcommand
  • ID selection dropdowns and other functionality based on ID store (GUI)
  • Daemon (agent) that remembers the passphrase for a while
  • Time-based expiration of keys (locally, not planned to be included in messages)
  • Code cleanup / refactoring & UX polish
  • Workflow to "reply" to messages in an easy way (CLI, GUI)
  • Forward secrecy in conversations

@codecov
Copy link

codecov bot commented Feb 3, 2022

Codecov Report

Merging #81 (4ead68c) into main (f9343a0) will increase coverage by 3.12%.
The diff coverage is 91.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #81      +/-   ##
==========================================
+ Coverage   75.43%   78.55%   +3.12%     
==========================================
  Files          23       27       +4     
  Lines        2190     2649     +459     
  Branches      513      609      +96     
==========================================
+ Hits         1652     2081     +429     
- Misses        419      431      +12     
- Partials      119      137      +18     
Impacted Files Coverage Δ
covert/passphrase.py 59.58% <0.00%> (-0.42%) ⬇️
covert/path.py 81.81% <81.81%> (ø)
covert/cli.py 82.03% <87.16%> (+3.10%) ⬆️
covert/__main__.py 96.09% <87.50%> (-0.53%) ⬇️
covert/idstore.py 89.92% <89.92%> (ø)
covert/clihelp.py 93.93% <93.93%> (ø)
covert/blockstream.py 87.80% <96.00%> (+0.75%) ⬆️
covert/cryptoheader.py 95.12% <100.00%> (+3.69%) ⬆️
covert/ratchet.py 100.00% <100.00%> (ø)
covert/tty.py 18.09% <100.00%> (+4.33%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9343a0...4ead68c. Read the comment docs.

@heikkiorsila
Copy link
Collaborator

If the sender profile (alice) does not exist, it will be created. If no -i is provided, a new keypair is created.

Would it make sense to not create a new sender automatically, but ask for a confirmation (y/n) before creating a new key? There is a small possibility of a typo in sender name. Creating unused redundant keys is bad. It does not matter for receiver names much.

covert/idstore.py Outdated Show resolved Hide resolved
covert/idstore.py Outdated Show resolved Hide resolved
@covert-encryption
Copy link
Owner Author

Would it make sense to not create a new sender automatically, but ask for a confirmation (y/n) before creating a new key? There is a small possibility of a typo in sender name. Creating unused redundant keys is bad. It does not matter for receiver names much.

There certainly could be a prompt when running in a terminal. Probably needs to default to "yes" in non-interactive use.

@covert-encryption
Copy link
Owner Author

covert-encryption commented Feb 13, 2022

Static keys fully implemented and working but still needs further work on UX and an implementation of temporary or ratchet keys. On decryption any keys in ID store are automatically tried before asking for passphrase, and the -I parameter is only used on encryption.

I plan to add -I for decryption too, to specify where to save any keys received, if the peer is not already known (in which case it can be matched automatically). Alternatively, random peer names could be created whenever decrypted using some identity from the store but with unknown peer, and implementing renaming of IDs separately via covert id subcommand.

@covert-encryption
Copy link
Owner Author

covert-encryption commented Feb 13, 2022

# Generate a keypair (new id:bob) and use it as a recipient
covert enc -I bob:bob ...

# Generate another keypair (new id:alice) and use the existing local id:bob as recipient
covert enc -I alice:bob -A
 🔒 covert    🔗 id:alice:bob 🔷 01e502fd4e5eda4a5cef9513  🖋️ id:alice  📋 copied

# Decrypt the message just created
covert dec -A
 💬
Hello
 🔑 Unlocked with id:bob
 🔷 File hash: 01e502fd4e5eda4a5cef9513
 ✅ id:alice Signature verified

@covert-encryption
Copy link
Owner Author

covert id --secret      # List all stored keys in Age format
covert id alice         # Create idstore and/or generate/show the public key for ID alice
covert id --passphrase  # Change the master password
covert id alice:bob --delete
covert id --delete-entire-idstore  # With shredding

@covert-encryption
Copy link
Owner Author

Basic CLI for forward secrecy as of now:

covert enc --id alice:bob -r (bob's public key)  # Initial message

Multiple initial messages may be sent, all use normal public key format. Bob needs to decrypt one or more of them, in any order:

covert dec   # Using bob's key already in idstore, decrypts the message as normal
covert dec -I bob:alice   # Additional parameter to make Covert store Alice's keys and start a conversation
covert enc -I bob:alice   # All messages sent use forward secrecy and require no key inputs

Alice receives and responds

covert dec   # Conversation automatically detected as alice:bob, no need for parameters
covert enc -I alice:bob

@covert-encryption
Copy link
Owner Author

This PR is getting big, and I will need to do major code refactoring for cleanup before proceeding with further tasks. Since the refactoring will make diffs unusable, I am asking for a review at this point - to get the biggest problems fixed but no full QA prior to merge - so that work can continue with another PR keeping the diffs and the scope of review reasonable.

@covert-encryption
Copy link
Owner Author

I am considering to entirely remove support for creation/modification of local keys via covert enc, as that is now supported via covert id which seems more suited for the job. Adding recipients on encryption remains a useful convenience shortcut but local IDs rarely need to be added.

@covert-encryption covert-encryption marked this pull request as ready for review March 6, 2022 00:00

### Initial messages using public keys

The sender of a public-key encrypted message may signal his capability to support Double Ratchet in replies by adding an `r: N` field in the archive index header. This should only be used when there is a single recipient, as the double ratchet only functions between two parties. Further, both parties will need to use a persistent **idstore** to keep track of the ratchet keys in use. The sender stores locally the filehash of each message sent, which becomes the header encryption key for the initial ratchet reply.
Copy link
Collaborator

@heikkiorsila heikkiorsila Mar 6, 2022

Choose a reason for hiding this comment

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

Since this is the first time "N" is mentioned, it should also be explained. This would be message number starting from 0, 1, ... presumably.


The sender of a public-key encrypted message may signal his capability to support Double Ratchet in replies by adding an `r: N` field in the archive index header. This should only be used when there is a single recipient, as the double ratchet only functions between two parties. Further, both parties will need to use a persistent **idstore** to keep track of the ratchet keys in use. The sender stores locally the filehash of each message sent, which becomes the header encryption key for the initial ratchet reply.

Thus Alice sends a message to Bob, advertising this capability and including her public key or a random ratchet key. Bob can respond using normal public keys but assuming that he chooses to use a ratchet, he needs to initialise in his idstore a new ratchet with the filehash of the incoming message and store it in his idstore. Note that Alice may have sent multiple initial messages, and Bob may respond to any of these, not necessarily reading the others. The initial messages do not have forward secrecy but the message number `N` (0, 1, ...) included in the archive index can be used to track lost messages.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Word ordering "initialise a new ratchet in his idstore" is more clear.


Thus Alice sends a message to Bob, advertising this capability and including her public key or a random ratchet key. Bob can respond using normal public keys but assuming that he chooses to use a ratchet, he needs to initialise in his idstore a new ratchet with the filehash of the incoming message and store it in his idstore. Note that Alice may have sent multiple initial messages, and Bob may respond to any of these, not necessarily reading the others. The initial messages do not have forward secrecy but the message number `N` (0, 1, ...) included in the archive index can be used to track lost messages.

Alice receives the message, not knowing it is from Bob, but trying all her stored keys to decrypt the encrypted header until a match is found with the filehash of the message she sent to Bob earlier. After this Alice initialises her end of the ratchet and expires the filehash from her idstore. Bob may also send multiple replies, and Alice can decrypt any of them using the same filehash but using nonce increments.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does "expiring the filehash" mean, please clarify.

@heikkiorsila
Copy link
Collaborator

I am considering to entirely remove support for creation/modification of local keys via covert enc, as that is now supported via covert id which seems more suited for the job. Adding recipients on encryption remains a useful convenience shortcut but local IDs rarely need to be added.

I think that's a good idea. It can be later added as an option for "covert enc" when and if we know better what is needed/practical.

covert/__main__.py Outdated Show resolved Hide resolved
self.ciphertext.release()
self.ciphertext = None
self.file = None
self.pos = self.end = 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is more clear to zero these variables on separate lines.

self.header.try_key(anykey)
else:
self.header.try_pass(anykey)

@contextmanager
Copy link
Collaborator

Choose a reason for hiding this comment

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

For clarity, decrypt.py:DecryptView class should call decrypt_init() inside a with statement.

Copy link
Owner Author

Choose a reason for hiding this comment

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

There is no decrypt.py or DecryptView class, and decrypt_init() is called within two with statements in blockstream.py and cli.py. Also, this does not seem relevant to this PR.

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.

2 participants