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

Ability to optionally provide a custom key for nsupdate #17

Merged
merged 3 commits into from
Nov 14, 2020

Conversation

desaster
Copy link
Contributor

This PR implements the feature requested in the issue #13.

New option --nskeyfile can be used to provide a pre-generated keyfile, which will be then passed to nsupdate using the -k argument.

-k keyfile
   The file containing the TSIG authentication key. Keyfiles may be in two formats: a
   single file containing a named.conf-format key statement, which may be generated
   automatically by ddns-confgen, or a pair of files whose names are of the format
   K{name}.+157.+{random}.key and K{name}.+157.+{random}.private, which can be generated
   by dnssec-keygen. The -k may also be used to specify a SIG(0) key used to authenticate
   Dynamic DNS update requests. In this case, the key specified is not an HMAC-MD5 key.

Open for suggestions, changes or a complete takeover of the PR.

This to consider:

  • Is the documentation clear? I consider custom keyfile to be an important feature, but I'm not sure if the users understand when or why to use it.
  • I named the argument --nskeyfile with no shorthand, since -k is already taken.
  • I may have forgotten some details about this, since I originally wrote the issue a year ago. Could probably use some testing.

This feature allows for having a pre-generated keyfile, which can be
used to overcome permission issues regarding the auto-generated keyfile.
@TheJJ TheJJ added the new thing A new feature that was not there before. label Oct 28, 2020
Copy link
Member

@TheJJ TheJJ left a comment

Choose a reason for hiding this comment

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

Thank you :)
The documentation is alright!

In general, I think we should rather pass the keyfile as argument in the sftdyn config file, where we can customize the nsupdate command anyway. Then one does not need to modify the systemd unit and invocation, but instead just write it in the sftdyn config. What are your thoughts on that?

Instead of explaining how to set the keyfile as a command line parameter,
use the configuration file instead for a simpler setup.
@desaster
Copy link
Contributor Author

It turns out the nskeyfile option was already usable from the conf file, but I had forgotten about it. I changed the documentation to use the config instead of overriding the systemd unit.

@desaster desaster requested a review from TheJJ November 11, 2020 20:19
Copy link
Member

@TheJJ TheJJ left a comment

Choose a reason for hiding this comment

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

great!

@TheJJ
Copy link
Member

TheJJ commented Nov 13, 2020

Unless you have any additions, we can merge it. Maybe, as a future extension, we can allow to specify arbitrary additional command line options in the config file.

@desaster
Copy link
Contributor Author

I have nothing else planned.

Yes, I suppose the whole feature could be replaced by a setting that lets you specify nsupdate parameters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new thing A new feature that was not there before.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants