Skip to content

Fix fetching of SAML metadata for large aggregate endpoints#23531

Merged
oliverguenther merged 2 commits into
devfrom
fix/saml-metadata-fetching-aggregate
Jun 9, 2026
Merged

Fix fetching of SAML metadata for large aggregate endpoints#23531
oliverguenther merged 2 commits into
devfrom
fix/saml-metadata-fetching-aggregate

Conversation

@oliverguenther

@oliverguenther oliverguenther commented Jun 3, 2026

Copy link
Copy Markdown
Member

We used ruby samls own metadata fetching instead of parsing the XML ourselves. This fails when trying to fetch large aggregated entities descriptors (such as the one linked in the ticket), for example when one idp manages a lot of EntitiesDescriptor.

Then it's useful to be able to locate the relevant descriptor for our service using the idp_entity_id. This field was added to the form to allow fetching only a subset of the data.

While we're at it, also use the SSRF protection helpers to fetch the request, rather than ruby-saml directly

https://community.openproject.org/work_packages/OP-19420

@oliverguenther oliverguenther force-pushed the fix/saml-metadata-fetching-aggregate branch 3 times, most recently from 9266765 to 59b1e58 Compare June 3, 2026 09:15
Comment thread modules/auth_saml/spec/services/saml/metadata_document_spec.rb Dismissed
@oliverguenther oliverguenther force-pushed the fix/saml-metadata-fetching-aggregate branch from 59b1e58 to 4beb426 Compare June 3, 2026 09:25
@oliverguenther oliverguenther marked this pull request as ready for review June 3, 2026 09:26

@NobodysNightmare NobodysNightmare left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Found two typos and have two questions regarding the implementation.

The general concept looks sound to me.

Comment thread modules/auth_saml/config/locales/en.yml Outdated
Comment thread modules/auth_saml/app/services/saml/metadata_document.rb Outdated
Comment thread modules/auth_saml/app/services/saml/metadata_document.rb Outdated
include ActionView::Helpers::NumberHelper

READ_TIMEOUT = 120
OPEN_TIMEOUT = 15

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Another surprise: Why do we need to deviate from default timeouts here?

I'd expect that opening a connection would take as long as always. And if I understand the concept of a read timeout correctly, it's for "silence" between received bytes, not for the total transfer duration of all the bytes, so I am surprised that we'd want to wait up to two minutes.

@oliverguenther oliverguenther Jun 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll remove them for now, but shouldn't we use the httpx defaults which specify read/timeouts in the SSRF helper explicitly? I know they are worded specifically for httpx, but they shouldn't be different?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

True, that would make sense. Ideally we'd be able to call those settings "HTTP connect timeout" and use them everywhere. (assuming that other clients support the same kinds of timeouts)

@oliverguenther oliverguenther force-pushed the fix/saml-metadata-fetching-aggregate branch from 7982b3d to fb8da3c Compare June 8, 2026 06:44
@oliverguenther oliverguenther force-pushed the fix/saml-metadata-fetching-aggregate branch 2 times, most recently from 5b03a19 to 786e63f Compare June 8, 2026 06:53
@oliverguenther oliverguenther force-pushed the fix/saml-metadata-fetching-aggregate branch from 786e63f to 01cb4a5 Compare June 9, 2026 06:49
@oliverguenther oliverguenther merged commit 92a0c03 into dev Jun 9, 2026
15 of 17 checks passed
@oliverguenther oliverguenther deleted the fix/saml-metadata-fetching-aggregate branch June 9, 2026 07:13
@github-actions github-actions Bot locked and limited conversation to collaborators Jun 9, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Development

Successfully merging this pull request may close these issues.

3 participants