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 Server and Transfer #1331

Merged
merged 4 commits into from Feb 5, 2022

Conversation

tmthrgd
Copy link
Collaborator

@tmthrgd tmthrgd commented Jan 25, 2022

This adds support for TsigProvider to both Server and Transfer. It also simplifies the implementation for Client and Conn.

Not sure why I didn't think to do this earlier. A miekg/dns/v2 could just push the TsigSecret code out into the callers.

This also has the advantage that it doesn't add a bunch of extra code paths throughout, so it's safer to land.

Fixes #1300
Closes #1323
Fixes #1325

@tmthrgd
Copy link
Collaborator Author

tmthrgd commented Jan 25, 2022

/cc @Fattouche

This is largely copied from github.com/miekg/pull/1323 by
@Fattouche.
server.go Show resolved Hide resolved
@@ -74,6 +74,24 @@ func (key tsigHMACProvider) Verify(msg []byte, t *TSIG) error {
return nil
}

type tsigSecretProvider map[string]string
Copy link
Contributor

Choose a reason for hiding this comment

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

This is good for a default TsigProvider, however it still doesn't solve the problem described in

#1325 (comment)

We need some way for Generate to be able to unwrap the Message so that we can see the query name. This is specifically useful in situations where multiple tsigs have the same name, but exist under a different zone.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That can be handled either in a separate PR or externally, but yes we do need to do something.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How common a use case is that do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say it's fairly common. I ran into this problem early on with this library when trying to serve multiple zones that had different primaries, therefore different TSIGs on each primary meaning they could not use the TSIG name as the zone Name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @tmthrgd have you thought any more about how we can best obtain the query name from the Generate function. I think having the opposite of TsigBuffer available in a function would make this possible. Alternatively we could consider passing in a context? It would be useful to have contexts everywhere anyway so that we could pass data around without needing to alter signatures.

Copy link
Owner

@miekg miekg left a comment

Choose a reason for hiding this comment

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

lgtm

@tmthrgd
Copy link
Collaborator Author

tmthrgd commented Feb 5, 2022

/merge

@cbot cbot bot merged commit 33e6400 into miekg:master Feb 5, 2022
@tmthrgd tmthrgd deleted the tsig-provider branch February 5, 2022 00:23
aanm pushed a commit to cilium/dns that referenced this pull request Jul 29, 2022
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.

TSIG names cannot be reused across different zones. XFR is not using the TsigProvider
3 participants