Allow component modules to be loaded from any path

quattor / ncm-ncd

Node Configuration Dispatcher Framework for Components

www.quattor.org

Other

4 stars 9 forks source link

Allow component modules to be loaded from any path #117

Closed jrha closed 6 years ago

jrha commented 7 years ago

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.

stdweird commented 7 years ago

@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 commented 7 years ago

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.

jrha commented 6 years ago

Make sure I squash these commits before this gets merged.

stdweird commented 6 years ago

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

stdweird commented 6 years ago

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

jrha commented 6 years ago

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

jrha commented 6 years ago

@hpcugentbot please retest this please

jrha commented 6 years ago

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

jrha commented 6 years ago

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

stdweird commented 6 years ago

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

ned21 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

@ned21 for ncm-ncd this was done in https://github.com/quattor/ncm-ncd/pull/123/commits/5cfe99375bae66cd176fdad8629ae44534d4a51e

jrha commented 6 years ago

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