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

Improve how we create new authorizations #7643

Merged
merged 6 commits into from
Aug 8, 2024
Merged

Improve how we create new authorizations #7643

merged 6 commits into from
Aug 8, 2024

Conversation

aarongable
Copy link
Contributor

@aarongable aarongable commented Aug 3, 2024

Within the NewOrderAndAuthzsRequest, replace the corepb.Authorization field with a new sapb.NewAuthzRequest message. This message has all of the same field types and numbers, and the RA still populates all of these fields when constructing a request, for backwards compatibility. But it also has new fields (an Identifier carrying both type and value, a list of challenge types, and a challenge token) which the RA preferentially consumes if present.

This causes the content of our NewOrderAndAuthzsRequest to more closely match the content that will be created at the database layer. Although this may seem like a step backwards in terms of abstraction, it is also a step forwards in terms of both efficiency (not having to transmit multiple nearly-identical challenge objects) and correctness (being guaranteed that the token is actually identical across all challenges).

After this change is deployed, it will be followed by a change which removes the old fields from the NewAuthzRequest message, to realize the efficiency gains.

Part of #5913

@aarongable aarongable force-pushed the new-authz-req branch 2 times, most recently from 840f988 to 26f18db Compare August 6, 2024 19:09
@aarongable aarongable marked this pull request as ready for review August 6, 2024 20:42
@aarongable aarongable requested a review from a team as a code owner August 6, 2024 20:42
@aarongable aarongable requested review from jsha and beautifulentropy and removed request for jsha August 6, 2024 20:42
Copy link
Member

@beautifulentropy beautifulentropy left a comment

Choose a reason for hiding this comment

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

These look like excellent improvements. I'd like a small doc comment change and some consideration on another comment.

ra/ra.go Show resolved Hide resolved
sa/model.go Outdated Show resolved Hide resolved
@aarongable
Copy link
Contributor Author

To ensure there are no deployability problems with this change, I have run the Go subset of our integration tests with both a new RA / old SA combo, and an old RA / new SA combo. Note the BuildIDs in the startup version logs, indicating the branch that the service was built from:

❯ docker compose up boulder
<snip>
boulder-1  | 16:58:00.416181 6 boulder-sa n_ifwwM Versions: boulder-sa=(main Unspecified) Golang=(go1.22.2) BuildHost=(Unspecified)
boulder-1  | 16:58:04.592247 6 boulder-ra jsHX0gc Versions: boulder-ra=(new-authz-req Unspecified) Golang=(go1.22.2) BuildHost=(Unspecified)
<snip>
❯ docker compose exec boulder bash
root@c77883ff0f71:/boulder# go test -tags integration -count=1 -race ./test/integration
ok      github.com/letsencrypt/boulder/test/integration 36.365s
❯ docker compose up boulder
<snip>
boulder-1  | 17:01:57.291694 6 boulder-sa mub6og0 Versions: boulder-sa=(new-authz-req Unspecified) Golang=(go1.22.2) BuildHost=(Unspecified)
boulder-1  | 17:02:01.575147 6 boulder-ra w63SxgY Versions: boulder-ra=(main Unspecified) Golan
g=(go1.22.2) BuildHost=(Unspecified)
<snip>
❯ docker compose exec boulder bash
root@c77883ff0f71:/boulder# go test -tags integration -count=1 -race ./test/integration
ok      github.com/letsencrypt/boulder/test/integration 36.964s

For posterity, the steps I followed to get this working were:

  1. Edit startservers.py's install command to add -ldflags "-X \"github.com/letsencrypt/boulder/core.BuildID=foobar' so that the binary it produces will have an actual build ID to report
  2. Check out main, start the integration tests to build a binary, and copy //bin/boulder to //bin/boulder-main
  3. Check out this branch, edit startservers to set a different BuildID, rebuild the binary, and copy //bin/boulder to //bin/boulder-branch
  4. Comment out the ./link.sh line from the Makefile
  5. Edit startservers.py to invoke //bin/boulder-ra and //bin/boulder-sa directly (instead of using subcommands like //bin/boulder boulder-ra)
  6. Symlink //bin/boulder-ra to //bin/boulder-branch, and //bin/boulder-sa to //bin/boulder-main
  7. Run the commands shown above
  8. Repeat steps (6) and (7), but with the symlinks swapped

There has to be an easier way, but this definitely worked.

@aarongable aarongable merged commit 35b0b55 into main Aug 8, 2024
12 checks passed
@aarongable aarongable deleted the new-authz-req branch August 8, 2024 17:15
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