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

Expose MSAL PoP for Consistent CSP Integration #542

Merged
merged 11 commits into from
Dec 2, 2024
Merged

Expose MSAL PoP for Consistent CSP Integration #542

merged 11 commits into from
Dec 2, 2024

Conversation

JorgeDaboub
Copy link
Contributor

@JorgeDaboub JorgeDaboub commented Oct 31, 2024 •
edited
Loading

This PR is needed to unify the custom PoP token implementation in the MSAL library for kubelogin and CSP (Client-Side Proxy), an Arc component used for line-of-sight and authentication with Arc resources.

@julienstroheker

Changes in this PR:

  • Exposed Methods:
    • AcquirePoPTokenConfidential
    • GetSwPoPKey
  • Exposed Structs and Methods:
    • PoPAuthenticationScheme and its FormatAccessToken method.
    • SwKey and associated methods.
  • Updated AcquirePoPTokenConfidential to accept popKey as a parameter, enabling reuse of the same key over time instead of generating a new one for each call, per our scenario's requirements.

@JorgeDaboub JorgeDaboub marked this pull request as ready for review November 18, 2024 23:58
julienstroheker
julienstroheker reviewed Nov 19, 2024
View reviewed changes
pkg/pop/types.go Outdated Show resolved Hide resolved
julienstroheker
julienstroheker reviewed Nov 19, 2024
View reviewed changes
pkg/pop/types.go Outdated Show resolved Hide resolved
@JorgeDaboub
Copy link
Contributor Author

Hi @bcho

This is the follow-up PR to #447
As requested, I’ve only exposed the necessary methods. Julien and I have already reviewed the changes.
Could you please take a look? Thanks!

bcho
bcho reviewed Nov 21, 2024
View reviewed changes
pkg/pop/types.go Outdated Show resolved Hide resolved
bcho
bcho approved these changes Nov 22, 2024
View reviewed changes
Copy link
Member

@bcho bcho left a comment

Choose a reason for hiding this comment

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

LGTM

@bcho
Copy link
Member

bcho commented Nov 22, 2024

@weinong PTAL

@julienstroheker
Copy link
Contributor

@rharpavat PTAL

rharpavat
rharpavat approved these changes Nov 22, 2024
View reviewed changes
Copy link
Contributor

@rharpavat rharpavat left a comment

Choose a reason for hiding this comment

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

LGTM!

@weinong
Copy link
Contributor

weinong commented Dec 1, 2024

LGTM, but how will these exposed functions be used?

@weinong weinong merged commit 8d6ce2f into Azure:main Dec 2, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rharpavat rharpavat rharpavat approved these changes

@julienstroheker julienstroheker julienstroheker approved these changes

@bcho bcho bcho 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.
5 participants
@JorgeDaboub @bcho @julienstroheker @weinong @rharpavat