Skip to content
This repository has been archived by the owner on Sep 7, 2023. It is now read-only.

[fix] ERROR:searx:server.secret_key is not changed #2386

Closed
wants to merge 2 commits into from
Closed

[fix] ERROR:searx:server.secret_key is not changed #2386

wants to merge 2 commits into from

Conversation

return42
Copy link
Contributor

@return42 return42 commented Dec 13, 2020

What does this PR do?

fix #2256

Related issues

Closes #2278

@dalf
Copy link
Contributor

dalf commented Dec 14, 2020

When do searx use a secret_key ?

The secret_key is not useful for:

  • utils/standalone.py
  • make test
  • make docs

In the master branch, the only use case of secret_key is the webapp.

Debug/secret_key Set Unset
True Ok Ok
False Ok KO

Why do we need a secret_key ?

According to https://flask.palletsprojects.com/en/1.1.x/config/#SECRET_KEY :

A secret key that will be used for securely signing the session cookie and can be used for any other security related needs by extensions or your application.

According to https://github.com/pallets/flask/search?l=Python&q=secret_key , the only use case of secret_key is the flask.session.

searx don't use flask.session on purpose.

According to https://github.com/searx/searx/search?q=secret_key , the only searx usage of secret_key is the image proxy (the image proxy written in Python, not Morty).

Sum up

  1. this line is useless:

    app.secret_key = settings['server']['secret_key']

  2. when the key is not set, the Python image proxy doesn't return an unique URL for the searx instance.
    said differently:
    when the key is not set, someone can use a searx an open proxy (at least for the images).

About this PR

I'm not comfortable adding another step to configure a searx instance ( make sure INSTANCE_ENV='production' ).

A different solution

move these lines

searx/searx/__init__.py

Lines 64 to 66 in 3660011

if not searx_debug and settings['server']['secret_key'] == 'ultrasecretkey':
logger.error('server.secret_key is not changed. Please use something else instead of ultrasecretkey.')
exit(1)

to the run function in webapp.py (just before the logger.debug) :

searx/searx/webapp.py

Lines 1090 to 1098 in 3660011

def run():
logger.debug('starting webserver on %s:%s', settings['server']['bind_address'], settings['server']['port'])
app.run(
debug=searx_debug,
use_debugger=searx_debug,
port=settings['server']['port'],
host=settings['server']['bind_address'],
threaded=True
)

The test_webapp still require this line:

webapp.app.config['TESTING'] = True # to get better error messages

but utils/standalone.py, make test, make docs will be okay.

A third solution

If the key is None, empty string or 'ultrasecretkey' then searx creates a secure random key.

The key will change after each restart, so the image proxy URL won't be reliable over time.

(I prefer the previous solution).

@return42 return42 changed the title [env] add INSTANCE_ENV to distinguish development and production env fix] ERROR:searx:server.secret_key is not changed Dec 16, 2020
@return42 return42 changed the title fix] ERROR:searx:server.secret_key is not changed [fix] ERROR:searx:server.secret_key is not changed Dec 16, 2020
@return42
Copy link
Contributor Author

@dalf: thanks a lot for your detailed analyses , I didn't know this details before.

I moved the condition to webapp.py .. and renamed this PR.

@dalf
Copy link
Contributor

dalf commented Dec 16, 2020

May I ask you to remove SEARX_DEBUG=1 in this line ?

run: SEARX_DEBUG=1 make V=1 travis-gh-pages

since it is not more mandatory with this PR.

[EDIT] And there are the tests too :

os.environ['SEARX_DEBUG'] = '1'

But here, this test will fail (perhaps another ?):
self.app = webapp.app.test_client()

A quick solution is not to change the tests for now.

@return42
Copy link
Contributor Author

May I ask you to remove SEARX_DEBUG=1 in this line ?

good point, I removed all the obsolete SEARX_DEBUG ..

A quick solution is not to change the tests for now.

for me the test works .. may you like to verify.

@dalf
Copy link
Contributor

dalf commented Dec 20, 2020

for me the test works .. may you like to verify.

This was puzzling me, actually the PR doesn't work:

So the run check for the SECRET_KEY when searx is started using python searx/webapp.py , most probably in debug mode.

The actual check must be done on wsgi application.

Flask doesn't provide callbacks when the app starts, only on the first request: https://flask.palletsprojects.com/en/1.1.x/api/#flask.Flask.before_first_request

So the only solution I see is a wsgi middleware, that doesn't nothing except check for the SECRET_KEY when initialize.

@return42
Copy link
Contributor Author

So the only solution I see is a wsgi middleware, that doesn't nothing except check for the SECRET_KEY when initialize.

Or we bury the idea to test SECRET_KEY,thoughts? Since #2256 we have a lot of trouble and the only benefit is to catch up sloppy admins.

said differently: when the key is not set, someone can use a searx an open proxy (at least for the images).

Admins have to read the documentation, how to configure searx

@dalf
Copy link
Contributor

dalf commented Dec 20, 2020

The other solution is to create a random key each time: at least it is secure even for a sloppy admin: no open proxy

@return42
Copy link
Contributor Author

to create a random key each time:

I will have a look ... WIP

@dalf
Copy link
Contributor

dalf commented Dec 20, 2020

Perhaps there is a better version, just for reference:

https://github.com/dalf/searx/blob/1cfe7f2a7543b2994a1afd0d81da1962d04423b0/utils/update_user_settings.py#L19-L24

Signed-off-by: Markus Heiser <markus.heiser@darmarit.de>
Signed-off-by: Markus Heiser <markus.heiser@darmarit.de>
@return42
Copy link
Contributor Author

@dalf: key is now generated in the settings_loader

if settings['server'].get('secret_key', 'ultrasecretkey') == 'ultrasecretkey':
settings['server']['ultrasecretkey'] = urandom(16).hex()

@kvch
Copy link
Member

kvch commented Dec 20, 2020

@dalf Generating a secret key every time searx starts up is not viable if image proxy is enabled. Once the instance is restarted, it gets a new secret key. Thus, previously working links image result links break on the instance because the secret key is changed. I prefer the initial solution I have submitted. (To me, it is somewhat weird that the documentation requires starting the web application.)

@return42
Copy link
Contributor Author

return42 commented Dec 20, 2020

To me, it is somewhat weird that the documentation requires starting the web application.

The application is not started, but searx will be imported. The documentation build itself needs a some values from the searx package like version number ..

from searx.version import VERSION_STRING

You can test it by :

$ . ./local/py3/bin/activate
(py3) (0) user@c340:/tmp/searx [master|+ 2] 
19:57 $ python
...
>>> import searx
ERROR:searx:server.secret_key is not changed. Please use something else instead of ultrasecretkey.

see https://github.com/searx/searx/pull/2256/files#diff-5b9df979b3e9792a21e5b0e3786eb590c34fd036a5fce4f79150a5fa0261cbf4

@return42
Copy link
Contributor Author

Generating a secret key every time searx starts up is not viable ..

key is only generated when it equals to ultrasecretkey see

@dalf
Copy link
Contributor

dalf commented Dec 20, 2020

@kvch currently import searx trigger the secret_key value check.

@kvch @return42

Here a patch so the documentation doesn't import searx.webapp.

[LAST EDIT] is it still weird written like this ?

It requires some adjustments, at least:

  • the jinja context is still webapp
  • most probably, if not searx_debug and settings.... should be at the top of the file
diff --git a/docs/conf.py b/docs/conf.py
index d6fde9be..5f0ab02b 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -27,9 +27,15 @@ numfig = True
 
 exclude_patterns = ['build-templates/*.rst']
 
-from searx import webapp
+import searx.search
+import searx.engines
+import searx.plugins
+searx.search.initialize()
 jinja_contexts = {
-    'webapp': dict(**webapp.__dict__),
+    'webapp': {
+        'engines': searx.engines.engines,
+        'plugins': searx.plugins.plugins
+    },
 }
 
 # usage::   lorem :patch:`f373169` ipsum
diff --git a/searx/__init__.py b/searx/__init__.py
index 9bbc7c8c..579fcf66 100644
--- a/searx/__init__.py
+++ b/searx/__init__.py
@@ -61,6 +61,3 @@ if 'SEARX_SECRET' in environ:
 if 'SEARX_BIND_ADDRESS' in environ:
     settings['server']['bind_address'] = environ['SEARX_BIND_ADDRESS']
 
-if not searx_debug and settings['server']['secret_key'] == 'ultrasecretkey':
-    logger.error('server.secret_key is not changed. Please use something else instead of ultrasecretkey.')
-    exit(1)
--- a/searx/webapp.py
+++ b/searx/webapp.py
@@ -1141,3 +1141,7 @@ app.wsgi_app = ReverseProxyPathFix(ProxyFix(application.wsgi_app))
 
 if __name__ == "__main__":
     run()
+
+if not searx_debug and settings['server']['secret_key'] == 'ultrasecretkey':
+    logger.error('server.secret_key is not changed. Please use something else instead of ultrasecretkey.')
+    exit(1)

So this fix make docs / make docs-live. Not the tests (if it makes sense):

os.environ['SEARX_DEBUG'] = '1'

My idea about the generated key is to avoid an open proxy, and yes it breaks the links, but only for misconfigured instance.

@return42
Copy link
Contributor Author

What is the problem with this PR? Every time I implement the solution other solutions are preferred.

If this PR does not match the needs and someone knows a better solution close it

@dalf
Copy link
Contributor

dalf commented Dec 20, 2020

Note: the patch just above is a different thing, basically it shows the documentation doesn't depend on the webapp only some other components.

@return42
Copy link
Contributor Author

The patch just above is totally OK for me, but it is not mine solution. Lets close this PR and send a PR with your solution.

BTW: the main problem is loading setup stuff and decide to exit the whole execution at import time.
The article "Don’t run code at import time" lists some of the drawbacks with execution at import time.

Your patch reduces the problem to the webapp, so I would prefer your solution.

@return42 return42 closed this Dec 20, 2020
dalf added a commit to dalf/searx that referenced this pull request Dec 21, 2020
Before this commit the secret_key is checked as soon as the searx module is imported.

So the documentation, utils/standalone_searx.py have to set SEARX_DEBUG=1

With this commit:
* searx check the secret_key when the webapp is loaded
* after searx loads that the engines.

For reference see searx#2386
@dalf dalf mentioned this pull request Dec 21, 2020
dalf added a commit to dalf/searx that referenced this pull request Dec 27, 2020
Without this commit the module searx checks the secret_key value.

With this commit, make docs, utils/standalone_searx.py,
utils/fetch_firefox_version.py works without SEARX_DEBUG=1

For reference see searx#2386
dalf added a commit to dalf/searx that referenced this pull request Dec 27, 2020
Without this commit the module searx checks the secret_key value.

With this commit, make docs, utils/standalone_searx.py,
utils/fetch_firefox_version.py works without SEARX_DEBUG=1

For reference see searx#2386
@return42 return42 deleted the instance-env branch January 28, 2021 18:45
mikeri pushed a commit to mikeri/searx that referenced this pull request Mar 7, 2021
Without this commit the module searx checks the secret_key value.

With this commit, make docs, utils/standalone_searx.py,
utils/fetch_firefox_version.py works without SEARX_DEBUG=1

For reference see searx#2386
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Makefile target 'make doc' is broken
3 participants