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

Add filter for security warnings from urllib3 #2688

Merged

Conversation

matthewrmshin
Copy link
Contributor

Obviously, we are aware of the security issues on the affected
platforms, but these warnings serve no purpose on those platforms except
to confuse or annoy users.

Sufficient to close #2686. (Real fix will require #1874.)

Obviously, we are aware of the security issues on the affected
platforms, but these warnings serve no purpose on those platforms except
to confuse or annoy users.
@hjoliver
Copy link
Member

hjoliver commented Jun 5, 2018

Is it possible to emit just a single warning somehow? (I know that's difficult if this happens with every comms attempt). Just wondering in case total suppression of these warnings looks bad to security boffins.

@matthewrmshin
Copy link
Contributor Author

Not sure how because the warnings come from client commands, which are individual invocations. At the moment, for example, each job on the affected system will get two sets. One from the started message command and one from the succeeded message command.

The warnings are really pointless from the user's perspective because there is nothing they can do. It may be better if we simply document the potential issue of using HTTPS on python <2.7.10 in the user guide until we fully migrate to python 3.

@hjoliver
Copy link
Member

hjoliver commented Jun 6, 2018

Fair enough, that's fine with me.

@oliver-sanders
Copy link
Member

Would it be possible to configure which warnings we ignore on a per-host basis so that sys-admins must explicitly ignore them?

@matthewrmshin
Copy link
Contributor Author

Yes, but is it worth investing so much complexity? It will not be long before we migrate Cylc to Python 3, so I think we can live with this. I'll add some extra docs to the user guide to document this issue.

Also add extra info to user guide.
@@ -394,6 +396,8 @@ \subsection{Third-Party Software Packages}
\item {\bf pyOpenSSL} - \url{http://www.pyopenssl.org/}
\item {\bf python-requests} - \url{http://docs.python-requests.org/}
\item ({\bf python-urllib3} - should be bundled with python-requests)
\item ({\bf Python \lstinline@>=@ 2.7.9} (but not yet Python 3) is
recommended for the best security.)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps merge this into the previous paragraph to make it clear we support Python 2.6 something like:

{\bf Python \lstinline@>=2.6 <3@} is required {\bf Python \lstinline@>=2.7.9@} is recommended. Python
should already be installed in your Linux system. \url{https://python.org}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I have (hopefully) improved the statement regarding Python 2 requirement.

Cylc runs on Unix variants, usually Linux, and including Apple OS X.
Cylc runs on Linux. It is tested quite thoroughly on modern RHEL and Ubuntu
distros. Some users have also managed to make it work on other Unix variants
including Apple OS X.
Copy link

Choose a reason for hiding this comment

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

Some users have also managed to make it work on other Unix variants including Apple OS X.

That phrasing sounds to me like It is working under OSX because some users could make it to work. However, it is unfortunately not documented nor obvious how one can make it work under OSX (see #2689), so I would rather phrase it more transparent, e.g.

Some users have also managed to make it work on other Unix variants including Apple OS X, but they are not officially tested and supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added the suggested change to the latest commit.

Make it explicit that Apple OSX is not officially tested.
@oliver-sanders
Copy link
Member

Do we not also need to filter InsecurePlatformWarning?

@matthewrmshin
Copy link
Contributor Author

InsecrePlatformWarning is covered by SecurityWarning.

Copy link
Collaborator

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

This approach seems sensible & sufficient.

@hjoliver
Copy link
Member

@oliver-sanders - pinging you to sign off on this one, to get the release out early next week.

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

(Looks fine to me, although not tested - I don't have Python 2.6 available)

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Security warnings documented.

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.

SubjectAltNameWarning
5 participants