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

DDNS RFC2136 algorithm choice #3893

Merged

Conversation

JoeriCapens
Copy link
Contributor

@JoeriCapens JoeriCapens commented Dec 28, 2017

Add Dynamic DNS RFC 2136 Client server key algorithm choice.

The first commit (7ca8845) will change the key files used by pfSense to the ddns-confgen format which is much simpler than the dnssec-keygen format:

  • it only needs one file (instead of public and private key files with different syntax)
  • it does not require the Key Type (zone, host or user) so that choice is removed from the web interface
  • it allows simple algorithm choice (e.g. "hmac-md5" instead of "157")

The second commit (16c260f) is just a small cleanup.

The third commit (ab1112e) replaces the fixed hmac-md5 algorithm with a variable choice. This is comparable to https://redmine.pfsense.org/issues/6621

The last commit (0ccfd70) removes old .key and .secret files from disk during an upgrade. Is this correctly implemented? It works for me when I reboot.

@jim-p jim-p self-requested a review December 29, 2017 13:24
Copy link
Contributor

@jim-p jim-p left a comment

Choose a reason for hiding this comment

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

Looks good, needs a couple small adjustments and then it can be tested

'keyalgorithm',
'*Key algorithm',
$pconfig['keyalgorithm'],
array(
Copy link
Contributor

Choose a reason for hiding this comment

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

You will need to add some input validation to ensure that the submitted value for keyalgorithm is one from this list, which means it's also easier to define this array once at the start of the file and then use the array to validate and also to build this list.

* Delete old Dynamic DNS dnssec-keygen style files,
* they are replaced with a simpler single ddns-confgen style file.
*/
function upgrade_173_to_174() {
Copy link
Contributor

Choose a reason for hiding this comment

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

As long as you're doing upgrade code, you may as well unset the keytype field on the rfc2136 entries since it was removed.

@JoeriCapens
Copy link
Contributor Author

Thanks for the feedback!

I found out that the cleanup of the old keyfiles is actually not needed at all; the /var/etc directory is cleared at each reboot (by the /etc/pfSense-rc script). So I removed that part and added the code to unset the keytype field from the main config (commit 16f8df9).

I added the input validation as suggested and cleaned up a bit of weird coding (commit c0305bb).

The DHCP pages from #3887 could also use this input validation. What is the best way to handle this in GitHub? Do I create a new branch and a new pull request or can I add it to the old branch / pull request or perhaps this one?

@jim-p
Copy link
Contributor

jim-p commented Jan 4, 2018

The input validation for the DHCP pages should be in a separate branch/PR, you should be able to make a new branch of your own from your existing fork, make changes, and make a new PR there.

@jim-p
Copy link
Contributor

jim-p commented Jan 4, 2018

This looks good now, I made an hmac-sha512 key and tried it out and it worked, and my existing hostname can still use its old hmac-md5 key.

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants