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

Budget tweaks #164

Merged
merged 3 commits into from Sep 6, 2023
Merged

Budget tweaks #164

merged 3 commits into from Sep 6, 2023

Conversation

djc
Copy link
Member

@djc djc commented Sep 5, 2023

Two minor style nits and an attempt at better forcing consumption of signature verification budget.

This is basically follow-up from #163.

@djc djc requested review from cpu and ctz September 5, 2023 16:10
@codecov
Copy link

codecov bot commented Sep 5, 2023

Codecov Report

Merging #164 (98da366) into main (99b64c5) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #164      +/-   ##
==========================================
- Coverage   96.62%   96.61%   -0.02%     
==========================================
  Files          15       15              
  Lines        4480     4463      -17     
==========================================
- Hits         4329     4312      -17     
  Misses        151      151              
Files Changed Coverage Δ
src/crl.rs 99.53% <100.00%> (+<0.01%) ⬆️
src/ring_algs.rs 100.00% <100.00%> (ø)
src/signed_data.rs 98.98% <100.00%> (-0.20%) ⬇️
src/verify_cert.rs 97.83% <100.00%> (+<0.01%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cpu
Copy link
Member

cpu commented Sep 5, 2023

😅 I have 2/3 of these changes done in a local branch. I also refactored the rcgen test code a little bit for deduplication.

@djc
Copy link
Member Author

djc commented Sep 5, 2023

Could we do the sub_ca_count as part of Budget, too? I suppose that needs something more advanced because we need to reincrement the chain length as we bubble out of the chain...

Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

I went back and forth in my head a little bit about whether CRL signature validations should count against the budget.

Initially I was thinking they shouldn't, since the set of potentially consulted CRLs isn't an attacker controlled input. In a perfect world it would be a fixed number of CRL validation signature checks based on the number of configured CRLs, and not influenced by the certificates found in path building, but we know that's not the case today (#18). Applying a budget seems reasonable with that in mind.

In either case since this is a hard error for path building it doesn't seem as though it would introduce any security risk. E.g. its not as though path building would continue without revocation status checking if the budget were expended.

Does that reasoning match up with what you were thinking?

@djc
Copy link
Member Author

djc commented Sep 5, 2023

For me, the main idea is that signed_data::verify_signed_data() should be the function that takes a &mut Budget. That way, you can't forget to consume budget in a function that calls verify_signed_data(). I figured that the amount of calls originating from CRL signature verification is probably relatively limited compared to the number of calls for certificate verification, such that having to pass Budget from CertRevocationList::verify_signature() is sort of just collateral damage.

@ctz ctz added this pull request to the merge queue Sep 6, 2023
Merged via the queue into main with commit 7e38e97 Sep 6, 2023
45 of 46 checks passed
@ctz ctz deleted the budget-tweaks branch September 6, 2023 13:58
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.

None yet

3 participants