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

[chrony] Allow symmetric NTP authentication. #3824

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

B3K7
Copy link

@B3K7 B3K7 commented Feb 18, 2024

Candidate solution for [Feature request] Chrony Security - chrony.keys #3823

@B3K7
Copy link
Author

B3K7 commented Feb 18, 2024

If possible, please consult with [email protected].

@@ -1,7 +1,7 @@
<model>
<mount>//OPNsense/chrony/general</mount>
<description>Chrony configuration</description>
<version>0.0.2</version>
<version>0.0.3</version>
Copy link
Author

Choose a reason for hiding this comment

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

Bumped to 0.0.3 on account of the append-only schema change.

{% if not helpers.empty('OPNsense.chrony.general.peers') %}
{% for peer in OPNsense.chrony.general.peers.split(',') %}
server {{ peer }} iburst {% if helpers.exists('OPNsense.chrony.general.ntsclient') and OPNsense.chrony.general.ntsclient == '1' %}nts{% endif %}

Copy link
Author

Choose a reason for hiding this comment

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

Removed a line..

Copy link
Author

Choose a reason for hiding this comment

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

Added a new template. chrony.keys

@@ -15,13 +15,26 @@ nosystemcert
nocerttimecheck 1
{% endif %}

{% if helpers.exists('OPNsense.chrony.general.symmetric_auth') and OPNsense.chrony.general.symmetric_auth == '1' %}
{% if helpers.exists('OPNsense.chrony.general.symmetric_keys_file') and not helpers.empty('OPNsense.chrony.general.symmetric_keys_file') %}
keyfile /usr/local/etc/chrony.keys
Copy link
Author

Choose a reason for hiding this comment

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

Please verify that /usr/local/etc/chrony.keys is indeed the correct path.

# Don't use example keys! It's recommended to generate random keys using
# the chronyc keygen command.

{{ OPNsense.chrony.general.symmetric_keys_file }}
Copy link
Author

Choose a reason for hiding this comment

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

Dump the multi-line text widget as-is.

Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid this goes against our coding guidelines... https://docs.opnsense.org/development/guidelines/basics.html#safeguard-user-input

What is the problem scope? Writing the file manually will overwrite it later or just convenience to store the value in the GUI?

Copy link
Author

Choose a reason for hiding this comment

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

NTPD does the same thing. What's the best way to make the chrony.keys file persistent? Is there away to bypass the PHP UI?

Copy link
Author

Choose a reason for hiding this comment

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

I want the chrony.keys file to be created each time the router boots up. (Preferably WORM - write once.)

Copy link
Author

Choose a reason for hiding this comment

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

Does the NTPD widget violate the coding guidelines?
image

Copy link
Author

Choose a reason for hiding this comment

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

@fichtner. Is the ask to re-write chrony like services_ntpd.php? (It seems like overkill. Likewise, there is a risk of creating a tarpit.)

Copy link
Member

Choose a reason for hiding this comment

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

Let’s slow down a bit. NTPd is not the scope a here and we don’t accept static PHP submission anymore anyway.

I’m going to reiterate:

Config blobs into files are discouraged, because they cannot be validated. You can split up the content in a model in these cases.

what is odder a that /etc/ntpd.keys is a file in use by ntpd, but not chrony? Is that correct? If it’s also used by chrony i’d say it’s problematic as well for other reasons.

Copy link
Member

Choose a reason for hiding this comment

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

Ok I misread the second part. No file overlap is good then.

{% endfor %}
{% endif %}

{% if helpers.exists('OPNsense.chrony.general.symmetric_auth') and OPNsense.chrony.general.symmetric_auth == '1' %}
{% if helpers.exists('OPNsense.chrony.general.auth_servers') and not helpers.empty('OPNsense.chrony.general.auth_servers') %}
{{ OPNsense.chrony.general.auth_servers }}
Copy link
Author

Choose a reason for hiding this comment

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

Dump the multi-line text widget as-is.

Copy link
Author

Choose a reason for hiding this comment

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

Elected to make the schema changes optional.

@B3K7
Copy link
Author

B3K7 commented Feb 19, 2024

@mimugmail. Michael, when you have time can you please review and comment? Thanks!

@fichtner fichtner self-assigned this Feb 19, 2024
@fichtner
Copy link
Member

@B3K7 re: irc

Ok now I got it ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants