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

Fixed Key Record support based on TLS client certificates #1196

Merged
merged 17 commits into from
Nov 23, 2023

Conversation

folkertdev
Copy link
Contributor

No description provided.

Copy link

codecov bot commented Nov 14, 2023

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (97c731c) 83.65% compared to head (cc058e1) 83.81%.

Files Patch % Lines
ntp-proto/src/nts_record.rs 92.77% 12 Missing ⚠️
ntpd/src/daemon/keyexchange.rs 90.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1196      +/-   ##
==========================================
+ Coverage   83.65%   83.81%   +0.16%     
==========================================
  Files          63       63              
  Lines       17457    17669     +212     
==========================================
+ Hits        14604    14810     +206     
- Misses       2853     2859       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@squell squell force-pushed the fixed-key-request branch 2 times, most recently from e588bbc to 23947b8 Compare November 14, 2023 14:58
Base automatically changed from nts-pool-records to main November 16, 2023 09:18
@squell squell force-pushed the fixed-key-request branch 5 times, most recently from 15495c6 to 6d31331 Compare November 20, 2023 14:43
@squell
Copy link
Member

squell commented Nov 20, 2023

There is a bit of churn in some of the commits as test-cases were added and then removed; so I'd recommend squashing this. This PR adds:

  • FixedKeyRequests (scaffolding already present in main)
  • support in the configuration for adding NTS pool server certificates to the NtsKe configuration (authorized-pool-server-certificates = [ "file.pem", "file2.pem", ...]). I'm not particularly wedded to the exact name of "pool server certificate".

Still TODO: SupportedProtocolList support (prefer to do that in a separate PR).

@squell squell marked this pull request as ready for review November 20, 2023 14:51
@squell squell marked this pull request as draft November 20, 2023 16:22
@squell squell marked this pull request as ready for review November 21, 2023 12:01
@squell squell changed the title Fixed Key Record Fixed Key Record support based on TLS client certificates Nov 21, 2023
Copy link
Member

@davidv1992 davidv1992 left a comment

Choose a reason for hiding this comment

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

Mostly looks good, some minor things that still need a feature flag and one style issue.

ntpd/src/daemon/config/server.rs Outdated Show resolved Hide resolved
ntp-proto/src/nts_record.rs Show resolved Hide resolved
ntp-proto/src/nts_record.rs Outdated Show resolved Hide resolved
ntp-proto/src/nts_record.rs Outdated Show resolved Hide resolved
ntpd/src/daemon/keyexchange.rs Outdated Show resolved Hide resolved
@squell squell requested a review from davidv1992 November 23, 2023 13:17
@squell
Copy link
Member

squell commented Nov 23, 2023

Note; this does not actually add client TLS certificate to the live server; that requires implementing an own ClientCertVerifier.

@davidv1992 davidv1992 added this pull request to the merge queue Nov 23, 2023
Merged via the queue into main with commit f281bed Nov 23, 2023
20 checks passed
@davidv1992 davidv1992 deleted the fixed-key-request branch November 23, 2023 14:12
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.

3 participants