Skip to content

Update to modern cross-platform IDP image#797

Open
LucasPluta wants to merge 8 commits into
mainfrom
testing/update-saml
Open

Update to modern cross-platform IDP image#797
LucasPluta wants to merge 8 commits into
mainfrom
testing/update-saml

Conversation

@LucasPluta

@LucasPluta LucasPluta commented Jun 11, 2026

Copy link
Copy Markdown

Description

Replace the previous SAML IdP container with a cross-architecture, actively maintained SimpleSAMLphp-based setup for local development. The previous image was amd64-only and unmaintained for ~8 years, making it unsuitable for modern dev environments (especially arm64).

Why the rest of the changes?

The two images have very different "convenience" layers. The old image hid a lot of SAML configuration behind environment variables and shipped opinionated defaults; the new image is much closer to a vanilla SimpleSAMLphp install, so the configuration we used to get "for free" now has to be provided explicitly.

New/changed file Replaces (old image did this automatically)
saml20-sp-remote.php SP registration via SIMPLESAMLPHP_SP_* env vars. Without it the IdP returns MetadataNotFound for our backend.
cert/server.pem + .crt The IdP's assertion-signing keys (not TLS). The new image ships an empty cert dir and doesn't generate them, so it can't sign assertions.
apache-http.conf Plain-HTTP serving. The new image only exposes /simplesaml on its HTTPS vhost; this re-adds a port-80 vhost.
config-override.php Enables the IdP role + exampleauth, and sets session.cookie.secure = false so the cookie survives over http://.
saml20-idp-hosted.php Sets the entity ID, advertises the emailAddress NameID, and uses identifyingAttribute (the SSP 2.x spelling; old key was attribute).

Resolved issues

Resolves the inability to run the local SAML IdP on arm64 hosts. The previous kristophjunge/test-saml-idp image is amd64-only and unmaintained, which broke the local SAML login flow for developers on arm64 machines.

Documentation

No public documentation changes are required. The change only affects local development authentication infrastructure. The new USE_LOCAL_LOGIN flag is wired up in docker-compose.yml for local development only.

Web service API changes

No new external API endpoints are introduced. This change is limited to local development authentication infrastructure and backend configuration behavior.

Backend authentication behavior changes include:

  • Support for USE_LOCAL_LOGIN environment variable to override Launchpad integration for test environments.

Tests

  • Validated full stack startup via docker-compose (IdP + backend + dependencies).
  • Verified end-to-end SAML login flow using SimpleSAMLphp IdP.
  • Tested backend authentication with USE_LOCAL_LOGIN=true mode.
  • Verified that generate-certs.sh correctly generates certs on first startup and is idempotent (no-op if certs already exist).
  • Tested on:
    • linux-amd64 (Chrome and Firefox)
    • macOS-arm64 (Safari and Chrome)

Copilot AI review requested due to automatic review settings June 11, 2026 15:23
@LucasPluta LucasPluta changed the title Update to modern cross-platform IDP image and remove unused port Update to modern cross-platform IDP image Jun 11, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the local-development SAML Identity Provider setup to use an actively maintained, cross-architecture SimpleSAMLphp container, and adjusts local backend configuration so the SAML login flow works reliably (including on arm64 hosts).

Changes:

  • Replace the legacy kristophjunge/test-saml-idp container with cirrusid/simplesamlphp and add the required SimpleSAMLphp config/metadata overrides for local dev.
  • Update docker-compose.yml networking/env so the backend talks to Postgres and the IdP over the compose network (and exposes the backend via port mapping instead of network_mode: host).
  • Add a USE_FAKE_LAUNCHPAD_API toggle path to allow local/test SAML users to authenticate without requiring real Launchpad directory entries.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
REUSE.toml Adds REUSE annotations for the new SimpleSAMLphp cert/key files.
docker-compose.yml Switches to the new SimpleSAMLphp IdP image and updates backend DB/SAML env + networking.
backend/test_observer/controllers/auth/saml.py Adds a Launchpad API indirection to optionally use a fake implementation for local/dev SAML users.
backend/saml/simplesamlphp/saml20-sp-remote.php Adds SP metadata for SimpleSAMLphp to recognize the backend as the SP.
backend/saml/simplesamlphp/saml20-idp-hosted.php Updates hosted IdP metadata configuration (entity ID and NameID/attribute handling).
backend/saml/simplesamlphp/config-override.php Adds SimpleSAMLphp overrides for HTTP/local-dev operation.
backend/saml/simplesamlphp/cert/server.pem Adds the IdP private key for local SAML signing.
backend/saml/simplesamlphp/cert/server.crt Adds the IdP certificate for local SAML signing.
backend/saml/simplesamlphp/apache-http.conf Overrides Apache vhost so SimpleSAMLphp is reachable over HTTP on :80 for local dev.

Comment thread backend/saml/simplesamlphp/cert/server.pem Outdated
Comment thread backend/test_observer/controllers/auth/saml.py Outdated
Comment thread docker-compose.yml Outdated
Comment thread backend/saml/simplesamlphp/saml20-sp-remote.php Outdated
Copilot AI review requested due to automatic review settings June 11, 2026 15:39

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Comment thread backend/test_observer/controllers/auth/saml.py Outdated
Comment thread docker-compose.yml Outdated
Comment thread backend/saml/simplesamlphp/cert/server.pem Outdated

@wctaylor wctaylor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This will be a welcome improvement, especially decoupling local logins from Launchpad calls! Just a few comments

Comment thread backend/saml/simplesamlphp/apache-http.conf
Comment thread REUSE.toml
Comment thread backend/test_observer/controllers/auth/saml.py Outdated
Copilot AI review requested due to automatic review settings June 11, 2026 16:53

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.

Comment thread REUSE.toml Outdated
Comment thread backend/test_observer/controllers/auth/saml.py Outdated
Comment thread backend/test_observer/controllers/auth/saml.py Outdated
Comment thread backend/saml/simplesamlphp/saml20-idp-hosted.php
Comment thread docker-compose.yml
Copilot AI review requested due to automatic review settings June 11, 2026 17:47

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.

Comment thread backend/test_observer/controllers/auth/saml.py Outdated
Comment thread backend/saml/simplesamlphp/cert/.gitkeep Outdated
@LucasPluta LucasPluta force-pushed the testing/update-saml branch from 5aa12d9 to c23dac9 Compare June 11, 2026 17:53
Copilot AI review requested due to automatic review settings June 11, 2026 17:58

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.

Comment thread backend/test_observer/controllers/auth/saml.py Outdated
@LucasPluta LucasPluta force-pushed the testing/update-saml branch from 156a6de to b862dfa Compare June 11, 2026 18:54
Copilot AI review requested due to automatic review settings June 11, 2026 18:54

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.

Comment thread docker-compose.yml
@LucasPluta LucasPluta requested a review from wctaylor June 11, 2026 19:00
Copilot AI review requested due to automatic review settings June 11, 2026 19:15

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.

Comment thread backend/saml/simplesamlphp/generate-certs.sh Outdated
Comment thread backend/test_observer/controllers/auth/saml.py

@wctaylor wctaylor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can you update the PR description with additional details about what changed between the previous Docker image and the current one so that we have a clearer idea of why some of these extra simplesamlphp files and the extra IdP rewrite test helper stuff is needed now, when it wasn't needed before?

Comment thread backend/saml/simplesamlphp/generate-certs.sh Outdated
Comment thread backend/tests/controllers/auth/test_saml.py Outdated
Comment on lines +223 to +225
# The compose stack runs with USE_LOCAL_LOGIN=true, so the SAML flow
# skips the Launchpad lookup and no launchpad handle is recorded.
assert user.launchpad_handle is None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't really like this helper assuming the value of an environment variable for correctness. I feel like the environment variable should either be used in the check, or there should be an additional use_local_login: bool = False input to the helper, so that it can be called in a way that is less tightly coupled to the environment. I just don't really like the idea of a test suite depending on environment variables.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fair point — I was following the README’s suggested test approach using docker exec.

I’ve wrapped the assertion in a check for USE_LOCAL_LOGIN. I don’t currently have an environment where I can test the USE_LOCAL_LOGIN=false path, so I haven’t been able to validate that behavior directly.

Copilot AI review requested due to automatic review settings June 12, 2026 02:56

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.

Comment thread backend/tests/controllers/auth/test_saml.py Outdated
Comment thread backend/saml/simplesamlphp/generate-certs.sh Outdated
Comment thread backend/tests/controllers/auth/test_saml.py

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.

Comment on lines +224 to +227
# The compose stack runs with USE_LOCAL_LOGIN=true, so the SAML flow
# skips the Launchpad lookup and no launchpad handle is recorded.
if USE_LOCAL_LOGIN:
assert user.launchpad_handle is None
@LucasPluta LucasPluta requested a review from wctaylor June 12, 2026 03:42
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.

4 participants