-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add test to validate SSO config for bookworm #164
base: master
Are you sure you want to change the base?
Conversation
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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" ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ...?
There was a problem hiding this comment.
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 ...)
There was a problem hiding this comment.
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;
byinclude fastcgi_params;\nfastcgi_param REMOTE_USER $header_ynh_user;
so we override thefastcgi_param REMOTE_USER
frominclude 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 ?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ...
There was a problem hiding this comment.
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.
Problem
full description here: YunoHost/yunohost#1981
PR checklist