[kea-dev] Accessing DHCP options within hooks libs

Marcin Siodelski marcin at isc.org
Tue Apr 26 11:24:19 UTC 2016

On 26.04.2016 13:07, Thomas Markwalder wrote:
> On 4/26/16 5:43 AM, Marcin Siodelski wrote:
> > 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
> >
> > _______________________________________________
> > kea-dev mailing list
> > kea-dev at lists.isc.org
> > https://lists.isc.org/mailman/listinfo/kea-dev
> While the API is interesting, I think it introduces additional
> complexity.  As you stated above, by replicating configured options into
> the outbound packets, we keep a distinct line between the reference
> (configuration value), thereby eliminating the possibility of
> contaminating values, not just in hooks but anywhere else.  I would
> prefer this, unless we feel the performance hit is too large.
> If we go with the API, I would suggest altering the names to:
> // Makes a copy of the first option found by option code and returns it.
> OptionPtr getOptionCopy(const uint16_t option_code) const;
> // Makes a copy of the specified option.
> static OptionPtr copyOption(const OptionPtr&);
> // Inserts copy of the specified option into the packet.
> void putOptionCopy(const OptionPtr&, const Algorithm&);
> // Inserts pointer to the specified option into the packet.
> void putOption(const OptionPtr&, const Algorithm&);

One has to remember that the last putOption() is in fact a variant of
existing Pkt::addOption() function (excluding second parameter) so while
I like the proposed names we have to remember that there may be existing
software relying on the existing name. Though, we can probably keep the
old one and make it an alias of the new one.


More information about the kea-dev mailing list