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

Allow component modules to be loaded from any path #117

Merged
merged 1 commit into from
Oct 31, 2018

Conversation

jrha
Copy link
Member

@jrha jrha commented Jun 27, 2017

Remove hasFile method which artificially restricts the paths which component
modules can be loaded from, allowing loading from any valid path in @INC.

This allows component modules to be placed in a location appropriate for each
operating system environments we target.

This change breaks backwards compatability by:

  • Removing the file? column from the output of reportComponents
    • Used by ncm-ncd --list/quattor-list so could potentially be being parsed by someone.
  • Removing hasFile entirely.
  • Removing getComponentFilename entirely.
  • Allowing modules to be loaded from other paths.
    • Potentially resulting in different/unexpected versions of code being run.

Resolves #116.

@jrha jrha force-pushed the remove-hasfile branch 2 times, most recently from 865af62 to 38c7ea1 Compare June 27, 2017 15:31
@stdweird
Copy link
Member

@jrha i don't know the original reason not to use the perl system path, but this is against that (obviously).

it also allows to pick up any component from PERL5LIB env variable, not sure that is an issue.

we can probably add a restriction to also consider the perl builtin INC paths, and not only /usr/lib/perl

also, please add unittests

@jrha jrha force-pushed the remove-hasfile branch 2 times, most recently from e8b4e5f to 6ea1a0a Compare June 27, 2017 15:40
@jrha
Copy link
Member Author

jrha commented Jun 28, 2017

I guess @Piojo might have some insight into the benefits of path restrictions, I assumed that these were just kludges left over from pre EL6 days. I'll sort out the tests later, just wanted to get this out so we could discuss it.

ttyS4 pushed a commit to ttyS4/ncm-ncd that referenced this pull request Dec 15, 2017
Fetch::Config: existing attributes and parameters should precede resp parameters and configvalues
@jrha jrha added this to the 18.3 milestone Feb 6, 2018
@jrha
Copy link
Member Author

jrha commented Feb 6, 2018

Make sure I squash these commits before this gets merged.

@stdweird
Copy link
Member

we agreed during the workshop of april 2018 to allow this, and this will be merged once reviewed

@stdweird
Copy link
Member

@jrha just a minor remark, this is otherwise good to go

@jrha jrha force-pushed the remove-hasfile branch 3 times, most recently from 5ba8b87 to effff19 Compare April 30, 2018 13:01
@jrha
Copy link
Member Author

jrha commented Apr 30, 2018

Well I broke it, will try and figure out what I did later.

@ned21 ned21 modified the milestones: 18.6, 18.9 Jul 5, 2018
@jrha
Copy link
Member Author

jrha commented Jul 31, 2018

@hpcugentbot please retest this please

@jrha
Copy link
Member Author

jrha commented Aug 1, 2018

Removed the test checking an internal function before the component was initialised - was causing the test failures.

@jrha
Copy link
Member Author

jrha commented Aug 2, 2018

@stdweird could you re-review this when you have a moment? 😇

@stdweird
Copy link
Member

@ned21 @jouvin i'll let you merge this so you aware of this new behaviour
@jrha thx for fixing this!

@ned21
Copy link
Contributor

ned21 commented Oct 20, 2018

I suspect this was originally in place because of CVE-2016-1238 https://rt.perl.org/Public/Bug/Display.html?id=127834 has more details but it's related to . in @INC. So I'm not sure we can or should trust @INC even in EL7.

Can we block any modules being loaded from . before merging this?

@stdweird
Copy link
Member

@ned21 you want that filtered here? i tought we already did this in the client tools (because otherwise you could load NCM::Component etc from .)?

@ned21
Copy link
Contributor

ned21 commented Oct 22, 2018

I thought I remembered that conversation too. So maybe that mitigation means it's now safe to remove from here? Can we confirm where the protection is now implemented and get that documented either inline as a comment or in the commit log?

@stdweird
Copy link
Member

@ned21 for ncm-ncd this was done in 5cfe993

Remove hasFile method which artificially restricts the paths which
component modules can be loaded from, allowing loading from any
valid path in `@INC`.

This allows component modules to be placed in a location appropriate
for each operating system environment we target.

This functionality may originally have been part of a mitigation
against CVE-2016-1238, but this was also addressed in 5cfe993 in a
far less restrictive way.

* Remove usage of hasFile from ComponentProxyList
* Declare mod_fn once and only when a value exists
@jrha
Copy link
Member Author

jrha commented Oct 31, 2018

@stdweird @ned21 commit message updated with explanation about CVE-2016-1238

@ned21 ned21 merged commit 95b3a2b into quattor:master Oct 31, 2018
@jrha jrha deleted the remove-hasfile branch November 1, 2018 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants