Opened 7 years ago

Closed 6 years ago

#772 closed enhancement (complete)

Update xfrout to use ACL checking library

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

Description

Dependent on #770, update the xfrout module to use the ACL checking library to determine if an xfrout should proceed.

Subtickets

Change History (17)

comment:1 Changed 6 years ago by stephen

  • Defect Severity set to N/A
  • Milestone changed from Year 3 Task Backlog to Sprint-20110712
  • Sub-Project set to DNS

comment:2 Changed 6 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 3

comment:3 Changed 6 years ago by vorner

  • Owner set to vorner
  • Status changed from new to accepted

comment:4 Changed 6 years ago by vorner

  • Milestone changed from Sprint-20110802 to Sprint-20110712
  • Owner changed from vorner to UnAssigned
  • Status changed from accepted to reviewing

This should be ready for review. It is based on today snapshot of #983, which should be quite close to completion. There are few minor cleanups in it as well.

It provides the ACL configuration option to put the global ACL for xfrout. There's no per-zone configuration currently (I believe we didn't yet decide where we put per-zone configs, if into each module separately or globally) and it would need the map with any names to implement it, so there's no way to configure ACLs per zone.

The changelog entry could be:

[func]		vorner
It is possible to specify ACL for the xfrout module. It is in the ACL
configuration key and has the usual ACL syntax. It currently supports
only the source address currently. Default ACL rejects everything.

comment:5 Changed 6 years ago by vorner

  • Milestone changed from Sprint-20110712 to Sprint-20110802

comment:6 Changed 6 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:7 follow-up: Changed 6 years ago by jinmei

First, I've made a few trivial changes/cleanups and directly pushed them.

general

  • I'm not sure if we should reject incoming AXFR request by default. At least it's different from BIND 9's default. It will also break current deployment of BIND 10 (although I think it's acceptable at the current maturity of BIND 10 as long as we warn it in a release note or something).

misc in xfrout.py.in

  • parse_query_message(): shouldn't we skip performing ACL if TSIG check fails? In fact, (although quite unlikely) if the fail is MAC mismatch, it may mean the message is broken, so performing ACL itself may not even make sense especially if and when we introduce ACl based on message content. We may want to apply a "DROP" rule only by remote address even before the TSIG check, but as we discussed in the resolver case, that would probably be done by a separate ACL.
  • parse_query_message(): not sure if it's intentional, but you can omit "isc.acl.acl" below:
                if acl_result == isc.acl.acl.DROP:
    ...
                elif acl_result == isc.acl.acl.REJECT:
    
  • It seems we can merge reply_query_with_error_rcode() and reply_query_with_format_error().
  • update_config_data: we should probably log first so that it's logged even if load() fails with exception:
            if 'ACL' in new_config:
                self._acl = REQUEST_LOADER.load(new_config['ACL'])
            logger.info(XFROUT_NEW_CONFIG)
    
  • update_config_data: just checking, is it okay to not to protect it by lock? maybe we should add comment about that.
  • config_handler: technically "Bad configuration" may be too restricted, because exception would happen for other (rare) reasons such as memory allocation failure. maybe simply say "Failed to handle new configuration" or something?
                except Exception as e:
                    answer = create_answer(1, "Bad configuration: " + str(e))
    

guess_remote

  • in the doc, "The socket must be a socket" sounds awkward. Maybe "sock_fd must refer to a socket" or something?
  • The trick employed in this function seems to be the best current practice, but I'm afraid it's fragile. I wouldn't be surprised there's a system that is more strict about the AF matching, for example. For longer term, I guess we should pass some auxiliary information (at least the AF, socktype and protocol, and possibly local/remote addresses) when we pass a socket FD from one process to another.
  • As for this:
                # To make it work even on hosts without IPv6 support
                # (Any idea how to simulate this in test?)
    
    It seems we can override socket.has_ipv6. So a test could tweak it before testing. (but we might not bother to do this minor case - today's many system already supports IPv6, and if we implement the "longer term" solution in not far future, the problem will be gone anyway)

xfrout_test

  • maybe a matter of taste, but you should be able to use assertEqual here:
            self.assertTrue(rcode is None)
            #self.assertEqual(None, rcode)
    
  • you mean "REFUSED"?
    +        # This should be rejected, therefore NOTAUTH
    

xfrout_message.mes

I'd explain %1 and %2 such as in '<zone name>/<zone class>'. '%3:%4'
to represent address+port is not a good way when it's IPv6 address.
Ideally we should resolve the unification of the notation (should be a
ticket for this, don't remember the number), but assuming it's not
done yet, I'd suggest either %3#%4 (BIND 9 way) or [%3]:%4.

changelog
As discussed in 'general', if we keep rejecting requests by default,
we should note it more explicitly that it's a backward incompatible
change (and we should mark it with '*').

comment:8 follow-up: Changed 6 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:9 in reply to: ↑ 8 Changed 6 years ago by jinmei

Two more things:

  • The configuration name "ACL" may not be the best one in that the all upper-case name is inconsistent with that of b10-resolver (which uses "query_acl") and also that it may be too generic (for example, we may want to have a different ACL that is performed even before TSIG check).
  • this is probably abusing the task, but now that we have the remote address, it would be nice we can add some small extensions to some log messages so that they print the remote address. (but maybe it should rather be deferred to a separate ticket)

comment:10 in reply to: ↑ 7 ; follow-up: Changed 6 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

general

  • I'm not sure if we should reject incoming AXFR request by default. At least it's different from BIND 9's default. It will also break current deployment of BIND 10 (although I think it's acceptable at the current maturity of BIND 10 as long as we warn it in a release note or something).

I'm not sure about that either. I'm OK with any reasonable default, I just thought this one is the safest one. What do you think should be the default?

misc in xfrout.py.in

  • parse_query_message(): shouldn't we skip performing ACL if TSIG check fails? In fact, (although quite unlikely) if the fail is MAC

Ah, right, I overlooked it. This should be now fixed and tested.

  • update_config_data: just checking, is it okay to not to protect it by lock? maybe we should add comment about that.

To be honest, I have no idea here. XfrOut? hits some kind of mental block in my mind when trying to understand what part of code does exactly what, what does run in which thread, etc. It wasn't in one before, though.

I'm quite sure the update can happen only one at a time, since the CC loop runs only one, but it might be possible some other thread reads them while being updated. Should we open a ticket to investigate it?

  • The trick employed in this function seems to be the best current practice, but I'm afraid it's fragile. I wouldn't be surprised there's a system that is more strict about the AF matching, for example. For longer term, I guess we should pass some auxiliary information (at least the AF, socktype and protocol, and possibly local/remote addresses) when we pass a socket FD from one process to another.

Hmm, maybe. I wanted to pass the addresses when sending it, but I found out the current auth side has no tests and it would require changes in a lot of places to pass it around, so I decided this might be easier.

Anyway, it should be tested, so I suggest we keep this simpler approach and let the test catch it, if the problem exists anywhere.

  • As for this:
                # To make it work even on hosts without IPv6 support
                # (Any idea how to simulate this in test?)
    
    It seems we can override socket.has_ipv6. So a test could tweak it before testing. (but we might not bother to do this minor case - today's many system already supports IPv6, and if we implement the "longer term" solution in not far future, the problem will be gone anyway)

Overriding it doesn't really test anything. Such system still has IPv6 support, I'd like to test it with something that really doesn't have IPv6 support. Maybe we have a buildbot without IPv6?

changelog
As discussed in 'general', if we keep rejecting requests by default,
we should note it more explicitly that it's a backward incompatible
change (and we should mark it with '*').

OK, depends on what we decide to do about that.

And with the addresses for logs, I think it might be nice thing to have, but I'd rather like to put it into a different task.

Thank you

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

Replying to vorner:

I made one small change directly. (Apparently I was not clear enough
on this one in my previous comment, and the different part than I
meant was changed.)

general

  • I'm not sure if we should reject incoming AXFR request by default. At least it's different from BIND 9's default. It will also break current deployment of BIND 10 (although I think it's acceptable at the current maturity of BIND 10 as long as we warn it in a release note or something).

I'm not sure about that either. I'm OK with any reasonable default, I just thought this one is the safest one. What do you think should be the default?

Personally, I'd accept by default because conceptually xfrout would be
part of auth, and we'd accept queries by default in auth. But I may
be biased because while I know there are some paranoid people who
never want to answer xfr queries except those from the "authorized
secondaries", I never agree with them (I see some valid cases such as
a very big zone where xfr queries could be a DoS, but that's an
exceptional case, not a reason to set the default).

But now that you're leaving, I'm okay, e.g., with leaving this open
and deferring it to a separate ticket.

  • update_config_data: just checking, is it okay to not to protect it by lock? maybe we should add comment about that.

To be honest, I have no idea here. XfrOut? hits some kind of mental block in my mind when trying to understand what part of code does exactly what, what does run in which thread, etc. It wasn't in one before, though.

I'm quite sure the update can happen only one at a time, since the CC loop runs only one, but it might be possible some other thread reads them while being updated. Should we open a ticket to investigate it?

From a bit closer look at it, it's probably safe as ACL is always
replaced as a whole and specific sessions have their own copy of the
entire ACL.

For longer term, as you indicated I also think the current design and
implementation of xfrout are mess. It's a naive mixture of threads
and events, taking bad sides of these. So, for a longer term, I'd
rather redesign it (hopefully via incremental refactoring) than
"investigating" each of these problems that come from the bad design.
And, in either case, creating a ticket would be nice.

  • As for this:
                # To make it work even on hosts without IPv6 support
                # (Any idea how to simulate this in test?)
    
    It seems we can override socket.has_ipv6. So a test could tweak it before testing. (but we might not bother to do this minor case - today's many system already supports IPv6, and if we implement the "longer term" solution in not far future, the problem will be gone anyway)

Overriding it doesn't really test anything. Such system still has IPv6 support, I'd like to test it with something that really doesn't have IPv6 support. Maybe we have a buildbot without IPv6?

You can at least test the 'else' case (if I read it correctly that
part isn't tested at all right now). But I'd leave it to you.

And with the addresses for logs, I think it might be nice thing to have, but I'd rather like to put it into a different task.

Fair enough.

Other points look okay.

comment:12 Changed 6 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:13 in reply to: ↑ 11 ; follow-up: Changed 6 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

Personally, I'd accept by default because conceptually xfrout would be
part of auth, and we'd accept queries by default in auth. But I may
be biased because while I know there are some paranoid people who
never want to answer xfr queries except those from the "authorized
secondaries", I never agree with them (I see some valid cases such as
a very big zone where xfr queries could be a DoS, but that's an
exceptional case, not a reason to set the default).

But now that you're leaving, I'm okay, e.g., with leaving this open
and deferring it to a separate ticket.

OK, I changed it, it is small change.

From a bit closer look at it, it's probably safe as ACL is always
replaced as a whole and specific sessions have their own copy of the
entire ACL.

Well, I don't know if python assignement is atomic and if inconsistent config is OK. But it's not the main point here I guess.

You can at least test the 'else' case (if I read it correctly that
part isn't tested at all right now). But I'd leave it to you.

OK, done.

Thank you

comment:14 in reply to: ↑ 13 Changed 6 years ago by jinmei

Replying to vorner:

Personally, I'd accept by default because conceptually xfrout would be
part of auth, and we'd accept queries by default in auth. But I may
be biased because while I know there are some paranoid people who
never want to answer xfr queries except those from the "authorized
secondaries", I never agree with them (I see some valid cases such as
a very big zone where xfr queries could be a DoS, but that's an
exceptional case, not a reason to set the default).

But now that you're leaving, I'm okay, e.g., with leaving this open
and deferring it to a separate ticket.

OK, I changed it, it is small change.

Ack, but this results in having two settings of the "default": in
xfrout.py.in and the spec file. As we discussed before, I guess we
should allow RequestLoader?.load() to have the default action, and then
we can change the default in the spec file "[]" (meaning no specific
ACLs).

But for now I'm okay with the duplicate. Please leave a comment in
xfrout.py about this, then feel free to merge it.

comment:15 Changed 6 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:16 Changed 6 years ago by jinmei

ah, overlooked one point: we should probably reset has_ipv6 to the
original value after this test:

        xfrout.socket.has_ipv6 = False
        sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
        sock.connect(('127.0.0.1', 12345))
        self.assertEqual(('127.0.0.1', 12345),
                         self.unix._guess_remote(sock.fileno()))

comment:17 Changed 6 years ago by vorner

  • Resolution set to complete
  • Status changed from reviewing to closed

Thanks, I updated these two things and merged. Hopefully this doesn't break anything.

Note: See TracTickets for help on using tickets.