-
Notifications
You must be signed in to change notification settings - Fork 378
Revise MSI v2 token revocation spec to focus on certificate revocation #5498
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
Conversation
Updated the document to reflect changes from 'Short-Lived Credential (SLC)' to 'Certificate' terminology and clarified the handling of certificate revocation scenarios.
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.
Approved with comments
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.
Not sure about telemetry. Maybe start with having those values logged. Let's focus more on server telemetry.
docs/msi_v2/msiv2_revocation_spec.md
Outdated
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 CSR metadata request is part of the flow. I think you should add it here.
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.
added
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.
I still don't see 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.
both alts are the same except for 5a and 5b. Do you need to repeat the entire portion of the graph after those sections? Can you just group 5a and 5b together on the graph?
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.
these are two different error conditions
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.
Please explain the difference to me. Why aren't the diagrams the same for each condition?
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.
Why is there a "IMDS/eSTS" section? Why not point towards the already existing "IMDS" and "eSTS" sections?
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.
one is for cert revocation, that MSAL handles internally. The other is for token revocation.
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.
This isn't clear on the diagram. Can you use this wording on the diagram?
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.
specify which retry policy
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.
not sure I follow this comment
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.
You mention "retry" several times in this spec. I'm asking you: what retry policy are we using for these retries?
Updated revocation scenarios and clarified certificate request process.
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.
Is this going to be implemented for preview?
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.
no, this is not for preview.
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.
MSAL first "makes a CSR metadata request"
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.
I will re-review after you'd had a chance to respond to the comments I just left.
Updated the document to reflect changes from 'Short-Lived Credential (SLC)' to 'Certificate' terminology and clarified the handling of certificate revocation scenarios.
Fixes - Spec update
Changes proposed in this request
This pull request updates the documentation for MSI V2 credential revocation, clarifying and expanding the specification to focus on certificate revocation (rather than short-lived credentials) and aligning terminology, flows, error handling, and acceptance tests with current implementation and Azure AD error codes. The changes provide detailed guidance on how MSAL should handle certificate revocation, claims challenges, and telemetry.
Key documentation improvements:
Terminology and Flow Updates:
Error Handling and Remediation:
Claims Challenge Handling:
Acceptance Tests and Telemetry:
MsalMsiCounterto reflect the new tags and expected values for improved diagnostics.Testing
n/a
Performance impact
n/a
Documentation
n/a