letsencrypt / boulder

An ACME-based certificate authority, written in Go.
Mozilla Public License 2.0
5.06k stars 593 forks source link

Deprecate Challenge.ProvidedKeyAuthorization #7515

Closed aarongable closed 4 weeks ago

aarongable commented 1 month ago

The core.Challenge.ProvidedKeyAuthorization field is problematic, both because it is poorly named (which is admittedly easily fixable) and because it is a field which we never expose to the client yet it is held on a core type. Deprecate this field, and replace it with a new vapb.PerformValidationRequest.ExpectedKeyAuthorization field.

Within the VA, this also simplifies the primary logic methods to just take the expected key authorization, rather than taking a whole (largely unnecessary) challenge object. This has large but wholly mechanical knock-on effects on the unit tests.

While we're here, improve the documentation on core.Challenge itself, and remove Challenge.URI, which was deprecated long ago and is wholly unused.

Part of https://github.com/letsencrypt/boulder/issues/7514

aarongable commented 4 weeks ago

What if, instead of adding ExpectedKeyAuthorization as a field, we added Account as a field?

To me, this feels like a totally reasonable approach, but from the opposite end of a design spectrum. My approach here is "provide only the information necessary": we don't need to be passing around a whole challenge object, with a bunch of irrelevant fields, so don't. Adding a whole Account to the request feels like the opposite approach: provide as much context as possible, so we have all the information we could ever need.

For example, if we did include a whole Account object in the gRPC request, I wouldn't use req.Account.ID to double-check the value of req.Authz.RegID -- I'd use it to replace that value.

I think if I wanted something more strongly-typed (which I'm not opposed to!), my personal dev philosophy would lead me towards something more like

// core/proto/core.proto
message KeyAuthorization {
  string token = 1;
  string thumbprint = 2;
}
// core/objects.go
type KeyAuthorization struct {
  token string;
  thumbprint string;
}

func NewKeyAuthorization(ch *core.Challenge, key *jose.JSONWebKey) (KeyAuthorization, error) { ... }

func (ka KeyAuthorization) String() string {
  return fmt.Sprintf("%s.%s", ka.token, ka.thumbprint)
}

or maybe even the same thing, but storing the bytes of the pubkey directly and computing the thumbprint live inside .String().

But even that has disadvantages -- you can't actually make the fields of core.KeyAuthorization private, because then you can't convert a protobuf into one. So you can't force people to use the proper New... method to create new values, and you can't force people to use the proper .String() method to get the value back out, so how much type safety have you really gained? A layer of indirection, yes, but still just publicly-accessible-and-modifiable strings below that.

Anyway, all of that is a very long winded way to say that I'm not strongly opposed to doing what you suggest, but it doesn't feel like simplifying things in the way that this PR sets out to do, and I haven't fallen in love with any of the midpoints I've come up with so far.