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

protect environment variables from being modified by modulefiles #429

Closed
xdelaruelle opened this issue Oct 25, 2021 · 8 comments
Closed
Milestone

Comments

@xdelaruelle
Copy link
Member

xdelaruelle commented Oct 25, 2021

As discussed on the mailing-list, it appears interesting to be able to protect some environment variables from being changed when loading or unloading modulefiles.

This could be implemented by adding a protected_envvars, a colon-separated list holding the environment variables that cannot be modified by modulefiles. This configuration is linked to the MODULES_PROTECTED_ENVVARS environment variable.

A warning message should be emitted when a modulefile attempts to modify a protected environment variable. This warning does not end the ongoing modulefile evaluation.

Modulefile commands affected by this warning message are:

  • append-path
  • prepend-path
  • remove-path
  • setenv
  • unsetenv
@xdelaruelle
Copy link
Member Author

@adrien-cotte I have started above the design for the feature you wished (to protect some env var from being changed by modulefiles). These guidelines may help you if you want to provide a PR for this.

@adrien-cotte
Copy link
Contributor

Thank you Xavier, could you give me hints about files to edit in order to implement this feature, please?

@xdelaruelle
Copy link
Member Author

@adrien-cotte I have just written a document to guide developers willing to add new configuration option to Modules:

https://modules.readthedocs.io/en/latest/design/add-new-config-option.html

Let me know if this doc is not clear enough (I have created a chatroom for Modules on matrix.org for questions and feedback like this: #modules:matrix.org)

@xdelaruelle
Copy link
Member Author

@adrien-cotte In addition to the guidelines for introducing new configuration option.

All environment variable changes go through the set-env or unset-env procedures that are defined in tcl/envmngt.tcl.in.

setenv, unsetenv, append-path, prepend-path and remove-path modulefile commands rely on these procedures, whether the modulefile is evaluated in load or unload mode.

So I suggest to update both set-env or unset-env procedures, add there a check that designated environment variable is not from the protected list otherwise emit a warning message then return (not to proceed with the environment variable change).

@adrien-cotte
Copy link
Contributor

Hi @xdelaruelle

This is my first try.

diff --git a/tcl/envmngt.tcl.in b/tcl/envmngt.tcl.in
index 36021556..a31d59ef 100644
--- a/tcl/envmngt.tcl.in
+++ b/tcl/envmngt.tcl.in
@@ -1154,11 +1154,20 @@ proc get-env {var {valifunset {}}} {
 proc set-env {var val} {
    set mode [currentState mode]
    reportDebug "$var=$val"
-
+    
+   # leave function if $var is protected by protected_envvars config
+   set isprotected false
+   foreach protectedvar [split [getConf protected_envvars]] {
+      if {$var eq $protectedvar} {
+         reportDebug "$var not set (protected_envvars config)"
+         set isprotected true
+      }
+   }
+        
    # an empty string value means unsetting variable on Windows platform, so
    # call unset-env to ensure variable will not be seen defined yet raising
    # an error when trying to access it
-   if {[getState is_win] && $val eq {}} {
+   if {[getState is_win] && $val eq {} && !$isprotected} {
       unset-env $var
    } else {
       interp-sync-env set $var $val
diff --git a/tcl/init.tcl.in b/tcl/init.tcl.in
index b21dc69b..46b9e93c 100644
--- a/tcl/init.tcl.in
+++ b/tcl/init.tcl.in
@@ -95,6 +95,7 @@ array set g_config_defs [list\
    pager {MODULES_PAGER {@pagercmd@} 0}\
    rcfile {MODULERCFILE <undef> 0}\
    redirect_output {MODULES_REDIRECT_OUTPUT 1 0 {0 1}}\
+   protected_envvars {MODULES_PROTECTED_ENVVARS <undef> 0}\
    quarantine_support {MODULES_QUARANTINE_SUPPORT @quarantinesupport@ 0 {0\
       1}}\
    run_quarantine {MODULES_RUN_QUARANTINE <undef> 0}\

What do you think?
Should I protect variables from unset-env too?
My handmade tests make me feel this patch is enough.

I still have to write configure, documentation, completion and testsuite for this new feature.

@xdelaruelle
Copy link
Member Author

xdelaruelle commented Feb 17, 2022

Good start!

Some suggestions:

  • Should also protect from unset-env.
  • Warning message should be emitted if variable is protected.
  • Could rewrite the foreach if sequence in:
    if {$var in [split [getConf protected_envvars] :]} {

@xdelaruelle
Copy link
Member Author

@adrien-cotte Any news from your side on this pull request ?

@adrien-cotte
Copy link
Contributor

This is on my vacation to-do list 😊

@xdelaruelle xdelaruelle added this to the 5.2 milestone Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants