-
Notifications
You must be signed in to change notification settings - Fork 202
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
Conversation
There was a problem hiding this 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. :-)
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. |
…nd certificate string
There was a problem hiding this 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.
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. |
There was a problem hiding this 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.
There was a problem hiding this 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.
This type of authentication expects authority url to be mentioned with the actual tenant id at the moment. Using organizations will fail the request