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

TSIG Verify/Generate using TsigProvider #1379

Closed
wants to merge 1 commit into from
Closed

Conversation

jelu
Copy link

@jelu jelu commented Jun 9, 2022

By exposing TsigVerifyProvider and TsigGenerateProvider functions I am able to fully implement TSIG support in DNS-OARC/golang-dns-server-doq similar to how it's done in dns.Server and without needed to copy almost all of tsig.go.

I see no harm in exposing these functions and maybe others will find them useful also, hope it can be merged.

- `tsig`: Expose `TsigVerifyProvider` and `TsigGenerateProvider` so that others can use these TSIG functions using a `TsigProvider`
@jelu jelu requested review from miekg and tmthrgd as code owners June 9, 2022 09:35
@miekg
Copy link
Owner

miekg commented Jun 10, 2022

I can't find a good reason to say 'no', so likely to be merged.

as the function docs are now public, some comments there.

1 similar comment
@miekg
Copy link
Owner

miekg commented Jun 10, 2022

I can't find a good reason to say 'no', so likely to be merged.

as the function docs are now public, some comments there.

@@ -223,7 +225,9 @@ func TsigVerify(msg []byte, secret, requestMAC string, timersOnly bool) error {
return tsigVerify(msg, tsigHMACProvider(secret), requestMAC, timersOnly, uint64(time.Now().Unix()))
}

func tsigVerifyProvider(msg []byte, provider TsigProvider, requestMAC string, timersOnly bool) error {
// TsigVerify verifies the TSIG on a message using a TsigProvider, for
// more details and return see TsigVerify.
Copy link
Owner

Choose a reason for hiding this comment

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

this comments isn't finished. 'for more details ... ?'

Copy link
Author

Choose a reason for hiding this comment

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

It says see TsigVerify(?)

}

func tsigGenerateProvider(m *Msg, provider TsigProvider, requestMAC string, timersOnly bool) ([]byte, string, error) {
// TsigGenerate fills out the TSIG record attached to the message using
// a TsigProvider, for more details and return see TsigGenerate.
Copy link
Owner

Choose a reason for hiding this comment

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

also unfinished docs

Copy link
Author

Choose a reason for hiding this comment

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

same here

@@ -166,10 +166,12 @@ type timerWireFmt struct {
// timersOnly is false.
// If something goes wrong an error is returned, otherwise it is nil.
func TsigGenerate(m *Msg, secret, requestMAC string, timersOnly bool) ([]byte, string, error) {
Copy link
Owner

Choose a reason for hiding this comment

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

on the docs - it's weirdly formatted, also Tsig RR -> TSIG RR..

"... called for the first time requestMAC should be set to the empty string and timersOnly should be false.":

would also be nice to tell what subsequent calls should look like,.

The "if something goes wrong an error is returned..." can be removed IMO

Copy link
Author

Choose a reason for hiding this comment

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

I have made no change in the documentation for that function.

@jelu
Copy link
Author

jelu commented Jun 10, 2022

The return of the functions are the same, do you really want duplicated documentation?

@jelu jelu requested a review from miekg June 13, 2022 08:36
@jelu
Copy link
Author

jelu commented Jun 14, 2022

Could anyone please let me know what kind of timeline I should expect for getting this merged and into a release?

Just want to know so I can plan the release of my library that depends on this PR.

@jelu
Copy link
Author

jelu commented Jun 20, 2022

The return of the functions are the same, do you really want duplicated documentation?

@miekg Do you really want duplicated documentation?

@miekg
Copy link
Owner

miekg commented Jun 20, 2022

this lgtm although WithProvider would be more Go-like and we already have some other 'With...' functions.

I'll close this and send in a new PR with that name in it and also adjust any docs

@miekg miekg closed this Jun 20, 2022
@jelu
Copy link
Author

jelu commented Jun 20, 2022 via email

@miekg
Copy link
Owner

miekg commented Jun 20, 2022

#1382

please double check for anything stupid

@miekg
Copy link
Owner

miekg commented Oct 11, 2022 via email

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.

None yet

2 participants