Add support for multiple devices per user

[mcdope]

Currently pam_usb can't handle users having more then one device defined, see #28. This is long standing issue and related to the XML based configuration handling.

For now this was solved by making sure pamusb-conf doesn't add devices for users that are already configured. However, this is actually a valid use case in my opinion and should be supported.

Current assumption: Users are located in the config file by their id, but pamusb-conf doesn't add a second device in the existing user - instead it adds another user entry. Which will mess with the DOM parsing since we now have two DOMNodes having id="username". This needs to be fixed. The PAM module itself will most likely require changes too, so it iterates over all devices for the user instead of just assuming there is a single one.

[Fuseteam]

wait so when you say "devices" you mean USB devices? what about the usecase for multiple computers with the same username?

[mcdope]

Multiple computers with the same username is already supported

[AlexOConnorHub]

I'm hoping to help out with this issue. While looking at the current code and the XML standard, I could only think of one way to resolve this issue -> use json instead of xml. Is there a better idea someone has had that would be simpler to implement? Maybe some way to restructure the xml? I am willing to work on this issue and would like to first see what ideas are out there before I start writing code.

The reason I was thinking json is because it would allow for something like users -> user -> device_1 and users -> user -> device_2, and this could also allow for device specific settings specific the the user -> device -> setting level. Another pro of json is it's a newer standard, and would use less space in the pam_usb.conf file (although not much of a worry). Similar to xml, there are multiple simple c libraries that allow for read/write of json.

[mcdope]

Well, your supposed structure can be done in XML as well - in fact I did just that some days ago for one of my commercial thingies.

But like you say, the XML is huge - even if that's not really a concern. A larger issue is that the parsing itself is crappy too. There are other ways, except adding multiple id'd tags, to break the parsing too. Though I never managed to reliable reproduce that. Not to mention the insane indentation the conf tool creates sometimes - as a webdev I'm really triggered by that ^^

But changing the config file format would be quite a task, guess will be three-digit-count of changed lines. Also this would require some migration to make a JSON conf out of the existing XML so we can upgrade existing installs.

I could handle the debian packaging integration, so apt will run the migration script. But with the remaining work you would be on your own for now.

tl;dr: feel free to create a PR, but make sure to migrate existing configurations when changing to a different format.

[AlexOConnorHub]

True, the indentation at times is wacky, I literally copied it into an IDE, formated, and copied back while setting it up 😅. Two follow up questions.

1. Could you give some sudo xml outlining how this might be accomplished, I don't use it that much, so I would need some sort of example if I were to go that route.

2. Any idea where I would put migration code? Doing it shouldn't be too bad, but I don't know where I would put that.

[mcdope]

XML could look like this:

<users>
    <user id="$usernameHere">
        <devices>
            <device>
                <!-- device options tags we have, serial etc -->
            </device>
            <!-- zero to N addditional device tags>
        </devices>
    </user>
</users>

With this structure code would be looping over all devices until it finds the first connected one. (Using drive serial etc for ID content wouldn't be unique enough, seen to many crappy media to trust that info)

About the migration: guess best option would be a dedicated method in the conf tool which can be invoked by command line argument (so we can trigger it from debconf) and also is invoked auto-magically when conf tool is used and detect an existing xml config (so we cover installations not using the deb package too, like arch or source builds).

[mcdope]

Addition: of course the xml code would need to stay for the migration to work, so the existing conf can be loaded. 2 or 3 releases after the new config we could then remove the xml code.

[mcdope]

FYI: just found a very old branch containing stuff for multiple devices. But unsure how useful it will be considering the age.

See aluzzardi/pam_usb@master...dj95:pamusb-multiple-devices

[azzydoesgit]

FYI: just found a very old branch containing stuff for multiple devices. But unsure how useful it will be considering the age.

See aluzzardi/pam_usb@master...dj95:pamusb-multiple-devices

Merging with aluzzardi's branch seems fine, I'll try and see what would have to be changed to merge with this branch

[azzydoesgit]

FYI: just found a very old branch containing stuff for multiple devices. But unsure how useful it will be considering the age.
See aluzzardi/pam_usb@master...dj95:pamusb-multiple-devices

Merging with aluzzardi's branch seems fine, I'll try and see what would have to be changed to merge with this branch

Making the changes to xpath.c does not work. How naive of me to think this would be easy.

[mcdope]

aluzzardis code is highly outdated and maybe not even compatible (in terms of merging) anymore. You shouldn't test with that, but with this codebase if you want to work on this.

[azzydoesgit]

aluzzardis code is highly outdated and maybe not even compatible (in terms of merging) anymore. You shouldn't test with that, but with this codebase if you want to work on this.

I'd love to work on this.

Could you (or anybody else) give me an overview on what would have to be changed? Certainly the xpath.c file, or whatever else is used to run the auth check (so that it iterates through a list of devices). We would also have to update the pamusb-conf tool to allow for adding multiple devices via the CLI.

What else has to be changed?

[mcdope]

Can't remember, quite a time since I locked at this.

I will make a list for you later this day. 👍

[mcdope]

Here we go. But keep in mind, this is NOT an exact description or implementation template - most likely I missed some bits. You still need to check if maybe more is required etc pp, but I guess this will give you a good starting point.

There are some scripts existing in the tests folder which you can you use to check if your changes break something obviously. But keep in mind these tests suck, they are just there to catch really bad errors.

Feel free to open a PR, even if not finished yet, to use the automatic CI. After I authorized it once to run it will then auto-run when you push new commits and run the tests on/against them.

The CI also supports building, installing and testing on your own server if you have one. For this to work you would need to set some secrets in your Github account. If you are interested in that I can write a small Wiki entry on it.

---

conf.c:
- needs support for multiple devices, this mainly affects the xpath queries most likely

device.c
- needs support for multiple device, this mainly affects pusb_device_check() I guess

pamusb-conf:
- must be able to properly handle N devices for user, there should be no limit to the amount of configured devices (if we do it, lets do it properly)
- check ensuring only one device is added needs to be removed

pamusb-agent:
- must ensure it doesnt trigger by accident / incorrectly (see example 1).

example 1: user configures two devices, i.e one USB-Stick and one SD-Card. Usually he uses the SD, because it can be left in the laptops cardreader without annoying. Now the user needs to transfer some files for which he uses his USB-Stick also configured for pamusb. He copies the files and removes the thumb drive, but his SD-Card is still in the reader. In this scenario the unplug signal received from the USB-Stick should be ignored since there is still another valid device plugged in.

[mcdope]

todo after implementing: test how agent behaves if lock was triggered by devA and unlock by devB and similiar scenarios

[mcdope]

Testing in VM looks actually quite good. Some smaller issues, but the general principle works. "Buggy" output fixed now too.

Todo:

• Adjust pamusb-conf
• Adjust pamusb-agent
• Adjust Wiki

1. log_vmtest_multidevice_both_connected.txt
2. log_vmtest_multidevice_first-in-config_disconnected.txt
3. log_vmtest_multidevice_first-in-config_disconnected-then-reconnected.txt
4. log_vmtest_multidevice_second-in-config_disconnected.txt
5. log_vmtest_multidevice_both_disconnected.txt

pamusb-conf now properly adds a second device for an existing user, tests adjusted too.

Still todo: Adjust pamusb-agent, right now it only watches for the first defined device of the user.

[mcdope]

This is pretty much finished, but needs agent support.

Too lazy to get used to Python again since I personally don't need multi-device support.

In other words: looking for people to contribute the agent code for this

[mcdope]

The feature can now be previewed, all basic functionality is there.

However, well possible there are still some bugs left. It's only VM-tested for short time now. Also it's still full of debugging output that will vanish before it will be released.

You can find signed .deb and .rpm packages in this Dropbox folder: https://www.dropbox.com/scl/fo/piormhnmfnqquubspwu4o/AM_yFraRt9H6qUt3umqDY8E?rlkey=o8idyui7cf1klmteqh5m35ov2&dl=0

Be aware: If you install this build AND configure multiple devices, your config wont be backwards compatible anymore afterwards. To downgrade to another version you will have to manually remove all but one device from your user section in the config.

[mcdope]

Todo:

• agent shouldn't lock if another device is still connected

[mcdope]

Fixed