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

Fix passing self signed cert via arg/env #128

Merged
merged 1 commit into from
Sep 20, 2019

Conversation

lixin9311
Copy link
Collaborator

We cannot pass standard x509 cert, which includes at least two EOLs, via argument/env.

This patch fixes the issue with self signed certificates on Android and other clients.

@lixin9311
Copy link
Collaborator Author

lixin9311 commented Sep 20, 2019

btw, this also fixes #65

@lixin9311
Copy link
Collaborator Author

And checking the CA certificate before initializing v2ray would be better. v2ray err handling/logging is a mess.

@madeye madeye merged commit ca36119 into shadowsocks:master Sep 20, 2019
@lixin9311
Copy link
Collaborator Author

@madeye Could you pls update the Android plugin accordingly to fix the issue? I'm currently using a self-build version to use my self signed certificate.

@Mygod
Copy link
Collaborator

Mygod commented Sep 21, 2019

I thought DisableSystemRoot is required to make this work. Did you test your change?

@lixin9311
Copy link
Collaborator Author

I thought DisableSystemRoot is required to make this work. Did you test your change?

Yes, I have tested it on Android, the issue mentioned in DisableSystemRoot was caused by appending an invalid cert here.
https://github.com/v2ray/v2ray-core/blob/master/transport/internet/tls/config_other.go#L48

So we won't need to DisableSystemRoot.

@Mygod
Copy link
Collaborator

Mygod commented Sep 21, 2019

If we are using self signed certificate, we can disable system root certificates to enhance security as we know exactly what certificate we will get, even if the attacker has compromised the root certificate system.

@lixin9311
Copy link
Collaborator Author

Sure, I think it could be another feature. Anyway, this will work either disable system root cert or not.

@Mygod
Copy link
Collaborator

Mygod commented Sep 21, 2019

Disabling system root cert is also useful for debugging when the your certificate is not self signed.

@lixin9311
Copy link
Collaborator Author

While getting self-signed certs working on Windows, I found an upstream issue here.

https://github.com/v2ray/v2ray-core/blob/master/transport/internet/tls/config_windows.go#L9

v2ray will only load the given self-signed cert when DisableSystemRoot is set. I don't think that is the correct behavior. And it is not consistent with other OSs.

The relevant issue here, v2ray/v2ray-core#1513

I think it should load the given cert, whether the DisableSystemRoot was set or not.

I will send a pull request to upstream. For now, self-signed certs cannot work on Windows until we update the upstream.

@Mygod
Copy link
Collaborator

Mygod commented Dec 18, 2019

By the way what exactly does this PR fix? I don't think this PR serves any purpose. You are supposed to remove EOLs before passing it to arg.

@madeye Revert?

@lixin9311
Copy link
Collaborator Author

lixin9311 commented Dec 19, 2019

By the way what exactly does this PR fix? I don't think this PR serves any purpose. You are supposed to remove EOLs before passing it to arg.

@madeye Revert?

No. Standard x509 cert in PEM format MUST include at least two EOLs. Removing the EOLs will make the cert invalid.

-----BEGIN ENCRYPTED PRIVATE KEY-----asdfasdfasdf-----END ENCRYPTED PRIVATE KEY-----

Is invalid PEM format

-----BEGIN ENCRYPTED PRIVATE KEY-----
AS_LONG_AS_YOU_WANT
-----END ENCRYPTED PRIVATE KEY-----

Will be a valid PEM private key or cert.

It will fix E v2ray : v2ray.com/core/transport/internet/tls: failed to load system root certificate > v2ray.com/core/transport/internet/tls: append cert to root error.

Along with v2ray/v2ray-core@3b087bf all issues mentioned in #65 will be solved. But unfortunately, the v2ray upstream somehow reverted the patch. It seems like the guy was misconfiguring his certificates v2ray/v2ray-core#1982

@lixin9311
Copy link
Collaborator Author

By the way what exactly does this PR fix? I don't think this PR serves any purpose. You are supposed to remove EOLs before passing it to arg.

@madeye Revert?

Using self-signed cert by passing it -certRaw, which just like Android

After this patch:
patched

Unpatched version (Current HEAD):
head

@lixin9311
Copy link
Collaborator Author

@Mygod
Copy link
Collaborator

Mygod commented Dec 19, 2019

I see. Thanks.

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.

3 participants