Repository: isso-comments/isso

Issue:
feature request: Allow config parser to detect environment variables
relevant code section: https://github.com/posativ/isso/blob/7f55c81e0875e1564eeab8ec6dd605c5699c9137/isso/config.py#L129-L130

See: https://stackoverflow.com/questions/26586801/configparser-and-string-interpolation-with-env-variable

Discussion:
@ix5: I'd be wary of having Isso be configured from multiple places. For now, it already reads `ISSO_SETTINGS` to find its own config files.

Having too many possible scenarios of mixed configs and inputs creates an untenable situation for testing. A bug in the parsing of another env variable slipped completely under the radar for that reason, see https://github.com/posativ/isso/issues/720

You'd also have to deal with which config source would take precedence and what to do with duplicates. Parsing env variables, which is untrusted input that potentially any user can set, also creates a bit of a security nightmare.

I'm doing to close this for now, unless you have a valid reason for this feature to be implemented.@andrewzah: 1. Env vars allow people to check config files into version control. I would like to be able to do this, and specify secrets as needed via env vars. 
2. Env vars are *not* less secure than reading files. Neither method provides security if the system is compromised. Only the user and root can see them, unlike files and directories which also need to be managed via permissions.
3. Env vars take priority over files typically. Just pick one and state it in the documentation. Allowing interpolation in the config file itself solves this issue.
4. _"Parsing env variables, which is untrusted input that potentially any user can set"_ the vast majority of the computing world is completely unsecure, then. Just about [every service that I use](https://github.com/andrewzah/lilac-docker/tree/master/services) allows config setting via env vars.

Not allowing env vars doesn't provide any additional security if the system is compromised, and it makes it harder for system administration because I can't check the config into source control systems so I have to manage it by hand every time I (re)install isso (via docker, etc).@ix5: Having thought about this a bit, your arguments convince me.
Would you like to submit a PR that introduces such ways of configuring Isso?@ix5: Related: https://github.com/posativ/isso/issues/763

Solution patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 01a344d..f218e9c 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -14,6 +14,7 @@ New Features
 - Add support for comment search by Thread URL in admin interface (`#1020`_, pkvach)
 - Add sorting option for comments (`#1005`_, pkvach)
 - admin: Add log out button (`#870`_, bbaovanc)
+- Add support for environment variables in config (`#1037`_, pkvach)
 
 .. _#870: https://github.com/posativ/isso/pull/870
 .. _#966: https://github.com/posativ/isso/pull/966
@@ -22,6 +23,7 @@ New Features
 .. _#1001: https://github.com/isso-comments/isso/pull/1001
 .. _#1020: https://github.com/isso-comments/isso/pull/1020
 .. _#1005: https://github.com/isso-comments/isso/pull/1005
+.. _#1037: https://github.com/isso-comments/isso/pull/1037
 
 Breaking Changes
 ^^^^^^^^^^^^^^^^
diff --git a/docs/conf.py b/docs/conf.py
index 3defa54..12bfde9 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -66,7 +66,7 @@ root_doc = 'docs/toc'
 
 # General information about the project.
 project = 'Isso'
-copyright = '2022, Martin Zimmermann & contributors'
+copyright = '2012-2024, Martin Zimmermann & contributors'
 
 # The language for content autogenerated by Sphinx. Refer to documentation
 # for a list of supported languages.
diff --git a/docs/docs/reference/server-config.rst b/docs/docs/reference/server-config.rst
index ea3c91d..4ef837a 100644
--- a/docs/docs/reference/server-config.rst
+++ b/docs/docs/reference/server-config.rst
@@ -572,3 +572,24 @@ Booleans
    ``on``/``off`` etc.
 
 .. _getboolean: https://docs.python.org/3/library/configparser.html#configparser.ConfigParser.getboolean
+
+Environment Variables
+^^^^^^^^^^^^^^^^^^^^^
+.. versionadded:: 0.13.1
+
+You can use environment variables in the configuration file. This is useful for keeping sensitive information, such as passwords, out of the configuration file itself. Environment variables are referenced using the ``$VAR_NAME`` or ``${VAR_NAME}`` syntax.
+
+Example:
+
+.. code-block:: ini
+   :caption: ``isso.cfg``
+
+    [general]
+    dbpath = /var/lib/isso/comments.db
+    host = https://example.tld/
+
+    [smtp]
+    username = $SMTP_USERNAME
+    password = ${SMTP_PASSWORD}
+
+In this example, the values for ``username`` and ``password`` will be taken from the environment variables ``SMTP_USERNAME`` and ``SMTP_PASSWORD``, respectively.
diff --git a/isso/config.py b/isso/config.py
index d0572cf..65bef1b 100644
--- a/isso/config.py
+++ b/isso/config.py
@@ -2,6 +2,7 @@
 
 import datetime
 import logging
+import os
 import pkg_resources
 import re
 
@@ -82,6 +83,7 @@ class IssoParser(ConfigParser):
         * Parse human-readable timedelta such as "15m" as "15 minutes"
         * Handle indented lines as "lists"
         * Disable string interpolation ('%s') for values
+        * Support environment variable substitution
     """
 
     def __init__(self, *args, **kwargs):
@@ -95,6 +97,10 @@ class IssoParser(ConfigParser):
         return super(IssoParser, self).__init__(
             allow_no_value=True, interpolation=None, *args, **kwargs)
 
+    def get(self, section, key, **kwargs):
+        value = super(IssoParser, self).get(section, key, **kwargs)
+        return os.path.expandvars(value)
+
     def getint(self, section, key):
         try:
             delta = timedelta(self.get(section, key))


Test patch:
diff --git a/isso/tests/test_config.py b/isso/tests/test_config.py
index 8a15d14..ba691e1 100644
--- a/isso/tests/test_config.py
+++ b/isso/tests/test_config.py
@@ -2,6 +2,7 @@
 
 import unittest
 import io
+import os
 
 from isso import config
 
@@ -34,3 +35,51 @@ class TestConfig(unittest.TestCase):
         # Section.get() should function the same way as plain IssoParser
         foosection = parser.section("foo")
         self.assertEqual(foosection.get("password"), '%s%%foo')
+
+    def test_parser_with_environment_variables(self):
+
+        parser = config.IssoParser()
+        parser.read_file(io.StringIO("""
+            [foo]
+            bar = $TEST_ENV_VAR
+            baz = ${TEST_ENV_VAR2}
+        """))
+
+        # Set environment variables
+        os.environ['TEST_ENV_VAR'] = 'test_value'
+        os.environ['TEST_ENV_VAR2'] = 'another_test_value'
+
+        # Test environment variable expansion
+        self.assertEqual(parser.get("foo", "bar"), 'test_value')
+        self.assertEqual(parser.get("foo", "baz"), 'another_test_value')
+
+        # Test Section.get() with environment variables
+        foosection = parser.section("foo")
+        self.assertEqual(foosection.get("bar"), 'test_value')
+        self.assertEqual(foosection.get("baz"), 'another_test_value')
+
+        # Clean up environment variables
+        del os.environ['TEST_ENV_VAR']
+        del os.environ['TEST_ENV_VAR2']
+
+    def test_parser_with_missing_environment_variables(self):
+
+        parser = config.IssoParser()
+        parser.read_file(io.StringIO("""
+            [foo]
+            bar = $MISSING_ENV_VAR
+        """))
+
+        self.assertEqual(parser.get("foo", "bar"), '$MISSING_ENV_VAR')
+
+    def test_parser_with_special_characters_in_environment_variables(self):
+
+        os.environ['SPECIAL_ENV_VAR'] = 'value_with_$pecial_characters'
+        parser = config.IssoParser()
+        parser.read_file(io.StringIO("""
+            [foo]
+            bar = $SPECIAL_ENV_VAR
+        """))
+
+        self.assertEqual(parser.get("foo", "bar"), 'value_with_$pecial_characters')
+        del os.environ['SPECIAL_ENV_VAR']