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
NTP authentication Feature #8794 #4658
base: master
Are you sure you want to change the base?
NTP authentication Feature #8794 #4658
Conversation
MatthewA1
commented
Dec 1, 2023
- Redmine Issue: https://redmine.pfsense.org/issues/8794
- Ready for review
Implements #8794 Adds NTP Key ID field to NTP Services page Adds NTP Authentication status to NTP Status page
This is my first time contribution to pfSense, so please do thoroughly review. I applied these patches to a pfSense CE 2.7.1 instance and tested with an authenticated NTP server using MD5 and everything seemed to work. |
I had to strip off /src for pfSense plus tested with 23.05.01 |
There are multiple issues around NTP which need to be resolved. I think the page needs some refactoring to fix those issues and to better implement authentication. I've added notes on the redmine regarding the issues. As for this specific MR, I don't think we'll merge this until further consideration to the issues in the redmine is given, as well as implementing server-specific keys. Still, here's a patch with some revisions to the MR (note that I have not tested this myself).
|
@marcos-ng your patch worked. I also just attempted the patch with commit id in it named "Incorporated changes from @marcos-ng" that @MatthewA1 added and it would not debug for some reason however. Marcos-ng I was able to just copy paste yours in and it works GUI option appears and it's functional for pfSense plus. Testing with 847e417 This fails when I use the github commit ID however when testing this for fetch patch. |
This is what is pulled with the commit id
|
This is Marcos patch that worked
|
Side Note: Original https://redmine.pfsense.org/users/38059 Lamaz patch here.
Second part
|
… used only for specific NTP servers
@marcos-ng Could I get some feedback on this PR? It still has the changes requested label applied, but I think I've made the requested changes for the minimum viable feature. If additional changes are still needed, or there is someone else I need to mention, please let me know. |