Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

MEP32: matplotlibrc/mplstyle with Python syntax.#9528

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
anntzer wants to merge3 commits intomatplotlib:mainfromanntzer:mplrc-mep

Conversation

anntzer
Copy link
Contributor

@anntzeranntzer commentedOct 22, 2017
edited
Loading

See proposed MEP text in the PR. The rendered version is available by clicking onview at the top right of the "files changed" tab.

I had this written for a while but was hoping to push this a bit later (I don't really have the time to work on it now).#6157 (comment) made me consider at least publishing my current thoughts.

attn @matplotlib/developers

rotsee reacted with thumbs up emoji
@jklymak
Copy link
Member

jklymak commentedOct 22, 2017
edited
Loading

Cool - I think I prefer the straight python syntax. How does that look to the user in their code where they call the style sheet? Same as before?

@anntzer
Copy link
ContributorAuthor

Indeed, I did not mention that point (nothing changes for the end user). Edited accordingly.

jklymak reacted with thumbs up emoji

@anntzeranntzerforce-pushed themplrc-mep branch 3 times, most recently froma1dff3f to43cab58CompareOctober 23, 2017 08:49
@jklymakjklymak added this to thev3.0 milestoneDec 21, 2017
@jklymak
Copy link
Member

Milestoning as 3.0, a) because I think this would be great, and popping to the top of everyone's queue, and b) because I would be surprised if it was accepted for 2.2.

tacaswell reacted with thumbs up emoji

@efiring
Copy link
Member

I also like plain python for configuration. I ran into a speed bump, but got over it, when doing something like this. See theBunch.from_pyfile() method inhttps://currents.soest.hawaii.edu/hgstage/pycurrents/file/tip/system/misc.py.

@tacaswell
Copy link
Member

Thanks for putting this together! I am coming around to this being the right thing to do (despite my grumbling).

A third option for the full-python implementation is to look for modules with a function

defapply_style(rcparams)->None:   ...

and havestyle.use pass the rcParams file through them in sequence. This would also let us do the update atomically by collecting the changes all into a temporary namespace and then update the global one once. It would also avoid having to delete the module to re-import it.

This would also allow for some interesting import hook implementations...

An other option would be to make rcparams aTraitlets object and leverage all of their configuration management.

anntzer and jenshnielsen reacted with thumbs up emoji

@jklymak
Copy link
Member

Ping@anntzer - I personally think this would be a great feature for 3.0.... (once you are done expunging any py2-isms that have been driving you nuts).

@WeatherGod
Copy link
Member

WeatherGod commentedMar 7, 2018 via email

This is basically dead on arrival as far as I am concerned. I have spokento a few other people in the private sector who have to put their systemsthrough security review, and they all agreed that matplotlib loadingarbitrary, unsanitized, code upon import, with no ability to exclude thatoperation or force a plain config approach, would fail security review.This is particularly problematic because matplotlib is a dependency to somany other projects, people may not even be aware that their system isvulnerable.The argument that "ipython does it, why can't we?" is misleading. ipythonis intended to be an end-user component, with its design and implicationsall up front. You don't accidentally have ipython be a dependency becauseit was a dependency of something else a few layers deep. matplotlib is usedas both an end-user component *and* as an API library. Its use is almost asprevalent as numpy. Imagine if numpy executed a config file every time itwas imported!If this were to go forward, serious work would need to go into its design,particularly for sanitizing the input and constraining its effects. I doubtthat the value proposition is there versus using a proper parser, severalof which exists and could be adapted to our use.
On Wed, Mar 7, 2018 at 4:38 PM, Jody Klymak ***@***.***> wrote: Ping@anntzer <https://github.com/anntzer> - I personally think this would be a great feature for 3.0.... (once you are done expunging any py2-isms that have been driving you nuts). — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#9528 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARy-LTdfWOy_TZ3Vr7kmhfkQOwIo2h-ks5tcFNlgaJpZM4QCEt2> .

@jklymak
Copy link
Member

Good point. I do wonder about MPL arbitrarily loading a config file, without the opportunity to turn that convenience off, or even better to have it turned off by default. I think it makes sense that downstream packages should never load any matplotlibrc file.

@anntzer
Copy link
ContributorAuthor

anntzer commentedMar 8, 2018
edited
Loading

I am not particularly interested in relitigating this question (although I will state once again that 1) did you know that np.load can also execute arbitrary code? 2) the "proper parser" proposed has been in a state of vaporware for quite a while), but given that both@tacaswell and@efiring consider that this is a good approach, I'll let them present their thoughts (if any) on the security implications.

As for the implementation strategy I think I quite like@tacaswell's suggestion of requiring anapply_style hook.

@WeatherGod
Copy link
Member

WeatherGod commentedMar 8, 2018 via email

which is why I don't use np.load() in my production code! We alreadycovered this. The point is that I can choose to not use np.load(). I canchoose to not use ipython. But if matplotlib goes down this route, it wouldhappen at import time, and can't be avoided.
On Wed, Mar 7, 2018 at 9:16 PM, Antony Lee ***@***.***> wrote: I am not particularly interested in relitigating this question (although I will still say that 1) did you know that np.load can also execute arbitrary code? 2) the "proper parser" proposed has been in a state of vaporware for quite a while), but given that both@tacaswell <https://github.com/tacaswell> and@efiring <https://github.com/efiring> consider that this is a good approach, I'll let them present their thoughts (if any) on the security implications. As for the implementation strategy I think I quite like@tacaswell <https://github.com/tacaswell>'s suggestion of requiring an apply_style hook. — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#9528 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARy-D4YXLPGMF05OQgji7688Mg8zM6-ks5tcJR6gaJpZM4QCEt2> .

@pwuertz
Copy link
Contributor

Just to throw in my 2 cents: From what I see none of the arguments againstYAML mentioned in the linked discussion apply here (any more). Theruamel.yaml implementation is actively maintained (forget aboutPyYAML) and does safe parsing by default, i.e. there is no security risk of importing/constructing arbitrary objects by default. You just explicitly add the constructors for the custom types (like cycler) to be supported.

In my personal opinionYAML currently is the most human-readable, human-editable configuration language out there. And having a parser/dumper with round-trip support (likeruamel.yaml) makes it machine-readable/writable too.

@anntzer
Copy link
ContributorAuthor

I guess I'm +0 on using yaml (if it is indeed possible to define custom type loaders), I don't like the language but it's not too bad andmost importantly at least it has a spec...

@WeatherGod
Copy link
Member

WeatherGod commentedMar 8, 2018 via email

Thanks for pointing out ruamel! I hadn't noticed it before!
On Thu, Mar 8, 2018 at 3:08 AM, Antony Lee ***@***.***> wrote: I guess I'm +0 on using yaml (if it is indeed possible to define custom type loaders), I don't like the language but it's not too bad and *most importantly* at least it has a spec... — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#9528 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARy-CihlfOEKXAX5zZzB0dSjqWjwCU4ks5tcOcRgaJpZM4QCEt2> .

@dopplershift
Copy link
Contributor

Given how it shows up in my conda updates, I think ruamel is used by conda for parsing its config.

I agree some with@WeatherGod here--as a library, we need to tread differently than an application. In theory, the same level of permissions required to modify the config would be used to run the script (baring misconfiguration), so no security risk. On the other hand, we're literally talking about adding a hook to trigger arbitrary code execution, which screams out CVE to me (as irrational as that might be). (I think I've changed my feeling from in the past.)

@anntzer
Copy link
ContributorAuthor

In theory, the same level of permissions required to modify the config would be used to run the script (baring misconfiguration), so no security risk.

Thanks for pointing that out. At that point I'm genuinely curious about what is the threat model that we're talking about avoiding. As in, can you present any way in which such a patch (say e.g. matplotlib executes ~/.config/matplotlibrc.py on import, just to pick a concrete case) actually makes things more dangerous?

Note also that youcan opt out: just set the MATPLOTLIBRC environment variable to os.devnull (/dev/null) before importing matplotlib. (Well, if that doesn't work, then there should be a patch that makes it work.)

@dopplershift
Copy link
Contributor

I'm not a security expert, nor do I want to pretend to be, so I feel really uncomfortable trying to decide what's "safe".

There's also a difference between what's actually safe and what our user community would perceive as safe.

@anntzer
Copy link
ContributorAuthor

anntzer commentedMar 8, 2018
edited
Loading

I guess it's good that the discussion restarted there, because I got to think about the issue again and am going to renege on my "uninterest in relitigating it" mentioned above.

Let's, again, use a concrete proposal as basis of discussion: at import time, matplotlib tries to import a module named matplotlibrc(.py) from the normal config path (possibly modified by MATPLOTLIBRC), and tries to call the apply_style(rc) function defined there. It is possible to disable this feature (for example) by setting MATPLOTLIBRC to os.devnull. Note that neithereval norexec appear in this proposal :-)

The fear appears to be that that module can be modified by an attacker or an oblivious user to execute arbitrary code, e.g.os.system("rm -rf /"), and we don't want to be responsible for that. But wait! An attacker or an oblivious user had a much simpler way to achieve the same thing: they could just write this into six.py and put that in the user's cwd (or ~/.local/lib/python3.6/site-packages), andwe do import six (and other dependencies) without any kind of validation (just as nearly all Python packages in the world do).

From this I conclude that we (and nearly all other Python packages that have external dependencies) are already vulnerable to arbitrary code execution (in that model), and can essentially do nothing about it.

Edit: for improved impact, replace six.py by some stdlib module name, of course.

@choldgraf
Copy link
Contributor

Just my 2 cents, I think this conversation should have input from somebody with experience in security issues who can vet the proposal. Security vulnerabilities are hard to spot and, as suggested in this thread, can be a deal-breaker for some groups. I'd feel more comfortable moving forward on this if a trusted voice said that it was OK from a security standpoint.

jkseppan reacted with thumbs up emoji

@anntzeranntzer mentioned this pull requestMay 13, 2018
@jklymakjklymak modified the milestones:v3.0,v3.1Jul 9, 2018
@anntzeranntzer mentioned this pull requestOct 12, 2018
6 tasks
@tacaswelltacaswell modified the milestones:v3.1.0,v3.2.0Feb 26, 2019
@anntzeranntzer mentioned this pull requestJul 31, 2019
6 tasks
@tacaswelltacaswell modified the milestones:v3.2.0,v3.3.0Aug 19, 2019
@QuLogicQuLogic modified the milestones:v3.3.0,v3.4.0Apr 30, 2020
@jklymakjklymak marked this pull request as draftSeptember 12, 2020 19:54
@QuLogicQuLogic modified the milestones:v3.4.0,v3.5.0Jan 21, 2021
@h-vetinari
Copy link

In my personal opinionYAML currently is the most human-readable, human-editable configuration language out there. And having a parser/dumper with round-trip support (likeruamel.yaml) makes it machine-readable/writable too.

Sorry for resurrecting an old issue, but I just saw it milestoned for 3.5.0 and had a look. I was wondering iftoml had been considered (it's not part of the list of formats in the alternatives). It basically combines (IMO) the best of both worlds between YAML & JSON, and results in a very readable format (and is getting ever more widespread use throughPEP518, cargo, etc.).

Since this allows easily defining arrays etc., and has a fully specified & verifiablegrammar, that might also help with having toeval strings that currently contain more complicated expressions.

NeilGirdhar reacted with thumbs up emoji

@anntzer
Copy link
ContributorAuthor

I don't think PEP518 is a good argument here; PEP518 explicitly chose to introduce a new format rather than using Python literals (https://www.python.org/dev/peps/pep-0518/#python-literals) because they expect build systems to be written in other languages than Python (note that this is theonly point against them), whereas we certainly don't expect matplotlibrc being read by anyone other than Matplotlib.

@h-vetinari
Copy link

I'm not saying it's a complete answer (e.g. I haven't understood thecycler requirements beyond the fact that some arrays are incompatible with the current comma-dependent parsing), my core point was mainly: TOML > YAML

anntzer and NeilGirdhar reacted with thumbs up emoji

@tacaswelltacaswell modified the milestones:v3.5.0,v3.6.0Aug 5, 2021
@timhoffmtimhoffm modified the milestones:v3.6.0,unassignedApr 30, 2022
@story645story645 modified the milestones:unassigned,needs sortingOct 6, 2022
@github-actions
Copy link

Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it.

@github-actionsgithub-actionsbot added the status: inactiveMarked by the “Stale” Github Action labelApr 21, 2023
@anntzer
Copy link
ContributorAuthor

I think the idea is well-advertised now and whether the issue is open or closed won't change much to the discussion.

@timhoffm
Copy link
Member

As a remark, strictYAML may be an improvement over the very general and complex YAML standard.https://github.com/crdoconnor/strictyaml

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
status: inactiveMarked by the “Stale” Github Actiontopic: rcparams
Projects
None yet
Milestone
future releases
Development

Successfully merging this pull request may close these issues.

12 participants
@anntzer@jklymak@efiring@tacaswell@WeatherGod@pwuertz@dopplershift@choldgraf@h-vetinari@timhoffm@QuLogic@story645

[8]ページ先頭

©2009-2025 Movatter.jp