Merge pull request #4 from PlaceOS/PPT-2536-saml-relaystate

PPT-2536: fix SAML/ADFS login + OAuth callback lost-id (UAT-breaking)

Author: camreeves

Dockerfile.e2e

@@ -0,0 +1,97 @@
+ARG CRYSTAL_VERSION=latest
+
+FROM placeos/crystal:$CRYSTAL_VERSION AS build
+WORKDIR /app
+
+# Set the commit via a build arg
+ARG PLACE_COMMIT="DEV"
+# Set the platform version via a build arg
+ARG PLACE_VERSION="DEV"
+
+# Create a non-privileged user, defaults are appuser:10001
+ARG IMAGE_UID="10001"
+ENV UID=$IMAGE_UID
+ENV USER=appuser
+
+# See https://stackoverflow.com/a/55757473/12429735
+RUN adduser \
+    --disabled-password \
+    --gecos "" \
+    --home "/nonexistent" \
+    --shell "/sbin/nologin" \
+    --no-create-home \
+    --uid "${UID}" \
+    "${USER}"
+
+# Install package updates since image release
+RUN apk update && apk --no-cache --quiet upgrade
+
+RUN update-ca-certificates
+
+# Install shards for caching
+COPY shard.yml shard.yml
+COPY shard.override.yml shard.override.yml
+COPY shard.lock shard.lock
+
+RUN shards install --production --ignore-crystal-version --skip-postinstall --skip-executables
+
+# Add src
+COPY ./src /app/src
+
+# Build application
+RUN UNAME_AT_COMPILE_TIME=true \
+    PLACE_COMMIT=$PLACE_COMMIT \
+    PLACE_VERSION=$PLACE_VERSION \
+    shards build --production --error-trace --static
+
+SHELL ["/bin/ash", "-eo", "pipefail", "-c"]
+
+# Extract binary dependencies
+RUN mkdir -p /app/deps && for binary in /app/bin/*; do \
+        { ldd "$binary" 2>/dev/null || true; } | \
+        tr -s '[:blank:]' '\n' | \
+        grep '^/' | \
+        xargs -I % sh -c 'mkdir -p $(dirname deps%); cp % deps%;' || true; \
+    done
+
+# Generate OpenAPI docs while we still have source code access
+RUN ./bin/placeos-auth --docs > openapi.yml
+
+RUN git config --system http.sslCAInfo /etc/ssl/certs/ca-certificates.crt
+
+# Build a minimal docker image
+FROM scratch
+WORKDIR /
+ENV PATH=$PATH:/
+
+# Copy the user information over
+COPY --from=build etc/passwd /etc/passwd
+COPY --from=build /etc/group /etc/group
+
+# These are required for communicating with external services
+COPY --from=build /etc/hosts /etc/hosts
+
+# These provide certificate chain validation where communicating with external services over TLS
+COPY --from=build /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/
+ENV SSL_CERT_FILE=/etc/ssl/certs/ca-certificates.crt
+
+# This is required for Timezone support
+COPY --from=build /usr/share/zoneinfo/ /usr/share/zoneinfo/
+
+# This is your application
+COPY --from=build /app/deps /
+COPY --from=build /app/bin /
+
+# Copy the docs into the container, you can serve this file in your app
+COPY --from=build /app/openapi.yml /openapi.yml
+
+# Use an unprivileged user.
+USER appuser:appuser
+
+# Spider-gazelle has a built in helper for health checks
+HEALTHCHECK CMD ["/placeos-auth", "-c", "http://127.0.0.1:8080/auth/healthz"]
+
+# Run the app binding on port 8080
+EXPOSE 8080
+ENTRYPOINT ["/placeos-auth"]
+CMD ["/placeos-auth", "-b", "0.0.0.0", "-p", "8080"]

docker-compose.override.yml

@@ -0,0 +1,11 @@
+# LOCAL-ONLY (untracked): remap published test-DB ports off the host's
+# native postgres:5432 / redis:6379 so the isolated spec stack can start.
+# The `test` service still reaches these over the compose network by
+# hostname, so connectivity is unchanged.
+services:
+  postgres:
+    ports:
+      - "15499:5432"
+  redis:
+    ports:
+      - "16399:6379"

spec/controllers/oauth_provider_flows_spec.cr

@@ -140,6 +140,35 @@ module PlaceOS::Auth
         cleanup_login.call(uid) if uid
         strat.try &.destroy
       end
+
+      it "recovers the strat id from the session when the IdP drops ?id= on the callback" do
+        strat = google_strat.call
+        uid = "google-noid-#{Random.rand(999999)}"
+        email = "grace-#{Random.rand(999999)}@localhost"
+
+        WebMock.stub(:post, "https://accounts.google.com/token").to_return(
+          status: 200, headers: json_headers,
+          body: {access_token: "g-access", token_type: "Bearer", expires_in: 3600}.to_json,
+        )
+        WebMock.stub(:get, "https://openidconnect.googleapis.com/v1/userinfo").to_return(
+          status: 200, headers: json_headers,
+          body: {sub: uid, email: email, name: "Grace Hopper", given_name: "Grace", family_name: "Hopper"}.to_json,
+        )
+
+        k = kickoff.call(strat.id.as(String))
+        # The callback URL deliberately OMITS `?id=` (some IdPs don't round-trip
+        # it). The id must be recovered from the session state stashed at kickoff
+        # — otherwise this 401s "oauth state mismatch".
+        result = client.get(
+          "/auth/oauth2/callback?code=g-code&state=#{k[:state]}",
+          headers: HTTP::Headers{"Host" => "localhost", "Cookie" => k[:cookie]},
+        )
+        result.status_code.should eq 303
+        ::PlaceOS::Model::UserAuthLookup.find?("auth-#{authority_id.call}-oauth2-#{uid}").should_not be_nil
+      ensure
+        cleanup_login.call(uid) if uid
+        strat.try &.destroy
+      end
     end
 
     # ---- Azure AD / Entra ID -----------------------------------------

spec/controllers/saml_callbacks_spec.cr

@@ -109,5 +109,39 @@ module PlaceOS::Auth
         result.status_code.should eq 404
       end
     end
+
+    # The callback's session-state CSRF check is skipped for SAML — the
+    # cross-site HTTP-POST binding drops the SameSite=Lax cookie so the stashed
+    # state is unavailable, and the signed assertion authenticates instead
+    # (PPT-2536). It stays fully enforced for the OAuth2 path.
+    describe "callback state check" do
+      it "does NOT reject a SAML callback as an 'oauth state mismatch'" do
+        strat = create_saml_strat.call
+        # No prior kickoff => no stored session state. An OAuth2 callback would
+        # 401 "oauth state mismatch" here; a SAML callback must get PAST that
+        # check (and instead fail later at signed-assertion validation).
+        result = client.post(
+          "/auth/saml/callback?id=#{URI.encode_www_form(strat.id.as(String))}",
+          headers: HTTP::Headers{
+            "Host"         => "localhost",
+            "Content-Type" => "application/x-www-form-urlencoded",
+          },
+          body: "SAMLResponse=#{URI.encode_www_form("not-a-real-assertion")}&RelayState=xyz",
+        )
+        result.body.should_not contain "oauth state mismatch"
+      ensure
+        strat.try &.destroy
+      end
+
+      it "still enforces the session-state check for an OAuth2 callback" do
+        # No stored state => the OAuth2 path rejects with a state mismatch.
+        result = client.get(
+          "/auth/oauth2/callback?id=whatever&state=whatever",
+          headers: HTTP::Headers{"Host" => "localhost"},
+        )
+        result.status_code.should eq 401
+        result.body.should contain "oauth state mismatch"
+      end
+    end
   end
 end

src/placeos-auth/controllers/provider_callbacks.cr

@@ -98,9 +98,33 @@ module PlaceOS::Auth
       raise Error::NotFound.new("authority not found") if authority.nil?
 
       expect_state, expect_provider, expect_id = consume_stored_state
-      if expect_state.nil? || state != expect_state || expect_provider != provider || expect_id != (id || "")
-        Log.warn { {action: "provider_callbacks.callback", message: "state mismatch", provider: provider} }
-        raise Error::Unauthorized.new("oauth state mismatch")
+
+      # Recover the strategy id from the stored session state when the IdP
+      # dropped the `?id=` query param on the callback redirect (some OAuth
+      # providers don't round-trip query params on `redirect_uri`). The exact
+      # id was stashed in `#initiate`, so use it rather than 401 — more precise
+      # than the legacy Ruby guess (first strat / authority callback URI). A
+      # present-but-mismatched id is left alone, so the state check below still
+      # rejects it as CSRF.
+      if (id.nil? || id.empty?) && (recovered_id = expect_id.presence)
+        id = recovered_id
+      end
+
+      # SAML (adfs) uses the HTTP-POST binding: the IdP auto-submits the signed
+      # assertion cross-site to the ACS URL, so the browser withholds our
+      # SameSite=Lax auth-flow cookie and the CSRF `state` stashed in
+      # `#initiate` is unavailable here — and the IdP echoes the CSRF value
+      # back as `RelayState`, not `state`, anyway. The legacy Ruby service
+      # (omniauth-saml) never did session-state CSRF for SAML; the callback was
+      # authenticated purely by the signed assertion (idp_cert /
+      # idp_cert_fingerprint, want_assertions_signed). Mirror that: skip the
+      # session-state check for SAML and let `engine.user` validate the
+      # signature. The OAuth2 path (Lax cookie sent) keeps full validation.
+      unless saml_provider?(provider)
+        if expect_state.nil? || state != expect_state || expect_provider != provider || expect_id != (id || "")
+          Log.warn { {action: "provider_callbacks.callback", message: "state mismatch", provider: provider} }
+          raise Error::Unauthorized.new("oauth state mismatch")
+        end
       end
 
       redirect_uri = callback_uri(provider, id)
@@ -187,6 +211,15 @@ module PlaceOS::Auth
       authorize_uri.gsub("%3Fid%3D", "%2F").gsub("?id=", "/")
     end
 
+    # SAML callbacks arrive on the `adfs`/`saml` provider names registered by
+    # `ExternalProviders`. They cannot participate in session-state CSRF (the
+    # cross-site HTTP-POST binding drops the SameSite=Lax cookie) and are
+    # authenticated by the signed assertion instead.
+    private def saml_provider?(provider : String) : Bool
+      provider == ExternalProviders::SAML_PROVIDER ||
+        provider == ExternalProviders::SAML_PROVIDER_ALIAS
+    end
+
     private def consume_stored_state : Tuple(String?, String?, String?)
       raw = session.delete(SESSION_OAUTH_STATE)
       return {nil, nil, nil} unless raw.is_a?(String)