Add ssl_verify to manage selfsigned certificate #88

heurtematte · 2023-02-02T17:15:12Z

Allow to param ssl_verify in configuration, allow to request https localhost homeserver with self signed certificate.

~/.config/synadm.yaml

ssl_verify: False

Signed-off-by: sebastien.heurtematte sebastien.heurtematte@eclipse-foundation.org

JOJ0

Hi @heurtematte, thanks for the submission, I'm sure this is a very useful features for a lot of other synadm users as well! Nice!

One thing that comes up in a first and unthorough review is that you define boolean as for the new verify flag on cli level but at some point in the code it is changed / has a default value of None which not really is boolean. Is there a reason for that or could we change it to stay True/False everywhere?

Also, some tiny formatting issues are detected by our linter (flake8) in the CI.

Thanks again and speak soon!

heurtematte · 2023-02-03T08:37:42Z

Is there a reason for that or could we change it to stay True/False everywhere?

I wanted to keep the method query in the same state, and offer the possibility to override the value programmatically if really necessary over configuration setting, and testing None is easire than a boolean value.

def query(self, method, urlpart, params=None, data=None, token=None,
              base_url_override=None, verify=None):

Tell me if you want me to change this behavior.

I'll check the linter.

Signed-off-by: sebastien.heurtematte <sebastien.heurtematte@eclipse-foundation.org>

JOJ0

Sorry for being picky but we are partly changing something very fundamental here and need to make sure synadm behaves correctly in all use cases. Thanks for your patience and further refinement.

In the end these things count most IMO:

What will happen if a user updates synadm to this new version and thus, does not have this new option in the config already? I guess the configurator will pop up automatically on any run AFAIR . That still the case?
The default for new synadm users in the auto-configurator should still be "SSL verification is enabled."
It should be clear to the user on installation and upgrade what the state of the setting SSL verification currently is.

It would help me reviewing if you post some cli outputs of the configurator and other related cli calls. I'm always concerened with the user experience and --help the most :-)))

JOJ0 · 2023-02-03T09:15:47Z

synadm/cli/__init__.py

@@ -451,6 +462,11 @@ def get_redacted_token_prompt(cli_token):
            "Homeserver name (auto-retrieval or matrix.DOMAIN)",
            default=homeserver if homeserver else helper.config.get(
                "homeserver", homeserver)),
+        "ssl_verify": click.prompt(
+            "Verify certificate (Default True, False allow to manage \
+                selfsigned certificate)",


The backslash and the indentation is not required in Python. See other help variables using triple double quotes a little further above. Thanks :-)

Although I feel it's sufficient here to leave the False... part out entirely. Most people/admins will be aware of what ssl_verify option means. It's kind of self-descriptive int that context.

Please note that we have currently not defined the default values of the other config variables explicitly in the help string. Because if you run the configurator the current value (by default is set to the values defined in APIHelper.CONFIG) is presented behind each prompt within brackets, so the user exactly knows which value the corresponding config variable is currently set to. For example it looks like Synapse base URL [http://localhost:8008].
Therefore I don't think that there is a need to explicitly state the default value within the help string

sounds good!

JOJ0 · 2023-02-03T09:41:03Z

synadm/api.py

@@ -375,11 +388,13 @@ def __init__(self, log, user, token, base_url, admin_path, timeout, debug):
            timeout (int): Requests module timeout used in ApiRequest.query
                method
            debug (bool): enable/disable debugging in requests module
+            verify(bool): SSL verification is turned on by default
+                and can be turned off using this method.


I'd prefer the wording: ....using this argument.

synadm/api.py

JOJ0 · 2023-02-03T09:49:43Z

synadm/cli/__init__.py

-            if not value:
-                self.log.error("Config entry missing: %s", key)
+            if value is None:
+                self.log.error("Config entry missing: %s, %s", key, value)


Youwould like to see whatexactly is wrong? Value missing or empty string for example? Or other reason why you added value to be displayed?

You are now only checking for None. The if not value would have also catched empty strings which also often would cause errors. Not sure but did you test thorougly with setting other values in the config to weird things and made sure things will be catched?

Also note: The usual default of things we get from a Click-defined options is actually None if not specified otherwise. But please doublecheck all possible things that might go wrong here even if a user edits config.yaml manually (without the configurator)

Good catch. I roll back my modification.

I remember why now.

If I set false to the ssl_verify configuration. I have this message

ERROR Config entry missing: ssl_verify, False

So I must deal with string value and boolean value.

I understand!

JOJ0 · 2023-02-03T11:03:04Z

synadm/api.py

@@ -55,6 +56,8 @@ def __init__(self, log, user, token, base_url, path, timeout, debug):
                base_url to form the basis for all API endpoint paths
            timeout (int): requests module timeout used in query method
            debug (bool): enable/disable debugging in requests module
+            verify(bool): SSL verification is turned on by default
+                and can be turned off using this method.


I'd prefer the wording: ....using this argument.

Also, is the type now still bool?

Good question! I'll check

We can keep bool type

JOJ0 · 2023-02-03T11:29:59Z

synadm/cli/__init__.py

+    "--ssl_verify", "-n",  default=True,
+    help="""Verify certificate (Default True, False allow to manage \
+        selfsigned certificate)."""
+)


Default true can be specified by Click's show_default=True flag. Use that and I think we could then rephares this help text to something like: "Whether or not SSL certificates should be verified. Set to False to allow self-signed certifcates." or maybe even shorter because ssl_verify tells it all to a regular Matrix user/admin already:

Set to False to allow self-signed certifcates. Also of I recall correctly, in yaml true needs to be spelled in non capital letters! Other than in Python! I wonder if that example should already use that spelling? Oooor - I didn't check - if the configurator would handle this lower-casing automatically.... Please doulbe check!

JOJ0 · 2023-02-08T07:19:36Z

Hi @heurtematte can I assist with anything else?

There are some things to fix. No hurry in doing that right away. Take your time, the merge of this new feature can wait. I just would like to know if you're still on it.

In the meantime I did some testing and another thing comes up. It seems the actual value set in the config is not displayed in the interactive configurator. This is misleading. Have a look how other config settings do it. Any non-sensitive information should be printed while the user reconfigures.

(synadm38) ~/git/heurtematte_synadm (master) $ grep verify ~/.config/synadm.yaml 
ssl_verify: 'false'
(synadm38) ~/git/heurtematte_synadm (master) $ 
(synadm38) ~/git/heurtematte_synadm (master) $ 
(synadm38) ~/git/heurtematte_synadm (master) $ 
(synadm38) ~/git/heurtematte_synadm (master) $ synadm config 
Running configurator...
Synapse admin user name [admin]: 
Synapse admin user token [REDACTED]: 
Synapse base URL [https://example.org]: 
Synapse admin API path [/_synapse/admin]: 
Matrix API path [/_matrix]: 
Default output format (yaml, json, human, pprint) [yaml]: 
Default http timeout [3600]: 
Homeserver name (auto-retrieval or matrix.DOMAIN) [auto-retrieval]: 
Verify certificate (Default True, False allow to manage                 selfsigned certificate) [True]:

Also we see that the (text in brackets) is rather long and contains a line break. I've mentionend that above already but now I'm sure that it would be sufficient here to make this prompt look like this:

Verify certificate [true]:

Cert verification (or not) is a logical thing for any admin, no further visual distraction required.

heurtematte · 2023-02-08T15:48:04Z

sorry a bit busy! I'll take a look by the end of the week.

Signed-off-by: sebastien.heurtematte <sebastien.heurtematte@eclipse-foundation.org>

…text option Signed-off-by: sebastien.heurtematte <sebastien.heurtematte@eclipse-foundation.org>

JOJ0 · 2023-03-10T08:34:25Z

Hi @heurtematte, thanks for your changes last week. We've been busy with other things in the project but I think around next week might be able to conintue work here. We'd like to do some testing and tiny changes to finalize this. Do you mind if we take over your branch to incorporate said finalization? Thanks!

Remove redundant backslash and fix indentation.

- Fix long option: - Dashes instead of underscores. - Make sure it is boolean: Set is_flag. - Fix short option: -n is in use already, let's call it -i ("insecure", like other tools use it, eg. curl)

We need to make sure click.prompt saves the value as a boolean, i.e false instead of 'false', true instead of 'true'. Lower-case true/false is valid yaml syntax for boolean values.

JOJ0 · 2023-03-15T11:40:56Z

Hi @JacksonChen666, I've added a couple of formatting but also functional fixes to make a real boolean value in the config work.

Please could you possibly try this in your testing environment:

What happens if the config is missing at all? For example if a user had used synadm before and now upgrades withouth taking care of the config. -> Should be just ignored and the default of "True" is used.
What happens if the user issues synadm config for the first time after an upgrade (withouth the option)
What happens if rubbish is written into the config? E.g ssl_verify: bollocks text

Thanks a lot!

JacksonChen666

Please could you possibly try this in your testing environment:
* What happens if rubbish is written into the config? E.g ssl_verify: bollocks text

default value is used regardless of config values, even if configured with False in synadm config it will default to True (very likely not intended behavior).

I have made a comment in the place where I think the issue is, plus something else I noticed.

JacksonChen666 · 2023-03-15T13:11:16Z

synadm/cli/__init__.py

-               output, timeout, server_discovery, homeserver):
-    """ Modify synadm's configuration.
+               output, timeout, server_discovery, homeserver, ssl_verify):
+    """ Modify test synadm's configuration.


test? "Modify test synadm's configuration" doesn't make sense language-wise

Oh, that seems to be a leftover of some debugging by the OP or something. Corrected.

JacksonChen666 · 2023-03-15T13:12:24Z

synadm/cli/__init__.py

@@ -376,10 +385,15 @@ def root(ctx, verbose, batch, output, config_file):
    default value 'auto-retrieval' will try to discover the name using the
    method set by --server-discovery."""
 )
+@click.option(
+    "--ssl-verify", "-i",  is_flag=True, default=True, show_default=True,


default=True is odd in this case, as defaults aren't set in any options above but instead below in the click.prompt things.

this makes for a UI inconsistency where all other configs use the default value of whatever is set (in synadm config) in the options except for --ssl-verify.

Yes! You are absolutely right, we don't handle defaults that way! Defaults should always be handled via APIHelper.CONFIG and the click.prompt things as you noticed! Good catch! I fixed this.

I think now the behaviour is quite fine. Can you please give it another testrun?

To me it looks like this:

We silently default to true if the option is missing.

We display an error when the option is manually set wrongly to some non-boolean string.
but we don't quit synadm and continue with subsequent errors. This is good, since otherwhise the configurator could not be run. For example:

(synadm38) ~/git/heurtematte_synadm (master) $ synadm -vv user details testuser2 DEBUG Config entry read. user: admin DEBUG Config entry read. token: REDACTED DEBUG Config entry read. base_url: https://peek-a-boo.at DEBUG Config entry read. admin_path: /_synapse/admin DEBUG Config entry read. matrix_path: /_matrix DEBUG Config entry read. timeout: 3600 DEBUG Config entry read. server_discovery: well-known DEBUG Config entry read. homeserver: peek-a-boo.at ERROR Config value error: ssl_verify, crap must be boolean DEBUG Config entry read. ssl_verify: crap DEBUG Config entry read. format: yaml DEBUG Formatter in use: yaml - <function dump at 0x7f69b9e70e18> DEBUG A proper localpart was passed, generating MXID for local homeserver. INFO Querying get on https://peek-a-boo.at/_synapse/admin/v2/users/@testuser2:peek-a-boo.at ERROR OSError while querying Synapse: Could not find a suitable TLS CA certificate bundle, invalid path: crap User details could not be fetched. (synadm38) ~/git/heurtematte_synadm (master) $

A configurator run would still display the current value crap, and the user intentionally has to set 1 or true or True to fix it. I think that is ok:

(synadm38) ~/git/heurtematte_synadm (master) $ synadm config ERROR Config value error: ssl_verify, crap must be boolean Running configurator... Synapse admin user name [admin]: Synapse admin user token [REDACTED]: Synapse base URL [https://peek-a-boo.at]: Synapse admin API path [/_synapse/admin]: Matrix API path [/_matrix]: Default output format (yaml, json, human, pprint) [yaml]: Default http timeout [3600]: Homeserver name (auto-retrieval or matrix.DOMAIN) [peek-a-boo.at]: Verify certificate [crap]: 1 Server discovery mode (used with homeserver name auto-retrieval) (well-known, dns) [well-known]: Restricting access to config file to user only.

They could still leave crap but then would get displayed an error again:

$ synadm config ERROR Config value error: ssl_verify, crap must be boolean Running configurator... Synapse admin user name [admin]: Synapse admin user token [REDACTED]: Synapse base URL [https://peek-a-boo.at]: Synapse admin API path [/_synapse/admin]: Matrix API path [/_matrix]: Default output format (yaml, json, human, pprint) [yaml]: Default http timeout [3600]: Homeserver name (auto-retrieval or matrix.DOMAIN) [peek-a-boo.at]: Verify certificate [crap]: Server discovery mode (used with homeserver name auto-retrieval) (well-known, dns) [well-known]: Restricting access to config file to user only. ERROR Config value error: ssl_verify, crap must be boolean

We don't set defaults while initially setting up with the @click.option decorator. We handle defaults via the APIHelper.CONFIG dict and some magic in the configurators cli code.

JOJ0 requested changes Feb 3, 2023

View reviewed changes

heurtematte force-pushed the master branch from eacc7bf to 4560912 Compare February 3, 2023 08:46

feat: add ssl_verify to manage selfsigned certificate

cf16d4a

Signed-off-by: sebastien.heurtematte <sebastien.heurtematte@eclipse-foundation.org>

heurtematte force-pushed the master branch from 4560912 to cf16d4a Compare February 3, 2023 08:51

JOJ0 requested a review from Ascurius February 3, 2023 09:07

JOJ0 requested changes Feb 3, 2023

View reviewed changes

JOJ0 reviewed Feb 3, 2023

View reviewed changes

heurtematte added 2 commits March 1, 2023 17:28

docs: reword function to argument

a4b1062

Signed-off-by: sebastien.heurtematte <sebastien.heurtematte@eclipse-foundation.org>

fix: add more tests on ssl_verify boolean value, and change some cli …

3ad2ade

…text option Signed-off-by: sebastien.heurtematte <sebastien.heurtematte@eclipse-foundation.org>

JacksonChen666 self-requested a review March 9, 2023 16:29

JOJ0 added 3 commits March 15, 2023 07:35

Fix "config --ssl_verify" help

352859b

Remove redundant backslash and fix indentation.

Fix config --ssl-verify option long and short form

bb0b015

- Fix long option: - Dashes instead of underscores. - Make sure it is boolean: Set is_flag. - Fix short option: -n is in use already, let's call it -i ("insecure", like other tools use it, eg. curl)

Fix ssl_verify data type in configurator

d3c7f3f

We need to make sure click.prompt saves the value as a boolean, i.e false instead of 'false', true instead of 'true'. Lower-case true/false is valid yaml syntax for boolean values.

JacksonChen666 requested changes Mar 15, 2023

View reviewed changes

JOJ0 added 2 commits March 16, 2023 06:57

Don't set --ssl-verify option default to True

490c178

We don't set defaults while initially setting up with the @click.option decorator. We handle defaults via the APIHelper.CONFIG dict and some magic in the configurators cli code.

Correct config command docstring

d53ccca

JOJ0 merged commit 9c3b718 into JOJ0:master Mar 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ssl_verify to manage selfsigned certificate #88

Add ssl_verify to manage selfsigned certificate #88

heurtematte commented Feb 2, 2023

JOJ0 left a comment •

edited

Loading

heurtematte commented Feb 3, 2023

JOJ0 left a comment •

edited

Loading

JOJ0 Feb 3, 2023 •

edited

Loading

Ascurius Feb 15, 2023

heurtematte Mar 1, 2023

JOJ0 Feb 3, 2023

heurtematte Mar 1, 2023

JOJ0 Feb 3, 2023 •

edited

Loading

heurtematte Mar 1, 2023

heurtematte Mar 1, 2023

JOJ0 Mar 10, 2023

JOJ0 Feb 3, 2023

heurtematte Mar 1, 2023

heurtematte Mar 1, 2023

JOJ0 Feb 3, 2023 •

edited

Loading

heurtematte Mar 1, 2023

JOJ0 commented Feb 8, 2023 •

edited

Loading

heurtematte commented Feb 8, 2023

JOJ0 commented Mar 10, 2023

JOJ0 commented Mar 15, 2023 •

edited

Loading

JacksonChen666 left a comment

JacksonChen666 Mar 15, 2023 •

edited

Loading

JOJ0 Mar 16, 2023

JacksonChen666 Mar 15, 2023

JOJ0 Mar 16, 2023 •

edited

Loading

Add ssl_verify to manage selfsigned certificate #88

Add ssl_verify to manage selfsigned certificate #88

Conversation

heurtematte commented Feb 2, 2023

JOJ0 left a comment • edited Loading

Choose a reason for hiding this comment

heurtematte commented Feb 3, 2023

JOJ0 left a comment • edited Loading

Choose a reason for hiding this comment

JOJ0 Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JOJ0 Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JOJ0 Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JOJ0 commented Feb 8, 2023 • edited Loading

heurtematte commented Feb 8, 2023

JOJ0 commented Mar 10, 2023

JOJ0 commented Mar 15, 2023 • edited Loading

JacksonChen666 left a comment

Choose a reason for hiding this comment

JacksonChen666 Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JOJ0 Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

JOJ0 left a comment •

edited

Loading

JOJ0 left a comment •

edited

Loading

JOJ0 Feb 3, 2023 •

edited

Loading

JOJ0 Feb 3, 2023 •

edited

Loading

JOJ0 Feb 3, 2023 •

edited

Loading

JOJ0 commented Feb 8, 2023 •

edited

Loading

JOJ0 commented Mar 15, 2023 •

edited

Loading

JacksonChen666 Mar 15, 2023 •

edited

Loading

JOJ0 Mar 16, 2023 •

edited

Loading