Skip to content

Add biometric acr_values for OIDC protocol#10993

Merged
Sgtpluck merged 4 commits intomainfrom
dmm/backup-acr
Jul 29, 2024
Merged

Add biometric acr_values for OIDC protocol#10993
Sgtpluck merged 4 commits intomainfrom
dmm/backup-acr

Conversation

@Sgtpluck
Copy link
Contributor

@Sgtpluck Sgtpluck commented Jul 26, 2024

🎫 Ticket

https://gitlab.login.gov/lg-people/lg-people-appdev/Melba/backlog-fy24/-/issues/43

This change builds off the changes in #10948

🛠 Summary of changes

This is the initial work to replace our public-facing Vectors of Trust (VOT) API interface(s) with acr_values. This work is focused on the OIDC implementation; we will follow up with whatever changes we need to make for SAML in the next week.

This change:

  • Maintains the current VoT implementation and API for current SPs
  • Adds support for selecting proofing with biometric comparison using new Authentication Context Class Reference values (ACR values) in our OIDC API
  • Does not change other ACR implementations

📜 Testing Plan

Acceptance testing detailed here
Release plan details here

@Sgtpluck Sgtpluck requested a review from vrajmohan July 26, 2024 19:25
Comment on lines +124 to +127
result.
component_values.
map(&:name).
include?(Saml::Idp::Constants::IAL2_BIO_REQUIRED_AUTHN_CONTEXT_CLASSREF)
Copy link
Contributor

Choose a reason for hiding this comment

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

wait can't we do this?

Suggested change
result.
component_values.
map(&:name).
include?(Saml::Idp::Constants::IAL2_BIO_REQUIRED_AUTHN_CONTEXT_CLASSREF)
result.biometric_comparison?

Copy link
Contributor

Choose a reason for hiding this comment

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

and if not, maybe we need to split into result.biometric_requested? vs result.biometric_required? (could be a follow-up but my expectation is that the point of the result class is to define capabiltiies so we don't have to check for specific ACRs throughout the codebase)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we definitely could do something like that! the initial implementation did add a biometric_comparison_required, but hooper thought that was a little confusing as someone trying to work downstream with the code. but maybe requested vs required would be clearer? what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the reason we need both is because we need a way to "fall back" onto our base identity proofing for partners who don't want to force their users to go through it again.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I think the fact that this PR defines biometric_is_required? means definitely we need a way to distinguish, so I would vote to rename the existing one, so yes there are both required + requested, but that could def be a candidate for a separate PR to keep this one focused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay yeah -- i can add this thought to the SAML implementation ticket (which, work on that can start as soon as this is merged in), since i think there won't be a ton of work in that, so it can focus a little more on optimizations like this.

Copy link
Contributor

@vrajmohan vrajmohan left a comment

Choose a reason for hiding this comment

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

Some minor comments, all to do with tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

When does this case arise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you trace the self.no_sp_result method back in the Vot::Parser, you can find this PR that explains it better #10092 but basically some requests don't include service provider details.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the context really "when an IAL without identity proofing is requested"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the context is that the requested IAL ACR value takes precedence. but happy to change it to your wording if that's helpful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, looking at the rest of the context blocks within this one, i think that this is correct. we do include a test for when identity proofing is requested, so i am going to leave it as-is

Copy link
Contributor

Choose a reason for hiding this comment

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

I find this expectation surprising. Don't we only support aal2's? Requesting no AAL results in an aal1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we require aal2 for identity proofing. for ial1 we only require MFA, which is not aal2. https://developers.login.gov/oidc/authorization/#aal_values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(also this test includes aal/1 as part of the request, if you look at the acr_values fixture)

Co-Authored-By: Lauren George <lauren.george@gsa.gov>
@Sgtpluck Sgtpluck merged commit 400b06a into main Jul 29, 2024
@Sgtpluck Sgtpluck deleted the dmm/backup-acr branch July 29, 2024 12:47
mitchellhenke pushed a commit that referenced this pull request Jul 31, 2024
changelog: Upcoming Features, IdV with Biometric Comparison, Adding acr_values
---------

Co-authored-by: Lauren George <lauren.george@gsa.gov>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants