-
Notifications
You must be signed in to change notification settings - Fork 26
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
Improve Sign1Message.Sign1() docs #85
Conversation
Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
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! I left a non-blocking comment that you may (or may not) decide to address.
// The signature is stored in m.Signature. | ||
// | ||
// Note that m.Signature is only valid as long as m.Headers.Protected and | ||
// m.Payload remain unchanged after calling this method. |
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.
another condition that may be worth to flag is when external
has changed
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.
external
is not kept in any Sign1Message
field but encoded as part of the signature, so modifying external
after calling Sign1
should be a problem.
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.
yes, I'm not sure here we want to highlight all possible ways the computed signature can be invalidated, or only those conditions that depend on the state of the message that we store. That's why I left it to your judgement :-)
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.
LGTM
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.
LGTM
Fix #68
Signed-off-by: qmuntal qmuntaldiaz@microsoft.com