Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define If Logic to Validate Audience in SAML2 Token Handler Is Accurate For Both New and Old Paths. #2901

Open
FuPingFranco opened this issue Oct 11, 2024 · 2 comments
Assignees
Labels
Internal Indicates issue was opened by the IdentityModel team P3 If we have time in the milestone or it just is easy when addressing a more important issue

Comments

@FuPingFranco
Copy link
Contributor

During the work to migrate Saml2SecurityTokenHandler to our new validation model I'd like to clarify a couple of scenarios that are possible to break on the snip of code below:

var foundAudienceRestriction = false;
foreach (var audienceRestriction in samlToken.Assertion.Conditions.AudienceRestrictions)
{
    if (!foundAudienceRestriction)
        foundAudienceRestriction = true;

    ValidateAudience(audienceRestriction.Audiences, samlToken, validationParameters); //This will only run once reagardless of a successful validation or not.
}

Points to clarify:

  • Do we need the loop at all?
  • If we keep the loop, should we return as soon as we find a valid audience
  • Is there an scenario where the valid audience is on the second audience restriction obj?
  • Is unclear from the SAML 2 spec, what is our current guideline for this case?
@FuPingFranco
Copy link
Contributor Author

This is also handled differently in SamlSecurityTokenHandler

            var foundAudienceRestriction = false;
            foreach (var condition in securityToken.Assertion.Conditions.Conditions)
            {
                if (condition is SamlAudienceRestrictionCondition audienceRestriction)
                {
                    if (!foundAudienceRestriction)
                        foundAudienceRestriction = true;

                    ValidateAudience(audienceRestriction.Audiences.ToDictionary(x => x.OriginalString).Keys, securityToken, validationParameters);
                }
            }

It might be worthwhile to have them follow the same logic but there is a caveat where samlCondition.audiences return different values. One returns an ICollection<Uri> and the other ICollection<String>

@brentschmaltz brentschmaltz added the P3 If we have time in the milestone or it just is easy when addressing a more important issue label Oct 21, 2024
@brentschmaltz
Copy link
Member

We should be trying all Audience Conditions and not faulting on the first one.

@FuPingFranco FuPingFranco removed their assignment Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Indicates issue was opened by the IdentityModel team P3 If we have time in the milestone or it just is easy when addressing a more important issue
Projects
None yet
Development

No branches or pull requests

3 participants