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

feat(tls): allow users to use array for ciphers #361

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat(tls): allow users to use array for ciphers #361

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 6, 2021

Hi everyone,

I noticed that the current cipher is usually too long when specifying multiple ciphers.

Therefore here is my proposal on this section that use an array named ciphers for it.

Here is how it looks like:

ssl:
  cipher: TLS_CHACHA20_POLY1305_SHA256:TLS_AES_256_GCM_SHA384:TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256:TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
  ciphers:
    - TLS_CHACHA20_POLY1305_SHA256
    - TLS_AES_256_GCM_SHA384
    - TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
    - TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384

I am looking forward to your ideas and feedback.

Regards,
Chinese White Dolphin

@Loyalsoldier
Copy link
Collaborator

Loyalsoldier commented Jun 6, 2021

Different orders of ciphers perform different fingerprints of TLS data, which means that the order of ciphers matters a lot. How will the json or yaml parser guarantee the order?

If they can not guarantee it, I think it's better to keep the long cipher string convention.

@ghost
Copy link
Author

ghost commented Jun 6, 2021

Different orders of ciphers perform different fingerprints of TLS data, which means that the order of ciphers matters a lot. How will the json or yaml parser guarantee the order?

If they can not guarantee it, I think it's better to keep the long cipher string convention.

Hi Loyalsoldier,

Good morning.

As far as I saw in the original implementation (fingerprint.ParseCipher(strings.Split(cfg.TLS.Cipher, ":"))), I found that strings.Split is used to split the original colon-separated string into an array.

I do make a test that

{
    "ciphers": ["A", "B", "C", "D"]
}

in JSON or

ciphers:
  - A
  - B
  - C
  - D

is the same as "A:B:C:D" in raw string.

I am looking forward to your ideas and feedback.

Regards,
Chinese White Dolphin

@Loyalsoldier
Copy link
Collaborator

This depends on implementations of parsers. I think we should leave this important thing to users, not the parsers.

@ghost
Copy link
Author

ghost commented Jun 6, 2021

This depends on implementations of parsers. I think we should leave this important thing to users, not the parsers.

Hi Loyalsoldier,

Good morning.

I agree with your idea that it should be left to users to determine their style of config files.
Therefore I think the deprecation hints should be removed.

Now the things works in the following approach:

  1. Had user specified the cipher in "cipher" using colon-separated representation, we use it;
  2. Had user not specified in "cipher" but in "ciphers" using array representation, we use it;
  3. Had user not specified in neither section, use the default value.

I am looking forward to your ideas and feedback.

Regards,
Chinese White Dolphin

@Loyalsoldier
Copy link
Collaborator

Loyalsoldier commented Jun 6, 2021

You got me wrong. What I mean is that I don't like the idea of this PR because there are chances that different implementations of parsers have different behaviors. We should NOT put users at risk by introducing an array with unpredictable order in ciphers in config.

Thanks for your contributions though.

@ghost
Copy link
Author

ghost commented Jun 6, 2021

You got me wrong. What I mean is that I don't like the idea of this PR because there are chances that different implementations of parsers have different behaviors. We should NOT put users at risk by introducing an array with unpredictable order in ciphers in config.

Thanks for your contributions though.

You are welcome. We heavily use trojan-go in our internal projects and modify it a lot, so it is obligatory to share our ideas and modifications with you.

I do see your idea that the dependency on parsers may cause inconsistency. In case of such situations, I will have our colleagues implement test units for parsers (JSON and YAML) to ensure they parse the input in an expected approach.

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