Skip to content

Graceful failure (log) with SSO Connector loading#35528

Merged
jentfoo merged 4 commits intomasterfrom
jent/saml_graceful_fail
Dec 8, 2023
Merged

Graceful failure (log) with SSO Connector loading#35528
jentfoo merged 4 commits intomasterfrom
jent/saml_graceful_fail

Conversation

@jentfoo
Copy link
Copy Markdown
Contributor

@jentfoo jentfoo commented Dec 7, 2023

This PR fixes https://github.com/gravitational/security-findings/issues/47

This was originally attempted to be fixed in #34896, however this PR reverts those changes. The validation logic prior to #34896 was correct, and we should not attempt to fix this issue by making validation more lenient.

Instead a second commit provides an alternative fix where the connectors are being looped over and unmarshaled. Errors at that point will be logged rather than returned.

changelog: Fixed bug where configuration errors with an individual SSO connector impacted other connectors.

@jentfoo jentfoo self-assigned this Dec 7, 2023
@github-actions github-actions Bot requested a review from klizhentas December 7, 2023 22:32
@jentfoo jentfoo requested review from stevenGravy and removed request for klizhentas December 7, 2023 22:33
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Dec 7, 2023

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

Copy link
Copy Markdown
Contributor

@lxea lxea left a comment

Choose a reason for hiding this comment

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

This looks good to me, I think it would also be good to remove the Validate from Marshall and Unmarshall
Here https://github.com/gravitational/teleport/blob/master/lib/services/saml.go#L306-L308
and here: https://github.com/gravitational/teleport/blob/master/lib/services/saml.go#L328-L330
All the callers where it makes sense to validate do so manually already, and it doesnt seem right to have a http request in a marshaller

Copy link
Copy Markdown
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

A few minor comments.

Comment thread lib/services/identity.go Outdated
Comment thread lib/services/local/users.go Outdated
Comment thread lib/services/local/users.go Outdated
Comment thread lib/services/local/users.go Outdated
Comment thread lib/services/saml.go
Comment thread lib/services/saml.go
@jentfoo jentfoo force-pushed the jent/saml_graceful_fail branch from ce08e5e to b9c4e27 Compare December 8, 2023 17:59
Comment thread lib/services/local/users.go Outdated
@jentfoo jentfoo added this pull request to the merge queue Dec 8, 2023
Merged via the queue into master with commit 2de1fdb Dec 8, 2023
@jentfoo jentfoo deleted the jent/saml_graceful_fail branch December 8, 2023 19:38
@public-teleport-github-review-bot
Copy link
Copy Markdown

@jentfoo See the table below for backport results.

Branch Result
branch/v12 Failed
branch/v13 Failed
branch/v14 Failed

jentfoo added a commit that referenced this pull request Dec 8, 2023
* Revert "log error when entity descriptor cant be fetched in saml (#34896)"

This reverts commit 27b79a2.

* Fix so that individual SSO Connector failures don't prevent other connectors from working
jentfoo added a commit that referenced this pull request Dec 8, 2023
* Revert "log error when entity descriptor cant be fetched in saml (#34896)"

This reverts commit 27b79a2.

* Fix so that individual SSO Connector failures don't prevent other connectors from working
jentfoo added a commit that referenced this pull request Dec 8, 2023
* Revert "log error when entity descriptor cant be fetched in saml (#34896)"

This reverts commit 27b79a2.

* Fix so that individual SSO Connector failures don't prevent other connectors from working
github-merge-queue Bot pushed a commit that referenced this pull request Dec 8, 2023
* Revert "log error when entity descriptor cant be fetched in saml (#34896)"

This reverts commit 27b79a2.

* Fix so that individual SSO Connector failures don't prevent other connectors from working
github-merge-queue Bot pushed a commit that referenced this pull request Dec 8, 2023
* Revert "log error when entity descriptor cant be fetched in saml (#34896)"

This reverts commit 27b79a2.

* Fix so that individual SSO Connector failures don't prevent other connectors from working
github-merge-queue Bot pushed a commit that referenced this pull request Dec 8, 2023
* Revert "log error when entity descriptor cant be fetched in saml (#34896)"

This reverts commit 27b79a2.

* Fix so that individual SSO Connector failures don't prevent other connectors from working
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants