Skip to content

Support AAL2 phishing-resistant and PIV/CAC authentication contexts#7063

Merged
mitchellhenke merged 9 commits intomainfrom
mitchellhenke/aal2-phishing-resistant
Oct 4, 2022
Merged

Support AAL2 phishing-resistant and PIV/CAC authentication contexts#7063
mitchellhenke merged 9 commits intomainfrom
mitchellhenke/aal2-phishing-resistant

Conversation

@mitchellhenke
Copy link
Contributor

@mitchellhenke mitchellhenke commented Sep 29, 2022

🎫 Ticket

LG-5904

🛠 Summary of changes

TBD

📜 Testing Plan

  • Can continue requesting AAL3 authentication
  • Can continue requesting AAL3 HSPD-12 required
  • Can request AAL2 phishing-resistant
  • Can request AAL2 HSPD-12 required

Copy link
Contributor

@orenyk orenyk left a comment

Choose a reason for hiding this comment

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

Love where this is going - I think a little more test coverage for the backwards compatibility and to ensure we're returning the right AuthnContext in SAML and this should be good to go, but also defer to you 😄. THANK YOU!!!

Copy link
Contributor

Choose a reason for hiding this comment

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

🙌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the last redirect to this was removed a couple years ago in #4014 so I deleted it

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/aal2-phishing-resistant branch 2 times, most recently from 2b073c8 to ff06d1e Compare October 3, 2022 20:49
@mitchellhenke
Copy link
Contributor Author

Love where this is going - I think a little more test coverage for the backwards compatibility and to ensure we're returning the right AuthnContext in SAML and this should be good to go, but also defer to you 😄. THANK YOU!!!

Had a go at adding the AuthnContext SAML specs in ff06d1e and some backwards compatibility tests in 244ea07 and 0b62a5e

I also renamed some of the AAL3 pieces to be a bit more precise where we are explicitly still using AAL3 value (like for default_aal)

@mitchellhenke mitchellhenke marked this pull request as ready for review October 3, 2022 22:03
Copy link
Contributor

@orenyk orenyk 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 but overall this looks/feels good to me. Not sure if you want another set of 👀 since I'm not quite as familiar with how the sp_session and mfa_policy get passed around, but from looking through the changes to the code everything made sense. Nice work, thank you!!

Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: looks like we'll also have some I18n keys to rename as well 😭

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, planning to tackle that in a future PR to keep the diff smaller here since it's already kind of unwieldy

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/aal2-phishing-resistant branch from c570f1f to 4c1cf37 Compare October 4, 2022 14:42
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/aal2-phishing-resistant branch from 4c1cf37 to a585ea9 Compare October 4, 2022 14:43
@mitchellhenke mitchellhenke merged commit 50209b7 into main Oct 4, 2022
@mitchellhenke mitchellhenke deleted the mitchellhenke/aal2-phishing-resistant branch October 4, 2022 15:06
jskinne3 pushed a commit that referenced this pull request Oct 12, 2022
…7063)

* Support AAL2 phishing-resistant and PIV/CAC authentication contexts

changelog: Improvements, Authentication, Support AAL2 phishing-resistant and PIV/CAC authentication contexts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants