Set random percentages through admin wfe at runtime #313
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Changing these percentages can simplify creating test suites where certain tests exercise authorization reuse or nonce error handling.
squizzling
reviewed
May 2, 2021
wfe/wfe.go
Outdated
| } | ||
|
|
||
| if requestObj.AuthzReusePercent != nil { | ||
| wfe.authzReusePercent = int(*requestObj.AuthzReusePercent) |
There was a problem hiding this comment.
Although hard to trigger, these create data races:
==================
WARNING: DATA RACE
Read at 0x00c0001161f8 by goroutine 66:
github.com/letsencrypt/pebble/wfe.(*WebFrontEndImpl).verifyJWS()
/tank/tank/src/pebble/wfe/wfe.go:934 +0xd85
github.com/letsencrypt/pebble/wfe.(*WebFrontEndImpl).verifyPOST()
/tank/tank/src/pebble/wfe/wfe.go:876 +0x395
github.com/letsencrypt/pebble/wfe.(*WebFrontEndImpl).Authz()
/tank/tank/src/pebble/wfe/wfe.go:2040 +0x91
github.com/letsencrypt/pebble/wfe.(*WebFrontEndImpl).Authz-fm()
/tank/tank/src/pebble/wfe/wfe.go:2033 +0x84
github.com/letsencrypt/pebble/wfe.(*WebFrontEndImpl).HandleFunc.func1()
/tank/tank/src/pebble/wfe/wfe.go:283 +0x9b8
github.com/letsencrypt/pebble/wfe.wfeHandlerFunc.ServeHTTP()
/tank/tank/src/pebble/wfe/wfe.go:134 +0x83
github.com/letsencrypt/pebble/wfe.(*topHandler).ServeHTTP()
/tank/tank/src/pebble/wfe/wfe.go:146 +0x6e
net/http.StripPrefix.func1()
/usr/local/go/1.16.0/src/net/http/server.go:2112 +0x535
net/http.HandlerFunc.ServeHTTP()
/usr/local/go/1.16.0/src/net/http/server.go:2069 +0x51
net/http.(*ServeMux).ServeHTTP()
/usr/local/go/1.16.0/src/net/http/server.go:2448 +0xaf
net/http.serverHandler.ServeHTTP()
/usr/local/go/1.16.0/src/net/http/server.go:2887 +0xca
net/http.(*conn).serve()
/usr/local/go/1.16.0/src/net/http/server.go:1952 +0x87d
Previous write at 0x00c0001161f8 by goroutine 76:
github.com/letsencrypt/pebble/wfe.(*WebFrontEndImpl).updateRuntimeConfig()
/tank/tank/src/pebble/wfe/wfe.go:519 +0x3a4
github.com/letsencrypt/pebble/wfe.(*WebFrontEndImpl).updateRuntimeConfig-fm()
/tank/tank/src/pebble/wfe/wfe.go:489 +0x84
github.com/letsencrypt/pebble/wfe.wfeHandlerFunc.ServeHTTP()
/tank/tank/src/pebble/wfe/wfe.go:134 +0x83
net/http.StripPrefix.func1()
/usr/local/go/1.16.0/src/net/http/server.go:2112 +0x535
net/http.HandlerFunc.ServeHTTP()
/usr/local/go/1.16.0/src/net/http/server.go:2069 +0x51
net/http.(*ServeMux).ServeHTTP()
/usr/local/go/1.16.0/src/net/http/server.go:2448 +0xaf
net/http.serverHandler.ServeHTTP()
/usr/local/go/1.16.0/src/net/http/server.go:2887 +0xca
net/http.(*conn).serve()
/usr/local/go/1.16.0/src/net/http/server.go:1952 +0x87d
Goroutine 66 (running) created at:
net/http.(*Server).Serve()
/usr/local/go/1.16.0/src/net/http/server.go:3013 +0x644
net/http.(*Server).ServeTLS()
/usr/local/go/1.16.0/src/net/http/server.go:3053 +0x433
net/http.(*Server).ListenAndServeTLS()
/usr/local/go/1.16.0/src/net/http/server.go:3208 +0x192
net/http.ListenAndServeTLS()
/usr/local/go/1.16.0/src/net/http/server.go:3174 +0x126e
main.main()
/tank/tank/src/pebble/cmd/pebble/main.go:101 +0x126f
Goroutine 76 (finished) created at:
net/http.(*Server).Serve()
/usr/local/go/1.16.0/src/net/http/server.go:3013 +0x644
net/http.(*Server).ServeTLS()
/usr/local/go/1.16.0/src/net/http/server.go:3053 +0x433
net/http.(*Server).ListenAndServeTLS()
/usr/local/go/1.16.0/src/net/http/server.go:3208 +0x192
net/http.ListenAndServeTLS()
/usr/local/go/1.16.0/src/net/http/server.go:3174 +0x1e4
main.main.func1()
/tank/tank/src/pebble/cmd/pebble/main.go:80 +0x1e5
==================
There was a problem hiding this comment.
@squizzling thanks for the review, and props for triggering the race detector especially sans applicable tests. I think my shields were probably lowered in this respect because I anticipated use cases where adjustment of the percentages was de-facto sequential before usage of Pebble that would generate reads, but just eliminating the possibility is definitely better.
The data was conveniently of a type that can be accessed using processor-level atomics, and since there was not any traditional locking already going on in wfe I just fixed it that way.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changing these percentages can simplify creating test suites where certain
tests exercise authorization reuse or nonce error handling.