Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1298 closed defect (fixed)

don't do xfrin upon notify when address is unknown

Reported by: jelte Owned by: jelte
Priority: medium Milestone: Sprint-20111122
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 4
Total Hours: 1.18 Internal?: no

Description

right now, if xfrin gets a notify command (signaling that a notify packet has been received), it'll look up the master address and perform transfer. We should probably check the address the notify came from, and only do the transfer if it matches the address in the configuration.

This has the side-effect of making the notify-self problem (#965) less bad, as it will still notify, but won't act upon it (this is probably also the cause of #988)

It is also more as specified in the RFC (except we can only configure one master address right now)

Subtickets

Change History (21)

comment:1 follow-up: Changed 8 years ago by jelte

  • Owner set to UnAssigned
  • Status changed from new to reviewing

the full master address (including address, inet family, port) is kind of an ad-hoc structure, which is completely checked, but the log message only mentions the address itself. So this code isn't perfect, but depending on whether we want to sneak this into the release we can make it better or make that a todo

comment:2 follow-up: Changed 8 years ago by jreed

It appears to work, but the error message happened 29 seconds later. Why delayed?

2011-10-12 11:07:45.559 DEBUG [b10-auth.auth] AUTH_PACKET_RECEIVED message received:
;; ->>HEADER<<- opcode: NOTIFY, status: NOERROR, id: 22742
...

2011-10-12 11:07:45.560 DEBUG [b10-auth.cc] CC_GROUP_SEND sending message '{ "command": [ "notify", { "master": "192.168.1.2", "zone_class": "IN", "zone_name": "foo." } ] }' to group 'Zonemgr'

2011-10-12 11:07:45.560 DEBUG [b10-zonemgr.zonemgr] ZONEMGR_RECEIVE_NOTIFY received NOTIFY command for zone foo. (class IN)

2011-10-12 11:07:45.565 DEBUG [b10-zonemgr.zonemgr] ZONEMGR_RECEIVE_XFRIN_FAILED received XFRIN FAILED command for zone foo. (class IN)

2011-10-12 11:08:14.170 DEBUG [b10-zonemgr.zonemgr] ZONEMGR_REFRESH_ZONE refreshing zone foo. (class IN)

2011-10-12 11:08:14.173 ERROR [b10-xfrin.xfrin] XFRIN_NOTIFY_UNKNOWN_MASTER got notification to retransfer zone foo. from 192.168.1.2, expected 192.168.1.4

Also ERROR probably should not be used for issues out of our control. Maybe WARN?

Also will this work with a list of acceptable master addresses?

comment:3 in reply to: ↑ 1 ; follow-up: Changed 8 years ago by jinmei

Replying to jelte:

the full master address (including address, inet family, port) is kind of an ad-hoc structure, which is completely checked, but the log message only mentions the address itself. So this code isn't perfect, but depending on whether we want to sneak this into the release we can make it better or make that a todo

I've reviewed the code, and while I personally didn't think it sufficiently
urgent for a last minute push I merged the current snapshot of the branch
(with some editorial fixes) to master and pushed it as Jeremy suggested so.

To fully complete this task I'd like to discuss some details, so I'll
keep this ticket open. I've not deleted the branch yet. I also guess
we should defer this task to the next sprint unless we consider it an
urgent fix.

Now the review comments:

  • I guess we need a changelog entry for this change.
  • We generally use the format of <zone_name>/<zone_rr_class> for logging in xfrin. I suggest revising the format for XFRIN_NOTIFY_UNKNOWN_MASTER in this sense. We also might want to extract XfrinConnection?.zone_str() so that it can be used module-wise.
  • As you also seem to have noticed, I'm not sure if it makes sense to compare two tuples of address information:
                        if notify_addr == master_addr:
    
    Note also that we should probably consider the case where the address matches but the ports don't.
  • I agree with Jeremy about the log level for XFRIN_NOTIFY_UNKNOWN_MASTER. I'd even use info in such cases. FWIW BIND 9 uses the info level in this situation:
    	} else if (i >= zone->masterscnt) {
    		UNLOCK_ZONE(zone);
    		dns_zone_log(zone, ISC_LOG_INFO,
    			     "refused notify from non-master: %s", fromtext);
    		inc_stats(zone, dns_zonestatscounter_notifyrej);
    		return (DNS_R_REFUSED);
    	}
    
  • With this change, the last two assertEqual in test_command_handler_notify_known_zone don't make sense any more (so I commented them out)
    #         self.assertEqual(TEST_MASTER_IPV4_ADDRESS,
    #                          self.xfr.xfrin_started_master_addr)
    #         self.assertEqual(int(TEST_MASTER_PORT),
    #                          self.xfr.xfrin_started_master_port)
    
    For a longer term we'll allow to have a list of master addresses, and select one of the listed addresses for the source of xfr, preferring the notify source. At that point we'll probably need a completely different types of tests, so, as a result, I suspect we should simply remove these commented-out tests right now.

comment:4 in reply to: ↑ 2 Changed 8 years ago by jinmei

Replying to jreed:

It appears to work, but the error message happened 29 seconds later. Why delayed?

It's due to #1299. For now we can ignore it. When we complete #1299
it will be solved.

comment:5 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jelte

comment:6 Changed 8 years ago by shane

  • Milestone changed from New Tasks to Sprint-20111025

comment:7 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111025 to Sprint-20111108

comment:8 in reply to: ↑ 3 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

To fully complete this task I'd like to discuss some details, so I'll
keep this ticket open. I've not deleted the branch yet. I also guess
we should defer this task to the next sprint unless we consider it an
urgent fix.

This has been dormant in my queue for quite some time now, but this is
as good a time to handle it as any, I guess.

Now the review comments:

  • I guess we need a changelog entry for this change.

Hmm, right, bit late though :/

Proposal:
[func?] Check master address upon NOTIFY
b10-xfrin now checks the address a NOTIFY packet came from, and only
performs xfrin when the address and port match those from the master
address in the xfrin configuration.

But perhaps I need to discuss with jeremy on how to phrase this, as the
functionality really was already in the previous release

  • We generally use the format of <zone_name>/<zone_rr_class> for logging in xfrin. I suggest revising the format for XFRIN_NOTIFY_UNKNOWN_MASTER in this sense. We also might want to extract XfrinConnection?.zone_str() so that it can be used module-wise.

Ack, added a format_zone_str() module-level convenience function. Also
did the same for addresses.

I think we should make these general utility functions, but esp. for
the second we need to look at what input data we have available in
other modules first, so i'll defer this to another ticket (for which
we can use this code as a base, if this is acked and merged)

  • As you also seem to have noticed, I'm not sure if it makes sense to compare two tuples of address information:
                        if notify_addr == master_addr:
    
    Note also that we should probably consider the case where the address matches but the ports don't.

one thing that certainly didn't make sense is comparing the ports as
well, but only reporting the address on error, but the above change
with the format strings should at least fix the disconnect.

  • I agree with Jeremy about the log level for XFRIN_NOTIFY_UNKNOWN_MASTER. I'd even use info in such cases. FWIW BIND 9 uses the info level in this situation:
    	} else if (i >= zone->masterscnt) {
    		UNLOCK_ZONE(zone);
    		dns_zone_log(zone, ISC_LOG_INFO,
    			     "refused notify from non-master: %s", fromtext);
    		inc_stats(zone, dns_zonestatscounter_notifyrej);
    		return (DNS_R_REFUSED);
    	}
    

ok, changed to INFO

  • With this change, the last two assertEqual in test_command_handler_notify_known_zone don't make sense any more (so I commented them out)
    #         self.assertEqual(TEST_MASTER_IPV4_ADDRESS,
    #                          self.xfr.xfrin_started_master_addr)
    #         self.assertEqual(int(TEST_MASTER_PORT),
    #                          self.xfr.xfrin_started_master_port)
    
    For a longer term we'll allow to have a list of master addresses, and select one of the listed addresses for the source of xfr, preferring the notify source. At that point we'll probably need a completely different types of tests, so, as a result, I suspect we should simply remove these commented-out tests right now.

Yes. Removed them.

Regarding multiple addresses, ack, we will need that at some point.
I think there are more configuration layout changes we need or want,
perhaps we should discuss and/or propose those in a separate (set of)
ticket(s).

comment:9 in reply to: ↑ 8 ; follow-up: Changed 8 years ago by jinmei

Replying to jelte:

Proposal:
[func?] Check master address upon NOTIFY
b10-xfrin now checks the address a NOTIFY packet came from, and only
performs xfrin when the address and port match those from the master
address in the xfrin configuration.

But perhaps I need to discuss with jeremy on how to phrase this, as the
functionality really was already in the previous release

We could say something like "the main fix appeared in release XXX, but
there was deferred cleanups". But I'd leave it to you and Jeremy.

As for the category, I'd say it's [bug], and explain what was wrong
before (more clearly).

  • As you also seem to have noticed, I'm not sure if it makes sense to compare two tuples of address information:
                        if notify_addr == master_addr:
    
    Note also that we should probably consider the case where the address matches but the ports don't.

one thing that certainly didn't make sense is comparing the ports as
well, but only reporting the address on error, but the above change
with the format strings should at least fix the disconnect.

I'm still not sure if tuple match is the best way. For example, if
and when we support IXFR/UDP, comparing socktype will cause confusion.
But maybe it's too minor and doesn't matter in practice, so I'd leave
it to you.

Some other comments:

  • I fixed a minor error and made an editorial change. (btw without the fix it didn't pass tests)
  • format_zone_str() looks good, but now I have another suggestion: maybe it looks nicer if we omit the final dot in the zone name, i.e., instead of 'example.com./IN', 'example.com/IN'. (but apparently we need to update the isc.dns wrapper so that it accepts the optional argument for Name.to_text()).
  • I think we need tests for format_zone_str() and format_addrinfo(), especially for the latter due to its complexity.
  • For the same reason for UNKNOWN_MASTER, I'd use 'info' (not error) for XFRIN_RETRANSFER_UNKNOWN_ZONE.
  • the number of arguments doesn't match between the message and code:
                            logger.info(XFRIN_NOTIFY_UNKNOWN_MASTER, zone_str,
                                        notify_addr_str, master_addr_str)
    
    % XFRIN_NOTIFY_UNKNOWN_MASTER got notification to retransfer zone %1 from %2/%3, expected %4/%5
    

comment:10 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:11 in reply to: ↑ 9 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

Replying to jelte:

Proposal:
[func?] Check master address upon NOTIFY
b10-xfrin now checks the address a NOTIFY packet came from, and only
performs xfrin when the address and port match those from the master
address in the xfrin configuration.

But perhaps I need to discuss with jeremy on how to phrase this, as the
functionality really was already in the previous release

We could say something like "the main fix appeared in release XXX, but
there was deferred cleanups". But I'd leave it to you and Jeremy.

As for the category, I'd say it's [bug], and explain what was wrong
before (more clearly).

[bug] jelte
b10-xfrin would previously initiate incoming transfers upon receiving
NOTIFY packets from any address (if the zone was known to b10-xfrin).
It now only starts a transfer if the source address from the NOTIFY
packet matches the configured master address and port. This was really
already fixed in release bind10-devel-20111014, but there were some
deferred cleanups to add.

  • As you also seem to have noticed, I'm not sure if it makes sense to compare two tuples of address information:
                        if notify_addr == master_addr:
    
    Note also that we should probably consider the case where the address matches but the ports don't.

one thing that certainly didn't make sense is comparing the ports as
well, but only reporting the address on error, but the above change
with the format strings should at least fix the disconnect.

I'm still not sure if tuple match is the best way. For example, if
and when we support IXFR/UDP, comparing socktype will cause confusion.
But maybe it's too minor and doesn't matter in practice, so I'd leave
it to you.

ack, only comparing family, addr and port now. BTW this seems more
like a big hint that we are using bad datastructures here, but anyway.

Some other comments:

  • I fixed a minor error and made an editorial change. (btw without the fix it didn't pass tests)

ohright, how did that get in. Thanks.

  • format_zone_str() looks good, but now I have another suggestion: maybe it looks nicer if we omit the final dot in the zone name, i.e., instead of 'example.com./IN', 'example.com/IN'. (but apparently we need to update the isc.dns wrapper so that it accepts the optional argument for Name.to_text()).

good idea. Added wrapper support (in a separate commit,
23cfc5b4d9b384172d0eadd2269ed6a6121966a8)

  • I think we need tests for format_zone_str() and format_addrinfo(), especially for the latter due to its complexity.

added some.

  • For the same reason for UNKNOWN_MASTER, I'd use 'info' (not error) for XFRIN_RETRANSFER_UNKNOWN_ZONE.

oh right, done

  • the number of arguments doesn't match between the message and code:
                            logger.info(XFRIN_NOTIFY_UNKNOWN_MASTER, zone_str,
                                        notify_addr_str, master_addr_str)
    
    % XFRIN_NOTIFY_UNKNOWN_MASTER got notification to retransfer zone %1 from %2/%3, expected %4/%5
    

ah, doh, forgot to update after deciding to go ahead with the format
functions. Fixed. (yes we still need that checking script :p)

comment:12 in reply to: ↑ 11 ; follow-up: Changed 8 years ago by jinmei

Replying to jelte:

[bug] jelte
b10-xfrin would previously initiate incoming transfers upon receiving
NOTIFY packets from any address (if the zone was known to b10-xfrin).

  • s/packets/messages/
  • I'd say: "...was known to b10-xfrin, and using the configured master address)".

It now only starts a transfer if the source address from the NOTIFY
packet matches the configured master address and port. This was really

s/packet/message/

already fixed in release bind10-devel-20111014, but there were some
deferred cleanups to add.

ack, only comparing family, addr and port now. BTW this seems more
like a big hint that we are using bad datastructures here, but anyway.

Perhaps. At least we should have a consistent style for comparing
addresses in this type of context. But that will be beyond the scope
of this ticket.

Other comments:

  • I've made a couple of fixes and pushed them. Both minor, but the first one was necessary to compile with clang.
  • format_addrinfo: it doesn't check invalid combination of family address like this: format_addrinfo((socket.AF_INET, None, ("::1", 54))) (not sure if we want to catch them in this function though)
  • test_format_addrinfo: mostly a matter of taste in this context, but I'd use 2001:db8::XX for testing instead of ::1 or ::2 (especially the latter)
  • test_format_addrinfo: I think it's better to use more typical value for addr[1] (i.e. 'sockettype'), so instead of
            self.assertEqual("192.0.2.1:53",
                             format_addrinfo((socket.AF_INET, None,
                                              ("192.0.2.1", 53))))
    
    do this:
            self.assertEqual("192.0.2.1:53",
                             format_addrinfo((socket.AF_INET, socket.SOCK_STREAM,
                                              ("192.0.2.1", 53))))
    
    (it may still make sense to use a bogus value, too, to see if it's really ignored)

comment:13 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:14 in reply to: ↑ 12 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

Replying to jelte:

  • I've made a couple of fixes and pushed them. Both minor, but the first one was necessary to compile with clang.

ok, thanks

  • format_addrinfo: it doesn't check invalid combination of family address like this: format_addrinfo((socket.AF_INET, None, ("::1", 54))) (not sure if we want to catch them in this function though)

not sure either. It would feel a bit too much to me (what would we do?
raise an error? if so, why? Would only be something more the caller
would have to take into account. And if we add code that recognizes
valid addresses for families, we wouldn't even need the address family
in the argument in the first place (except then perhaps for said
checking). So for now let's keep it bogus-in-bogus-out. Perhaps we can
reconsider in #1379.

  • test_format_addrinfo: mostly a matter of taste in this context, but I'd use 2001:db8::XX for testing instead of ::1 or ::2 (especially the latter)

oh sure, updated

  • test_format_addrinfo: I think it's better to use more typical value for addr[1] (i.e. 'sockettype'), so instead of
            self.assertEqual("192.0.2.1:53",
                             format_addrinfo((socket.AF_INET, None,
                                              ("192.0.2.1", 53))))
    
    do this:
            self.assertEqual("192.0.2.1:53",
                             format_addrinfo((socket.AF_INET, socket.SOCK_STREAM,
                                              ("192.0.2.1", 53))))
    
    (it may still make sense to use a bogus value, too, to see if it's really ignored)

ok, done

comment:15 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111108 to Sprint-20111122

comment:16 in reply to: ↑ 14 Changed 8 years ago by jinmei

Replying to jelte:

  • format_addrinfo: it doesn't check invalid combination of family address like this: format_addrinfo((socket.AF_INET, None, ("::1", 54))) (not sure if we want to catch them in this function though)

not sure either. It would feel a bit too much to me (what would we do?
raise an error? if so, why? Would only be something more the caller
would have to take into account. And if we add code that recognizes
valid addresses for families, we wouldn't even need the address family
in the argument in the first place (except then perhaps for said
checking). So for now let's keep it bogus-in-bogus-out. Perhaps we can
reconsider in #1379.

Works for me.

Other changes are okay. I think this is ready for merge.

comment:17 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:18 Changed 8 years ago by jinmei

  • Total Hours changed from 0 to 1:00

comment:19 Changed 8 years ago by jelte

  • Add Hours to Ticket changed from 0 to 4
  • Resolution set to fixed
  • Status changed from reviewing to closed

Thanks, merged, closing ticket.

comment:20 Changed 8 years ago by jinmei

  • Total Hours changed from 1:00 to 2.18

comment:21 Changed 8 years ago by jinmei

  • Total Hours changed from 2.18 to 1.18
Note: See TracTickets for help on using tickets.