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: Add config file support #90

Merged
merged 6 commits into from
May 16, 2023
Merged

Feat: Add config file support #90

merged 6 commits into from
May 16, 2023

Conversation

erfan-khadem
Copy link
Contributor

Closes #76

@ihciah
Copy link
Owner

ihciah commented May 13, 2023

Thanks for your contribution!
Cound you add some optional markers for fields like fastopen or disable_nodelay? It's better to make json field optional with the same default value if it is optional in command line parameter.
Also, it is better to use toml than json. Toml is better for human reading and editing.

@erfan-khadem
Copy link
Contributor Author

For sure toml is easier to read, but json tools are ubiquitous (like jq) and people who want to make shell scripts to make setting up the VPN easier are more important IMO. Also many existing tools use json, so non technical people who have learned to setup other tools will also have an easier time.

In any case, if it is a hard requirement, sure. I will do it. It won't take much more than changing from serde_json to toml and changing a line or two.

@ihciah
Copy link
Owner

ihciah commented May 15, 2023

For sure toml is easier to read, but json tools are ubiquitous (like jq) and people who want to make shell scripts to make setting up the VPN easier are more important IMO. Also many existing tools use json, so non technical people who have learned to setup other tools will also have an easier time.

In any case, if it is a hard requirement, sure. I will do it. It won't take much more than changing from serde_json to toml and changing a line or two.

It's ok to use json :) Using toml is just a suggestion. It can also be implemented as pre-read the first non-space char and using json or toml.

I think the optional's default value should be the same as in clap. So maybe only one default false function need to be defined and applied to all bool parameters(like fastopen, which should be false by default): fastopen, v3, strict, disable_nodelay, and others?

Thanks!

@erfan-khadem
Copy link
Contributor Author

It's ok to use json :) Using toml is just a suggestion. It can also be implemented as pre-read the first non-space char and using json or toml.

Using more than one standard is just going to complicate stuff. I was thinking of the same thing (detecting the config type by using the file extension) but I decided against it. You know, KISS.

I think the optional's default value should be the same as in clap. So maybe only one default false function need to be defined and applied to all bool parameters(like fastopen, which should be false by default): fastopen, v3, strict, disable_nodelay, and others?

You're right. what the hell was I thinking while writing that code :/
On the other hand though, can we start to change to more secure defaults? I know it should happen on a major release, but wouldn't it be better if we do it sooner?

@ihciah
Copy link
Owner

ihciah commented May 15, 2023

Using more than one standard is just going to complicate stuff. I was thinking of the same thing (detecting the config type by using the file extension) but I decided against it. You know, KISS.

Sure, json only is ok.

On the other hand though, can we start to change to more secure defaults? I know it should happen on a major release, but wouldn't it be better if we do it sooner?

If we can defend against some active attack easily, even we may not enable it, the attackers tend to not attack it. With the current default settings, users are free to use some domains with tls1.2 only.

There's a plan that support udp in next major version(tcp is compatiable). The current default works well, so I don't think there is a need to rush to release a new major version. Except for incompatiable changes, It should come with some new features.

@erfan-khadem
Copy link
Contributor Author

There's a plan that support udp in next major version(tcp is compatiable). The current default works well, so I don't think there is a need to rush to release a new major version. Except for incompatiable changes, It should come with some new features.

Can you explain this plan a little bit more to me? Or create a roadmap so that everybody can see where we are heading. RN I am very busy with university exams, but once I have a little bit of free time I would love to work on this project.

@ihciah
Copy link
Owner

ihciah commented May 16, 2023

There's a plan that support udp in next major version(tcp is compatiable). The current default works well, so I don't think there is a need to rush to release a new major version. Except for incompatiable changes, It should come with some new features.

Can you explain this plan a little bit more to me? Or create a roadmap so that everybody can see where we are heading. RN I am very busy with university exams, but once I have a little bit of free time I would love to work on this project.

Thanks! The udp support plan is a little bit ambigious now. The protocol is like what here in tcp. The traffic will look like QUIC, and the identity is hidden in SCID. We can try to make it clear later.

@ihciah ihciah merged commit 5484ecc into ihciah:master May 16, 2023
Kosette added a commit to Kosette/shadow-tls that referenced this pull request May 16, 2023
Feat: Add config file support (ihciah#90)
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