Cleanup PATH variable mangling #1026

jerome-benoit · 2023-01-18T10:12:28Z

Description

Checklist

Code compiles correctly
Relevant tests were added (unit / contract / integration)
Relevant logs were added
Formatting and linting run locally successfully
All tests pass
UA review
Design is documented
Extended the README / documentation, if necessary
Open source is approved

Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>

Dockerfile_mbtci_template

rodibrin · 2023-01-23T16:08:57Z

Dockerfile_mbtci_template

@@ -212,6 +212,12 @@ RUN set -ex \
  && python2.7 --version \
  && python3 --version

+# Allow global npm packages install without sudo
+RUN set -ex \
+  && mkdir ${USER_HOME_DIR}/.npm-global \


wonder why USER/USER_HOME_DIR instead of MTA_USER/MTA_USER_HOME is used!?

Cut&paste typo ...

Thanks.

Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>

rodibrin · 2023-01-23T16:25:35Z

Dockerfile_mbtci_template

+RUN set -ex \
+  && mkdir ${MTA_USER_HOME}/.npm-global \
+  && chown -R ${MTA_USER}:${MTA_USER} ${MTA_USER_HOME}
+ENV NPM_CONFIG_PREFIX ${MTA_USER_HOME}/.npm-global


according to the spec it's
ENV <key>=<value>
does it work without the equal too?

Both are working. The current Dockerfile use space, so let's keep it consistent with the style already used.

rodibrin · 2023-01-23T16:29:41Z

Dockerfile_mbtci_template

+  && mkdir ${MTA_USER_HOME}/.npm-global \
+  && chown -R ${MTA_USER}:${MTA_USER} ${MTA_USER_HOME}
+ENV NPM_CONFIG_PREFIX ${MTA_USER_HOME}/.npm-global
+
 ENV PATH=$PATH:./node_modules/.bin HOME=${MTA_USER_HOME}


on the image i used i do not have this path entry:

mta@63fceb6d640d:~$ echo $PATH /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin

is it intended that the root user has the home of mta?

root@b40df04c7063:/project# id uid=0(root) gid=0(root) groups=0(root) root@b40df04c7063:/project# echo $HOME /home/mta

It's not intended and a genuine security bug, and as well as the HOME env variable redefinition.
The PATH env variable mangling is buggy as hell in that Dockerfile. I plan to fix it but in another PR.

Thanks.

rodibrin · 2023-01-23T16:51:16Z

Does this PR solve #1018 ?

How does this PR relate to #1023 ?

jerome-benoit · 2023-01-23T17:07:48Z

Does this PR solve #1018 ?

As stated in the PR description, yes, but for currently supported node.js/java versions combination. It does not include node 14 and java 8.

How does this PR relate to #1023 ?

That PR is a duplicate of #1013 unrelated to the issue fixed here.
It's relevant for you as it reintroduces the support for the version combination you are using.
The original image with the issue have been built with the previous Dockerfile before its rewrite and has serious security issues. In the meantime, you can use the one you've successfully tested to work that introduce the fix in that PR until the java 8 support is re-added and that fix goes in. It does not contain the insecure chmod -R 777 usage in #1023.

rodibrin · 2023-01-24T08:19:24Z

As stated in the PR description, yes, but for currently supported node.js/java versions combination. It does not include node 14 and java 8.

Which node/java versions does it cover. we offer the following to the customer:

devxci/mbtci-java11-node14:
devxci/mbtci-java11-node16:
devxci/mbtci-java11-node18:
devxci/mbtci-java8-node16:
devxci/mbtci-java8-node18:
devxci/mbtci-java8-node14:

In the meantime, you can use the one you've successfully tested to ...

Unfortunately, the CICD service doesn't allow to easily switch to an arbitrary image. So we have to wait for the release of #1023, right?

jerome-benoit · 2023-01-24T12:07:52Z

devxci/mbtci-java11-node14: devxci/mbtci-java11-node16: devxci/mbtci-java11-node18: —

Only Java 8 based images are not yet shipped as images generated with the latest Dockerfile in that repo.

…

-- Jerome BENOIT - SAP E-Mobility R&D Security and Software Engineer SAP S/4HANA IC & Q2C ENG Europe III SAP Labs France SAS, BP1216,805 Av. du Dr. Maurice Donat, 06254 Mougins Cedex, France ***@***.*** - +33 6 35 50 78 40 OpenPGP Key ID : 27B535D3 Key fingerprint : B799 BBF6 8EC8 911B B8D7 CDBC C3B1 92C6 27B5 35D3 Please consider the impact on the environment before printing this email.

rodibrin

LGTM

jerome-benoit · 2023-01-31T15:11:44Z

LGTM

@young-yang03: I think the HOME variable override is causing issues at creating the directory structure under NPM_CONFIG_PREFIX path. I will remove it in that PR.

Since that PR can't cause any regressions as as long as npm -g is used, it's fine to merge it, ship the images and let @rodibrin confirm it works.

Dockerfile_mbtci_template

jerome-benoit · 2023-01-31T15:20:39Z

LGTM

@rodibrin: Have you also tested the node 16 java variant: https://hub.docker.com/repository/docker/fraggle0/mbt-node16-java8-docker/general?
If not, could you please confirm (or infirm) it works?

rodibrin · 2023-02-02T09:25:51Z

Test of
fraggle0/mbt-node16-java8-docker:latest 0f64f79118ce 9 days ago 665MB
failed:

npm ERR! code 254
npm ERR! path /project/node_modules/hana-cli
npm ERR! command failed
npm ERR! command sh -c -- npm link @sap/cds-dk --local
npm ERR! npm ERR! code ENOENT
npm ERR! npm ERR! syscall lstat
npm ERR! npm ERR! path /home/mta/.npm-global/lib
npm ERR! npm ERR! errno -2
npm ERR! npm ERR! enoent ENOENT: no such file or directory, lstat '/home/mta/.npm-global/lib'
npm ERR! npm ERR! enoent This is related to npm not being able to find a file.
npm ERR! npm ERR! enoent
npm ERR!
npm ERR! npm ERR! A complete log of this run can be found in:
npm ERR! npm ERR!     /home/mta/.npm/_logs/2023-02-02T09_12_44_959Z-debug-0.log

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/mta/.npm/_logs/2023-02-02T08_55_30_802Z-debug-0.log

mta@ba01bd395d75:/project$ ls -ali /home/mta/
total 48
1926508 drwxrwxrwx 1 mta  mta  4096 Feb  2 09:12 .
1926507 drwxr-xr-x 1 root root 4096 Jan 16 22:47 ..
1926509 -rw-r--r-- 1 mta  mta   220 Mar 27  2022 .bash_logout
1926510 -rw-r--r-- 1 mta  mta  3526 Mar 27  2022 .bashrc
1934075 drwxr-xr-x 3 mta  mta  4096 Feb  2 09:12 .cache
1934070 drwx------ 3 mta  mta  4096 Feb  2 09:12 .hdb
1926511 drwxr-xr-x 1 mta  mta  4096 Feb  2 08:55 .npm
1926517 drwxr-xr-x 2 mta  mta  4096 Jan 23 10:52 .npm-global
1926518 -rw-r--r-- 1 mta  mta   807 Mar 27  2022 .profile
mta@ba01bd395d75:/project$ ls -ali /home/mta/.npm-global/
total 12
1926517 drwxr-xr-x 2 mta mta 4096 Jan 23 10:52 .
1926508 drwxrwxrwx 1 mta mta 4096 Feb  2 09:12 ..

jerome-benoit · 2023-02-02T14:49:43Z

Test of fraggle0/mbt-node16-java8-docker:latest 0f64f79118ce 9 days ago 665MB failed:

Ok, thanks. I'm pushing the test images with the directory pre-creation. Dunno why npm does not properly populate it with latest version.

Could you please re-test?

Dockerfile_mbtci_template

rodibrin · 2023-02-02T17:31:56Z

test succeeded:

fraggle0/mbt-node16-java8-docker latest bcc6a780a618 3 hours ago 665MB

Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>

Fix npm link usage at build

24a0f5e

jerome-benoit requested a review from ShimiT as a code owner January 18, 2023 10:12

jerome-benoit requested a review from young-yang03 January 18, 2023 10:14

Improve npm usage without sudo

7f5f1fd

Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>

jerome-benoit changed the title ~~Fix npm link usage at build~~ Fix npm packages global installation Jan 23, 2023

jerome-benoit commented Jan 23, 2023

View reviewed changes

Dockerfile_mbtci_template Outdated Show resolved Hide resolved

Update Dockerfile_mbtci_template

9fa7ca9

rodibrin reviewed Jan 23, 2023

View reviewed changes

Fix variables name usage

68a9b75

Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>

rodibrin reviewed Jan 23, 2023

View reviewed changes

rodibrin approved these changes Jan 25, 2023

View reviewed changes

jerome-benoit commented Jan 31, 2023

View reviewed changes

Dockerfile_mbtci_template Outdated Show resolved Hide resolved

Update Dockerfile_mbtci_template

8b3a2a0

young-yang03 mentioned this pull request Feb 1, 2023

MBT Java 8 Docker Image #1023

Merged

9 tasks

jerome-benoit commented Feb 2, 2023

View reviewed changes

Dockerfile_mbtci_template Outdated Show resolved Hide resolved

Update Dockerfile_mbtci_template

31f6cb1

young-yang03 approved these changes Feb 3, 2023

View reviewed changes

Merge branch 'master' into jerome-benoit-patch-1

8edaa91

jerome-benoit changed the title ~~Fix npm packages global installation~~ Cleanup PATH variable mangling Feb 7, 2023

Fix mismerge

14f578f

Signed-off-by: Jérôme Benoit <jerome.benoit@sap.com>

Merge branch 'master' into jerome-benoit-patch-1

bcf6c26

young-yang03 closed this Sep 22, 2023

young-yang03 deleted the jerome-benoit-patch-1 branch September 22, 2023 07:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup PATH variable mangling #1026

Cleanup PATH variable mangling #1026

jerome-benoit commented Jan 18, 2023 •

edited

Loading

rodibrin Jan 23, 2023

jerome-benoit Jan 23, 2023

rodibrin Jan 23, 2023

jerome-benoit Jan 23, 2023

rodibrin Jan 23, 2023

jerome-benoit Jan 23, 2023 •

edited

Loading

rodibrin commented Jan 23, 2023

jerome-benoit commented Jan 23, 2023 •

edited

Loading

rodibrin commented Jan 24, 2023 •

edited

Loading

jerome-benoit commented Jan 24, 2023 via email •

edited

Loading

rodibrin left a comment

jerome-benoit commented Jan 31, 2023 •

edited

Loading

jerome-benoit commented Jan 31, 2023 •

edited

Loading

rodibrin commented Feb 2, 2023

jerome-benoit commented Feb 2, 2023 •

edited

Loading

rodibrin commented Feb 2, 2023

Cleanup PATH variable mangling #1026

Cleanup PATH variable mangling #1026

Conversation

jerome-benoit commented Jan 18, 2023 • edited Loading

Description

Checklist

rodibrin Jan 23, 2023

Choose a reason for hiding this comment

jerome-benoit Jan 23, 2023

Choose a reason for hiding this comment

rodibrin Jan 23, 2023

Choose a reason for hiding this comment

jerome-benoit Jan 23, 2023

Choose a reason for hiding this comment

rodibrin Jan 23, 2023

Choose a reason for hiding this comment

jerome-benoit Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

rodibrin commented Jan 23, 2023

jerome-benoit commented Jan 23, 2023 • edited Loading

rodibrin commented Jan 24, 2023 • edited Loading

jerome-benoit commented Jan 24, 2023 via email • edited Loading

rodibrin left a comment

Choose a reason for hiding this comment

jerome-benoit commented Jan 31, 2023 • edited Loading

jerome-benoit commented Jan 31, 2023 • edited Loading

rodibrin commented Feb 2, 2023

jerome-benoit commented Feb 2, 2023 • edited Loading

rodibrin commented Feb 2, 2023

jerome-benoit commented Jan 18, 2023 •

edited

Loading

jerome-benoit Jan 23, 2023 •

edited

Loading

jerome-benoit commented Jan 23, 2023 •

edited

Loading

rodibrin commented Jan 24, 2023 •

edited

Loading

jerome-benoit commented Jan 24, 2023 via email •

edited

Loading

jerome-benoit commented Jan 31, 2023 •

edited

Loading

jerome-benoit commented Jan 31, 2023 •

edited

Loading

jerome-benoit commented Feb 2, 2023 •

edited

Loading