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.

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

ood_auth_openidc does not support multi-valued options #211

Closed
ltalirz opened this issue Sep 29, 2023 · 8 comments · Fixed by #212
Closed

ood_auth_openidc does not support multi-valued options #211

ltalirz opened this issue Sep 29, 2023 · 8 comments · Fixed by #212

Comments

@ltalirz
Copy link
Contributor

ltalirz commented Sep 29, 2023

Some OIDC apache options are multi-valued, like

oidc_state_max_number_of_cookies: "3 true"

It seems to me it is currently not possible to set such options via the ood_auth_openidc variable

OIDCStateMaxNumberOfCookies: "10 true" 

results in OIDCStateMaxNumberOfCookies '10 true' in the apache config file, which will fail.

OIDCStateMaxNumberOfCookies: [10, true] 

results in a quoted list, also not correct.

The issue is that this approach

{{ key }} {{ value | quote }}

is not general enough here.

One could do the following (tested)

{{ key }} {% if value is iterable and (value is not string and value is not mappi
ng) %}{{ value | map('quote') | join(' ') }}{% else %}{{ value | quote }}{% endif 
%}

Now, the following will produce the correct result

OIDCStateMaxNumberOfCookies: [10, true] 

An alternative would be to create a custom filter module

@johrstrom
Copy link
Collaborator

Some OIDC apache options are multi-valued, like

oidc_state_max_number_of_cookies: "3 true"

So I think what you want to do here is use this config oidc_state_max_number_of_cookies instead of supplying it in the key value map ood_auth_openidc

# OIDC max number of state cookies and if to automatically clean old cookies
# Example:
# oidc_state_max_number_of_cookies: "10 true"
# Default: "10 true"
{% if oidc_state_max_number_of_cookies is defined %}oidc_state_max_number_of_cookies: {{ oidc_state_max_number_of_cookies }}
{% else %}#oidc_state_max_number_of_cookies: "10 true"
{% endif %}

I think ood_auth_openidc is for things that aren't directly supported in ood_portal.yml, which oidc_state_max_number_of_cookies is.

However, using oidc_state_max_number_of_cookies does template out the conf file like so (no quotes).

OIDCStateMaxNumberOfCookies 3 true

@johrstrom
Copy link
Collaborator

That said - I'm looking into your fix for ood_auth_openidc for other configs that may be like this.

@ltalirz
Copy link
Contributor Author

ltalirz commented Oct 2, 2023

Thanks!

So, our deployment recipes were already using ood_auth_openidc (for other purposes).
The default value of oidc_state_max_number_of_cookies would actually work fine for us but it seems it does not end up in the apache config file when ood_auth_openidc is used.
Not sure whether this is intended behavior - if not, then one could try to fix that as well.

@johrstrom
Copy link
Collaborator

Would #212 work for you?

@xpillons
Copy link
Contributor

@johrstrom @ltalirz I'm testing this fix and I can't get it to work.
I don't know which value to set to the ood_auth_openidc.OIDCStateMaxNumberOfCookies variable to avoid being quoted.
I'm on AlmaLinux 8.7.
In the apache configuration file /etc/httpd/conf.d/auth_openidc.conf if OIDCStateMaxNumberOfCookies contains quotes apache failed to start. If I remove quotes it starts.

Is there a way to avoid quotes for that value or does the fix is wrong ?

@johrstrom
Copy link
Collaborator

I think you need to configure it as iterable in the config. Can you try this? I'll reopen this ticket for docs and a test case.

ood_auth_openidc:
  OIDCStateMaxNumberOfCookies:
    - 10
    - true
  OtherStringValue: 'yesplz'

@johrstrom johrstrom reopened this Oct 10, 2023
@xpillons
Copy link
Contributor

xpillons commented Oct 10, 2023

Thanks @johrstrom I confirm it need an iterable. You can also specify it in this format too

ood_auth_openidc:
  OIDCStateMaxNumberOfCookies: [10, true]

@ltalirz
Copy link
Contributor Author

ltalirz commented Oct 10, 2023

Yes, as described at the end of the issue

@ltalirz ltalirz closed this as completed Nov 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants