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

SIP002 and SIP003 support. #1298

Merged
merged 2 commits into from
Aug 21, 2017
Merged

SIP002 and SIP003 support. #1298

merged 2 commits into from
Aug 21, 2017

Conversation

rwasef1830
Copy link
Contributor

Hello,
This is a first draft WIP to get feedback on my approach. This pull request implements SIP002 url (parsing and generation for QR code with backward-compatibility of accepting old-style urls and generating old-style urls when there is no plugin for the config), and SIP003 plugin interface for shadowsocks-windows.

  • SIP002 has unit tests and proven to work.
  • SIP003 needs a little bit of testing, would appreciate feedback on the approach and fresh eyes to see if I made any mistakes.

Hope to eventually get this merged soon :-)

@rwasef1830 rwasef1830 force-pushed the sip003 branch 3 times, most recently from 0f32572 to fe2411e Compare August 6, 2017 01:53
@rwasef1830
Copy link
Contributor Author

rwasef1830 commented Aug 6, 2017

Fixed it enough, seems to work now. Tested with simple-obfs but is it normal to have plugin process for each connection ? Waiting for feedback, thanks in advance.

[EDIT: Removed simple-obfs windows build, I made an msys2 build by mistake that depends on msys2 installed on machine, will compile properly using mingw32]
[EDIT: Build in https://github.com//pull/1298#issuecomment-321794702]

@rwasef1830
Copy link
Contributor Author

I will implement a better approach for this in the coming days (single process for plugin, and cleaner implementation).

@celeron533
Copy link
Contributor

Thank you @rwasef1830
Since this is a significant change and functional modification, need more developers to do the review.

@rwasef1830 rwasef1830 force-pushed the sip003 branch 2 times, most recently from 2538f46 to 2731d06 Compare August 11, 2017 02:48
@rwasef1830
Copy link
Contributor Author

rwasef1830 commented Aug 11, 2017

New cleaner approach done. Did a force push. Now it is a single plugin process per server and it is automatically restarted if it crashes or is killed. Process launch is protected by lock.

Waiting for review and merge ;-)

@rwasef1830
Copy link
Contributor Author

Turns out that it is not possible to compile simple-obfs using mingw-w64 (because of posix semantics, error in sys/select.h), so I redid build on latest master and bundled the needed msys dlls in the archive.

64-bit simple-obfs shadowsocks/simple-obfs@1126971 msys2 build. Should run on any 64-bit Windows 7 and higher.
simple-obfs-0.0.3-1126971.zip

Still waiting for feedback and/or merge ;-)

@chenshaoju
Copy link
Collaborator

chenshaoju commented Aug 11, 2017

The simple-obfs-0.0.3-1126971.zip look's work for me.

Saved,Thanks. 👍

2

@rwasef1830
Copy link
Contributor Author

rwasef1830 commented Aug 12, 2017

Rebased and force pushed on latest master. I hope this get merged soon, can't wait to use this functionality :-) Any devs around to review ? :-)

@celeron533
Copy link
Contributor

I saw you added ProcessStartInfo.Environment. Will all the plugins follow this standard in the future?

    Environment =
    {
        ["SS_REMOTE_HOST"] = serverAddress,
        ["SS_REMOTE_PORT"] = serverPort.ToString(),
        ["SS_PLUGIN_OPTIONS"] = pluginOpts
    }

@rwasef1830
Copy link
Contributor Author

Yes, this is described in the SIP003 standard shadowsocks/shadowsocks-org#28 and https://shadowsocks.org/en/spec/Plugin.html

Any SIP003 plugin to work with shadowsocks must read from these environment vars, so when I start the plugin, I pass them in ProcessStartInfo.

@rwasef1830
Copy link
Contributor Author

Made a force push to fix a tiny embarrassing one line null check in StopPlugins() (forgot to check if the server plugin was null :-D).

{
throw new ArgumentException("Value cannot be null or whitespace.", nameof(serverAddress));
}
if ((uint)serverPort != serverPort)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to check the negative port number?
Port number should between 1 to 65535.
Server port in config file should have been verified when user added the new entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah a silly mistake, I was intending to cast to ushort to verify range ... etc.
I don't like depending on checking only in higher levels of code in general, I consider each method should check its inputs (especially if there is negligible performance cost).

@rwasef1830
Copy link
Contributor Author

@celeron533 pushed a fix to the range check and moving the Sip003Plugin class.

@madeye
Copy link
Contributor

madeye commented Aug 21, 2017

I thinks this pull request is ready to merge? @celeron533

@rwasef1830
Copy link
Contributor Author

rwasef1830 commented Aug 21, 2017

@madeye @celeron533 For completeness sake, there are a few strings added to the GUI and I didn't add translations for them (in the Server config form)

@celeron533 celeron533 merged commit 71120e5 into shadowsocks:master Aug 21, 2017
@celeron533
Copy link
Contributor

Added a notification message on Forward Proxy form.

preview: celeron533@05df449

image

image

@dancju
Copy link

dancju commented Oct 27, 2017

do i have to compile simple-obfs on my own to use it?

@celeron533
Copy link
Contributor

@Nerddan #1379

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.

6 participants