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

Support TsigProvider for zone transfers #1311

Closed
wants to merge 1 commit into from

Conversation

tmthrgd
Copy link
Collaborator

@tmthrgd tmthrgd commented Oct 30, 2021

This adds TsigProvider support to Transfer.

I've not added any tests as the existing XFR TSIG code doesn't have any tests and I can't say I understand this code well enough to write any tests myself.

Fixes #1300

/cc @dmavrommatis

@tmthrgd tmthrgd requested a review from miekg October 30, 2021 03:14
@tmthrgd
Copy link
Collaborator Author

tmthrgd commented Oct 30, 2021

@dmavrommatis can you check if this fixes your problem?

@dmavrommatis
Copy link
Contributor

I remember when I was looking at this that there were some issues with this approach.
Without the provider when you create a secret you also pass the t.Hdr.Name field to get the algorithm while with the tsigProvider you don't. I can't recall exactly what was the issue though.

@tmthrgd
Copy link
Collaborator Author

tmthrgd commented Nov 2, 2021

It's slightly cumbersome, but you can get access to t.Hdr.Name from within TsigProvider. The first argument msg is the exact byte-slice we parsed initially. You could safely do something like this:

m := new(dns.Msg)
m.Unpack(msg) // error is always nil
name := m.IsTsig().Hdr.Name // IsTsig always returns non-nil

Considering the only way you can get there is to have Unpack succeed and IsTsig() return non-nil, you don't even need to check the error or return values.

@dmavrommatis
Copy link
Contributor

Should be good then. Are you going to do the same for the DNS server to support the TsigProvider? https://github.com/miekg/dns/blob/master/server.go#L715

@tmthrgd
Copy link
Collaborator Author

tmthrgd commented Nov 2, 2021

@dmavrommatis That would make sense. I'll do that in another PR.

@Fattouche
Copy link
Contributor

What is the status of this being merged? I didn't realize that it already existed and I ended up making my own PR to solve this problem. #1323.

@tmthrgd
Copy link
Collaborator Author

tmthrgd commented Jan 25, 2022

I'm withdrawing this in favour of a much nicer implementation.

@tmthrgd tmthrgd closed this Jan 25, 2022
@tmthrgd tmthrgd deleted the xfr-tsig-provider branch January 25, 2022 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

XFR is not using the TsigProvider
3 participants