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

Support AuthenticationScheme interface to enable PoP bound access tokens #443

Merged
merged 18 commits into from
Aug 22, 2023
Merged

Support AuthenticationScheme interface to enable PoP bound access tokens #443

merged 18 commits into from
Aug 22, 2023

Conversation

manojampalam
Copy link
Collaborator

@manojampalam manojampalam commented Jul 18, 2023 •
edited by julienstroheker
Loading

PR attempting to realize proposal - #439

This is to gather initial feedback. Comprehensive unit and integration tests will be added once code changes are agreed/acked.

  • Unit tests
  • Integration tests
  • Dev samples in apps/tests/devapps

Manoj Ampalam added 4 commits July 11, 2023 20:12
@manojampalam manojampalam mentioned this pull request Jul 18, 2023
Closed
@julienstroheker julienstroheker added the enhancement New feature or request label Aug 2, 2023
@manojampalam manojampalam changed the title Draft: Support AuthenticationScheme interface to enable PoP bound access tokens Support AuthenticationScheme interface to enable PoP bound access tokens Aug 2, 2023
@julienstroheker julienstroheker added enhancement New feature or request and removed enhancement New feature or request labels Aug 4, 2023
@julienstroheker julienstroheker mentioned this pull request Aug 4, 2023
Closed
9 tasks
@bgavrilMS bgavrilMS requested a review from chlowell August 7, 2023 16:04
chlowell
chlowell reviewed Aug 7, 2023
View reviewed changes
apps/confidential/confidential.go Outdated Show resolved Hide resolved
apps/internal/oauth/ops/authority/authority.go Outdated Show resolved Hide resolved
apps/internal/oauth/ops/authority/authority.go Outdated Show resolved Hide resolved
apps/internal/base/base.go Outdated Show resolved Hide resolved
apps/confidential/confidential_test.go Outdated Show resolved Hide resolved
apps/confidential/confidential_test.go Outdated Show resolved Hide resolved
apps/confidential/confidential_test.go Outdated Show resolved Hide resolved
apps/internal/oauth/ops/authority/authority.go Show resolved Hide resolved
apps/tests/integration/integration_test.go Outdated Show resolved Hide resolved
bgavrilMS
bgavrilMS reviewed Aug 9, 2023
View reviewed changes
apps/internal/base/internal/storage/items_test.go Outdated
)
if !extExpiresOn.Equal(actualAt.ExtendedExpiresOn.T) {
t.Errorf("Actual ext expires on %s differs from expected ext expires on %s", actualAt.ExtendedExpiresOn, extExpiresOn)
}
}

func TestKeyForAccessToken(t *testing.T) {
const want = "testhid-env-accesstoken-clientid-realm-user.read"
const want = "testhid-env-accesstoken-clientid-realm-user.read-bearer"
Copy link
Member

Choose a reason for hiding this comment

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

I am concerned that this will break backwards and forwards compatibility with the token cache. It's ok if older MSALs can't read POP tokens, but they should continue to read bearer tokens. As such, when the auth scheme is beaerr, we should leave the key alone.

Here's the equivalent logic in MSAL.NET https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/main/src/client/Microsoft.Identity.Client/AuthScheme/AuthSchemeHelper.cs

We should have some tests on this:

  • old cache format + new MSAL
  • new cache format (i.e. with POP and Bearer tokens )+ old MSAL (I guess this will have to be manual test)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. Fixed how key is generated in items.go. Updated test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Where can we find steps to to simulate
new cache format (i.e. with POP and Bearer tokens )+ old MSAL (I guess this will have to be manual test)

@julienstroheker julienstroheker added enhancement New feature or request and removed enhancement New feature or request labels Aug 10, 2023
@julienstroheker julienstroheker added enhancement New feature or request and removed enhancement New feature or request labels Aug 10, 2023
chlowell
chlowell reviewed Aug 10, 2023
View reviewed changes
apps/internal/oauth/ops/authority/authority.go Outdated Show resolved Hide resolved
apps/public/public.go Outdated Show resolved Hide resolved
apps/confidential/confidential_test.go Show resolved Hide resolved
apps/internal/oauth/ops/authority/authority.go Show resolved Hide resolved
@julienstroheker julienstroheker added enhancement New feature or request and removed enhancement New feature or request labels Aug 11, 2023
@sonarcloud
Copy link

sonarcloud bot commented Aug 11, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

No Coverage information No Coverage information
0.6% 0.6% Duplication

@julienstroheker
Copy link
Collaborator

We were able to validate this changes e2e on our side using Kubelogin to generate the token and Guard to validates it.

Kubelogin PR In Progress

Token generated by kubelogin via interactive mode :
image

Token validated by Guard :
image

Token expired rejected by Guard :
image

u claim mismatch rejected by Guard :
image

@julienstroheker julienstroheker merged commit 2f0c24b into dev Aug 22, 2023
6 checks passed
@julienstroheker julienstroheker deleted the authnscheme branch August 22, 2023 21:03
bgavrilMS pushed a commit that referenced this pull request Aug 23, 2023
@manojampalam @bgavrilMS
Support AuthenticationScheme interface to enable PoP bound access tok…
369fde6 
…ens (#443)
bgavrilMS added a commit that referenced this pull request Aug 23, 2023
@bgavrilMS
Revert "Support AuthenticationScheme interface to enable PoP bound ac…
f8205a6 
…cess tokens (#443)"

This reverts commit 369fde6.
bgavrilMS pushed a commit that referenced this pull request Aug 23, 2023
@manojampalam @bgavrilMS
Support AuthenticationScheme interface to enable PoP bound access tok…
e6db30a 
…ens (#443)
bgavrilMS pushed a commit that referenced this pull request Aug 23, 2023
@manojampalam @bgavrilMS
Support AuthenticationScheme interface to enable PoP bound access tok…
d89495c 
…ens (#443)
bgavrilMS added a commit that referenced this pull request Aug 23, 2023
@bgavrilMS
Revert "Support AuthenticationScheme interface to enable PoP bound ac…
ed7f828 
…cess tokens (#443)"

This reverts commit d89495c.
@bgavrilMS bgavrilMS mentioned this pull request Aug 23, 2023
Merged
bgavrilMS pushed a commit that referenced this pull request Aug 29, 2023
@manojampalam @bgavrilMS
Support AuthenticationScheme interface to enable PoP bound access tok…
ab2dc4b 
…ens (#443)
@bgavrilMS bgavrilMS linked an issue Aug 29, 2023 that may be closed by this pull request
[Feature Request] Extensibility for POP with SHR #439
Closed
9 tasks
This was referenced Jun 28, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julienstroheker julienstroheker julienstroheker left review comments

@chlowell chlowell chlowell approved these changes

@bgavrilMS bgavrilMS bgavrilMS approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request] Extensibility for POP with SHR
4 participants
@manojampalam @julienstroheker @chlowell @bgavrilMS