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.

Sign up for GitHub

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

Jump to bottom

Add test to validate SSO config for bookworm #164

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add test to validate SSO config for bookworm #164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
jsonschema
toml
toml
packaging
47 changes: 47 additions & 0 deletions tests/test_configurations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,13 @@

import re
import json
import sys

import toml
import subprocess

from packaging.version import Version

from lib.lib_package_linter import *
from lib.print import _print

Expand Down Expand Up @@ -456,3 +460,46 @@ def bind_public_ip(self):
"Please be sure that this behavior is intentional. "
"Maybe use '127.0.0.1' or '::1' instead."
)

@test()
def sso_config_auth_header(self):
app = self.app
manifest_path = app.path + "/manifest.toml"
try:
raw_manifest = open(manifest_path, encoding="utf-8").read()
manifest = toml.loads(raw_manifest)
yunohost_min_version = manifest["integration"]["yunohost"]
except Exception as e:
print(
c.FAIL
+ "✘ Can't load manifest. Need manifest to run this test ?\n ---> %s" % e
)
sys.exit(1)

if Version(yunohost_min_version.lstrip(">= ")) < Version("12"):
# for yunohost version<12 we don't test this as this might break a lot of app and
# on Yunohost the correct implementation is not clearly documented
return

for filename in (
os.listdir(app.path + "/conf") if os.path.exists(app.path + "/conf") else []
):
cmd = f"grep -q -IhEro 'fastcgi_param\\s+REMOTE_USER\\s+\\$remote_user;' {app.path}/conf/{filename}"
if os.system(cmd) == 0:
yield Warning(
"Since Yunohost >=12 you shouldn't use any more the nginx instruction "
"'fastcgi_param REMOTE_USER $remote_user;'. "
"This bring some security issues. Use the header 'YNH_USER' instead which is already defined by "
"ssowat and which is safe. "
"More details available on https://yunohost.org/fr/packaging_sso_ldap_integration"
)
return
Comment on lines +489 to +496
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really thinking we should autopatch the nginx conf directly in the nginx helper ? Though that doesn't change the fact that it should be applied in the app's code, but at least that allows for an ~immediate fix on bookworm even though the app is not marked as requiring yunohost >= 12

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this warning is mainly for a cleanup of something which is not needed any more. Note that for php app which use include fastcgi_params; this instruction include it implicitly. Keeping it is not really a problem. The main issue is about the usage, if the app use this header to authenticate the user. So autopatch don't really make sense, as it's not a requirement to remove it, it's more an improvement to don't leave something which is not needed and nobody know what it does.

And patching the app configuration is to me a bit complicated as it's depends of the app, note in php we have 2 different header, and so we don't exactly know which one to use depending of the app.

But yes in the apps which use the header to authenticate, in the transition time we will need something like this:

if yunohost11
  header=REMOTE_USER
else
  header=YNH_USER
fi

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in php we have 2 different header, and so we don't exactly know which one to use depending of the app.

Can you elaborate what you mean by "we have 2 different header" ?

Copy link
Contributor Author

@Josue-T Josue-T Oct 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes in PHP we can get the header this way getallheaders()["Ynh-User"] or $_SERVER["HTTP_YNH_USER"]

Note I not a specialist of php it's just what I discovered with my tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmokay but the isn't the point that the defacto standard is that apps will use the REMOTE_USER var which is provisioned with $remote_user in the nginx conf and we can just replace this with a different var, in this case $ynh_user or whatever the proper syntax is ...?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes indeed

(Also now that we're talking about it, we should probably tell people that they should always include the fastcgi_params snippet, because they should be enough for 90% of cases ? ... Or at least it should contain the REMOTE_USER snippet and every other common snippet ...)

Copy link
Contributor Author

@Josue-T Josue-T Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So,

After our discussion on matrix, propose this following thing for a migration time (should be removed in long term):

  • In ssowat we add a the legacy header so the current app will still work on bookworm, and since the app drop the support for bullseye, we require to use the new header by the linter that I already added.
  • We add an auto patch to replace fastcgi_param REMOTE_USER $header_ynh_user; (still need to check exactly how to retrieve the correct header which does this).
  • We add an auto patch to replace include fastcgi_params; by include fastcgi_params;\nfastcgi_param REMOTE_USER $header_ynh_user; so we override the fastcgi_param REMOTE_USER from include fastcgi_params.

Note that for the fastcgi_param instruction, we keep it only for the transition but it should be removed in long term and be replaced by the HEADER ssowat header.

For info from my test we have this by example in php, if I set:

  • In ssowat this header: MY_HEADER_FROM_SSOWAT: yolo
  • With fastcgi_param: FCGI_VAR: hello

We will end with this in the $_SERVER array: ["HTTP_MY_HEADER_FROM_SSOWAT"]=> string(4) "yolo" ["FCGI_VAR"]=> string(5) "hello.
So we probably don't really need the fastcgi_param it could probably be replaced by the corresponding key sent by ssowat.

What do you think about this ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cf YunoHost/yunohost@cd36757 for part of the yolopatch on-the-fly for bookworm

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we probably don't really need the fastcgi_param it could probably be replaced by the corresponding key sent by ssowat.

I don't know, to me my understanding is that application code may interface with the REMOTE_USER fastcgi param because it's a defacto standard, and wont easily allow to use a specific HTTP header. And it doesn't matter because we can map REMOTE_USER to the proper HTTP header in /etc/nginx/fastcgi_params and voila ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we can also do this, my original idea was to remove at all the confusing REMOTE_USER term. But yes we can also keep the idea to set the fastcgi_param REMOTE_USER to the correct value. In this case ideally we should change in /etc/nginx/fastcgi_params this ( fastcgi_param REMOTE_USER $remote_user;) to fastcgi_param REMOTE_USER $http_ynh_user; so we are sure that for the config which use the include fastcgi_params; we define the correct value for this.


cmd = f'grep -q -IhEro "REMOTE_USER" {app.path}/conf/{filename}'
if os.system(cmd) == 0:
yield Warning(
"Since Yunohost >=12 you shouldn't use any more the header 'REMOTE_USER' "
"to detect the authenticated user."
"Instead use the header 'YNH_USER' which is defined by ssowat and which is safe. "
"More details available on https://yunohost.org/fr/packaging_sso_ldap_integration"
)