Hey all,
I've been working recently on security updates started by Patryk prior
summer. What I have found made me question what would be proper way to
integrate his changes and also improve quality of code we have in
PLC4X/PLC4J.
1) Driver context - the driver context is something which we use
internally in some cases. Interestingly enough its responsibility is not
clear - in some cases driver context is used to retain some
configuration options passed by user to driver (s7, partially opcua), in
others it keeps information obtained during negotiation (knx) or
read/write cycles (profinet). OPC UA driver attempts to use driver
context to retain UA server certificate.
2) Discovery connection and connection - we received a bugfix for thread
leak prior 0.11 release which terminates connections created by plc4j.
The same fix also caused driver context to get blind (driver context is
tied to channel). Other fixes changed initialization sequence of drivers
known up to 0.10 release (call onDiscover, then onConnect) breaking
discovery of UA server endpoints (onConnect, then onDiscover).
I am writing about these two aspects because they are closely related
and need a further thought on how we would like to address these. For
now I do not have any strong opinion beyond necessity to clean up and
make a clear distinction where SPI/API border is.
The driver context and its use within channel lifecycle from one hand
makes a lot of sense, context will be tied to connection itself. Yet, it
causes a requirement for configuration to be mutable (onDiscover, then
populate driver configuration, then onConnect) - this approach is also
consistent, because it not only justifies separation of configuration
from driver context, but also makes it clear that onDiscover cycle can
always be avoided when extra configuration options appear.
On other hand it might also lead to situations where connection URI with
all options will get fairly long (imagine x.509 certificate encoded as
uri parameter) ;-), which is probably fine in 95% of cases.
In terms of connection handling I believe we started with early
prototype which been working fine, however due to several aspects it
fall of the supported (tested) code base. Effectively 0.11 opcua driver
can't a) discover server certificate and b) can't make use of it,
because it is stored in driver context, because c) driver context is
being reset between discovery and actual connection.
All this also navigates us to DefaultNettyPlcConnectiona and its
connect() implementation. To be fair, that thing got a number of
switches to configure behavior, out of which some are used for tests,
some are used by specific drivers ie. UA (discovery) or S7 (high
availability).
To me it feels like we should clarify that part first. If the logic is
specific to driver it should be held within driver or this driver stack
configurer, without necessity to have branch logic within SPI/common
base class. I believe that only this way we will save ourselves from
headaches. Because driver knows configuration before spinning actual
connection it could manage connections needed for negotiation/discovery
and also coordinate their termination.
Looking forward to your comments. In case of no further inputs - I'll
try move some portions of code back to OPCUA driver and see if it will
fit there.
Cheers,
Łukasz
