Implement mTLS (frozen version for audit)#22
Draft
cblgh wants to merge 17 commits into
Draft
Conversation
mTLs implementation does not follow spec yet but is the state of mTLS prior to it being removed from the previous sprint. most of the work has consisted in spec work so far :)
… connection flow the QR code flow has been disabled and removed from being visible. however QR code logic has been kept in case we reintroduce it at a later stage during this work (still in the air?) this commit has started to add changes to the connection flow as reflected in the new designs, including the new spinner modal graphic. this commit acts as a checkpoint before more widespread changes are attempted to be implemented
…npm audit warnings
If needed we can always retrieve the logic by looking back at previous commits and picking out the parts we need :) But until that day it is better to remove all of the logic relating to it to decrease the attack surface of hash verification.
note: in this commit the behaviour is almost exceedingly strict - once a request has come in which has a certificate, we this certificate's fingerprint as the fingerprint candidate and ignore + disallow any other requests from being considered to bring a certificate. benefit: it limits some attacks by swooping in and sending a malicious register request right when the other party press confirm & connect drawback: it opens up an attack where an active malicious attacker on the network could prohibit any other legitimate connections from being established by virtue of spamming the ping route with their own certificate (which ofc will not present a match when sender + reciever verify the sender fingerprint candidate). -- also in this commit: fixed a couple glaring interface issues that are completely unrelated
…hosen In order to minimize the attack surface of sender certificate hashes being either maliciously denied (attacker spamming known routes with their own certificate that would not match sender) or maliciously timed (timing a request with stolen credentials to make the receiver pin the wrong certificate) we in this commit implement a locking scheme. It works as follows: In each request that comes in pre-mTLS being established, and if the request has a certificate attached, then its fingerprint is calculated and saved as a potential candidate for a sender certificate hash. This candidate can be chosen many times. However, on successfully processing an authorised register request (PIN valid, nonce unseen), the candidate for sender certificate hash is locked and can no longer be changed to a different hash. This happens in such a way that the logic for getting the sender certificate hash to display in the desktop interface initiates the lock on the most recent fingerprint candidate. This candidate can now not be changed by future requests (unless connection is discarded and HTTPS server restarted). Since fingerprint candidate locking only happens after PIN and nonce have been checked to be valid, attacks to deny authorised request are prevented. Preventing the fingerprint candidate from being changed after a sender fingerprint has been displayed stops attacks that try to time sending a malicious request with stolen credentials. (Note for reviewers: As with all other commits by my user to date, this change and commit message has been fully authored by a human; no LLMs used.)
Currently this is mostly unused, but now it exists and can be attached to
'nearby-sharing-error' events emitted from the backend.
However, it is currently invoked inside of the
CertificateVerificationModal and might be better extracted elsewhere /
live alongside CertificateVefificationModal.
For example these connect-centric modals conditionally à la:
if !err then verifModal else errModal
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR exists to give a clean slate of the changes implemented as part of reintroducing mTLS for audit purposes. These changes implement the mTLS-centric changes documented in the version 2 Tella P2P Protocol specification.
The latest commit with any functional program code changes in this tree is (8bff1fb) and it corresponds to the Tella Desktop release candidate at https://github.com/Horizontal-org/Tella-Desktop/actions/runs/27941843030
1 additional commit has been pushed to prevent confusion in the audit process. It removed lingering and outdated TODOs (i.e. it contains no program code changes; only comment changes)
The PR at #21 may move ahead during the time of the audit with additional commits that focus on bugs and user-facing fixes for interface elements and such.