-
-
Notifications
You must be signed in to change notification settings - Fork 629
Add visibility into nonce redemption failure causes #8448
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
base: main
Are you sure you want to change the base?
Conversation
d2427c2
to
1d49e4f
Compare
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.
Style nits; otherwise it looks good. Nice job removing that unnecessary layer of abstraction 👍🏻
// the largest nonce we've actually handed out), then the age is negative. | ||
age := float64(ns.latest-c) / float64(ns.latest-ns.earliest) | ||
|
||
if c > ns.latest { // i.e. age < 0 |
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.
Stylistically, we avoid inline comments.
if c > ns.latest { // i.e. age < 0 | |
if c > ns.latest { | |
// i.e. age < 0 |
} | ||
|
||
if c <= ns.earliest { | ||
if c <= ns.earliest { // i.e. age >= 1 |
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.
Same style comment as above.
if c <= ns.earliest { // i.e. age >= 1 | |
if c <= ns.earliest { | |
// i.e. age >= 1 |
Add metrics to the nonce server which give visibility into how many nonces it is currently tracking, and what distribution of nonce ages it actually sees. This should help us better understand the in-memory behavior of the nonce server, so we can better debug issues related to invalid nonce errors.
Also add much more detailed error messages throughout the nonce redemption pipeline. Have the nonce server's
valid()
helper method return individual error messages based on why the nonce was invalid. Have the nonce service itself return those error messages to the WFE, rather than simply returning "Valid: false". Finally, have the WFE suppress those detailed error messages when displaying BadNonce errors to the end-user, so they end up in our logs but not in user-facing API responses.Finally, due to the refactoring of the nonce service's
.valid()
helper above, simplify some unnecessary layers of abstraction within the nonce package. Remove the distinction between the "NonceService" (which contained all nonce logic) and the "NonceServer" (which implemented the gRPC interface). Make the non-gRPC methods unexported, since they're really just helper methods. Simplify the inmem nonce service to look exactly like the other inmem services, forwarding calls to the real gRPC implementation rather than to the underlying NonceService.