Hi everyone,
the first topic I want to tackle in the effort to make nitrokey-rs more
modular and thereby ready for Nitrokey 3 is device management.
Szczepan, I’ve cc’ed you because I’d like to know your opinion on the
approach described in the first section of this mail from the
libnitrokey perspective.
libnitrokey synchronization
===========================
At the moment, libnitrokey has a global state and can only connect to
one device at a time. Multi-device support is planned but there is no
schedule for it. In nitrokey-rs, we’ve modeled the current
implementation using the global singleton Manager that is protected by a
Mutex and can be used to connect to one device at a time.
I suggest the following change: We allow connections to an arbitrary
number of devices at the same time. We introduce a new global mutex
that keeps track of the currently connected device path (in
libnitrokey). On every command execution, we compare the path of the
device to the currently connected device path. If they don’t match, we
re-connect to the correct device by calling NK_logout and
NK_connect_with_path.
This approach should enable a multi-device API while reducing the
overhead for both the single-device and the multi-device cases to a
minimum, namely the mutex handling.
Device synchronization / Transactions
=====================================
Our current implementation makes sure that a Rust program cannot access
the same device twice at the same time, eliminating all sorts of race
conditions. Of course, this does not apply to other programs accessing
the device, so it is only of limited value.
We could try to maintain this property by introducing per-device mutexes
(i. e. per device path), but I’m not convinved that this is the right
way. I’d rather leave this to the application using nitrokey-rs. For
example, in nitrocli, we don’t need that kind of synchronization. In
nitrokey-test, we already have the required infrastructure in the form
of nitrokey-test-state (though I’m not sure whether this currently works
per test or per test group).
Such a synchronization feature would only make sense in nitrokey-rs if
it would enforce synchronization between applications, similar to pcscd,
but I think that is out of our scope.
Device identificiation
====================
The main reason for these changes is that we want to prepare for using
multiple backends – libnitrokey for the legacy Nitrokey devices and
something else for the Nitrokey 3. This also poses the question device
indentification. Currently, we are directly exposing the USB device
path in DeviceInfo. I propose to replace it with a non-exhaustive
DeviceId enum that has one variant per backend. The actual ID would
then be backend-specific. The DeviceId enum would implement Display and
FromStr to provide a reliable serialization format that can be used in
configuration files and in the user interface. This format could be
“<backend>:<id>”, for example “libnitrokey:0001:000f:00”.
Global manager or context
=========================
Finally, we have to decide whether we want to keep a global manager or
context struct, and if it should be protected by a mutex. The latter is
in my opinion no longer needed. If we want to provide some kind of
synchronization, it should be on a per-device basis.
But what about a manager or context struct, similar to hidapi’s HidApi
and pcsc’s Context? It could be used to choose the available backends,
to keep track of the backend state and to share resources between
devices. We don’t need it right now, but it would give us more
flexibility when adding new backends.
I’m still undecided on this point.
(We will also have to change the Device struct to handle multiple
backends, but I’d like to discuss that together with the features
implementation.)
Please let me know what you think about all this.
Best,
Robin
Hi Robin,
Thanks for writing up this proposal! I think it makes sense to me.
On 11.06.21 10:58, Robin Krahl wrote:
> libnitrokey synchronization> ===========================> > I suggest the following change: We allow connections to an arbitrary> number of devices at the same time. We introduce a new global mutex> that keeps track of the currently connected device path (in> libnitrokey). On every command execution, we compare the path of the> device to the currently connected device path. If they don’t match, we> re-connect to the correct device by calling NK_logout and> NK_connect_with_path.
You'll probably have to include more than just the path in the critical
section, right? I'd expect the logout/connect path to touch global state
and would equally need serialization.
My only concern is that if two threads truly use two devices in lockstep
then there will be a lot of logouts/connects happening. We should at
least test that path thoroughly to make sure there are no spurious
errors (which I have seen a fair amount of already when running the
nitrocli test suite, btw., e.g., I recall an assertion failure in
libnitrokey).
> This approach should enable a multi-device API while reducing the> overhead for both the single-device and the multi-device cases to a> minimum, namely the mutex handling.
You mentioned that multi-device support is planned. Please excuse my
ignorance, but why not go all the way right away? Can you elaborate on
the complications, please? It does not strike me as a much more involved
feat, but I am sure I must be missing something.
> Device synchronization / Transactions> =====================================> > Our current implementation makes sure that a Rust program cannot access> the same device twice at the same time, eliminating all sorts of race> conditions. Of course, this does not apply to other programs accessing> the device, so it is only of limited value.> > We could try to maintain this property by introducing per-device mutexes> (i. e. per device path), but I’m not convinved that this is the right> way. I’d rather leave this to the application using nitrokey-rs. For> example, in nitrocli, we don’t need that kind of synchronization. In> nitrokey-test, we already have the required infrastructure in the form> of nitrokey-test-state (though I’m not sure whether this currently works> per test or per test group).
It should be per test. Test groups are just a way to change the error
reporting behavior from what I recall.
> Such a synchronization feature would only make sense in nitrokey-rs if> it would enforce synchronization between applications, similar to pcscd,> but I think that is out of our scope.
I tend to agree. But do we know what the failure behavior is if multiple
applications access the same device? At that point it should no longer
be libnitrokey's state that is a problem. Does it come down to collision
of higher level primitives? E.g., one application setting a temporary
password to authenticate, colliding with another one doing the same? Is
everything else covered by USB at a different layer?
> Device identificiation> ====================> > The main reason for these changes is that we want to prepare for using> multiple backends – libnitrokey for the legacy Nitrokey devices and> something else for the Nitrokey 3. This also poses the question device> indentification. Currently, we are directly exposing the USB device> path in DeviceInfo. I propose to replace it with a non-exhaustive> DeviceId enum that has one variant per backend. The actual ID would> then be backend-specific. The DeviceId enum would implement Display and> FromStr to provide a reliable serialization format that can be used in> configuration files and in the user interface. This format could be> “<backend>:<id>”, for example “libnitrokey:0001:000f:00”.
Sounds good.
> Global manager or context> =========================> > Finally, we have to decide whether we want to keep a global manager or> context struct, and if it should be protected by a mutex. The latter is> in my opinion no longer needed. If we want to provide some kind of> synchronization, it should be on a per-device basis.> > But what about a manager or context struct, similar to hidapi’s HidApi> and pcsc’s Context? It could be used to choose the available backends,> to keep track of the backend state and to share resources between> devices. We don’t need it right now, but it would give us more> flexibility when adding new backends.
I am unfortunately not familiar with what hidapi or pcsc do or need and
why. I think without an immediate use case it's hard to say whether it's
useful down the line and, more importantly, what it should look like. At
least I have trouble figuring it out. I wouldn't say it's a big deal to
add it later on. Yes, it would possibly be a breaking change, but that
does not have to be the end of the world -- if it's just creating an
object and passing it through that should be reasonable to accommodate
in most user code.
Best,
Daniel
Hi Daniel,
thanks for your comments!
On 2021-06-14 06:16:01, Daniel Müller wrote:
> On 11.06.21 10:58, Robin Krahl wrote:> > libnitrokey synchronization> > ===========================> > > > I suggest the following change: We allow connections to an arbitrary> > number of devices at the same time. We introduce a new global mutex> > that keeps track of the currently connected device path (in> > libnitrokey). On every command execution, we compare the path of the> > device to the currently connected device path. If they don’t match, we> > re-connect to the correct device by calling NK_logout and> > NK_connect_with_path.> > You'll probably have to include more than just the path in the critical> section, right? I'd expect the logout/connect path to touch global state and> would equally need serialization.
Yes, I was a bit unclear about that. My idea is to have a
Mutex<Option<CString>> (or a wrapper struct) that is used to synchronize
every access to non-trivial libnitrokey functions *and* keeps track of
the connected path. (Trivial functions would be for example getters for
constants.)
> My only concern is that if two threads truly use two devices in lockstep> then there will be a lot of logouts/connects happening. We should at least> test that path thoroughly to make sure there are no spurious errors (which I> have seen a fair amount of already when running the nitrocli test suite,> btw., e.g., I recall an assertion failure in libnitrokey).
True, but please keep in mind that logout and connect are purely
client-side operations (i. e. HID enumeration, connection and cleanup).
They shouldn’t affect the actual device state.
> > This approach should enable a multi-device API while reducing the> > overhead for both the single-device and the multi-device cases to a> > minimum, namely the mutex handling.> > You mentioned that multi-device support is planned. Please excuse my> ignorance, but why not go all the way right away? Can you elaborate on the> complications, please? It does not strike me as a much more involved feat,> but I am sure I must be missing something.
The problem is that it requires a major refactoring of libnitrokey.
Currently, all logic is contained in the NitrokeyManager. It would have
to be moved to the Device class, and NitrokeyManager would be a
front-end for that. Due to the current lack of documentation and
consistent coding style in libnitrokey, I don’t want to tackle this by
myself. Also, it requires a proper amount of testing.
And there are still some pending issues and pull requests for rather
small changs. So even if I would start implementing this upstream, it
could take quite some time to land in a release. Finally, I personally
don’t want to put too much work into libnitrokey while we are still
considering doing the hidapi communication directly in Rust.
We don’t have a timeline for an upstream implementation, and I don’t
want to wait indefinitely as I think we have a quite feasible workaround
available.
> > Such a synchronization feature would only make sense in nitrokey-rs if> > it would enforce synchronization between applications, similar to pcscd,> > but I think that is out of our scope.> > I tend to agree. But do we know what the failure behavior is if multiple> applications access the same device? At that point it should no longer be> libnitrokey's state that is a problem. Does it come down to collision of> higher level primitives? E.g., one application setting a temporary password> to authenticate, colliding with another one doing the same? Is everything> else covered by USB at a different layer?
I will have to investigate that. I think a single device can only be
connected to a single hidapi instance at the same time. (For example,
nitrocli cannot access a device while Nitrokey App is running.) But I
don’t know what happens if you use different hidapi backends (i. e.
libusb and hidraw). I’ll run some tests and report back.
> > Global manager or context> > =========================> > > > Finally, we have to decide whether we want to keep a global manager or> > context struct, and if it should be protected by a mutex. The latter is> > in my opinion no longer needed. If we want to provide some kind of> > synchronization, it should be on a per-device basis.> > > > But what about a manager or context struct, similar to hidapi’s HidApi> > and pcsc’s Context? It could be used to choose the available backends,> > to keep track of the backend state and to share resources between> > devices. We don’t need it right now, but it would give us more> > flexibility when adding new backends.> > I am unfortunately not familiar with what hidapi or pcsc do or need and why.> I think without an immediate use case it's hard to say whether it's useful> down the line and, more importantly, what it should look like. At least I> have trouble figuring it out. I wouldn't say it's a big deal to add it later> on. Yes, it would possibly be a breaking change, but that does not have to> be the end of the world -- if it's just creating an object and passing it> through that should be reasonable to accommodate in most user code.
I see. Maybe we just leave that question for later and just keep the
Manager struct around for the time being. When we implemented all other
API changes, we can have another look at this question.
/Robin
On 6/11/21 7:58 PM, Robin Krahl wrote:
> Szczepan, I’ve cc’ed you because I’d like to know your opinion on the> approach described in the first section of this mail from the> libnitrokey perspective.
Hi!
Thank you for inviting me into the discussion. I will look into this
tomorrow, sorry for the delay.
Briefly reading the approach looks good for me. Multiple devices were
planned to be implemented through per-device context handling AFAIR.
libnitrokey release is pending for a couple of weeks already
unfortunately (due to some high priority tasks in other projects emerged
in between). I hope this will change soon.
Best regards,
Szczepan
--
Szczepan Zalega
Senior Software Developer
Nitrokey GmbH
https://www.nitrokey.com
Email: szczepan@nitrokey.com
Nickname: szszszsz
Rheinstr. 10 C, 14513 Teltow, Germany
CEO / Geschäftsführer: Jan Suhr
Register: AG Potsdam, HRB 32882 P
VAT ID / USt-IdNr.: DE300136599
Thanks for your response Robin.
On 14.06.21 00:25, Robin Krahl wrote:
> On 2021-06-14 06:16:01, Daniel Müller wrote:>> On 11.06.21 10:58, Robin Krahl wrote:>>> libnitrokey synchronization>>> ===========================>>>>> My only concern is that if two threads truly use two devices in lockstep>> then there will be a lot of logouts/connects happening. We should at least>> test that path thoroughly to make sure there are no spurious errors (which I>> have seen a fair amount of already when running the nitrocli test suite,>> btw., e.g., I recall an assertion failure in libnitrokey).> > True, but please keep in mind that logout and connect are purely> client-side operations (i. e. HID enumeration, connection and cleanup).> They shouldn’t affect the actual device state.
Ah, thanks for clarifying that.
>>> This approach should enable a multi-device API while reducing the>>> overhead for both the single-device and the multi-device cases to a>>> minimum, namely the mutex handling.>>>> You mentioned that multi-device support is planned. Please excuse my>> ignorance, but why not go all the way right away? Can you elaborate on the>> complications, please? It does not strike me as a much more involved feat,>> but I am sure I must be missing something.> > The problem is that it requires a major refactoring of libnitrokey.> Currently, all logic is contained in the NitrokeyManager. It would have> to be moved to the Device class, and NitrokeyManager would be a> front-end for that. Due to the current lack of documentation and> consistent coding style in libnitrokey, I don’t want to tackle this by> myself. Also, it requires a proper amount of testing.> > And there are still some pending issues and pull requests for rather> small changs. So even if I would start implementing this upstream, it> could take quite some time to land in a release. Finally, I personally> don’t want to put too much work into libnitrokey while we are still> considering doing the hidapi communication directly in Rust.> > We don’t have a timeline for an upstream implementation, and I don’t> want to wait indefinitely as I think we have a quite feasible workaround> available.
Makes sense. I actually wrongly understood your first part to be
implemented in libnitrokey, which was the main reason for the question.
Best,
Daniel
On 24.02.22 02:01, Robin Krahl wrote:
> A first patch series that extracts the nitrokey-sys backend and adds> support for multipe devices (described in the “libnitrokey> synchronization” section) is available on the mailing list and on the> backend branch:> https://lists.sr.ht/~ireas/nitrokey-rs-dev/patches/29770> https://git.sr.ht/~ireas/nitrokey-rs/log/backend> > Daniel, could you please have a look at the changes?
Absolutely! I'll get to it by the end of the weekend, the latest.
Thanks,
Daniel