Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Smartcard implementation with PCSCLITE_CSOCK_NAME #1825

Open
wants to merge 11 commits into
base: devel
Choose a base branch
from

Conversation

zorgluf
Copy link

@zorgluf zorgluf commented Mar 5, 2021

Mainly based on the great work from jsorg71 (#963).
Fix issues with recent version of pcsc (1.8.8).
Merge with a more recent version of xrdp (0.9.14).

Made it working on RHEL7.9 with a gemalto MD840 and omnikey reader.
Feel free to suggest improvement, my dev skills are pretty low.

Copy link
Contributor

@aquesnel aquesnel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for taking to time to fix this up. I don't know the smart card code at all, so I can't comment on if there are bugs or not. I've only left some comments to help make the code easier to understand.

Comment on lines 1957 to 1925
val = recv_ior->cbPciLength > 0 ? 0x0002000c : 0;
val = recv_ior_is_null ? 0 : 0x00020008;
out_uint32_le(s, val); /* map 4 */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a comment or a constant for the 0x00020008 value to say where this value comes from. (Eg. If this value is a constant from a specification, please reference the specification, or if this value is derived, then add a comment to show the calculation.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -144,6 +144,8 @@ env_set_user(const char *username, char **passwd_file, int display,
g_setenv("XRDP_SESSION", "1", 1);
/* XRDP_SOCKET_PATH should be set even here, chansrv uses this */
g_setenv("XRDP_SOCKET_PATH", XRDP_SOCKET_PATH, 1);
g_sprintf(text, XRDP_PCSC_STR, display);
g_setenv("PCSCLITE_CSOCK_NAME", text, 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this environment variable name used only by xrdp chansrv, of is there another program/library that reads this environment variable?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another program reads it (pssc-lite).

sesman/session.c Outdated
g_snprintf(file, 255, XRDP_PCSC_STR, display);
if (g_file_exist(file))
{
log_message(LOG_LEVEL_DEBUG, "cleanup_sockets: deleting %s", file);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the logging macro LOG instead of the log_message function directly everywhere in your pull request because the LOG macro adds extra debug info when xrdp is compiled with the debug flag.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, done.

Comment on lines 604 to 601
out_s = uds_client->con->out_s;
init_stream(out_s, 8192);
out_uint32_le(out_s, 4);
out_uint32_le(out_s, 3);
out_uint32_le(out_s, 0x80100001); /* result */
s_mark_end(out_s);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For all of the places in the code where you have added sending or receiving a message, can you please add a comment or trace log message to say what is the message type that is being transmitted.
Can you please add comments and/or constants for the values in the message to clarify what is the meaning of the message.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fully understand your point, but unfortunately most of the code has been written by jsorg71 and I don't master it all... However I can manage to grab information through microsoft documentation about rdp protocol, but I need some extra time...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added comment on this part of code, and on 2 others parts (or fix them). Don't hesitate to report any other uncommented parts.

Comment on lines +696 to +702
if (status == 0)
{
in_uint8s(in_s, 16);
in_uint32_le(in_s, return_code);
}
out_s = trans_get_out_s(con, 8192);
s_push_layer(out_s, iso_hdr, 8);
out_uint32_le(out_s, status); /* SCARD_S_SUCCESS status */
init_stream(out_s, 0);
out_uint32_le(out_s, 0); /* hContext */
out_uint32_le(out_s, return_code); /* rv */
s_mark_end(out_s);
bytes = (int) (out_s->end - out_s->data);
s_pop_layer(out_s, iso_hdr);
out_uint32_le(out_s, bytes - 8);
out_uint32_le(out_s, 0x02); /* SCARD_RELEASE_CONTEXT 0x02 */
return trans_force_write(con);
rv = trans_write_copy(con);
uds_client->ref_count--;
free_uds_client(uds_client);
return rv;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Todo for reviewer: review this section in more detail because this diff seems odd that the steam is being reinitialized and it looks like there was data in the stream in the old version. Is this just an artifact of the diff?

@matt335672
Copy link
Member

Hi @zorgluf

Thanks for taking the time to contribute this, and for getting involved.

I'll dig out a spare yubikey and take a look at this when I can. At the moment however, I'm a bit snowed under with other PRs, which is great, but means I'll take a while to get to this, for which I apologise.

I see you've got a couple of minor issues above at the moment:-

  1. There's a conflict with a couple of files.
  2. The FreeBSD CI check is failing

I also see you've merged in to 0.9.14 above, which is fine, but will lead to a complicated git history when this (eventually) gets to devel.

Are you able to remove the last commit above and rebase the others on the latest devel? I'll be happy to give you a hand with this if you need it.

@aquesnel
Copy link
Contributor

Hi @zorgluf
I'd be happy to help test this out too, but I don't know how to setup xrdp and a client to use a smart card for authentication. Could you please help by writing up your server and client configuration and maybe adding it to the wiki pages?

@zorgluf
Copy link
Author

zorgluf commented Apr 2, 2021

Hi @zorgluf

Thanks for taking the time to contribute this, and for getting involved.

I'll dig out a spare yubikey and take a look at this when I can. At the moment however, I'm a bit snowed under with other PRs, which is great, but means I'll take a while to get to this, for which I apologise.

I see you've got a couple of minor issues above at the moment:-

  1. There's a conflict with a couple of files.
  2. The FreeBSD CI check is failing

I also see you've merged in to 0.9.14 above, which is fine, but will lead to a complicated git history when this (eventually) gets to devel.

Are you able to remove the last commit above and rebase the others on the latest devel? I'll be happy to give you a hand with this if you need it.

Thanks for the review !
The FreeBSD CI check is now OK.
I am not familiar with complexe manipulation with git commit, so I didn't manage to remove cleanly the last commit. Instead, I merge the devel branch into mine and create a new commit. Is it enough ?

@zorgluf
Copy link
Author

zorgluf commented Apr 2, 2021

Hi @zorgluf
I'd be happy to help test this out too, but I don't know how to setup xrdp and a client to use a smart card for authentication. Could you please help by writing up your server and client configuration and maybe adding it to the wiki pages?

Hi @aquesnel
This pull request is not about smart card authentication against xrdp, but forwarding a smartcard reader through the rdp session to the remote host.
A basic test for this feature is to use the pcsc-tools on the server : a pcsc_scan command should detect the readers and card inserted on your client side (don't forget to activate smartcard reader forwarding on your xrdp client). But this test is so basic that it cannot test all the functionnalities.
Fully testing this feature highly depends on your smartcard, since you will need :

  • for some exotic readers drivers on your client side
  • the middleware of your smartcard installed on the server side, with the pcsc-lite package.
    The pcsc-lite lib will read the PCSCLITE_CSOCK_NAME env variable, which communicate with the client reader.
    The middleware on server side communicate with the pcsc-lite lib, at least all middleware I am aware of working on linux.
    Hope it answer your questions. If it's still relevant, I can add this information on wiki pages, just tell me where...

@avolkov-astra
Copy link

Hi.
Here are these changes rebased onto devel branch: https://github.com/avolkov-astra/xrdp/commits/pcsc_devel
This way they are easier to review.
The correctness of rebasing can be checked by comparing final source trees (e.g. with kdiff3)
The most ineteresting difference is the line "scard_send_cancel(0, context->context, context->context_bytes);" which was removed by a commit of jsorg71, but returned later, probably by mistake.

@matt335672
Copy link
Member

Hi @avolkov-astra

Thanks for taking the trouble to do this.

@zorgluf - thank you for sticking with this. As @avolkov-astra says, it's well worth learning the basics of github rebasing as it makes it easier to communicate with folks both here and on gitlab. Furthermore, when you get the hang of that, a lot of the weirdness of git will make more sense (it did for me anyway).

There's a description of rebasing here. It's a bit dry though. I'd recommend just having a play with a copy of a repo and the git rebase -i command which helps a lot.

@matt335672
Copy link
Member

I've just started taking a look at this, and working out where we are - I haven't tried it yet.

@zorgluf - as far as I can tell, your main changes to @jsorg71's original branch are those in commit 2fe0a4e. Is that correct?

@zorgluf
Copy link
Author

zorgluf commented Jun 10, 2021

@zorgluf - as far as I can tell, your main changes to @jsorg71's original branch are those in commit 2fe0a4e. Is that correct?

Correct. Not so much changes. The other commit add comments and some raw int value converted to const variables.
98d5c8b remove also a part of dead code that cannot be accessed in code logic.
Sorry for the mess with all those commit. If it's not reviewable enough, I can try to push an other pull request with less commits.

@EmperorArthur
Copy link

I'd git isn't working for you, here's the simple solution.

  • First, clone the official devel branch to a different folder.
  • Make a new branch in that new folder.
  • Use git remote to add your GitHub repository to the folder.
  • Now use a diff tool to find everything that's changed between your folder and the devel folder.
  • Either copy the files over, or manually cut and paste the changes. Committing at logical times.
  • Do a git push to the new branch in your GitHub repo.

@matt335672
Copy link
Member

@zorgluf - I'm having trouble getting pcsc_scan working with a USB-A Yubikey 5 (which is all I've got).

Before I dive in with a debugger, I just thought I'd check with you that I'm not doing anything stupid.

I'm running off of @avolkov-astra's branch at the moment.

On a native Mint machine (pcsc-tools 1.5.5-1) I'm getting this:-

$ pcsc_scan -rv
Using reader plug'n play mechanism
Scanning present readers...
0: Yubico YubiKey OTP+FIDO+CCID 00 00

On an xrdp session to an Ubuntu 20.04 VM (pcsc-tools 1.5.5-1) the pcsc_scan -rv hangs, generating no output.

  • I've redirected smartcards in mstsc.exe
  • PCSCLITE_CSOCK_NAME is set to /tmp/.xrdp/xrdp_pcsc_socket_11 in my session.
  • The last lines in the chansrv log are as follows:-
    [20210611-14:55:43] [DEBUG] [scard_process_msg(smartcard_pcsc.c:1972)] scard_process_msg: command 0x0013
    [20210611-14:55:43] [INFO ] [scard_process_msg(smartcard_pcsc.c:2062)] scard_process_msg: CMD_WAIT_READER_STATE_CHANGE
    [20210611-14:55:43] [DEBUG] [scard_process_cmd_wait_reader_state_change(smartcard_pcsc.c:1693)] scard_process_cmd_wait_reader_state_change:
    [20210611-14:55:43] [DEBUG] [scard_process_cmd_wait_reader_state_change(smartcard_pcsc.c:1695)] scard_process_cmd_wait_reader_state_change: timeOut 2
    [20210611-14:55:43] [DEBUG] [scard_pcsc_get_wait_objs(smartcard_pcsc.c:450)] scard_pcsc_get_wait_objs:
    [20210611-14:55:43] [DEBUG] [get_timeout(chansrv.c:153)] get_timeout:
    

Is there anything else obvious I need to do which I haven't?

Thanks.

@matt335672
Copy link
Member

@zorgluf - what platform are you running on?

I see you've added changes for pcsc-lite 1.8.8. I'm running 1.8.26 (on Ubuntu 20.04) and this won't work.

The main problem seems to be that in the pcsc-lite 1.8.26 in winscard_clnt.c, an additional CMD_WAIT_READER_STATE_CHANGE message is sent here.

Your routine which is correspondingly called here doesn't handle this well. There's no return message, and also the timeout read from the stream doesn't exist.

Are you able to try running with pcsc-lite 1.8.26? The error can then be demonstrated by running pcsc_scan with no smartcard at all attached.

@zorgluf
Copy link
Author

zorgluf commented Jun 15, 2021

@matt335672 thanks for the report. I was using rhel7 for my dev, and yes it's probable due to a newer pcsclite version introducing a new behavior badly handled, since my yubikey works on a 1.8.8 version.
Thanks for pinning the part of the faulty code, I will propose a fix. Unfortunatly, it will take me a few days, quite busy at the moment, but will definitively do it.

@matt335672
Copy link
Member

Thanks @zorgluf

I think I owe you an apology. The faulty routine scard_process_cmd_wait_reader_state_change() isn't anything to do with you - it was added as part of commit fe60eeb.

A couple of things which may be useful to you:-

  • If you're happier working on RHEL/CentOS/Etc, RHEL8 has pcsc-lite 1.8.23. There are minor differences with 1.8.26 but from a first glance nothing looks that different.
  • If you rebase to the latest xrdp devel, or merge in the latest changes, you'll find that the configure option --enable-xrdpdebug has been replaced with --enable-devel-all. This includes extra checks when developing which may be useful to you. The routine scard_process_cmd_wait_reader_state_change() currently reads a value timeOut from the stream which isn't there. With --enable-devel-all this is now detected at runtime.

If you're interested in the latter option, I've set up a branch in my own repository which rebases the work by @avolkov-astra onto the latest devel. You can use it in your own working repository with these commands:-

git remote add matt335672 https://github.com/matt335672/xrdp.git
git fetch matt335672
git checkout -b aa_pcsc_devel --track matt335672/aa_pcsc_devel

If you go down this route, at some point you'll be able to overwrite your existing pcsc_0.9.14 branch with these commands (reference here):-

git checkout pcsc_0.9.14
git tag old_pcsc_0.9.14  ; # So we've got a copy
git reset --hard aa_pcsc_devel

You can then update your PR in git hub using git push as normal, but specifying the -f flag.

I appreciate that's a lot to take in! If you've got any questions, please let me know.

@matt335672
Copy link
Member

A quick addition to the above; at the moment my branch aa_pcsc_devel isn't passing CI checks.

That's probably something you don't need to worry about yet; we can fix that later.

@zorgluf
Copy link
Author

zorgluf commented Jul 19, 2021

@matt335672 thank you very much for your time on this request, it helps a lot.
I followed your instructions and add 2 commits from your rebased repo.
There was in fact two issues :

  • A change of behavior in pcsc lite libraries. Now the code is tracking protocol version to handle both behaviors
  • Redhat package of pcsc-lite has a patch to rise the PCSCLITE_MAX_READERS_CONTEXTS value of pcsc-lite. So now the code read this value form the devel version of the lib (requirement added to configure.ac). I also modified the Makefile.am in chansrv, maybe need some review (hard coded /usr/include directory).

Code tested on my original RHEL7 server and also on ubuntu 20.04.

Copy link
Member

@matt335672 matt335672 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments on fixes

@@ -1686,18 +1689,30 @@ scard_process_cmd_wait_reader_state_change(struct trans *con,
struct stream *in_s)
{
int rv;
//struct stream *out_s;
int timeOut;
int timeOut __attribute__((unused));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't need __attribute__((unused)) here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On RHEL, without this dircective, I have the following error :

smartcard_pcsc.c: In function âscard_process_cmd_wait_reader_state_changeâ:
smartcard_pcsc.c:1688:9: error: variable âtimeOutâ set but not used [-Werror=unused-but-set-variable]
     int timeOut;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry - you are of course quite correct, and this comes about because of the LOG_DEVEL() macro.

We've got a temporary macro you can use to do this, rather than the attribute marking. Can you try the following:-

int timeOut;
. . 
UNUSED_VAR(timeOut);

If you grep through for UNUSED_VAR you'll find a couple of uses of it.

Even better would be to move the declarations closer to the point-of-use (i.e.) :-

    int rv;
    <some declarations deleted here>
    struct pcsc_uds_client *uds_client;

    LOG_DEVEL(LOG_LEVEL_DEBUG, "scard_process_cmd_wait_reader_state_change:");
    uds_client = (struct pcsc_uds_client *) (con->callback_data);
    uds_client->waiting = 1;
    if ((uds_client->vMajor == 4) && (uds_client->vMinor > 2)) {
        //In these versions, return reader states immediately
       struct stream *out_s = con->out_s;
        int reader_state_bytes = sizeof(uds_client->readerStates);
        init_stream(out_s, reader_state_bytes);
        out_uint8a(out_s, uds_client->readerStates, reader_state_bytes);
        s_mark_end(out_s);
        rv = trans_write_copy(con);
    } else {
        int timeOut;
#ifndef USE_DEVEL_LOGGING
        /* TODO: remove UNUSED_VAR once the `timeOut` variable is used for more
        than logging in debug mode */
        UNUSED_VAR(timeOut);
#endif
        in_uint32_le(in_s, timeOut);
        LOG_DEVEL(LOG_LEVEL_DEBUG, "scard_process_cmd_wait_reader_state_change: timeOut %d",
           timeOut);
        in_uint32_le(in_s, rv);
        LOG_DEVEL(LOG_LEVEL_DEBUG, "scard_process_cmd_wait_reader_state_change: rv %d", rv);
    }

It's not pretty, but it's the same as we've used elsewhere.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, modified with UNUSED_VAR macro.

uds_client = (struct pcsc_uds_client *) (con->callback_data);
uds_client->waiting = 1;
rv = 0;
if ((uds_client->vMajor == 4) && (uds_client->vMinor > 2)) {
//In these versions, return reader states immediatly
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

immediatly -> immediately

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, modified

@@ -1650,7 +1652,8 @@ scard_process_cmd_version(struct trans *con, struct stream *in_s)
LOG_DEVEL(LOG_LEVEL_DEBUG, "scard_process_version: major %d minor %d", major, minor);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please replace this with LOG(LOG_LEVEL_INFO so we get useful error reports from users!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, modified

sesman/chansrv/smartcard_pcsc.c Outdated Show resolved Hide resolved
@matt335672
Copy link
Member

Hi @zorgluf

Thanks for sticking with this.

I'm not at all happy (sadly) with the 2nd of your commits I'm afraid. The reason is that if we're going to pull in the pcsc development package, we should really use the library where possible in the same way as the daemon. That's a lot of work! The current approach is a bit flaky in that it won't cope well with version changes, but we don't have a dependency.

I'd suggest (instead), just increasing the MAX_READERS value and adding a comment, e,g, :-

/* Fedora increases the MAX_READERS value from the pcsc-lite default */
# define MAX_READERS 48

You could also add a log message to the test in scard_readers_to_list() if we overrun this limit. So replace this code:-

            reader_index++;
            hold_reader = uds_client->readerStates[reader_index];
            if (reader_index > (MAX_READERS - 1))
            {
                return 0;
            }

with:-

            reader_index++;
            if (reader_index > (MAX_READERS - 1))
            {
                 LOG(LOG_LEVEL_WARNING,  "Max smart card readers exceeded");
                return 0;
            }
            hold_reader = uds_client->readerStates[reader_index];

I've moved the assignment to hold_reader, as if the overflow occurs at the moment the behaviour is undefined.

@zorgluf
Copy link
Author

zorgluf commented Jul 21, 2021

Hi @matt335672 ,
Thank you also to stick to this long pull request...

The problem is that the readerStates array (of size PCSCLITE_MAX_READERS_CONTEXTS) is send to the local pcsc lib and this lib is expecting the exact size of this array (there is no size check).
On xrdp size :

reader_state_bytes = sizeof(uds_client->readerStates);
init_stream(out_s, reader_state_bytes);
out_uint8a(out_s, uds_client->readerStates, reader_state_bytes);

and in the pcsc lib :
rv = MessageReceive(&readerStates, sizeof(readerStates), dwClientID);
If the size of the array differs (below or over), it doesn't work (tested).

I really don't have any more idea to fix this. Even between RHEL7 and RHEL8, this value change...
Maybe make this dependency and the smartcard redirection channel optional in configure ?

@matt335672
Copy link
Member

Thanks for your clear explanation of the MAX_READERS_CONTEXTS problem.

I think the situation is as follows. Please correct me if I'm wrong:-

  • At the moment smartcard processing doesn't work in the xrdp devel branch
  • The current design aims to emulate pcsd without introducing a dependency on pcsd
  • It's not really practical to emulate pcscd without having the pcsc-devel package installed

If that's the case, a sensible way forward seems to be to be:-

  • Introduce an optional dependency on pcsc-devel, so that users who do not need smartcard support (currently all of them) do not have to introduce the dependency.
  • Disable smartcard processing entirely if pcsc-devel is not present
  • Over time, replace some of the pcscd emulation code in xrdp with calls to the pcsc library, to reduce our dependency surface.

I'm not expecting you by any means to attempt this all on your own by the way - I'll be happy to help.

If that sounds reasonable to you, we can broaden this discussion out.

@EmperorArthur
Copy link

Might I suggest that everything except the MAX_READERS_CONTEXTS be merged. That may give anyone using the official (non OS) release a working feature.

While I have not examined this in detail, changing a protocol implementation without adjusting a version or otherwise giving an indication to the other party is what I would consider a major bug.

As I said, I have not looked at this in detail. However, if xrdp is sending data to such a buggy program, then you will have to deal with the potential for the client and server to have different protocol versions. And a host of other issues.

While I suggest ignoring it for this PR, you can also solve the issue by dynamically determining size at runtime. Just start by sending one and keep incrementing the number until the recipient stops failing.

@matt335672
Copy link
Member

Thanks for the suggestion @EmperorArthur

It's worth pointing out that the interface we're implementing here is not public - it's built in to the pcsc-lite project. I can see why the xrdp design has taken the route it has, but I'm personally not happy with it, hence my suggestion that we move towards using the pcsc-lite library if possible.

Also, I don't believe the approach you are also suggesting is going to work, as the data in question is sent in response to a client request. On the first failure we've lost the client.

@zorgluf
Copy link
Author

zorgluf commented Aug 2, 2021

I withdrew the patch with dependency against libpcsclite. Instead I suggest to fix the MAX_READERS value to 16, which is the "official" value PCSCLITE_MAX_READERS_CONTEXTS in libpcsclite.
For RHEL releases, this value has to be handle externaly with a patch, as what is made actually with libpcsclite.
At least it makes a "working version", that might interest some users.

@matt335672 : I totaly agree with you, the interface used to handle smartcard communication (internal socket protocol of libpcsclite and unsupported interface) is not a good and long term way to get the functionality. But the interface is quite close the way RDP handle smartcard redirection, maybe it was more convenient ?
An other way wight be to develop a PCSC virtual driver (like http://frankmorgner.github.io/vsmartcard/virtualsmartcard/README.html). But at first lecture, interfacing pcsc driver primitives and RDP smartcard channel need major rewrite. I will have a look... for an other pull...

@matt335672
Copy link
Member

Hi @zorgluf - thanks for sticking with this. I can't get to my Windows machine at the moment to test this, but I will do so as soon as I can.

@metalefty , @jsorg71 - I need some input from one of you on this. This PR moves us forward from where we are with smartcard support (which doesn't work at all at the moment), but more work will be needed in the future to support big-endian machines, and possibly to use pcsc-lite libraries directly to reduce the maintenance overhead (this is discussed in more detail above). Assuming the testing works out I'm happy to merge this as it is, but I'd appreciate your input before I do.

@metalefty
Copy link
Member

@matt335672 Unfortunately, I've never used the smartcard feature. I can do very few things on this.

@matt335672
Copy link
Member

@metalefty - understood

Would you be happy if I worked with @zorgluf on merging this straight after 0.9.17 is released? There's a lot to do here, but I think having something working for some people would be a great step forward.

@metalefty
Copy link
Member

Yes, I'm happy with that.

@spstarr
Copy link

spstarr commented Apr 27, 2022

What is the status of this PR?

@matt335672
Copy link
Member

It's not been looked at I'm afraid. We're looking at other things at the moment for the next major release, and there's no effort available to working on this one.

@spstarr
Copy link

spstarr commented Apr 13, 2023

Ping on this, is this possible to be raised in a priority request? More and more people are using 2FA for security and this is a good thing.

I can test builds, but I do not know the pcsc framework to write any code here.

@matt335672
Copy link
Member

@zorgluf - my apologies for not getting to this sooner.

I've rebased this to the latest devel at https://github.com/matt335672/xrdp/tree/pcsc_updates. I've kept attributions to @jsorg71 and @zorgluf as appropriate. An additional commit fixes potential memory leaks found by the latest cppcheck.

The good news is it seems to work from some very limited testing. I can see my Yubikey using the yubico-piv-tool over the link on Ubuntu 22.04, pscsclite 1.9.5-3

Looking at PRs in this area over the last few years, I see that #2454 has now been effectively removed. The PCSC socket is created in the XRDP socket directory and although the socket file has 660 permissions, the following statement should be borne in mind from UNIX(7) under 'Pathname socket ownership and permissions' :-

POSIX does not make any statement about the effect of the permissions on a socket file, and on some systems (e.g., older BSDs), the socket permissions are ignored. Portable programs should not rely on this feature for security.

In other words this isn't production ready yet. I think I need to go back to revisit #2731 at this point and get that merged. That will give me the tools to fix any potential permissions issues. I'll also try to get a more satisfactory solution to the MAX_READERS problem mentioned above.

@jsorg71
Copy link
Contributor

jsorg71 commented Oct 23, 2023

I did create https://github.com/jsorg71/libpcscd in an attempt to isolate the communication with libpcsclite. I was thinking to use it as a submodule. Maybe I can get Ludovic interested in it. It would be great if pcscd also used this library. I also tried to document the pcsclite protocol, I'll see if I can find that.

@matt335672
Copy link
Member

I've rebased https://github.com/matt335672/xrdp/tree/pcsc_updates on the latest devel again.

That now includes #2731, which means the PCSC socket is better protected than it was.

There's also a parallel discussion going on in #2625 where we're trying to work out what the best long-term architecture for this is.

@tymaoa2
Copy link

tymaoa2 commented Dec 7, 2023

Hi @matt335672 , I wish to use Yubikey as webauthn through XRDP, the key is plugged on my NB, and I want to use the key in the remote Ubuntu VM.

I follow your instruction to recompile XRDP on Ubuntu VM from the repo https://github.com/matt335672/xrdp/tree/pcsc_updates, however my RDP linking will shutdown immediately after I finish my account/password login on Windows remote desktop connection.

I also recompile the xorgxrdp according to its repo (devel branch).

Looking infos in the .xorgxrdp.10.log, there's an error: (EE) systemd-logind: failed to take device /dev/dri/card0: Operation not permitted

Wish you could give me some instructions, thank you.

@matt335672
Copy link
Member

That error's pretty normal.

Look in the sesman log. It's likely your session is terminating very quickly. You're not logged in on the console are you?

As a warning, it's very unlikely this branch will be merged now. We're working on a new approach which doesn't have the same dependency on PCSC-Lite internal interfaces.

@matt335672
Copy link
Member

Also, and more importantly, this interface only works with authentication stacks using libpcsclite. Your WebAuthn stack may or may not use this interface.

@tymaoa2
Copy link

tymaoa2 commented Dec 8, 2023

@matt335672 Thx for the reply :) , I wish to run the compiled XRDP successfully then check WebAuthN is valid or not then.

After I compiled xrdp, I sudo ln -s /usr/local/sbin/xrdp{,-sesman} /usr/sbin, then compiled the xorgxrdp.
I sudo systemctl start xrdp then, but the service failed with log "xrdp.service: Failed with result 'core-dump'.", and I do not find further infos.

Then I run the xrdp and xrdp-sesman by type the command in the terminal manually, no sure it is a correct way or not.
I also don't have xrdp-chansrv process running in this scenario.

@tymaoa2
Copy link

tymaoa2 commented Dec 8, 2023

I try to compile xrdp on another brand new Ubuntu.
I clone https://github.com/matt335672/xrdp.git recursivelly, switch to pcsc_updates branch and compile the xrdp.
I then clone https://github.com/neutrinolabs/xorgxrdp.git and compile the xrogxrdp.
I then sudo service xrdp start, however the error occurs:
/etc/init.d/xrdp: 48: .: Can't open /usr/share/xrdp/socksetup

@matt335672
Copy link
Member

@tymaoa2 - there's a lot you need to get working before you can even look at this.

  1. You have to use systemctl and systemd on Ubuntu. Don't use the service command.
  2. You have to be running xrdp-sesman at least as a service under systemd. Running it from the command line won't work as you will be unable to start systemd sessions.
  3. You have to use chansrv as this is the component which implements the virtual channel for the smartcard.

Bear in mind also that this is a development branch. If you want to work on something like that you need to be aware of things like the above.

You may be better off waiting until something more substantial is ready for some more user-focussed (rather than developer-focussed) testing. Otherwise I can see you wasting a great deal of time on this and not achieving anything useful.

@r34789000
Copy link

I have a problem with incorrect identification
Omnikey HID 3121 ID 076b:3031
It should be Omnikey HID 3121 ID 076b:3121
Can you help in which file the information about
registration for editing in Ubuntu server 16.04 is written

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants