[kea-dev] Accessing DHCP options within hooks libs

[Thomas Markwalder]

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&);

Thomas