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

feat: implement TSIGKey API #151

Merged
merged 7 commits into from
Jun 11, 2024
Merged

Conversation

bennetgallein
Copy link
Contributor

Description

Add APIs for the /tsigkeys route:
https://doc.powerdns.com/authoritative/http-api/tsigkey.html#tsigkey-endpoints

More information about TSIG:
https://doc.powerdns.com/authoritative/tsig.html

Motivation and context

TSIGKeys were not implemented

How has this been tested?

testing was done using the local docker containers

Screenshots (if appropriate)

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

Please, please, please, don't send your pull request until all of the boxes are ticked. Once your pull request is created, it will trigger a build on our continuous integration server to make sure your tests pass.

  • I have read the CONTRIBUTING document.
  • My pull request addresses exactly one patch/feature.
  • My pull request contains a title that can be used as a release note.
  • I have created a branch for this patch/feature.
  • Each individual commit in the pull request is meaningful.
  • I have added tests to cover my changes.
  • If my change requires a change to the documentation, I have updated it accordingly.

If you're unsure about any of these, don't hesitate to ask. We're here to help!

@bennetgallein
Copy link
Contributor Author

I'm really struggeling to understand how the API handles updates, like the PUT request is currently just creating new keys all the time, even when I only send the algorithms field.. I've pushed this to my branch here: https://github.com/bennetgallein/powerdns-php/tree/feat/tsig-keys-debug in case someone can help me out getting this working the right way..

@trizz trizz self-assigned this May 13, 2024
* feat: working on getting this working

* feat: add seperate functions to change the fields

* chore: add tests to cover all functions
@bennetgallein
Copy link
Contributor Author

Hey Tristan! @trizz

I think I've figured it out - there are some quirks with the way the API works that (imo) don't really make sense. I'll list them below to shed some light:

  • Whenever you send the name field, powerdns will clone the key with id and create a copy with the new name, but also giving it a new id, since the id seems to be based on the name (just a dot appended(?)).
  • when changing the algorithm the key doesn't change, but when creating it get's generated.. It kinda makes sense but why would one want to update the algo without regenerating the key? :D - I did not test changing the algo and sending an empty key to see if that would trigger a regenerate tho..

I think that's it - lmk if you have any feedback or questions :)

@trizz trizz added the new-feature New features or options. label Jun 3, 2024
Comment on lines 70 to 82
/**
* Set set to "TSIGKey".
*
* @param string $type Set to "TSIGKey"
*
* @return self
*/
public function setType(string $type)
{
$this->type = $type;

return $this;
}
Copy link
Member

Choose a reason for hiding this comment

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

Should it be possible to change the type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the type is changeable, but seems pretty fixed in the documentation i'll remove the setter :)

@trizz
Copy link
Member

trizz commented Jun 3, 2024

there are some quirks with the way the API works that (imo) don't really make sense

Yup, some things don't make any sense.

Whenever you send the name field, powerdns will clone the key with id and create a copy with the new name, but also giving it a new id, since the id seems to be based on the name

Yes, updating records happens in a similar fashion. When updating a record, PowerDNS deletes and re-adds the whole record. However, because no "real" IDs are returned by the PowerDNS API, this isn't very noticeable. I've tried some of your created methods and they seem to work fine. We don't use TSIG keys ourselves (hence it is missing), so I cannot test with real-world data.

just a dot appended

Probably to get a canonical domain. The same is required for zones, for example.

So, everything looks fine to me. If no other corrections are necessary (see comment on your code), I'll merge and release this feature.

@bennetgallein
Copy link
Contributor Author

We don't use TSIG keys ourselves (hence it is missing), so I cannot test with real-world data.

I wanted to use it in order to generate TSIG keys and the update the zone metadata for TSIG-ALLOW-DNSUPDATE to reflect the key. But that's not forbidden so I got stuck there. I actually don't know how usefull managing TSIGKeys might be, but in my mind it's better to offer that integration instead of people getting stuck when they want to do something with it and the library not supporting it.

@trizz trizz merged commit fd2a74e into exonet:master Jun 11, 2024
1 of 2 checks passed
@bennetgallein bennetgallein deleted the feat/tsig-keys branch August 1, 2024 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature New features or options.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants