Move scp to standard transport #1925

matt335672 · 2021-06-18T11:22:14Z

The PR is based on part of #1602.

#1602 is in two parts:-

Move SCP away from using struct SCP_CONNECTION as a connection type to the more standard struct trans transport type used everywhere else
Add the capability to use UNIX domain sockets for SCP

Both of these are necessary moving forwards to modernise SCP. This PR focusses on the first half only.

I've taken @jsorg71's original commits and rebased them. I've then completely removed the struct SCP_CONNECTION to avoid confusion, and to ensure that the transition to struct trans is complete. This has entailed updating the utilities.

There's still a lot to do for SCP. This is only a start.

aquesnel · 2021-07-15T02:44:09Z

sesman/libscp/libscp_v1s.c

+        case 1:
+            s->current_cmd = cmd;
+            result = scp_v1s_init_session(t, s);
+            break;


is it worth it to change these magic numbers to constants now or will all of this code be refactored with your next change?

I was going to replace all of this, but intentions are not the same as reality! I'll replace the magic numbers.

aquesnel · 2021-07-15T02:57:35Z

sesman/notes.txt

@@ -0,0 +1,58 @@
+


Assuming that committing this file was intentional, can you please add a header, or change the file name to clarify that this file is documenting the SCP protocol messages.

aquesnel · 2021-07-15T03:06:34Z

sesman/scp_v1.c

+    e = scp_v1s_list_sessions42(t, scount, slist);
+    if (SCP_SERVER_STATE_OK != e)
    {
-        /* here goes scp resource sharing code */
+        LOG(LOG_LEVEL_ERROR, "scp_v1s_list_sessions42 failed");
    }

-    /* cleanup */
-    if (do_auth_end)
+    return SCP_SERVER_STATE_OK;


the log message here is at the error level, but the connection is not disconnected. I think this should be lowered to Warning.

aquesnel · 2021-07-15T03:09:25Z

sesman/scp_v1.c

    {
-        auth_end(data);
+        e = scp_v1s_connection_error(t, "Internal error");
+        LOG(LOG_LEVEL_INFO, "Cannot find session item on the chain");


What's the "chain"? can you please make this log message more informative, like "The server does not have a session with id %s"?

aquesnel · 2021-07-15T03:17:11Z

sesman/sesman.c

+        /* reset for next message */
+        self->header_size = 8;
+        self->extra_flags = 0;
+        init_stream(self->in_s, 0);


setting the transport input stream to size zero doesn't feel right. Can you please clarify this?

I'll add a comment. The effect is to reset the stream pointers, but leave the buffer size unchanged.

aquesnel · 2021-07-15T03:27:59Z

sesman/sesman.c

+sesman_listen_conn_in(struct trans *self, struct trans *new_self)
+{
+    struct sesman_con *sc;
+    if (g_con_list->count >= 16)


why a maximum of 16 connections to sesman? are the connections to sesman short lived or do they last the duration of the client rdp connection?

The '16' is somewhat inherited. Your question is however, like most of your questions, very incisive.

At the moment, connections to sesman are short lived, and last for the duration of making a request and receiving a response.

This may, however, have to change in the future. There are two reasons I'm aware of for this:-

To support reconnections, sesman is going to need to keep a copy of a secret which is shared with the RDP client. This secret is regenerated hourly - see [MS-RDPBCGR] 5.5 which implies either a permanent connection between sesman and the xrdp instance, or xrdp reconnecting to sesman at regular intervals.

The xrdp-sesadmin utility should support more functionality that it currently does. Implentation of (e.g.) a 'disconnect session' command implies sesman will ned to talk to the relevant xrdp instance.

For now I'll add a define for this with a short comment.

matt335672 · 2021-07-19T09:32:15Z

Review comments addressed.

jsorg71 · 2021-07-26T06:36:50Z

Thanks for this, it will be great to removing this unnecessary TCP listener.

metalefty · 2021-07-27T09:33:56Z

There's 1 month until the next release. This is a breaking change so we need to triage if shipping this to August release or December release. Do you think 1 month is enough to test and debug this?

matt335672 · 2021-07-27T12:22:06Z

I should point out that a fair bit of this code isn't actually used at present.

The server-side SCP V0 code is used by xrdp and sesrun, and even they aren't using the SCP client-side library at the moment. The SCP V1 admin code is used by sesadmin, and the other SCP V1 code is used by sestest which isn't shipped by default, primarily as it isn't completed yet.

I've tested all the interfaces I can. xrdp/sesrun appear to work well, as does what there is of sesadmin. The sestest code run OK, but has other (existing) problems which I'm not planning on addressing right now.

In other words, I think an August release for this is realistic, but happy to discuss this further. I'm about for most of August apart from a few days from the 15th onwards.

If we need to bounce it until December that's fine, but I'd suggest a merge straight after v0.9.17 early in September if we do that. There are other changes to make in this area, but they're dependent on this one going in.

metalefty · 2021-07-28T04:16:31Z

Then, I'm fine with shipping this to next August release.
Just to be sure, does this PR switches connection between xrdp and sesman from tcp to uds?

matt335672 · 2021-07-28T16:00:41Z

No it doesn't.

That's a subsequent part of Jay's original #1602. Because that's a significantly more user-visible change however, it will be good to get some more preparation done before we make that switch, so it's clearer what's going on.

In particular, at the moment, xrdp and sesrun don't use libscp to talk to sesman. This PR doesn't address that, but I've got another one planned that will.

After what we should be able to move to UDS with a relatively simple PR.

metalefty · 2021-07-29T03:39:27Z

Okay, let's ship this to the next release.

matt335672 force-pushed the move_scp_to_trans branch 2 times, most recently from 8315e46 to eeaf8cd Compare June 25, 2021 09:37

matt335672 marked this pull request as ready for review June 25, 2021 09:49

aquesnel reviewed Jul 15, 2021

View reviewed changes

matt335672 force-pushed the move_scp_to_trans branch from eeaf8cd to f2dd07e Compare July 19, 2021 09:22

aquesnel approved these changes Jul 21, 2021

View reviewed changes

jsorg71 and others added 7 commits July 27, 2021 13:35

sesman: work on moving sesman to trans, v0 scp working

738e346

work on moving to trans for scp v1

95d4de2

Fix compile issues with SCP states

3643cc3

Removed struct SCP_CONNECTION type entirely

a10de5c

Minor const and comment fixes

c9afd80

Module testing now OK

d4c8122

Address review comments

5c9839a

matt335672 force-pushed the move_scp_to_trans branch from f2dd07e to 5c9839a Compare July 27, 2021 12:38

metalefty approved these changes Jul 29, 2021

View reviewed changes

matt335672 merged commit 34e56f3 into neutrinolabs:devel Aug 2, 2021

matt335672 deleted the move_scp_to_trans branch August 2, 2021 13:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move scp to standard transport #1925

Move scp to standard transport #1925

matt335672 commented Jun 18, 2021 •

edited

Loading

aquesnel Jul 15, 2021

matt335672 Jul 16, 2021

aquesnel Jul 15, 2021

matt335672 Jul 16, 2021

aquesnel Jul 15, 2021

matt335672 Jul 16, 2021

aquesnel Jul 15, 2021

matt335672 Jul 16, 2021

aquesnel Jul 15, 2021

matt335672 Jul 16, 2021

aquesnel Jul 15, 2021

matt335672 Jul 16, 2021

matt335672 commented Jul 19, 2021

jsorg71 commented Jul 26, 2021

metalefty commented Jul 27, 2021

matt335672 commented Jul 27, 2021

metalefty commented Jul 28, 2021

matt335672 commented Jul 28, 2021

metalefty commented Jul 29, 2021

Move scp to standard transport #1925

Move scp to standard transport #1925

Conversation

matt335672 commented Jun 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matt335672 commented Jul 19, 2021

jsorg71 commented Jul 26, 2021

metalefty commented Jul 27, 2021

matt335672 commented Jul 27, 2021

metalefty commented Jul 28, 2021

matt335672 commented Jul 28, 2021

metalefty commented Jul 29, 2021

matt335672 commented Jun 18, 2021 •

edited

Loading