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.

Sign up for GitHub

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

Jump to bottom

Ensure proper behavior for default and invalid NameID format requests #10629

Merged
merged 1 commit into from
May 23, 2024
Merged

Ensure proper behavior for default and invalid NameID format requests #10629

merged 1 commit into from
May 23, 2024
+123 −238

Conversation

vrajmohan
Copy link
Member

🎫 Ticket

Link to the relevant ticket:
https://gitlab.login.gov/lg-people/lg-people-appdev/protocols/saml/-/issues/2
-->

🛠 Summary of changes

  • Refactored tests to remove duplication and make contexts explicit
  • Fail on unsupported NameID formats unless service provider is configured with use_legacy_name_id_behavior.
  • Use persistent as default NameID format when the requested NameID format is "unspecified" or there is no requested NameID format. In particular, don't send back email even if the service provider is configured for using email, unless email is explicitly requested.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Ensure that use_legacy_name_id_behavior is false for the service provider, and make a SAML Authn request with an unsupported NameID format like 'urn:oasis:names:tc:SAML:1.1:nameid-format:transient'.
  • Make a SAML Authn request with NameID format missing or with the value 'urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified'. Ensure that the response is always 'urn:oasis:names:tc:SAML:1.1:nameid-format:persistent'. This should be true regardless of the service provider's email_nameid_format_allowed attribute.

@vrajmohan vrajmohan changed the title Nameid validation Ensure proper behavior for default and invalid NameID format requests May 15, 2024
@vrajmohan vrajmohan force-pushed the nameid-validation branch 2 times, most recently from d076f59 to 997d9db Compare May 15, 2024 00:38
@vrajmohan vrajmohan requested a review from Sgtpluck May 15, 2024 01:35
Sgtpluck
Sgtpluck reviewed May 16, 2024
View reviewed changes
Copy link
Member

@Sgtpluck Sgtpluck left a comment

Choose a reason for hiding this comment

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

A couple of questions and suggestions, but it's looking really good! Thanks for refactoring all those tests.

app/services/saml_request_validator.rb Outdated
'urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress',
'urn:oasis:names:tc:SAML:2.0:nameid-format:emailAddress',
].include?(nameid_format)
nameid_format == 'urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
nameid_format == 'urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress'
nameid_format == Saml::Idp::Constants::NAME_ID_FORMAT_EMAIL

app/controllers/concerns/saml_idp_auth_concern.rb Show resolved Hide resolved
spec/controllers/saml_idp_controller_spec.rb Outdated
service_provider = build(:service_provider, issuer: auth_settings.issuer)
IdentityLinker.new(user, service_provider).link_identity
user.identities.last.update!(verified_attributes: ['email'])
generate_saml_response(user, auth_settings)
ServiceProvider.
Copy link
Member

Choose a reason for hiding this comment

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

i would pull the service_provider out into a let, then you can do updates in describe blocks without having to do a query

let(:service_provider) do
  build(:service_provider, issuer: auth_settings.issuer, email_nameid_format_allowed: email_allowed)
end

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense. Unfortunately, when I tried it, I found issues with issuer uniqueness, certificate validation and seeded test data. Trying to clean this up...

@vrajmohan vrajmohan force-pushed the nameid-validation branch 3 times, most recently from e4fc472 to 70ae106 Compare May 17, 2024 17:04
Sgtpluck
Sgtpluck reviewed May 20, 2024
View reviewed changes
app/services/saml_request_validator.rb Outdated
def authorized_nameid_format
return if satisfiable_nameid_format?
return if email_nameid_format? && service_provider&.email_nameid_format_allowed
return if !email_nameid_format? && service_provider&.use_legacy_name_id_behavior
Copy link
Member

Choose a reason for hiding this comment

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

i think this one also would be helpful to pull out!

return if legacy_name_id_behavior_needed

def legacy_name_id_behavior_needed
  !email_nameid_format? && service_provider&.use_legacy_name_id_behavior
end

note: just to confirm, you double checked that folks with the legacy_name_id_behavior who are requesting the email_nameid_format are currently getting the emailAddress nameid format, yes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion. I will flip the predicate around to make it more clear.

def legacy_name_id_behavior_needed
  service_provider&.use_legacy_name_id_behavior && !email_nameid_format? 
end

The existing code (and I have left the behavior alone) already returns an error if the email format is requested when the email_nameid_format_allowed is false, regardless of the value of use_legacy_name_id_behavior. This corresponds to cells B4 and D4 in the spreadsheet. Also, as you noted, none of the use_legacy_name_id_behavior are currently requesting the email format.

Copy link
Member

Choose a reason for hiding this comment

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

great, thanks for reminding me!

@vrajmohan vrajmohan force-pushed the nameid-validation branch 4 times, most recently from 9cd1e23 to e47393f Compare May 23, 2024 17:26
@vrajmohan
Validate requested NameID formats and return appropriate response
8da246e 
See
https://gitlab.login.gov/lg-people/lg-people-appdev/protocols/backlog-fy24/-/issues/7

**Why**:
1. Email format was being returned for service providers configured with
    email_nameid_format_allowed, even when it was not explicitly
    requested (in the SAML request).
2. Unsupported NameID formats were still being honored by returning
    either the UUID or the email (depending on email_nameid_format_allowed),
    instead of returning an error.
    returned

**How**:
1. Return Email format only when email_nameid_format_allowed is
    configured AND the email format is explicitly requested
2. Fail on unsupported NameID formats

In addition, fixed a couple of minor issues:
1. Clean up a Regex
2. Remove support of a mythical email format -
    'urn:oasis:names:tc:SAML:2.0:nameid-format:emailAddress'

changelog: User-Facing Improvements, SAML, Validate requested NameID formats and return appropriate response
@vrajmohan vrajmohan merged commit 30cb0f3 into main May 23, 2024
2 checks passed
@vrajmohan vrajmohan deleted the nameid-validation branch May 23, 2024 20:37
@zachmargolis zachmargolis mentioned this pull request May 28, 2024
Merged
jmhooper added a commit to 18F/identity-saml-sinatra that referenced this pull request May 28, 2024
@jmhooper
Update NameID format to match expected value
951c894 
In 18F/identity-idp#10629 we updated the validation of NameID formats and apparently this application is using an invalid one. This commit updates it to use a valid NameID format.
@jmhooper jmhooper mentioned this pull request May 28, 2024
Merged
jmhooper added a commit to 18F/identity-saml-sinatra that referenced this pull request May 28, 2024
@jmhooper
Update NameID format to match expected value (#151)
365b29a 
In 18F/identity-idp#10629 we updated the validation of NameID formats and apparently this application is using an invalid one. This commit updates it to use a valid NameID format.
vrajmohan added a commit that referenced this pull request May 28, 2024
@vrajmohan
Revert "Validate requested NameID formats and return appropriate resp…
fa6fc78 
…onse (#10629)"

This reverts commit 30cb0f3.

changelog: Bug Fixes, SAML, Revert validation of NameID formats
vrajmohan added a commit that referenced this pull request May 28, 2024
@vrajmohan
Revert "Validate requested NameID formats and return appropriate resp…
e8da68f 
…onse (#10629)" (#10712)

This reverts commit 30cb0f3.

changelog: Bug Fixes, SAML, Revert validation of NameID formats
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sgtpluck Sgtpluck Sgtpluck approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@vrajmohan @Sgtpluck