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.

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

Subject Name and Issuer Authentication #71

Merged
merged 7 commits into from
Jul 3, 2019
Merged

Subject Name and Issuer Authentication #71

merged 7 commits into from
Jul 3, 2019

Conversation

abhidnya13
Copy link
Contributor

@abhidnya13 abhidnya13 commented Jun 28, 2019

  • This PR closes SubjectName/Issuer (SNI) authentication #60
  • Adds support for Subject Name Issuer Authentication which is to support cert auto rolls.
  • We have tested the end to end scenario by creating our own Test Environment. We will work with the lab team for creating test environment and then integrating in Test automation.
  • Note:
    This type of authentication expects authority url to be mentioned with the actual tenant id at the moment. Using organizations will fail the request

@abhidnya13 abhidnya13 requested a review from rayluo June 28, 2019 04:19
@abhidnya13 abhidnya13 added this to the MSAL Python 0.5.0 milestone Jun 28, 2019
Copy link
Collaborator

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

Thank you @abhidnya13 for your tireless effort on setting up a real test environment for this! That gave us high confidence on this implementation. Documenting below are our off-line conversations on how to polish this PR. Almost there. :-)

msal/application.py Outdated Show resolved Hide resolved
msal/application.py Outdated Show resolved Hide resolved
msal/application.py Outdated Show resolved Hide resolved
msal/application.py Outdated Show resolved Hide resolved
@rayluo
Copy link
Collaborator

rayluo commented Jun 28, 2019

Oh this is an extra requirement beyond the original task scope: we will probably want to investigate how to automatically compute the thumbprint from the public cert. That would save app dev the trouble of updating thumbprint when they rotate their certs.

Copy link
Collaborator

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

Thanks for your effort! I provide some more comments below. Let me know if you have more questions. :-)

On a side note, later we might also want to make the thumbprint input optional, ideally on the service-side, or we might still need to do it on client-side. So be prepared for such upcoming change.

msal/application.py Outdated Show resolved Hide resolved
tests/test_application.py Outdated Show resolved Hide resolved
@abhidnya13
Copy link
Contributor Author

Thanks for the review Ray ! This is really good learning for me :) I have made the changes. Let me know if it looks good now. Feel free to suggest anything else that we can do to make it better before we merge it in.
Lets look into how to automatically compute the thumbprint from the public cert. That will make it easier for the developers for sure.

Copy link
Collaborator

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

The rest of this PR looks great! Now let's make a final tuning on the regex part.
PS: The thumbprint enhancement will be an extra topic. I'll send an email to service team about it.

msal/application.py Outdated Show resolved Hide resolved
msal/application.py Outdated Show resolved Hide resolved
msal/application.py Show resolved Hide resolved
Copy link
Collaborator

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

Beautiful work! Ship it!

We can merge this PR now. The auto-compute-thumbprint topic can be addressed later in a different PR.

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.

SubjectName/Issuer (SNI) authentication
2 participants