Opened 9 years ago

Closed 8 years ago

#999 closed task (complete)

Integrate ACLs into b10-resolver

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

Description

This depends on #978 and will consist of:

  • placing ACL configuration into the resolver section (for now, may change in future).
  • calling the loader from #978 and storing the ACL produced there.
  • calling it on incoming packet.

Subtickets

Change History (22)

comment:1 Changed 8 years ago by jinmei

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

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

I don't want to stop you from working, but it should be pointed out that the loader from #978 is just started and it will be needed to load the ACLs. But the branch already contains a first shot at the interface, so if you really don't want to wait, you may want to use that.

comment:3 Changed 8 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 4

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

Replying to vorner:

I don't want to stop you from working, but it should be pointed out that the loader from #978 is just started and it will be needed to load the ACLs. But the branch already contains a first shot at the interface, so if you really don't want to wait, you may want to use that.

Thanks for the note. Yeah, I knew the dependency.

I examined relevant tasks, and while recognizing not all dependencies
were cleared, I figured out it would be possible to start the work
pretty effectively based on the latest version of #997 and #998,
without taking too much risk of redesigning/reimplementing many things
later. That way we can (hopefully) improve development concurrency,
and it would also be nice if multiple developers are involved in
actually using the API so that we can identify unclear points or
usability issues of the design/definition/description.

comment:5 follow-up: Changed 8 years ago by vorner

You might want to have a look at #769, it should contain the loader and convenience typedefs and the Context. It is not reviewed yet, but might be more useful than just guessing.

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

Replying to vorner:

You might want to have a look at #769, it should contain the loader and convenience typedefs and the Context. It is not reviewed yet, but might be more useful than just guessing.

Thanks for the pointer. Some quick comments about the Packet structure:

I've been thinking about the same concept for #999 and tentatively
defined a class named "Client":

class Client {
public:
    explicit Client(const isc::asiolink::IOMessage& request_message);
    ~Client();
    const isc::asiolink::IOEndpoint& getRequestSourceEndpoint() const;
    const isc::acl::IPAddress& getRequestSourceIPAddress() const;
...
};

It's not a very strong opinion, but I suspect "Packet" sounds a bit
too generic in that it could be either incoming/outgoing
request/response, while this class seems to focus on the incoming
side, e.g., from this:

    /// \brief The local IP address (ours, of the interface where we received).
    asiolink::IOAddress local_address;

Being generic is not necessarily bad, especially if we also want to
use it in outgoing context, but in that case the entire interface
should be able to represent the concept reasonably.

I (again tentatively) define the "Client" class in lib/server_common.
I'm not sure how many things that depend on specific contexts would be
reasonably defined under the acl namespace. Also, the concept of
"Packet" itself is not necessarily tied to access control. By
defining Client, I'd envision we'll eventually use it as a higher
level interface between ASIO and actual (auth/resolver) server
classes, that is, instead of getting a set of io_message,
query_message, ... etc. in Resolver::processMessage(), we'd pass a
"Client" object that encloses these types of parameters.

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

Hello

Replying to jinmei:

I've been thinking about the same concept for #999 and tentatively
defined a class named "Client":

I didn't find the trac999 in git, so I couldn't have a look. But, do we really need accessor methods for something that just holds some data together? Anyway, it's not the main problem.

I believe we need to integrate the two branches together sometime, otherwise registration of all the various checks will be problematic. I don't really care much about which class/struct is used as the context, I just put there the simplest think that looked it should work.

It's not a very strong opinion, but I suspect "Packet" sounds a bit
too generic in that it could be either incoming/outgoing
request/response, while this class seems to focus on the incoming
side, e.g., from this:

I expected it to be incoming. The local port and address are there, because we accept them on some and user might want to distinguish at which interface it came. But that wouldn't be too common I guess.

I (again tentatively) define the "Client" class in lib/server_common.

I don't think server_common is the place to go. That one is for implementation/technical stuff we have for our servers. The ACL should be a generally-available library I believe (with python bindings, etc). I thought of putting it as dns::acl as well, which would make some sense as well, but server_common feels odd.

I'm not sure how many things that depend on specific contexts would be
reasonably defined under the acl namespace. Also, the concept of
"Packet" itself is not necessarily tied to access control. By
defining Client, I'd envision we'll eventually use it as a higher
level interface between ASIO and actual (auth/resolver) server
classes, that is, instead of getting a set of io_message,
query_message, ... etc. in Resolver::processMessage(), we'd pass a
"Client" object that encloses these types of parameters.

Surely a good goal, I'm not against having the „context“ out of the ACL thing. I'd like to have a place where we put all the TSIG checks, etc, which is both ACL-related and DNS-related. The important part now is, I believe, using the singleton loader. As you mentioned, you expect to have this ready for review soon, this would be one of the TBD?

Thanks

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

Replying to vorner:

I've been thinking about the same concept for #999 and tentatively
defined a class named "Client":

I didn't find the trac999 in git, so I couldn't have a look. But, do we really need accessor methods for something that just holds some data together? Anyway, it's not the main problem.

I believe we need to integrate the two branches together sometime, otherwise registration of all the various checks will be problematic. I don't really care much about which class/struct is used as the context, I just put there the simplest think that looked it should work.

I've pushed the latest trac999 code as trac999preview for reference.
It's a complete implementation with full tests and documentation,
although I don't think it a good target to review since it depends on
some unclear points of #998.

After looking at #769, I decided to not rely on it in order to reduce
dependency. Right now it has its own config parser/ACL loader as it
only uses a very simple form of rules. My intention is to get #998
and this branch completed and merged, then to integrate the loader
part of #999 with #769. There will be some duplicate effort, but I
thought this would be the fastest way to include the desired feature
in the next release.

The "Client" class is tentatively in the server_common module
(directly), but I'm not strongly pushing it by doing so - it's a
rather placeholder right now. But as I said I thought ACl was not a
good place for this concept either. I'm not pushing the specific name
of "Client" either - again it's tentative.

I'm not sure how many things that depend on specific contexts would be
reasonably defined under the acl namespace. Also, the concept of
"Packet" itself is not necessarily tied to access control. By
defining Client, I'd envision we'll eventually use it as a higher
level interface between ASIO and actual (auth/resolver) server
classes, that is, instead of getting a set of io_message,
query_message, ... etc. in Resolver::processMessage(), we'd pass a
"Client" object that encloses these types of parameters.

Surely a good goal, I'm not against having the „context“ out of the ACL thing. I'd like to have a place where we put all the TSIG checks, etc, which is both ACL-related and DNS-related. The important part now is, I believe, using the singleton loader. As you mentioned, you expect to have this ready for review soon, this would be one of the TBD?

Yeah, that's a middle term idea.

comment:9 Changed 8 years ago by jinmei

Branch trac999 is ready for review.

It depends on a snapshot version of trac998, but I believe the
interface is now fixed and this branch can be safely reviewed.

I've played with the generic loader framework (introduced in #769
and now merged in the master) for a while, but decided to keep
the current approach (using a minimalist separate loader within
the resolver code) as this branch itself is already quite big, and
adapting the current code to the generic loader would make it even
larger. If it's okay, I'll create a separate follow-up ticket for
the unification work.

As a consequence, I've kept the Client class for the resolver ACL
context in the server_common module. In the follow-up work, I'd move
the ACL specific part under acl and merge it with the RequestContext?
class. Until then I'd propose holding off the discussion on where the
Client class should go (or whether we need it).

The first commit in this branch is a merge to incorporate trac998 and
should be ignored. The rest of the diff can be retrieved by
git diff 217c097.

Also note that some part of change in io_endpoint_unittest.cc is
irrelevant to the subject of this branch (commit 217c097), but I
noticed the bug while I worked on this branch and since it's quite
trivial I fixed it within the branch. If it's noisy, I'm okay with
reverting it and moving it to a separate ticket.

Finally, to enable the default ACL, I added generic code that had
b10-resolver load all initial configuration (as b10-auth does). It's
commit 181283a. As noted in the commit log, this eliminated the
hardcoded listen_on default. This would be a nice side effect, but
since the effect is larger than the minimal scope of this ticket,
please carefully review this change.

This is proposed changelog entry:

261.?	[func]*		jinmei
	b10-resolver: Introduced ACL on incoming queries.  By default the
	resolver accepts queries from ::1 and 127.0.0.1 and rejects all
	others.  The ACL can be configured with bindctl via the
	"Resolver/query_acl" parameter.  For example, to accept queries
	from 192.0.2.0/24 (in addition to the default list), do this:
	> config add Resolver/query_acl
	> config set Resolver/query_acl[2]/action "ACCEPT"
	> config set Resolver/query_acl[2]/from "192.0.2.0/24"
	> config commit
	(Trac #999, git TBD,
	also based on other ACL related work done by stephen and vorner)

comment:10 Changed 8 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:11 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:12 follow-ups: Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

I agree with putting the unification work into a followup ticket, but I think it should happen soon, because it will allow adding more types of checks easily and it already contains superset of the syntax your loader is able to load (for example it can have condition-less actions to modify the „default“ result, after merging two tickets currently in review, it will support shorter syntax).

Anyway, the current code fails to compile for me:

libtool: link: g++ -Wall -Wextra -Wwrite-strings -Woverloaded-virtual -Wno-sign-compare -Werror -fPIC -g -O0 -o .libs/run_unittests run_unittests-run_unittests.o run_unittests-check_test.o run_unittests-acl_test.o run_unittests-loader_test.o run_unittests-dns_test.o run_unittests-ip_check_unittest.o -pthread  -L/usr/lib64 -lgtest ../../../../src/lib/util/.libs/libutil.so ../../../../src/lib/util/unittests/.libs/libutil_unittests.so /home/vorner/work/bind10/src/lib/util/io/.libs/libutil_io.so ../../../../src/lib/acl/.libs/libacl.so ../../../../src/lib/cc/.libs/libcc.so ../../../../src/lib/exceptions/.libs/libexceptions.so ../../../../src/lib/acl/.libs/libdnsacl.so -L/usr/lib /home/vorner/work/bind10/src/lib/acl/.libs/libacl.so /home/vorner/work/bind10/src/lib/cc/.libs/libcc.so /home/vorner/work/bind10/src/lib/dns/.libs/libdns++.so /home/vorner/work/bind10/src/lib/cryptolink/.libs/libcryptolink.so -lbotan -lbz2 -lcrypto /usr/lib64/libgmp.so -lpthread -lrt -lz ../../../../src/lib/log/.libs/liblog.so -L/opt/log4cplus/lib /opt/log4cplus/lib/liblog4cplus.so /home/vorner/work/bind10/src/lib/util/.libs/libutil.so /home/vorner/work/bind10/src/lib/exceptions/.libs/libexceptions.so -pthread -Wl,-rpath -Wl,/home/vorner/testing/bind10/lib -Wl,-rpath -Wl,/opt/log4cplus/lib -Wl,-rpath -Wl,/usr/lib
run_unittests-ip_check_unittest.o: In function `isc::acl::IPCheck<(anonymous namespace)::GeneralAddress>::getAddress() const':
/home/vorner/work/bind10/src/lib/acl/tests/../../../../src/lib/acl/ip_check.h:258: undefined reference to `isc::acl::IPCheck<(anonymous namespace)::GeneralAddress>::IPV4_SIZE'
/home/vorner/work/bind10/src/lib/acl/tests/../../../../src/lib/acl/ip_check.h:258: undefined reference to `isc::acl::IPCheck<(anonymous namespace)::GeneralAddress>::IPV6_SIZE'
run_unittests-ip_check_unittest.o: In function `isc::acl::IPCheck<(anonymous namespace)::GeneralAddress>::getMask() const':
/home/vorner/work/bind10/src/lib/acl/tests/../../../../src/lib/acl/ip_check.h:264: undefined reference to `isc::acl::IPCheck<(anonymous namespace)::GeneralAddress>::IPV4_SIZE'
/home/vorner/work/bind10/src/lib/acl/tests/../../../../src/lib/acl/ip_check.h:264: undefined reference to `isc::acl::IPCheck<(anonymous namespace)::GeneralAddress>::IPV6_SIZE'
run_unittests-ip_check_unittest.o: In function `isc::acl::IPCheck<(anonymous namespace)::GeneralAddress>::setMask(int)':
/home/vorner/work/bind10/src/lib/acl/tests/../../../../src/lib/acl/ip_check.h:362: undefined reference to `isc::acl::IPCheck<(anonymous namespace)::GeneralAddress>::IPV4_SIZE'
/home/vorner/work/bind10/src/lib/acl/tests/../../../../src/lib/acl/ip_check.h:362: undefined reference to `isc::acl::IPCheck<(anonymous namespace)::GeneralAddress>::IPV6_SIZE'
collect2: ld returned 1 exit status
make[5]: *** [run_unittests] Error 1
make[5]: Leaving directory `/home/vorner/work/bind10/src/lib/acl/tests'

And, finally, some comments about the code:

  • In the io_endpoint_unittest, you introduced shared pointers. Wouldn't auto_ptr be enough? But if you like shared pointers more, I think it is OK in test, there's no performance problem in it.
  • EXPECT_THROW(IPAddress ipaddr(sa), isc::BadValue); ‒ this doesn't need the (unused) variable ipaddr, it should be enough to have EXPECT_THROW(IPAddress(sa), isc::BadValue);
  • Should the IPCheck<Client>::matches really be in server_common/client.{cc,h}?
  • Should we test localhost addresses as well? (and in the following test) They are default, so to check that they are default at the right place.
    TEST_F(ResolverConfig, defaultQueryACL) {
        // If no configuration is loaded, the default ACL should reject everything.
    
  • The name compoundQueryAcl might be confusing, considering that we have compound ACL checks. This ACL contains multiple entries, but none of them is compound in that sense. Would be longerAcl or multiEntryACL be acceptable?
  • You enable debug logging in tests. But should we spam the outputs with the debug logging?

And, do we plan to support checking the receiving IP address and the „from“ port in future (surely not in this ticket)?

comment:13 in reply to: ↑ 12 Changed 8 years ago by jinmei

Replying to vorner:

I agree with putting the unification work into a followup ticket, but I think it should happen soon, because it will allow adding more types of checks easily and it already contains superset of the syntax your loader is able to load (for example it can have condition-less actions to modify the „default“ result, after merging two tickets currently in review, it will support shorter syntax).

"soon" is fine. In fact, I already have made some progress locally.
And, actually, that work made me think completing that part would make
this branch too big and should better go to a separate ticket.

And, responding to the build problem first:

Anyway, the current code fails to compile for me:

run_unittests-ip_check_unittest.o: In function `isc::acl::IPCheck<(anonymous namespace)::GeneralAddress>::getAddress() const':
/home/vorner/work/bind10/src/lib/acl/tests/../../../../src/lib/acl/ip_check.h:258: undefined reference to `isc::acl::IPCheck<(anonymous namespace)::GeneralAddress>::IPV4_SIZE'

...

}}}

Hmm, this looks like the imported #998 part, not for the main code of
this branch. Does the latest master that has merged with #998
compiles in your environment? If so, We should probably merge this
branch with master.

BTW, I couldn't reproduce the problem with g++ on my machine or on
bind10.isc.org.

comment:14 Changed 8 years ago by vorner

Yes, you're right, this happens with current master as well. I'm going to send an email about it.

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

Replying to vorner:

Replying to the specific code comments:

And, finally, some comments about the code:

  • In the io_endpoint_unittest, you introduced shared pointers. Wouldn't auto_ptr be enough? But if you like shared pointers more, I think it is OK in test, there's no performance problem in it.

auto_ptr is not very friendly with STL containers (which often
internally involve object copies). So, in general, I'd avoid using
auto_ptr with STLs. It might happen to be safe in this specific case
(I've not examined that possibility), but even if so, as you said I
don't think it worth taking the risk in this context.

  • EXPECT_THROW(IPAddress ipaddr(sa), isc::BadValue); ‒ this doesn't need the (unused) variable ipaddr, it should be enough to have EXPECT_THROW(IPAddress(sa), isc::BadValue);

Unfortunately my clang++/g++ don't work at least for my usual compiler.
Strangely it requests the default constructor in this case:

ip_check_unittest.cc:207: error: no matching function for call to 'isc::acl::IPAddress::IPAddress()'
  • Should the IPCheck<Client>::matches really be in server_common/client.{cc,h}?

Where else do you think it should belong? As long as Client is
defined in server_common, it should be at least somewhere we can
reasonably refer to the Client's definition.

But, considering we'll merge this and #769 quite soon and it's likely
to use the definitions in lib/acl at that point. So, in this case,
this question will disappear.

  • Should we test localhost addresses as well? (and in the following test) They are default, so to check that they are default at the right place.
    TEST_F(ResolverConfig, defaultQueryACL) {
        // If no configuration is loaded, the default ACL should reject everything.
    

I don't understand these comments...in the first sentence are you
suggesting we should explicitly test 127.0.0.1 and ::1 instead of
192.0.2.1, etc? We could do so, but in this context I don't see
an essential difference between these cases.

As for the default, if you mean the "default for b10-resolver" (that
comes from the spec file), unfortunately it's mostly impossible to
test in our current framework, because (as far as I know) there's no
easy way to incorporate the default config values to the test cases
(it's not specific to the ACL). I think we should have a framework to
test these (and I agree it would especially important for ACLs because
if the expected default is not actually loaded due to a bug that would
be a much more significant effect than other similar bugs), but I'm
afraid that's beyond the scope of this ticket.

  • The name compoundQueryAcl might be confusing, considering that we have compound ACL checks. This ACL contains multiple entries, but none of them is compound in that sense. Would be longerAcl or multiEntryACL be acceptable?

Okay. I've changed it to multiEntryACL.

  • You enable debug logging in tests. But should we spam the outputs with the debug logging?

I believe we should, because otherwise we could overlook regression in
producing log messages (we in fact had such a bug, and since it was at
the DEBUG level we didn't notice that until after merging it to
master). Actually, I believe we should do this for other cases, too,
and created a general ticket for that (don't remember the number right
now). The increased noise level could be an issue as you hinted, so
we may have to address that part while enabling debug logging in
tests. But I still believe it's important to enable as many logs as
possible in our tests.

And, do we plan to support checking the receiving IP address and the „from“ port in future (surely not in this ticket)?

Perhaps, especially for the receiving (destination) address. But it's
a future work, and will only be an actual task when we see a clear
need for them.

comment:16 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:17 in reply to: ↑ 15 ; follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

auto_ptr is not very friendly with STL containers (which often
internally involve object copies). So, in general, I'd avoid using
auto_ptr with STLs. It might happen to be safe in this specific case
(I've not examined that possibility), but even if so, as you said I
don't think it worth taking the risk in this context.

OK, no problem.

  • Should the IPCheck<Client>::matches really be in server_common/client.{cc,h}?

Where else do you think it should belong? As long as Client is
defined in server_common, it should be at least somewhere we can
reasonably refer to the Client's definition.

I don't know. I find it strange and hard to follow, when the class lives somewhere and one of it's methods lives somewhere completely else. Anyway, that isn't a problem of this changeset, but I believe we don't want such things in the code generally and should do something about it. If you say this problem will disappear soon, it's OK.

  • Should we test localhost addresses as well? (and in the following test) They are default, so to check that they are default at the right place.

I don't understand these comments...in the first sentence are you
suggesting we should explicitly test 127.0.0.1 and ::1 instead of
192.0.2.1, etc? We could do so, but in this context I don't see
an essential difference between these cases.

Well, I think we should do it to test that the default (allowing localhost) comes from the spec file, not the code itself. Because, if the code loaded the currently-spec-file default without the spec file, the localhost would distinguish it, while the current 192.0.2.1 test wouldn't.

  • You enable debug logging in tests. But should we spam the outputs with the debug logging?

I believe we should, because otherwise we could overlook regression in
producing log messages (we in fact had such a bug, and since it was at
the DEBUG level we didn't notice that until after merging it to
master). Actually, I believe we should do this for other cases, too,
and created a general ticket for that (don't remember the number right
now). The increased noise level could be an issue as you hinted, so
we may have to address that part while enabling debug logging in
tests. But I still believe it's important to enable as many logs as
possible in our tests.

And isn't it better to enable it using the environment variables, globally? The problem I see with this is, I have stderr red-colored while running tests. And with the logging I see a lot of red, which makes it harder to spot real failures and complaints. Maybe I'll just turn the logging off using the variables. Anyway, I see it inconsistent with the rest of the tests. Maybe we want to discuss it and put the DEBUG level as the default?

comment:18 in reply to: ↑ 17 Changed 8 years ago by jinmei

Replying to vorner:

  • Should we test localhost addresses as well? (and in the following test) They are default, so to check that they are default at the right place.

I don't understand these comments...in the first sentence are you
suggesting we should explicitly test 127.0.0.1 and ::1 instead of
192.0.2.1, etc? We could do so, but in this context I don't see
an essential difference between these cases.

Well, I think we should do it to test that the default (allowing localhost) comes from the spec file, not the code itself. Because, if the code loaded the currently-spec-file default without the spec file, the localhost would distinguish it, while the current 192.0.2.1 test wouldn't.

Ah, okay. I believe I addressed it in ab31e2f.

  • You enable debug logging in tests. But should we spam the outputs with the debug logging?

[...]

And isn't it better to enable it using the environment variables, globally? The problem I see with this is, I have stderr red-colored while running tests. And with the logging I see a lot of red, which makes it harder to spot real failures and complaints. Maybe I'll just turn the logging off using the variables. Anyway, I see it inconsistent with the rest of the tests. Maybe we want to discuss it and put the DEBUG level as the default?

Making it switchable is a good idea (but I think the default log level
should be quite verbose). I also admit this is not the main subject
of this ticket. So I've reverted this part of change (041c3ec). I'll
update the general ticket (I found it; it's #1024) about the run time
switch.

Is there anything else I overlooked for this ticket?

comment:19 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:20 follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei

I believe this is OK, please merge.

As a side note, the runtime switch is already there in the form of environment variables like BIND10_LOGGER_SEVERITY and BIND10_LOGGER_DBGLEVEL.

comment:21 in reply to: ↑ 20 Changed 8 years ago by jinmei

Replying to vorner:

I believe this is OK, please merge.

Thanks for review. Merged and pushed. Closing ticket.

As a side note, the runtime switch is already there in the form of environment variables like BIND10_LOGGER_SEVERITY and BIND10_LOGGER_DBGLEVEL.

Ack.

comment:22 Changed 8 years ago by jinmei

  • Resolution set to complete
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.