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

std.crypto.tls: implement TLSv1.2 #21872

Merged
merged 14 commits into from
Nov 8, 2024
Merged

std.crypto.tls: implement TLSv1.2 #21872

merged 14 commits into from
Nov 8, 2024

Conversation

jacobly0
Copy link
Member

@jacobly0 jacobly0 commented Nov 1, 2024

@jacobly0
Copy link
Member Author

jacobly0 commented Nov 1, 2024

According to top_sites.zig, this PR goes from 228/500 succeeded to 260/500 succeeded (with only 1 flaky regression).

@jacobly0
Copy link
Member Author

jacobly0 commented Nov 2, 2024

Now at 358/500 succeeded with x25519_ml_kem768 key share fixed, not including at least 2 flaky regressions.

@jacobly0
Copy link
Member Author

jacobly0 commented Nov 5, 2024

Now at 374 379 396 399414/500 succeeded.

@jacobly0 jacobly0 force-pushed the tlsv1.2 branch 3 times, most recently from becac42 to 99958bc Compare November 6, 2024 07:02
Note that the removed `error.TlsIllegalParameter` case is still caught
below when it is compared to a fixed-length string, but after checking
the proper protocol version requirement first.
This condition is already checked less restrictively in
`KeyShare.exchange`.
This is mostly nfc cleanup as I was bisecting the client hello to find
the problematic part, and the only bug fix ended up being

    key_share.x25519_kp.public_key ++
    key_share.ml_kem768_kp.public_key.toBytes()

to

    key_share.ml_kem768_kp.public_key.toBytes() ++
    key_share.x25519_kp.public_key)

and the same swap in `KeyShare.exchange` as per some random blog that
says "a hybrid keyshare, constructed by concatenating the public KEM key
with the public X25519 key".  I also note that based on the same blog
post, there was a draft version of this method that indeed had these
values swapped, and that used to be supported by this code, but it was
not properly fixed up when this code was updated from the draft spec.

Closes ziglang#21747
This was preventing TLSv1.2 from working in some cases, because servers
are allowed to send multiple handshake messages in the first handshake
record, whereas this inital loop was assuming that it only contained a
server hello.
By default, programs built in debug mode that open a https connection
will append secrets to the file specified in the SSLKEYLOGFILE
environment variable to allow protocol debugging by external programs.
This is the same mode used by openssh for private keys.  This does not
change the mode of an existing file, so users who need something
different can pre-create the file with their designed permissions or
change them after the fact, and running another process that writes to
the key log will not change it back.
@jacobly0
Copy link
Member Author

jacobly0 commented Nov 8, 2024

I don't see any way to increase compatibility with these buggy tls implementations without removing features, so that's where I'll leave it for now.

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.

4 participants