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

RA: Don't reuse authzs with mismatched profiles #7967

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

Conversation

aarongable
Copy link
Contributor

@aarongable aarongable commented Jan 21, 2025

In the RA, inspect the profile of all authorizations returned when looking for authz reuse, and refuse to reuse any whose profile doesn't match the requested profile of the current NewOrder request.

Fixes #7949


Warning

Do not merge until #7956 has been merged

Base automatically changed from authz-profile to main January 27, 2025 22:53
@aarongable aarongable marked this pull request as ready for review January 29, 2025 23:37
@aarongable aarongable requested a review from a team as a code owner January 29, 2025 23:37
@aarongable aarongable requested a review from jprenken January 29, 2025 23:37
jprenken
jprenken previously approved these changes Jan 30, 2025
@jprenken jprenken requested review from a team and beautifulentropy and removed request for a team January 30, 2025 00:36
@aarongable aarongable requested a review from jsha January 30, 2025 16:00
Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

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

SA.GetValidAuthorizations2 has some logic to reduce the number of authorizations it sends back. If there are multiple authorizations for a single identifier, it will only return the most recent.

That produces a funny interaction with this code, where the SA might choose the newer of two authorizations, while the RA would prefer the older of the two because it had the right profile. But the RA doesn't get a chance because it was already filtered out.

I think it makes sense to move the filtering logic into the SA, before the de-duplicating logic. For instance, GetValidAuthorizationsRequest could get a new field, profile.

Alternately we could try to move all the filtering logic from the SA into the RA. That means slightly more data transfer, but also means more of the business logic is in the RA, relative to the SA.

Thinking longer term: do we want the profile to be part of the primary key for authorizations in our key-value data store? If so, that favors sending profile to the SA and having the SA filter on that field.

@beautifulentropy
Copy link
Member

I think it makes sense to move the filtering logic into the SA, before the de-duplicating logic. For instance, GetValidAuthorizationsRequest could get a new field, profile.

Alternately we could try to move all the filtering logic from the SA into the RA. That means slightly more data transfer, but also means more of the business logic is in the RA, relative to the SA.

Thinking longer term: do we want the profile to be part of the primary key for authorizations in our key-value data store? If so, that favors sending profile to the SA and having the SA filter on that field.

You bring up excellent points. I agree that we should consider the long-term implications for (key-value) storage. Given the two options, I’d lean toward keeping business logic out of the Storage Authority, particularly avoiding making it aware of profiles. We can defer that complexity until the schema migration is on our plates.

@beautifulentropy
Copy link
Member

beautifulentropy commented Jan 31, 2025

Thinking on the latter option a little more, the current query in SA.GetValidAuthorizations2 uses the regID_identifier_status_expires_idx index, which covers registrationID, identifierType, identifierValue, status, and expires. Filtering on certificateProfileName with a WHERE clause would likely cause the index scan to be a little less efficient. This isn't insurmountable but we should make sure do some tests with EXPLAIN and confirm that the query planner isn't going to do anything wild, like a full table scan.

@aarongable
Copy link
Contributor Author

Replying to comments in approximately-backwards order:

Filtering on certificateProfileName with a WHERE clause would likely cause the index scan to be a little less efficient.

Agreed, if this gRPC method does grow a profile field, we definitely wouldn't include in in the SQL query. We'd just do the filtering in-memory in the SA.

I’d lean toward keeping business logic out of the Storage Authority, particularly avoiding making it aware of profiles.

Of course, the SA already is aware of profiles, in order to store and retrieve them in both the order and authz tables.

do we want the profile to be part of the primary key for authorizations in our key-value data store?

The currently-proposed authzReuse table schema does not take profiles into account. We can change that, but we didn't think it was super important when discussing and designing that table.

That produces a funny interaction with this code, where the SA might choose the newer of two authorizations, while the RA would prefer the older of the two because it had the right profile. But the RA doesn't get a chance because it was already filtered out.

The same thing already happens for Order reuse: GetOrderForNames only returns a single order, which the RA then might refuse to use.

At the time, we judged the slight decrease in potential order reuse as an acceptable loss. I continued that line of thinking here. I'm happy to change course, but then I think we should make the same change (whatever it is) to GetOrderForNames as well.

As for what that change is... I'm pretty strongly in favor of including the profile as a field in the gRPC request message, and letting the SA do the filtering. This is for two reasons:

  1. It's more flexible in the future -- whether we have a profile index in mariadb, or include the profile in the primary key in rocksdb, or just do the filtering in memory, including the profile in the request message gives us the flexibility to do any of those.
  2. It's much less invasive. Doing the filtering in the RA requires changing these methods to have plural return types (i.e. GetOrderForNames would need to have a new return type with a repeated corepb.Order sub-message) which is a much more arduous change to make. Simply adding a new profile field and filtering if it is present is a tiny change by comparison.

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.

RA: Prevent cross-profile authorization reuse
4 participants