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

Expose OIDC config parameters #4

Merged
merged 2 commits into from
Dec 11, 2023
Merged

Conversation

arvchristos
Copy link
Contributor

This MR exposes OIDC configuration parameters in the default config entrypoint script and the env variables. Essentially, misp-docker can now easily work with external SSO providers, a requirement for our use-case.

Settings are configured according to https://github.com/MISP/MISP/blob/2.4/app/Plugin/OidcAuth/README.md

By default, OIDC is disabled but can be enabled with the env variable OIDC_ENABLE.

@ostefano
Copy link
Collaborator

Thank you @arvchristos , I like this.

We are using OIDC as well, so might take this as an opportunity to merge some additonal logic.
I will iterate later today.

return
fi

sudo -u www-data php /var/www/MISP/tests/modify_config.php modify "{
Copy link
Collaborator

@ostefano ostefano Dec 11, 2023

Choose a reason for hiding this comment

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

Note that this is not idempotent (can't recall the exact behavior :S).

Best fix, is to reset the value before setting it.

sudo -u www-data php /var/www/MISP/tests/modify_config.php modify "{
            \"Security\": {
                \"auth\": \"\"
            }
}" > /dev/null 

Copy link
Contributor Author

@arvchristos arvchristos Dec 11, 2023

Choose a reason for hiding this comment

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

I thought of this but this step is already happening at

sudo -u www-data php /var/www/MISP/tests/modify_config.php modify "{
which is by default executed and cannot be disabled with a flag.

template.env Outdated
# OIDC_PROVIDER_URL=
# OIDC_CLIENT_ID=
# OIDC_CLIENT_SECRET=
# OIDC_ROLES_PROPERTY=
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would add some example re the data type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

template.env Outdated
# OIDC_CLIENT_ID=
# OIDC_CLIENT_SECRET=
# OIDC_ROLES_PROPERTY=
# OIDC_ROLES_MAPPING=
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -6,10 +6,20 @@ source /rest_client.sh
[ -z "$GPG_PASSPHRASE" ] && GPG_PASSPHRASE="passphrase"
[ -z "$REDIS_FQDN" ] && REDIS_FQDN="redis"
[ -z "$MISP_MODULES_FQDN" ] && MISP_MODULES_FQDN="http://misp-modules"
[ -z "$OIDC_PROVIDER_URL" ] && OIDC_PROVIDER_URL="test_provider"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure whether we want default values if not working

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let me rephrase it. I would rather remove default values, and add some checks when setting up oidc (e.g., if not all values are set, error out).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done and added a generic utility which can be used for other configuration functions in the future

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice. I see the output is redirected to stderr. Should we leave it like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually no, you are totally right, fixed

@ostefano ostefano merged commit 2d1a3b5 into MISP:master Dec 11, 2023
1 check passed
@ostefano
Copy link
Collaborator

LGTM

dgujarathi pushed a commit to dgujarathi/misp-docker that referenced this pull request Oct 6, 2024
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 this pull request may close these issues.

2 participants