Mitigate against PyYaml CVE-2017-18342 #1808
Conversation
|
I'd be inclined to leave this off based on #1808 (comment) and #1806 (comment). |
|
Awaits yaml/pyyaml#259 👍 |
|
I am not sure if that closing is the right way to deal with it because is not really resolved, we are just waiting for an external dependency but our core is still affected by the use of an unsafe library. |
Sorry if my meaning didn't come across well (or, it always feels bad to get a PR closed!) but #1806 (comment) remains open and is IMHO sufficient to track the progress of the new release of the library. I prefer to minimise noise in the PR/issues list where possible. |
|
@ssbarnea we can always reopen/resubmit this once the external requirement is fulfilled. Besides, I'd like to emphasize that it is not a security issue so no need to rush. Oh, and also it might be wise first ensure that any other dists which are usually installed along with Molecule itself will not suffer the dependencies conflict. I can imagine that if some of our (direct or transitive) deps is following typical semver practices and has |
|
cc |
| @@ -12,7 +12,7 @@ Jinja2==2.10 | |||
| pbr==5.1.1 | |||
| pexpect==4.6.0 | |||
| psutil==5.4.6; sys_platform!="win32" and sys_platform!="cygwin" | |||
| PyYAML==3.13 | |||
| PyYAML>=5.1 # MIT | |||
There was a problem hiding this comment.
Why MIT? Also, this might deserve a major release.
There was a problem hiding this comment.
@webknjaz The comment is only informative, I have seen documenting licenses of dependencies as comments as a common practice in OpenStack and even outside.
I think it does not hurt to start documentig licenses this way.
There was a problem hiding this comment.
I doubt we need a major release only for bumping some requirments. As long we do not break our API everything would count as minor.
There was a problem hiding this comment.
The API remains unchanged, so think we're looking at a minor release here. Shall we add a change log entry for this? For the comments: it may confuse contributors to see comments with licenses on some dependencies and not on others (is this optional? What is this for? Should I do this?), so I'd rather just avoid this additional practice for our dependency management on this project.
Let's merge this soon!
There was a problem hiding this comment.
changelog entry: yes
merge: move to setup.cfg
There was a problem hiding this comment.
I would refrain from migrating more into setup.cfg until packaging is fixed. I am considering proposing revert of the famous packaging change from last week #1966.
I think it was too early as it was not tested enough.
There was a problem hiding this comment.
It's not broken. Your use-case is not supported and you didn't provide steps to reproduce so it doesn't count. It works as long as you use it as designed.
While we already use only safe_load on molecule we are better-of forcing use of a version on PyYaml which is not affected by this security issue. Fixes: #1806 Signed-off-by: Sorin Sbarnea <ssbarnea@redhat.com>
ssbarnea
force-pushed
the
fix/1806-pyyaml
branch
from
713bd2e
to
ae9a0da
Compare
|
Closing as it should be done via #1806 (comment). Feel free to start doing that. |
While we already use only safe_load on molecule we are better-of
forcing use of a version on PyYaml which is not affected by this
security issue.
Fixes: #1806
Signed-off-by: Sorin Sbarnea ssbarnea@redhat.com
Please include details of what it is, how to use it, how it's been tested
PR Type