[kea-dev] Accessing DHCP options within hooks libs

Marcin Siodelski marcin at isc.org
Tue Apr 26 09:43:09 UTC 2016


All,

Hooks libraries are meant to provide flexibility in accessing and
modifying packet contents, and we put no restrictions on what the hook
implementer can do with the packet. In case of misuse, this may have
significant consequences, e.g. server crashes, performance degradations
etc., but it would be hopeless to try to protect everyone from shooting
at their foot by imposing restrictions, and would inevitably cause
people to make horrible workarounds or at least whine that hooks API
doesn't allow them to achieve what their deployment needs.

However, we should also strive to provide mechanisms that would make it
more intuitive how the packet contents can be freely modified with some
level of guarantee that what I am doing in a hook doesn't have
unintended side effects (not necessarily a crash). Different scenarios
should probably be analyzed on case by case basis. I have one which I
want to bring up here, because it will be fairly common one, and we
already have reports from hooks developers that it does cause confusion:

https://lists.isc.org/pipermail/kea-users/2016-April/000314.html

Kea servers store configured DHCP options as pointers to instances of
objects derived from Option class. When the client's packet comes in,
selected Option instances are added to the response packet, represented
by the Pkt4/Pkt6 object. This is efficient approach because it
eliminates copies of Option objects and, for the most part, we only need
to create options' instances once during server reconfiguration. The
major flaw of this approach is that any alteration of an option added to
a response packet is in fact an alteration of that option within server
configuration data structures. In other words, modification of an option
for one packet may affect all subsequent packets.

The core server code doesn't really modify options already appended to
the packet object, so we benefit from higher performance with no side
effects. The issues come up within hooks, though.

The typical use case for hooks is to modify the packet contents based on
some additional information or algorithms. If this applies to
dynamically generated options, such as Client FQDN, it is not an issue.
However, the hook can also modify configured options, in which case
careless user can dynamically introduce changes to the server
configuration and disrupt its operation. It is going to be more obvious
when those modifications apply to suboptions, e.g. hook adds suboption
'bar' to configured option 'foo'. The first time option 'bar' is added
to 'foo' it stays there forever, even if the hook remains inactive.
There are ways to overcome this issue within the hook, by making a copy
of the entire top level option, applying modifications on a copy, and
replacing the option with a copy, but I am not sure this is all that
obvious for a typical user. It seems to me that the user has a right to
believe that any modifications applied to a particular packet wouldn't
affect anything beyond this packet.

I considered whether we could always make a copy of all configured
options into a packet, rather than copy pointers. This would be
functionally cleaner, and would draw a clearly visible line separating
configuration structures from a packet. But, that would also impose that
options are copied, even if they technically don't need to be copied
(like when no hooks are in use).

The other possible solution is to simply provide means/API through which
options can be safely modified in a packet and mandate the use of this
API within hooks framework. In fact, this might be pretty simple
addition to the Pkt{4,6} class:

// Specifies if option should be appended or should replace existing ones.
enum Algorithm {
    ADD,
    REPLACE
}

// Makes a copy of the first option found by option code and returns it.
OptionPtr checkoutOption(const uint16_t option_code) const;

// Makes a copy of the specified option.
static OptionPtr checkoutOption(const OptionPtr&);

// Inserts copy of the specified option into the packet.
void commitOption(const OptionPtr&, const Algorithm&);

// Inserts pointer to the specified option into the packet.
void addOption(const OptionPtr&, const Algorithm&);


The first two methods would return a copy of an option along with copies
of all suboptions it contains. This instance could be modified by the
hook with no effect on the original option instance. This modification
would also have no effect on the packet until this option is committed
or added, where in both cases the option could either replace an
existing option instance in the packet, or could coexist with it. Adding
(as opposed to commit) an option means copy a pointer into the packet
and could be preferred in many cases (over commit) for performance reasons.

If we go with this or similar approach we'd need to make sure that:
- all option types have appropriate copy constructors.
- it is clearly documented and hooks developers are aware that options
should be checked out before, they can be modified within a hook.


There are also other interesting things we do in this area. For example,
when the client's packet comes in, we "copy" options into the outbound
packet. And that "copy" means, that we copy a pointer. If there is a
hook that modifies a specific option in inbound packet, it would mean
that the outbound packet is modified too. Again, this can be mitigated
by using the solution above.


Please share your thoughts. If you think that the proposed approach is
wrong, please describe what approach would be better and why.

Marcin Siodelski
ISC



More information about the kea-dev mailing list