search

socialwifi / RouterOS-api

Python API to RouterBoard devices produced by MikroTik.
MIT License
255 stars 98 forks source link

Finish adding SSL support #31

Closed stevehaskew closed 6 years ago

stevehaskew commented 6 years ago

I've taken the existing stale branch for adding SSL support, brought it up to date and finished adding support. Tested it as best I can, and it seems to perform correctly. Comments/thoughts welcome.

kramarz commented 6 years ago

Hi, thanks for contributing. I am no longer working on this project and never realy had opportunity to test this ssl support, but I wanted to give some suggestions for this api.

I think it would be better to allow user to pass the whole ssl_context object instead of trying to cover every scenrio. I would leave just two flags: ca_cert for quick enabling ssl with full validation (as a shortcut), and ssl_context for any customization. This two flags should not be used at the same time. Adding additional flag like use_ssl looks redundant to me: you either add the ssl context or not.

@jgoclawski do you agree?

stevehaskew commented 6 years ago

I would argue that the vast majority of users of this API will actually be using self-signed certificates to provide encryption but without caring about validation of the end device, so I think supporting this scenario without having to pass a context in would make sense. However, having the ability to pass a context if desired makes sense.

I also think that for any basic enabling off SSL we should just use a boolean, not requiring giving a cert - I don't require to have to provide a CA cert/file/dir - I can just use the OS defaults.

stevehaskew commented 6 years ago

As per the previous post, I have now added the ability to pass in the ssl_context, which overrides all other SSL-related options. I also added support for auto-detecting the port (8728/8729) if it is not specified.

stevehaskew commented 6 years ago

@kramarz - Since there is no response from any other official contributor, can we please move to a decision on whether to accept and merge this or whether we want to make more changes? As the best RouterOS tool available via PIP I would like to see this released so myself and others can use it more easily.

kramarz commented 6 years ago

Sure I think we can merge it. I have talked to SocialWifi guys and they promised me to give me rights to do so, but something went wrong. I will ask them again. In the meantime, I would like some final touches. You have put a lot of comments in the code I think a bit too many, Try to remove obvious ones but maybe set more descriptive variable names or add some helper functions instead.

In example instead of comment "port switching depending on ssl" write it like port = port or select_default_port(use_ssl) So it is self documented. You don't have to remove all of the comments but try to as many as possible.

Thanks again for contribution and patience. I will try to also make some time to look at other PRs.

jgoclawski commented 6 years ago

Hey, @stevehaskew thanks a lot for your contribution and sorry for the delay. I think it would be best to release it on pypi together with #32 so I'll wait for that other PR to be ready. @kramarz thanks A LOT for the review and general help with the project! :heart: