fix saml IdP service provider validation#36057
Conversation
|
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 |
There was a problem hiding this comment.
nit: change tt.ok from a bool to a require.ErrorAssertionFunc and these 5 lines can collapse to a single line.
There was a problem hiding this comment.
For tests that care about the output of the fallible operation switching to AssertionFunc doesn't really save loc, since you have additional checks that are only run for the happy variants (meaning u still need all the same control-flow). I don't mind switching to AssertionFunc if thats something we'd like to standardize on, but IMO for a test like this that isn't worried about the specific error kinds of failures, it's just more visual noise without a meaningful upside.
fspmarshall
force-pushed
the
fspmarshall/fix-saml-validation
branch
from
0b8310b to
ccd7ace
Compare
| ed, err := samlsp.ParseMetadata([]byte(sp.GetEntityDescriptor())) | ||
| if err != nil { | ||
| return trace.BadParameter("invalid entity descriptor for SAML IdP Service provider %q: %v", sp.GetEntityID(), err) | ||
| } | ||
|
|
||
| // try filtering the entity descriptor. if it can't be filtered down to a useable looking state, reject | ||
| // the creation attempt. | ||
| if err := services.FilterSAMLEntityDescriptor(ed); err != nil { | ||
| return trace.Wrap(err) | ||
| } |
There was a problem hiding this comment.
Do we need this logic client side? Aren't Create/UpdateSAMLIdPServiceProvider performing the same checks?
There was a problem hiding this comment.
FilterSAMLEntityDescriptor generates detailed warn logs, which I think are good to have client-side for this type.
There was a problem hiding this comment.
Oops! This actually broke the new feature where we now allow service provider creation without upfront entity descriptor (that will be generated later in CreateSAMLIdPServiceProvider) - #34661
public-teleport-github-review-bot
Bot
removed the request for review
from ryanclark
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
|
@fspmarshall See the table below for backport results.
|
5 participants
Some recent SAML IdP validation changes started rejecting insecure SAML service provider descriptors. This ended up causing issues for two reasons:
httpandhttpsthe configuration would be rejected, instead of simply ignoring thehttpoption always using thehttpsoption).This PR is the first of two that will move us over to ignoring and warning about insecure IDP descriptors (a followup PR in
eis needed to fully switch over to the new behavior).These changes were manually tested against samltest.id.
changelog: fixed an issue where valid saml entity descriptors could be rejected.