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

feat(docker-jans-saml): turn off profile update on first login #9561

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

iromli
Copy link
Contributor

@iromli iromli commented Sep 20, 2024

Prepare


Description

Target issue

closes #9560

Implementation Details


Test and Document the changes

  • Static code analysis has been run locally and issues have been fixed
  • Relevant unit and integration tests have been added/updated
  • Relevant documentation has been updated if any (i.e. user guides, installation and configuration guides, technical design docs etc)

Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with docs: to indicate documentation changes or if the below checklist is not selected.

  • I confirm that there is no impact on the docs due to the code changes in this PR.

@iromli iromli requested a review from moabu as a code owner September 20, 2024 20:58
Copy link

dryrunsecurity bot commented Sep 20, 2024

DryRun Security Summary

The provided changes focus on improving the security and maintainability of the Jans SAML application by updating the Dockerfile, configuring Keycloak, and following best practices, but there are a few areas that require further review and attention, such as credential management and privilege escalation.

Expand for full summary

Summary:

The provided code changes are related to the Dockerfile for the docker-jans-saml component and the configuration of Keycloak for the Jans SAML application.

The Dockerfile changes include updating the source code version, using a well-maintained base image, installing necessary packages, and following best practices such as running the application as a non-root user. These changes generally improve the security posture of the application.

The configure_kc.py script is responsible for setting up the Keycloak realm, client, user, authentication flow, and user storage for the Jans SAML application. While the script appears to be following security best practices, such as disabling certain Keycloak features, there are a few areas that require further attention:

  1. Credential management: The script uses environment variables and a base64-encoded credentials file to store sensitive information, which is not recommended. It would be better to use a secure secrets management solution.
  2. Privilege escalation: The script attempts to grant the "XA_RECOVER_ADMIN" privilege to the Keycloak database user, which should be reviewed and granted with caution.

Overall, the changes made in this pull request focus on improving the security and maintainability of the Jans SAML application, but there are a few areas that should be further reviewed and addressed to ensure the application's security posture is robust.

Files Changed:

  1. docker-jans-saml/Dockerfile:

    • Updated the JANS_SOURCE_VERSION environment variable to a newer version.
    • Uses a well-maintained base image (quay.io/keycloak/keycloak:25.0.1) and installs necessary packages.
    • Follows best practices, such as running the application as a non-root user and exposing the appropriate port for the SAML component.
  2. docker-jans-saml/scripts/configure_kc.py:

    • Ensures the required Keycloak realm, client, and user are set up for the Jans SAML application.
    • Configures the authentication flow, user storage, and disables certain Keycloak features.
    • Attempts to grant the "XA_RECOVER_ADMIN" privilege to the Keycloak database user, which should be reviewed.
    • Uses environment variables and a base64-encoded credentials file to store sensitive information, which is not recommended.

Code Analysis

We ran 9 analyzers against 2 files and 2 analyzers had findings. 7 analyzers had no findings.

Analyzer Findings
Sensitive Files Analyzer 1 finding
Authn/Authz Analyzer 4 findings

Riskiness

🟢 Risk threshold not exceeded.

View PR in the DryRun Dashboard.

@mo-auto mo-auto added the kind-feature Issue or PR is a new feature request label Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind-feature Issue or PR is a new feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(docker-jans-saml): turn off profile update on first login
2 participants