Skip to content
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

Set random percentages through admin wfe at runtime #313

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mbaynton
Copy link

@mbaynton mbaynton commented Jun 5, 2020

Changing these percentages can simplify creating test suites where certain
tests exercise authorization reuse or nonce error handling.

Changing these percentages can simplify creating test suites where certain
tests exercise authorization reuse or nonce error handling.
wfe/wfe.go Outdated
}

if requestObj.AuthzReusePercent != nil {
wfe.authzReusePercent = int(*requestObj.AuthzReusePercent)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
==================

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants