From 6829f14a24e666e9f2f9ce9ab6fc67f0479ced19 Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Fri, 9 May 2025 13:17:20 +0200 Subject: [PATCH] Fix parsing of multiple certificates from metadata Once the XML contains multiple certificates, the metadata hash has a nil idp_cert. The idp_cert_multi key is always populated. However, we never tried parsing data from there, so it was lost. The way that the Saml::Provider right now represents multiple certificates is to concatenate them inside the idp_cert, so the metadata parsing has been adapted to use that. --- .../services/saml/update_metadata_service.rb | 17 +- .../saml/update_metadata_service_spec.rb | 229 ++++++++++++++++++ 2 files changed, 245 insertions(+), 1 deletion(-) create mode 100644 modules/auth_saml/spec/services/saml/update_metadata_service_spec.rb diff --git a/modules/auth_saml/app/services/saml/update_metadata_service.rb b/modules/auth_saml/app/services/saml/update_metadata_service.rb index 0f0c2b15f78..de618bf4c38 100644 --- a/modules/auth_saml/app/services/saml/update_metadata_service.rb +++ b/modules/auth_saml/app/services/saml/update_metadata_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + #-- copyright # OpenProject is an open source project management software. # Copyright (C) the OpenProject GmbH @@ -36,7 +38,7 @@ module Saml end def call - apply_metadata(fetch_metadata) + apply_metadata(merge_certificates(fetch_metadata)) rescue StandardError => e OpenProject.logger.error(e) ServiceResult.failure(result: provider, @@ -75,5 +77,18 @@ module Saml def parser_instance OneLogin::RubySaml::IdpMetadataParser.new end + + # Support reading multiple certificates from idp_cert_multi, instead of just a single one passed via idp_cert. + # Saml::Provider does not yet support storing "use" (signing vs. encryption). + def merge_certificates(metadata) + return metadata if metadata[:idp_cert].present? + + certs = Array(metadata.dig(:idp_cert_multi, :signing)) + Array(metadata.dig(:idp_cert_multi, :encryption)) + if certs.present? + metadata[:idp_cert] = certs.map { |cert| OneLogin::RubySaml::Utils.format_cert(cert) }.uniq.join("\n") + end + + metadata + end end end diff --git a/modules/auth_saml/spec/services/saml/update_metadata_service_spec.rb b/modules/auth_saml/spec/services/saml/update_metadata_service_spec.rb new file mode 100644 index 00000000000..16da4babc78 --- /dev/null +++ b/modules/auth_saml/spec/services/saml/update_metadata_service_spec.rb @@ -0,0 +1,229 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" + +RSpec.describe Saml::UpdateMetadataService do + subject(:parse_metadata) { described_class.new(user:, provider:).call } + + let(:user) { create(:user) } + let(:provider) { Saml::Provider.new(metadata_xml:) } + + describe "#idp_cert" do + let(:certificate) do + "MIIC/TCCAeWgAwIBAgIICu+WfBLOqBAwDQYJKoZIhvcNAQELBQAwLTErMCkGA1UE\n" \ + "AxMiYWNjb3VudHMuYWNjZXNzY29udHJvbC53aW5kb3dzLm5ldDAeFw0yNTAzMTYy\n" \ + "MDE3MjNaFw0zMDAzMTYyMDE3MjNaMC0xKzApBgNVBAMTImFjY291bnRzLmFjY2Vz\n" \ + "c2NvbnRyb2wud2luZG93cy5uZXQwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEK\n" \ + "AoIBAQCHPp9RIJIC6LJDPovWdCPjNryQi168nPGFVt5wyKBMiX2ldlIdlreDMyC1\n" \ + "qmdvWr3oIbmq9Hvx0fpm9MovwzM3hV8aBmG8sS/kskp6jS0aAEhLrnDEiIliP0TE\n" \ + "QOoWTD1F2FbWc3wg3147vo9sL590Q+0N6QDYFohNjYMBEhIo3gp7REsERY2sp4SO\n" \ + "M7OvKBLZ7dD01XMTnVkYZAAYdq7tq0fLwz+oDWed3Z0xSBQycRggzMMFNIrPXsbq\n" \ + "K0k51qca8bfBe92md0p9+cOmlo+TJZufJt0wjgg/urpawKqe3ca2D5toboYOplBA\n" \ + "QGn0L2AsAW/g5FNGWkPfDSAIyHvHAgMBAAGjITAfMB0GA1UdDgQWBBSsQvFDUwCT\n" \ + "JXK+ltZFLaHUGzIS6jANBgkqhkiG9w0BAQsFAAOCAQEAUsfNQA+O7eXGI4IL/Fma\n" \ + "fEmmFjoXC+Ym9UIzG/vXcXzQEK9S9nV35Q0Fn9PsL1w8Sud3itm/V6t9UtB9yaRv\n" \ + "WREPOdEYsHEkZahoSFi2fgOLP+AsTtQq0ePeBbqAQvnfrTvFuv+j1we3uxxov77p\n" \ + "t7U+pB+6Sq8+yww85qeTCWmV4av2WWXB+6pW9oUd/D9htlxKL5WzNsaVojP56eg3\n" \ + "mwhBmOpqxkYnL7RAPGOYRjaeHic9ONrctC8HImjf21UC5wK8G/lcVQATcvPZm/AY\n" \ + "Jg10fNsxZ/8ApFLblf9Q8l0QcKZfjs/si3VKcWvilDrfO9Dg83Ou6tvsLnPU5lV3\n" \ + "aA==" + end + let(:formatted_certificate) { "-----BEGIN CERTIFICATE-----\n#{certificate}\n-----END CERTIFICATE-----" } + + context "when the SAML contains a single signing certificate" do + let(:metadata_xml) do + <<~SAML + + + + + + + #{saml_certificate} + + + + + + + SAML + end + let(:saml_certificate) { certificate } + + it "populates a single IDP certificate" do + parse_metadata + + expect(provider.idp_cert).to eq(formatted_certificate) + end + + context "when the certificate inside the SAML is not pretty-printed" do + let(:saml_certificate) { certificate.tr("\n", "") } + + it "populates the IDP certificate with a pretty-printed representation" do + parse_metadata + + expect(provider.idp_cert).to eq(formatted_certificate) + end + end + end + + context "when the SAML contains a single encryption certificate" do + let(:metadata_xml) do + <<~SAML + + + + + + + #{saml_certificate} + + + + + + + SAML + end + let(:saml_certificate) { certificate } + + it "populates a single IDP certificate" do + parse_metadata + + expect(provider.idp_cert).to eq(formatted_certificate) + end + end + + context "when the SAML contains a certificate for both encryption and signing" do + let(:metadata_xml) do + <<~SAML + + + + + + + #{saml_certificate} + + + + + + + + + #{saml_certificate} + + + + + + + SAML + end + let(:saml_certificate) { certificate } + + it "populates a single IDP certificate" do + parse_metadata + + expect(provider.idp_cert).to eq(formatted_certificate) + end + end + + context "when the SAML contains multiple certificates" do + let(:metadata_xml) do + descriptors = certificates.map do |certificate| + <<~SAML + + #{certificate} + + SAML + end + + <<~SAML + + + #{descriptors.join("\n")} + + + SAML + end + + let(:certificates) do + [ + "MIIC/TCCAeWgAwIBAgIIY81p6sALmU8wDQYJKoZIhvcNAQELBQAwLTErMCkGA1UEAxMiYWNjb3VudHMuYWNjZXNzY29udHJvbC53aW5kb3dzLm5ldDA" \ + "eFw0yNTAyMjQwOTMyNTdaFw0zMDAyMjQwOTMyNTdaMC0xKzApBgNVBAMTImFjY291bnRzLmFjY2Vzc2NvbnRyb2wud2luZG93cy5uZXQwggEiMA0GCS" \ + "qGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDHExFy6zLBEh390lH951z6p78ze+Fc40LPtAOGNV+iodZu32VAJcJUiwij1UkjWcZPUXo2RcQgG3jrbHanX" \ + "DZyB/gAXzp0aFtOlH6TzVYtHmA2OSGk+oHU8KK2JyFUgpscC87JgQBomcTIKImoiVT9mDkvkXNGq/83/f5+Fi9YrSlzebU6HJ3aQ1GDU1tynmiC3uYG" \ + "N5zswSt+L43Sjni9d+wrqEzabuHSxnBV3gLGA+qekG2baG0z3FoqmfCRigQve9rds8jVUYan1AtnAxXpEAc7L85GnPjFqsb7PuVlQIs7RfVKjTmufvD" \ + "l50GZ9uVRCU8vcGWNtRUHdpt31I1fAgMBAAGjITAfMB0GA1UdDgQWBBQc8G/33OWrtOT/XnksiamjCcQKcDANBgkqhkiG9w0BAQsFAAOCAQEAt5GZmt" \ + "TxoJ4fQMS787qU8PHcw2ihIzx1gzP0JNYTG+7qdP/oZsYISZ4EyTnZ8gkJfgZIYHoGe/5BcZ4N56LtUl3HIw/b4WYPjbFNHaAiNmQDqPp1/HtIhv7FZ" \ + "NKXu6az0fBfSc5RetWGnZ7Ex3mmhjJisAt+Ml+fRYLfjvQghtiNTsdOCQRWQpaCVJC7lV9x5gfSWm6qIAquGJE3xqVWnUlCjFJk67UbqmqNltJ5dDNE" \ + "k6N2BSM2WlA9lf9FIhdBWBCn2zplQHcA0EU+0p3iwLH/AjwjJnW41NcJO51bN5Jye6dhSaS9yQm9iKTK8H6DOpkzj3oR4Sf9Ki31+kxiTQ==", + "MIIC/TCCAeWgAwIBAgIICu+WfBLOqBAwDQYJKoZIhvcNAQELBQAwLTErMCkGA1UEAxMiYWNjb3VudHMuYWNjZXNzY29udHJvbC53aW5kb3dzLm5ldDA" \ + "eFw0yNTAzMTYyMDE3MjNaFw0zMDAzMTYyMDE3MjNaMC0xKzApBgNVBAMTImFjY291bnRzLmFjY2Vzc2NvbnRyb2wud2luZG93cy5uZXQwggEiMA0GCS" \ + "qGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCHPp9RIJIC6LJDPovWdCPjNryQi168nPGFVt5wyKBMiX2ldlIdlreDMyC1qmdvWr3oIbmq9Hvx0fpm9Movw" \ + "zM3hV8aBmG8sS/kskp6jS0aAEhLrnDEiIliP0TEQOoWTD1F2FbWc3wg3147vo9sL590Q+0N6QDYFohNjYMBEhIo3gp7REsERY2sp4SOM7OvKBLZ7dD0" \ + "1XMTnVkYZAAYdq7tq0fLwz+oDWed3Z0xSBQycRggzMMFNIrPXsbqK0k51qca8bfBe92md0p9+cOmlo+TJZufJt0wjgg/urpawKqe3ca2D5toboYOplB" \ + "AQGn0L2AsAW/g5FNGWkPfDSAIyHvHAgMBAAGjITAfMB0GA1UdDgQWBBSsQvFDUwCTJXK+ltZFLaHUGzIS6jANBgkqhkiG9w0BAQsFAAOCAQEAUsfNQA" \ + "+O7eXGI4IL/FmafEmmFjoXC+Ym9UIzG/vXcXzQEK9S9nV35Q0Fn9PsL1w8Sud3itm/V6t9UtB9yaRvWREPOdEYsHEkZahoSFi2fgOLP+AsTtQq0ePeB" \ + "bqAQvnfrTvFuv+j1we3uxxov77pt7U+pB+6Sq8+yww85qeTCWmV4av2WWXB+6pW9oUd/D9htlxKL5WzNsaVojP56eg3mwhBmOpqxkYnL7RAPGOYRjae" \ + "Hic9ONrctC8HImjf21UC5wK8G/lcVQATcvPZm/AYJg10fNsxZ/8ApFLblf9Q8l0QcKZfjs/si3VKcWvilDrfO9Dg83Ou6tvsLnPU5lV3aA==", + "MIIC/jCCAeagAwIBAgIJAM52mWWK+FEeMA0GCSqGSIb3DQEBCwUAMC0xKzApBgNVBAMTImFjY291bnRzLmFjY2Vzc2NvbnRyb2wud2luZG93cy5uZXQ" \ + "wHhcNMjUwMzIwMDAwNTAyWhcNMzAwMzIwMDAwNTAyWjAtMSswKQYDVQQDEyJhY2NvdW50cy5hY2Nlc3Njb250cm9sLndpbmRvd3MubmV0MIIBIjANBg" \ + "kqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAruYyUq1ElSb8QCCt0XWWRSFpUq0JkyfEvvlCa4fPDi0GZbSGgJg3qYa0co2RsBIYHczXkc71kHVpktySA" \ + "gYK1KMK264e+s7Vymeq+ypHEDpRsaWric/kKEIvKZzRsyUBUWf0CUhtuUvAbDTuaFnQ4g5lfoa7u3vtsv1za5Gmn6DUPirrL/+xqijP9IsHGUKaTmB4" \ + "M/qnAu6vUHCpXZnN0YTJDoK7XrVJFaKj8RrTdJB89GFJeTFHA2OX472ToyLdCDn5UatYwmht62nXGlH7/G1kW1YMpeSSwzpnMEzUUk7A8UXrvFTHXEp" \ + "fXhsv0LA59dm9Hi1mIXaOe1w+icA/rQIDAQABoyEwHzAdBgNVHQ4EFgQUcZ2MLLOas+d9WbkFSnPdxag09YIwDQYJKoZIhvcNAQELBQADggEBABPXBm" \ + "wv703IlW8Zc9Kj7W215+vyM5lrJjUubnl+s8vQVXvyN7bh5xP2hzEKWb+u5g/brSIKX/A7qP3m/z6C8R9GvP5WRtF2w1CAxYZ9TWTzTS1La78edME54" \ + "6QejjveC1gX9qcLbEwuLAbYpau2r3vlIqgyXo+8WLXA0neGIRa2JWTNy8FJo0wnUttGJz9LQE4L37nR3HWIxflmOVgbaeyeaj2VbzUE7MIHIkK1bqye" \ + "2OiKU82w1QWLV/YCny0xdLipE1g2uNL8QVob8fTU2zowd2j54c1YTBDy/hTsxpXfCFutKwtELqWzYxKTqYfrRCc1h0V4DGLKzIjtggTC+CY=" + ] + end + + it "populates multiple IDP certificates" do + parse_metadata + + certificates.each do |cert| + expect(provider.idp_cert.tr("\n", "")).to include(cert) + end + end + end + end +end