-
Notifications
You must be signed in to change notification settings - Fork 1
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
cryptoauth: draft 2 #2
base: master
Are you sure you want to change the base?
Conversation
👍 |
Found it! Needs a lot more work, but please read and comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review the introduction.
## Introduction | ||
CryptoAuth's mode of operation is simple: | ||
it takes as input individual network packets, encodes them, then outputs them. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inserting a line here introducing the following list would be useful to understand what the list is. Something as simple as “CryptoAuth handles incoming network packets this way” would help a lot.
1. Read one network packet and the respective remote public key. | ||
2. If no matching session in `established` state: | ||
- start new session (`new` state) or reuse existing session (other states), | ||
- encode as Handshake Packet, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
encode what?
- encode as Handshake Packet, | ||
- write packet, | ||
3. With existing `established` session: | ||
- encode as Data Packet, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
encode what?
- write packet, | ||
|
||
|
||
## Dramatization |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you want this section to contain?
|
||
When an implementation displays a private or public key | ||
to a user, or to another program, | ||
it should encode the key as Base32, as used by [dnscurve](dnscurve). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which variant of Base32?
~~The node which sends the first packet is the initiating party.~~ | ||
|
||
~~A handshake packet or data packet MUST start with a session state field, | ||
which determines the state of the handshake, or the recipient's identifier for the session.~~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the strikes?
- crypto_box_curve25519xsalsa20xpoly1305(tempPubKey, sharedSecret, nonce) | ||
- HELLO receive | ||
- sharedSecret: scalarmult_curve25519(recvrPrivKey, senderPubKey) + sha256(authchallenge) | ||
- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This trailing empty item messes GitHub's rendering of the previous line (interpreted as title).
|
||
TODO: should it be "RepeatedHello" instead of "RepeatHello"? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming it's an Hello that was repeated because there was no response: +1 for RepeatedHello. It took me a while when reading the Whitepaper to understand what RepeatHello could mean.
Whether this assumption is true or not, some explanation of the meaning of this packet would be useful.
@@ -171,7 +192,7 @@ Next a temporary key is generated to protect the handshake payload. The | |||
of the local node and the permanent public key of the remote node. The | |||
product of the combined keys is concatenated with the SHA256 digest of the | |||
authentication password, and the SHA256 digest of that concatenation is the | |||
temporary key. | |||
temporary key. ONLY FOR AUTHTYPE 1 & 2, FOR 0 THERE IS NO COMBINING |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Authtypes are only defined far below this line. I suggest rewriting it like this:
ONLY FOR AUTHTYPE 1 & 2 (password & login+password), FOR 0 (no auth) THERE IS NO COMBINING
@@ -195,20 +216,19 @@ and the **Permanent public key** field is stored for future use. | |||
|
|||
The **Hash code** subfield of **Authentication challenge** is validated | |||
against local passwords using the same method of hash code generatation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generation *
|
||
``` | ||
1 2 3 | ||
0 1 2 3 4 5 6 7 0 1 2 3 4 5 6 7 0 1 2 3 4 5 6 7 0 1 2 3 4 5 6 7 | ||
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | ||
0 | Session State | | ||
0 | Session State (int32 < 4) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uint32 *
I can't comment on the xsalsa vs salsa, but otherwise I agree with the review. 👍 |
Right now this is mainly just cjd's notes (and my table-of-contents) notes on the first draft by @ehmry.
I had a big edit of this document somewhere, but it seems I lost it...