OXT-1521: Replace UID with Quark #1109

crogers1 · 2019-03-21T19:36:52Z

UID is an Ocaml based webserver in dom0 that supports the backend operations of the UI and UIVM. In the current state of OpenXT, much of the code is unused. It simply serves the javascript files to the browser in the UI and listens for POST messages for debug/logging.

We can replace UID with a simpler webserver, Quark, that has several benefits. It's more lightweight, written in C, and is actively maintained. This PR seeks to replace UID and update the refpolicy to define quark-specific types.

crogers1 · 2019-03-21T19:37:13Z

This should be considered for merge after #1101

stacktrust · 2019-03-21T20:18:48Z

Please create a JIRA ticket for this new component.

stacktrust · 2019-03-21T22:04:16Z

This new implementation requires discussion on JIRA before a merge can occur.

rneilturner · 2019-03-26T01:28:30Z

Tested downstream and works as expected. +1

crogers1 · 2019-04-22T17:55:57Z

@jean-edouard @eric-ch @dozylynx @jandryuk thoughts?

jean-edouard · 2019-04-22T18:04:02Z

Thoughts about replacing an obsolete and un-maintained ocaml solution with a current and upstream C one?
I think I'll go with +1 :)

eric-ch · 2019-04-22T18:08:20Z

recipes-extended/quark/files/add-POST-and-v4v-support.patch

+-LDFLAGS  = -s
+CPPFLAGS ?= -DVERSION=\"$(VERSION)\" -D_DEFAULT_SOURCE -D_XOPEN_SOURCE=700 -D_BSD_SOURCE
+CFLAGS   ?= -std=c99 -pedantic -Wall -Wextra -O
+LDFLAGS  ?=


Weak assignment is no issue, but why change the defaults?

The CFLAGS change was a leftover change from when I was debugging and can be reverted. The LDFLAGS change (I believe) was necessary to compile debug symbols for testing.

Indeed, -s is not desirable. Switching to weak assignment lets Bitbake override the variables with the toolchain configuration though.
It is unfortunately done in many projects :).

eric-ch · 2019-04-22T18:11:06Z

recipes-extended/quark/files/add-POST-and-v4v-support.patch

+-		die("getgrnam '%s': %s", group, errno ? strerror(errno) :
+-		    "Entry not found");
+-	}
+-


Why did you remove this? This is not documented in the patch and should be split in its own patch.

The user/group validation and the uid operations in your below comment required quark to have some admin-like SELinux privileges. I wanted to avoid that, so I just removed the functionality entirely. We don't need these as far as I'm aware anyway.

Actually you left the options definitions in getopt() and the usage, so not entirely. It really belongs in a separate patch "Add POST and v4v support to quark." is not really related to this.

SELinux handles these sort of perms, plenty of other daemons drop root. It can be added to the policy file.

eric-ch · 2019-04-22T18:14:49Z

recipes-extended/quark/files/add-POST-and-v4v-support.patch

+-		}
+-		if (getgid() == 0) {
+-			die("Won't run as root group", argv0);
+-		}


Same as before, also this is crippling options -u and -g which could be used should the default user/group hardcoded in there not be available.

See my comment above

eric-ch · 2019-04-22T18:19:53Z

recipes-extended/quark/files/add-POST-and-v4v-support.patch

+ 	            "Date: %s\r\n"
+ 	            "Connection: close\r\n"
+ 	            "Last-Modified: %s\r\n"
+	            "Expires: Wed, 1 Jan 2030 00:00:00 GMT\r\n"


Why is that necessary? Is there some clever caching being done on the UI side?[1].

The main reason this is here is because UID sent a similar line in its response header (see https://github.com/OpenXT/uid/blob/85bdfe033454efc8016362c466599b0658d91c79/file_handler.ml#L82). It's supposed to make use of HTTP caching, but I'm not even really sure that the client is taking advantage of that. i can remove it if desired.

UID does it on images only, this is probably to not reload the entire UI and get things a little faster, the browser likely handles the caching.
While the images in the UI page won't change, could this trigger some fun on other resources?

Also quark already has helpers to handle timestamps, this could be now + 10years, or something like that.

Ah good catch I didn't realize it was just for images. I can do something similar using the helpers.

eric-ch · 2019-04-22T18:21:23Z

recipes-extended/quark/files/add-POST-and-v4v-support.patch

+		FD_SET(fd, &writefds);
+	}
+
+	select(fd + 1, &readfds, &writefds, NULL, &tv);


select(2) can be interrupted by signals(7).

Concur. Also, since you have a timeout specified and the function returns void, you don't know which occurred when calling this function, either read access, timeout, or interrupt. The return of the select() can help make this determination clearer, or allow you to put in a bit of a spinlock if you want to keep this function returning void.

eric-ch · 2019-04-22T18:59:21Z

recipes-extended/quark/files/add-POST-and-v4v-support.patch

+	/* wait until our fd is available for the next operation */
+	wait_fd_ready(fd, true, true);
+	ret = resp_file(fd, RELPATH(realtarget), r, &st, mime, lower, upper);
+	wait_fd_ready(fd, true, false);


This should not fix anything. If it does, we have bigger problems.

resp_file() will only write to fd.

write(2) should only block if there is no space left to send the message.

With that in mind:

Waiting on read before writing would be equivalent to sleep(500ms).

Waiting on write before writing should not block.

Waiting on read after writing most likely sleep(500ms), but maybe the client will have time to reply something (not that anything is expected?) or close the connection.

So I am slightly curious: What was blocking exactly and are both wait_fd_ready necessary to no longer observe the issue?

In our first iteration of the issue, we had sleep(100)'s where the wait_fd_ready() functions are now. The delays were all necessary for proper execution, and to honest I still don't know why. In an effort to make the fix less hacky, I replaced them with select() calls, but based on your comment I'm guessing this was the wrong choice.

I am asking to figure out if this reveals a problem in V4V or between the UI and Quark (or something else entirely...).

If this is something V4V does not handle, that problem will occur somewhere else eventually.

I'm not confident this is a v4v issue, but unfortunately my knowledge of socket operations is not very strong at all. Would someone else who's more knowledgeable be able to take a look? I can stick a TODO in there for now so as to not hold up this merge.

dpsmith · 2019-04-23T11:23:06Z

I would be willing to +1 if the packagegroup explicit replace change is instead made to be a check against DISTRO_FEATURES for quark at which point it is included with the default being to use uid.

jean-edouard · 2019-04-23T11:46:46Z

@dpsmith -1 to that. If it's optional and disabled by default, it might as well not be there.
Besides, I'm also against build time switches since each one doubles the OpenXT combinations to maintain. OE layers should be used for that kind of scenario.
@crogers1, if you were to push, somewhere on the internet, an OE layer that enables quark, I will add it to the layers used by default by the AIS builder.

tsirakisn · 2019-04-23T12:37:02Z

@crogers1 not sure if this is on your radar but this should be merged with OpenXT/toolstack-data#51

dpsmith · 2019-04-23T12:39:58Z

@jed, okay then -1 from me.

jandryuk

I'm not opposed to switching from UID to quark. Getting rid of a custom component is good. However this particular PR feels a little messy. It made quark work, but didn't go back and clean up the patch set.

The one patch, add-POST-and-v4v-support.patch, states 2 things, but really does 5.

Add POST
This is okay and needed
Random wait_fd_ready calls
As @eric-ch said, these seem wrong. We control v4v, so if something is broken there then we should fix that.
Makefile changes
Again, needed, but they should be explained.
Strip out user/group changing
I think these should be dropped since we can change our SELinux policy to accommodate.
Hard-codes an Expires header
I don't know enough to evaluate this.

These are all independent changes, so I would structure them in separate patches.

We want to align with upstream sources to reduce our maintenance burden going forward. Therefore, we should only patch what we absolutely must. If we can re-work other parts of OpenXT, then we should. This is where I'd rather fix v4v and SELinux than patch quark.

jandryuk · 2019-04-23T12:43:24Z

recipes-extended/quark/files/add-POST-and-v4v-support.patch

+	/* keep the first newline */
+	p += 2;
+
+	/* find second occurrence - if exists, POST request, else GET */


Doesn't the request state "POST" or "GET" explicitly?

Assuming this is reading the full HTTP message, POST or GET should be the absolute first characters seen, so it would be far more efficient to look in the absolute positions than a string match when handling GET requests. The first occurrence of double-CRLF is required, but the second search could probably be taken out completely.

The second search was less about figuring out the type of request and more about finding where to null terminate it and copying the correct number of bytes from the payload. That being said the second search can be done conditionally to avoid searching unnecessarily for a GET request.

jandryuk · 2019-04-23T12:44:38Z

recipes-extended/quark/files/add-POST-and-v4v-support.patch

+	if (!status) {
+		if (r.method == M_POST) {
+			/* handle POST */
+			syslog(LOG_INFO, "[quark] %s", r.payload);


A syslog message for each post seems excessive. Maybe drop it to LOG_DEBUG? Also, POST requests don't get a response?

I agree, logging all requests at LOG_INFO seems excessive, especially if you are simply dumping the payload into the log message.

As far as I can tell this is how UID handled POST messages as well. The only time POST is ever used is for debug messages (see https://github.com/OpenXT/uid/blob/85bdfe033454efc8016362c466599b0658d91c79/httpserver.ml#L205).

And see https://github.com/OpenXT/toolstack-data/blob/master/dist/script/lib/debug.js#L42 for where the POST calls originate from. AFAIK there are none besides these UI logging calls.

Thanks, @tsirakisn . Yes, I forgot about the POST logging. I guess that means it should stay LOG_INFO.

I see uid sends an HTTP response to the POST messages. I think those should be maintained in quark. I don't know if it matters, but surf may be hanging onto resources for those unanswered requests.

+1 I'll mirror UID's response header in quark

I'll also add a lengthier comment above the syslog call explaining my above comments

pearsonk · 2019-04-23T14:29:09Z

recipes-extended/quark/files/quark.initscript

+    echo "Stopping Quark server"
+    stop_quark
+    ;;
+


Just a nitpick. Inconsistent whitespacing, some have a blank space between branches and some do not.

crogers1 · 2019-04-23T14:43:15Z

@jandryuk Just some background, the direction I gave my team was to avoid modifying v4v or the interposer if at all possible. Replacing UID with Quark affects a relatively small number of components, whereas a v4v or interposer change potentially affects every component that uses v4v, which is a considerable number, including critical ones such as rpc-proxy. With the massive changes moving to 64-bit, that seemed like the correct approach at the time.

Now that x64 is merged if we want to reevaluate how we approach Quark, that's fine with me.

@dpsmith @jean-edouard, Putting this in a layer as an optional component may be the best path forward, seeing as there's a pretty even split among contributors. I can get behind that.

dpsmith · 2019-04-23T16:11:15Z

@crogers1 that would be fine and to be clear, my opposition is not in that I don't want to see uid replaced. It's just that uid works today with no major bugs, it has been exercised across many releases, and has been proven in production. While this commit introduces quark as a wholesale replacement that modifies its behavior from upstream thus this patched implemented has no track record of production usage to demonstrate stability, e.g. it is an experimental feature. As an experimental feature it should be an explicit action to be enabled for those that want to use and test. After a release or two with better testing, iterations of bugs, and demonstrated stability in production, then uid could be retired and quark could be made the default.

Quark is a lightweight C-based webserver. This commit removes the old OCAML-based webserver, UID, and replaces it with Quark. We also add POST support to Quark to allow for the UI code to log debug messages to dom0 Signed-off-by: Chris Rogers <[email protected]>

The removal of UID and addition of Quark requires updates to the refpolicy. This commit adds a new quark module and removes several uid_t rules in other .te files. Signed-off-by: Nicholas Tsirakis <[email protected]> Signed-off-by: Chris Rogers <[email protected]>

crogers1 · 2020-12-24T15:08:13Z

Rebased this PR. Compiled on latest master. @eric-ch if we could get a custom build from the build server with this in it, that'd be awesome!

jandryuk

Sorry, I lost track of reviewing this.

Previous feedback mentioned the single add-POST-and-argo-support.patch patch names 2 things, but actually does 4 things. I think that should be split into 4.

Has any effort been made to investigated the need for wait_fd_ready? Including as much detail of "server lockup" would be good.

Instead of patching out setuid/setgid, was running as non-root investigated? /dev/argo_stream would need a group assignment, but it would be nice. Though I have no idea what other changes may be needed to get it to actually work :)

jandryuk · 2021-03-26T14:32:09Z

recipes-security/refpolicy/refpolicy-mcs-2.%/policy/modules/services/quark.te

-type uid_tmp_t;
-files_tmp_file(uid_tmp_t)
-files_tmp_filetrans(uid_t, uid_tmp_t, file)
+xc_files_rw_v4v_chr(quark_t)


I think you want: xc_files_rw_argo_chr

jandryuk · 2021-03-26T14:35:36Z

recipes-security/refpolicy/refpolicy-mcs-2.%/policy/modules/services/updatemgr.te

@@ -62,7 +62,6 @@ kernel_read_vm_overcommit_sysctl(updatemgr_t)
 logging_send_syslog_msg(updatemgr_t)

 dbd_dbus_chat(updatemgr_t)
-uid_dbus_chat(updatemgr_t)


You have quark_dbus_chat - don't you need to replace these?

eric-ch reviewed Apr 22, 2019

View reviewed changes

jandryuk reviewed Apr 23, 2019

View reviewed changes

pearsonk reviewed Apr 23, 2019

View reviewed changes

crogers1 force-pushed the quark-x64 branch 2 times, most recently from 2b9de62 to e0ef433 Compare December 24, 2020 14:58

Chris Rogers added 2 commits December 24, 2020 10:05

crogers1 force-pushed the quark-x64 branch from e0ef433 to 8c13423 Compare December 24, 2020 15:05

jandryuk reviewed Mar 26, 2021

View reviewed changes

OXT-1521: Replace UID with Quark #1109

Are you sure you want to change the base?

OXT-1521: Replace UID with Quark #1109

Conversation

crogers1 commented Mar 21, 2019

crogers1 commented Mar 21, 2019

stacktrust commented Mar 21, 2019

stacktrust commented Mar 21, 2019 via email

rneilturner commented Mar 26, 2019

crogers1 commented Apr 22, 2019

jean-edouard commented Apr 22, 2019

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

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

dpsmith commented Apr 23, 2019

jean-edouard commented Apr 23, 2019

tsirakisn commented Apr 23, 2019

dpsmith commented Apr 23, 2019

jandryuk left a 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

Choose a reason for hiding this comment

crogers1 commented Apr 23, 2019

dpsmith commented Apr 23, 2019

crogers1 commented Dec 24, 2020

jandryuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment