feat: permit-open #1402

XavierChanth · 2024-09-28T01:18:29Z

- What I did

Implemented permit open
Refactored parsing of manager list using a better approach which is more similar to permit open's
cleaned up memory bugs in handle_ssh_request and handle_npt_request

- How I did it

- How to verify it

No tests yet. will do on Monday.

- Description for the changelog
feat: permit-open

XavierChanth · 2024-09-28T01:24:20Z

packages/c/sshnpd/include/sshnpd/params.h

-  char **permitopen;
+  char **permitopen_hosts;
+  uint16_t *permitopen_ports; // 0 = '*'


previously permitopen was a list of strings, but now it is split up into a list of hosts and list of ports, "*" for port = 0

XavierChanth · 2024-09-28T01:25:19Z

packages/c/sshnpd/src/handle_npt_request.c

+  // NPT ONLY
+  // Don't try optimizing this to reuse the permitopen struct from main.c.
+  // none of the memory duplication here is expensive, and it's a surface for bugs
+  permitopen_params permitopen;
+  permitopen.permitopen_len = params->permitopen_len;
+  permitopen.permitopen_hosts = params->permitopen_hosts;
+  permitopen.permitopen_ports = params->permitopen_ports;
+  permitopen.requested_host = cJSON_GetStringValue(requested_host);
+  permitopen.requested_port = cJSON_GetNumberValue(requested_port);
+
+  if (!should_permitopen(&permitopen)) {
+    atlogger_log(LOGGER_TAG, ATLOGGER_LOGGING_LEVEL_DEBUG, "Ignoring request to localhost:%d\n",
+                 permitopen.requested_port);
+    cJSON_Delete(envelope);
+    return;
+  }
+  // END NPT ONLY
+


handle permit open check for npt, after envelope has been decoded

XavierChanth · 2024-09-28T01:25:38Z

packages/c/sshnpd/src/handle_npt_request.c

@@ -138,15 +156,13 @@ void handle_npt_request(atclient *atclient, pthread_mutex_t *atclient_lock, sshn
  char *buffer = NULL;

  res = atclient_get_public_key(atclient, &atkey, &buffer, NULL);
+  atclient_atkey_free(&atkey);


The rest of this file is memory clean up

XavierChanth · 2024-09-28T01:26:01Z

packages/c/sshnpd/src/handle_ssh_request.c

@@ -142,15 +142,13 @@ void handle_ssh_request(atclient *atclient, pthread_mutex_t *atclient_lock, sshn
  char *buffer = NULL;

  res = atclient_get_public_key(atclient, &atkey, &buffer, NULL);
+  atclient_atkey_free(&atkey);


The same memory clean up is the only change to handle_ssh_request

XavierChanth · 2024-09-28T01:26:25Z

packages/c/sshnpd/src/main.c

+  if (params.policy == NULL) {
+    atlogger_log(LOGGER_TAG, ATLOGGER_LOGGING_LEVEL_DEBUG, "Policy Manager: NULL");
+  } else {
+    atlogger_log(LOGGER_TAG, ATLOGGER_LOGGING_LEVEL_DEBUG, "Policy Manager: %s", params.policy);
+  }


Started policy implementation, but the flag is disabled for now.

XavierChanth · 2024-09-28T01:27:51Z

packages/c/sshnpd/src/main.c

+  char *buf = malloc(sizeof(char) * 1024);
  for (int i = 0; i < params.permitopen_len; i++) {
-    cJSON_AddItemToArray(allowed_services, cJSON_CreateString(params.permitopen[i]));
+    sprintf(buf, "%s:%u", params.permitopen_hosts[i], (unsigned int)params.permitopen_ports[i]);
+    cJSON_AddItemToArray(allowed_services, cJSON_CreateString(buf));
  }
+  free(buf);


Since I split params.permitopen into params.permitopen_hosts and params.permitopen_ports

XavierChanth · 2024-09-28T01:28:49Z

packages/c/sshnpd/src/main.c

+          // permitopen happens first for ssh so we can avoid a bunch of unnecessary tasks
+          permitopen.requested_host = "localhost";
+          permitopen.requested_port = params.local_sshd_port;
+          if (!should_permitopen(&permitopen)) {
+            atlogger_log(LOGGER_TAG, ATLOGGER_LOGGING_LEVEL_DEBUG, "Ignoring request to localhost:%d\n",
+                         params.local_sshd_port);
+            // TODO notify daemon doesn't permit connections to $requested_host:$requested_port
+            break;
+          }


permit open for ssh can be checked earlier since we don't need to decode the json in the request to check it.

XavierChanth · 2024-09-28T01:29:57Z

packages/c/sshnpd/src/params.c

+  if ((parse_permitopen(permitopen, &params->permitopen_hosts, &params->permitopen_ports, &params->permitopen_len,
+                        false) != 0)) {
+    printf("Failed to parse permit-open string\n");
+    free(params->permitopen_str);
+    return 1;
+  }


Parse permitopen early so we can check for invalid input.

XavierChanth · 2024-09-28T01:31:14Z

packages/c/sshnpd/src/params.c

+    if (params->manager_list == NULL) {
+      printf("Failed to allocate memory for manager list\n");
+      free(params->permitopen_str);
+      return 1;


refactored the manager_list code to a cleaner solution more similar to permitopen...
I was referencing this as I was doing the permitopen parsing and realized this need to be changed.

XavierChanth · 2024-09-28T01:32:13Z

packages/c/sshnpd/src/permitopen.c

+  // malloc pointers to each string, but don't malloc any more memory for individual char storage
+  *permitopen_hosts = malloc((sep_count) * sizeof(char *));
+  *permitopen_ports = malloc((sep_count) * sizeof(uint16_t));


we don't need individual char storage because we are pointing these pointers to the memory passed into the argv of the program. I replaced the : with null terminators to make the original hosts passed in valid c strings.

XavierChanth · 2024-09-28T01:35:46Z

Draft until I've written tests for parsing of permit-open and manager list

gkc

LGTM

XavierChanth added 2 commits September 27, 2024 21:16

feat: permit-open

f6a5f06

chore: clean up

2c4d59f

XavierChanth commented Sep 28, 2024

View reviewed changes

XavierChanth requested review from JeremyTubongbanua and gkc September 28, 2024 01:34

Merge branch 'trunk' into c-sshnpd-permit-open

126c57a

XavierChanth marked this pull request as draft September 28, 2024 01:35

gkc previously approved these changes Sep 28, 2024

View reviewed changes

XavierChanth added 2 commits September 30, 2024 13:27

chore: add todo

90808fa

fix: tests

e9f16be

XavierChanth dismissed gkc’s stale review via e9f16be September 30, 2024 17:27

XavierChanth added 3 commits September 30, 2024 23:05

test: permit open parsing and regression tests

9421613

ci: add unit tests

e609b41

fix: add test dependency

a769e5f

XavierChanth marked this pull request as ready for review October 1, 2024 03:12

Merge branch 'trunk' into c-sshnpd-permit-open

32b47ae

gkc approved these changes Oct 1, 2024

View reviewed changes

Merge branch 'trunk' into c-sshnpd-permit-open

ebb1072

gkc merged commit 0379157 into trunk Oct 1, 2024
9 checks passed

gkc deleted the c-sshnpd-permit-open branch October 1, 2024 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: permit-open #1402

feat: permit-open #1402

XavierChanth commented Sep 28, 2024 •

edited

Loading

XavierChanth Sep 28, 2024

XavierChanth Sep 28, 2024

XavierChanth Sep 28, 2024

XavierChanth Sep 28, 2024

XavierChanth Sep 28, 2024

XavierChanth Sep 28, 2024

XavierChanth Sep 28, 2024

XavierChanth Sep 28, 2024

XavierChanth Sep 28, 2024

XavierChanth Sep 28, 2024 •

edited

Loading

XavierChanth commented Sep 28, 2024

gkc left a comment

feat: permit-open #1402

feat: permit-open #1402

Conversation

XavierChanth commented Sep 28, 2024 • 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

XavierChanth Sep 28, 2024 • edited Loading

Choose a reason for hiding this comment

XavierChanth commented Sep 28, 2024

gkc left a comment

Choose a reason for hiding this comment

XavierChanth commented Sep 28, 2024 •

edited

Loading

XavierChanth Sep 28, 2024 •

edited

Loading