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

Enable TLS 1.3 #1651

Closed
wants to merge 4 commits into from
Closed

Enable TLS 1.3 #1651

wants to merge 4 commits into from

Conversation

ZenerBarrier
Copy link

This pull request including the following changes:

  1. Set mandatory environment variables for enabling TLS 1.3 in go 1.12 (https://golang.org/pkg/crypto/tls/)
  2. Add 3 TLS 1.3 ciphers.
  3. Prefer AES-128-GCM-SHA256 as Encryption(+AEAD MAC) than CHACHA20-POLY1305
  4. Prefer ECDSA than RSA as Authentication in TLS 1.0 to TLS 1.2
  5. Change GOCACHE to an absolute path /tmp to avoid failing the bazel building (go1.12.3 编译v2ray 失败(GOCACHE is not an absolute path) #1643 fix #1643, fix v2ray/discussion#207. GOCACHE is not an absolute path #1650 )

P.S.
Q. Why this account is registered recently
A. It's not my main account, it's a mitigation of privacy compromising.

add environment variables to enable TLS 1.3
according to https://golang.org/pkg/crypto/tls/
1. add 3 TLS 1.3 specified ciphers.
2. prefer AES-128-GCM than CHACHA20-POLY1305 as Encryption.
3. prefer ECDSA than RSA as Key Exchange.
Copy link

@wy15 wy15 left a comment

Choose a reason for hiding this comment

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

os.Setenv("GODEBUG", os.Getenv("GODEBUG")+",tls13=1")

@kslr kslr requested a review from xiaokangwang April 25, 2019 03:41
@xiaokangwang
Copy link
Contributor

You should have submit 3 changes separately so that they can be discussed individually.

You are setting tls13 environment variable from config.pb.go, which can be regenerated at any time.

It is better to set it in a separate file, in a way that can be disabled.

v2fly/v2ray-core@fa976b8

Can you provide justification for "Prefer AES-128-GCM-SHA256 as Encryption(+AEAD MAC) than CHACHA20-POLY1305" ?

As for “Change GOCACHE to an absolute path /tmp to avoid failing the bazel building”, there are other PR addressing this. I recommend you to drop this from your PR so that discussions can be focus on other topic.

@ZenerBarrier
Copy link
Author

Can you provide justification for "Prefer AES-128-GCM-SHA256 as Encryption(+AEAD MAC) than CHACHA20-POLY1305" ?

The reason to give AES-128-GCM-SHA256 a higher priority than CHACHA20-POLY1305 is most of devices has specified instruction sets to enhance the performance of AES-GCM encryption (e.g. AES-NI).

If CHACHA20 ciphersuites is the first option of the client, some TLS server (e.g. those use OpenSSL with SSL_OP_PRIORITIZE_CHACHA option parameter) will recognize the client as 'NO AES instruction sets' and use CHACHA20 ciphersuites.

Thus, Most of TLS Clients use AES-128-GCM-SHA256 -> CHACHA20-POLY1305 -> AES-256-GCM-SHA384 as the ciphersuites priority when running on a platform with AES instruction sets, this change is an enhancement on performance and makes the TLS of V2Ray more similar to a common one like Google Chrome or Mozilla Firefox.

@ZenerBarrier
Copy link
Author

Should I close this PR and re-create one, keeps the change on tls13 ciphers only?
(Remove the change to build.bzl and tls13 environment variable)

It's my first PR, sorry for any inconvenient caused by me.

@kslr
Copy link
Contributor

kslr commented Apr 28, 2019

@kslr kslr closed this May 17, 2019
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.

None yet

4 participants