Build improve installs #3905

jodh-intel · 2022-03-17T12:22:21Z

build: Improve file installation

Move the make(1) code used to install files to utils.mk, removing
all duplication in the process.

Notes:

This change tightens up the permissions so that all files are now
installed explicitly as user root and group adm. The "other"
permissions are zero meaning you need elevated privileges to run
and view the installed files.

This change also corrects a few buglets where some of the installation
targets were specified as a file. This isn't necessary as it's simpler
to allow install(1) to derive that from the specified source.

Summary of permission and ownership changes:

File type	Old metadata	New metadata
binaries	`-rwxr-xr-x 1 root root`	`-r-xr-x--- 1 root adm`
config files	`-rw-r--r-- 1 root root`	`-rw-r----- 1 root adm`

Fixes: #3904.

Signed-off-by: James O. D. Hunt james.o.hunt@intel.com

jodh-intel · 2022-03-17T12:24:15Z

/test

jodh-intel · 2022-03-17T12:27:06Z

@amshinde, @fidencio, @devimc, @wainersm, @liubin, @bergwolf - ptal. This PR is a delicate one as it will affect packaging. It will potentially also affect users given the changes to the group and perms.

liubin · 2022-03-17T13:01:29Z

utils.mk

+INSTALL_OWNER     ?= root
+
+# Group for installed files
+INSTALL_GROUP     ?= adm


Doubt whether group adm exists in all OS.

https://refspecs.linuxfoundation.org/LSB_5.0.0/LSB-Core-generic/LSB-Core-generic/usernames.html

It's strictly optional but afaik it is provided by the main distros. But if not, those distros could run make INSTALL_GROUP=root if they wished.

We could retain group root, but see below...

I think the adm group is present in the majority of the distros, @liubin.

Not sure about darwin, though. /cc @sameo @egernst

@why not just keep the group as root? Whats the benefit of having admin here?

We could make it root but I chose adm to give admins a change to add select users to that group (rather than the root group) in recognition that this PR may cause issues for some as currently any user can run the binaries, for instance.

liubin · 2022-03-17T13:04:59Z

utils.mk

+INSTALL_GROUP     ?= adm
+
+# Permissions for installed executable files
+INSTALL_EXE_PERMS ?= 0750


CI fails with:

20:43:23 .ci/install_kata.sh: line 56: /usr/local/bin/kata-runtime: Permission denied

The mod 750 means others can't execute these binaries, so that will need all calling of kata-runtime add sudo.

and this may clash with the rootless use case .. 🤔 ❓

cc @fengwang666

That's true. But I think that's what we should be encouraging. The reason is that the configuration file could contain sensitive information such as Jaeger credentials. That implies that "other" should not be able to read the file. But when you run kata-runtime, the code will read the configuration file. Overall, that means that admins should not allow non-priv / "other" users to run kata-runtime. We could keep the group ownership as root, but by using adm admins can give that group membership to some users without giving them full root privs. In summary, the behaviour of this PR is that you would need to be in the INSTALL_GROUP group to be able to run any Kata commands.

This changes makes the install "safe by default". Again if packagers / distro wish to change that behaviour, they can run make INSTALL_EXE_PERMS="..." as they wish.

That's right, and I think that's actually how users should be running it.

This means we would at least add some supplemental steps for validation, e.g. creating a kata group? Is that the intent?

Hi @c3d - I'm not sure I follow? The intent of this PR is to improve the installation of files and make them as secure as possible by default. A kata group could be used but I'm suggesting we use the adm group as the default as that should already exist. Having a kata group would be better imho in the longer term as that would give admins more scope and flexibility. And for some context lthough containerd doesn't provide it's own group, docker does.

An alternative to adm group ownership and clearing all the "other" perms and allowing non-priv users to run kata-runtime as they can currently could be to rework the config handling so that we store sensitive config details in a separate (more secure) file, but that's a much bigger change than this, and I'm personally not convinced about that idea currently.

Hi @devimc ,

It should be running on the baseline jobs (run daily). Example:
http://jenkins.katacontainers.io/view/Daily%20baseline/job/kata-containers-main-fedora-35-x86_64-CRIO_K8S-baseline/

And in jobs triggered by pull requests. Example:
http://jenkins.katacontainers.io/view/PR%20Triggered/job/kata-containers-fedora-35-x86_64-CRIO_K8S-PR/

Just to make it clear, there are two scenarios for rootless - rootless hypervisor and rootless runtime itself when the upper layers such as containerd/podman itself call kata as a non-priveleged user.
I am afraid that this PR will break the later case, as we restrict kata-runtime to be called by restricted user group.
We do not have control over what user calls kata-runtime in that case.

My suggestion would be to have kata-runtime as world-executable and the kata conf file as world-readable. I believe any sensitive information should not be part of config files itself. I know this exposes any sensitive information present in the kata-configuration file, but I don't think they should have been there in any place. We could have another file to hold any sensitive data and that could be restricted to be read on by admin/root group.

cc @jodh-intel

@amshinde - I'm a little confused. This PR is trying to resolve a permissions issue to ensure that if the admin enters sensitive information in the configuration file the effect is limited since only privileged users would be able to access that configuration file. Are you proposing that we just close this PR and come up with an alternative mechanism for storing the sensitive details? If so, maybe #4109 could be used/abused for this purpose since you could then store just the sensitive info in a secrets.toml or similar. However, if a non-priv user runs kata-runtime currently, the (entire) configuration needs to be available to that user. This PR resolves that issue by requiring the user running the command to be privileged, but if I'm understanding what you're saying, we'd need to make the fragments PR "tolerate" unreadable files by just ignoring them, to handle the scenario where a system has sensitive information stored in a config fragment.

@jodh-intel It certainly wouldn't be too hard to treat "Permission denied" while opening a fragment as a special case and ignore the file without treating the situation as an error. The question is whether the resulting semantics aren't a bit too subtle and whether it wouldn't invite unexpected problems - e.g. an admin drops in an ordinary config file fragment, with nothing security-sensitive in it, just forgets to adjust permissions and now the fragment is ignored, more or less silently (I can log the incident but that's not extremely visible I'm afraid).

Concerns aside this can be done, and I don't tend to see the requirement that user be a member of adm as prohibitive either. On the other hand I think I can see @amshinde 's point - handling ordinary and sensitive data seems just fundamentally different. Putting them together and trying to find a lowest common denominator in handling the whole might just result in the sensitive part not being secured quite enough and the ordinary part not being as accessible and easy-to-use as it could be.

If using sensitive config data isn't fundamental to (a large part of) kata-runtime's operation (Jaeger credential don't seem to be AFAICT) I think it could make sense to store them separately (in secure.d instead of config.d perhaps?) or even only try to load them when actually necessary.

The question is whether the resulting semantics aren't a bit too subtle and whether it wouldn't invite unexpected problems

Agreed. I really don't think that random thought is a good idea, it was more to try to understand @amshinde's comments.

handling ordinary and sensitive data seems just fundamentally different.

Again, I agree. I think it was a mistake to allow sensitive data to be stored in our config file. However, we do allow it so this PR was the most immediate solution to the problem using simple primitives. But that doesn't mean we cannot resolve this in other more secure ways in future given additional engineering effort.

@amshinde - if you wish to look into an alternative scheme, that's perfectly fine by me. However, it is worth pointing out that by default neither this PR nor a more elaborate scheme to store secrets is necessary since by default no secrets are stored in the configuration file. It's also worth stressing that if we allow non-priv users to continue to run the kata-runtime binary, we're going to hit complications if we store the secrets in a different location or use a different method since (atleast currently) the entire configuration needs to be read by the runtime at startup. I guess we could consider having kata-runtime and containerd-shim-kata-v2 special-case reading secrets based on the uid of the caller ("If running non-priv don't load secrets, else do"). However special casing like this isn't ideal imho and would need careful testing.

fidencio · 2022-03-17T19:29:17Z

/test_kata_deploy

fidencio · 2022-03-17T19:30:54Z

/test_kata_deploy

Let me see how kata-deploy behaves with this: https://github.com/kata-containers/kata-containers/actions/runs/2000458019

fidencio · 2022-03-18T08:28:07Z

/test_kata_deploy

Let me see how kata-deploy behaves with this: https://github.com/kata-containers/kata-containers/actions/runs/2000458019

kata-deploy seems to be very happy with these changes.

jodh-intel · 2022-03-21T09:33:12Z

Any thoughts on this @bergwolf, @liubin?

fidencio · 2022-03-21T11:52:23Z

@jodh-intel, for the sake of consistency, shall we ensure that when building & shipping qemu / firecracker / firecracker's jailer / cloud-hypervisor those also follow the very same pattern for the permissions? (no, this is not a suggestion, just a question).

Currently we have, as part of the kata-static:

-rwxr--r--. 1 root root 7.0M Mar 21 10:22 cloud-hypervisor
-rwxr-x---. 1 root adm   48M Mar 21 10:21 containerd-shim-kata-v2
-rwxr--r--. 1 root root 3.2M Mar 21 10:20 firecracker
-rwxr--r--. 1 root root 2.5M Mar 21 10:20 jailer
-rwxr-x---. 1 root adm   17K Mar 21 10:21 kata-collect-data.sh
-rwxr-x---. 1 root adm   39M Mar 21 10:21 kata-monitor
-rwxr-x---. 1 root adm   49M Mar 21 10:21 kata-runtime
-rwxr-xr-x. 1 root root  18M Mar 21 10:27 qemu-system-x86_64

With this in mind, I think we'd also need to change the permissions of the containerd-shim-kata-${hypevisor}-v2 files created by kata-deploy, and I think this change should be part of this series (this is where we could make sure the owner / group of the files are the expected ones.:

kata-containers/tools/packaging/kata-deploy/scripts/kata-deploy.sh

Line 102 in 3edf25b

).

wainersm · 2022-03-21T13:45:20Z

Hi @jodh-intel , I hope not too late...I will have time to review this today! \o/

jodh-intel · 2022-03-21T17:01:40Z

@fidencio - I've pushed another commit for the follow-on kata-deploy changes (which are currently untested).

It would be great if folk could help out testing this PR as I don't think we have full CI coverage for kata-deploy.

Also note that I've tightened the perms on binaries further (and updated the table at the top of this PR) since binary files don't need to be owner writable.

wainersm · 2022-03-21T20:04:53Z

src/agent/Makefile

@@ -146,7 +146,7 @@ run:
 install-services: $(GENERATED_FILES)
 ifeq ($(INIT),no)
 	@echo "Installing systemd unit files..."
-	$(foreach f,$(UNIT_FILES),$(call INSTALL_FILE,$f,$(UNIT_DIR)))
+	$(foreach f,$(UNIT_FILES),$(call INSTALL_FILE,$f,$(DESTDIR)/$(UNIT_DIR)))


In src/agent/Makefile:
DESTDIR :=
In src/runtime/Makefile:
DESTDIR ?= /

I didn't check other Makefiles nor variables but I think we should make the default values consistent across makefiles as well. In a follow up PR?

wainersm · 2022-03-21T20:26:45Z

utils.mk

+# params:
+# $1 : file to install
+# $2 : directory path where file will be installed
+define INSTALL_EXEC


INSTALL_EXEC and INSTALL_CONFIG used to honor DESTDIR so that the caller didn't need to explicitly prefix in the target path parameter. I'm wondering what's the rationale behind that change of behavior.

Implicitly prefixing DESTDIR makes it less flexible, however more robust as in the future a new caller might forget to consider DESTDIR .

wainersm · 2022-03-21T20:42:34Z

tools/packaging/guest-image/build_image.sh

@@ -33,7 +33,12 @@ build_initrd() {
 		ROOTFS_BUILD_DEST="${builddir}/initrd-image" \
 		USE_DOCKER=1 \
 		AGENT_INIT="yes"
-	mv "kata-containers-initrd.img" "${install_dir}/${initrd_name}"
+	install \
+		--owner "${KATA_INSTALL_OWNER}" \


Shouldn't default value for KATA_INSTALL_OWNER and friends be set in build_image.sh as well?

wainersm · 2022-03-21T20:49:29Z

tools/packaging/kata-deploy/local-build/kata-deploy-binaries.sh

@@ -34,6 +34,10 @@ workdir="${WORKDIR:-$PWD}"

 destdir="${workdir}/kata-static"

+KATA_INSTALL_OWNER="${KATA_INSTALL_OWNER:-root}"


Makes me sad see that block of initialization repeated in many places. is it a bad idea to move them all into a common.bash file in the root of the project?

wainersm · 2022-03-21T20:50:36Z

Hi @jodh-intel , I reviewed the PR fully and left some comments. I will be testing it doesn't break the scripts I have for OpenShift CI.

wainersm · 2022-03-22T19:01:51Z

with this PR:

[wmoschet@wainer-laptop kata-containers]$ make kata-tarball
tools/packaging/kata-deploy/local-build/Makefile:30: ../../../../utils.mk: No such file or directory
make: *** No rule to make target '../../../../utils.mk'.  Stop.

wainersm · 2022-03-22T19:56:09Z

Great, this PR doesn't break the workflow that I have for OpenShift CI :)

jodh-intel · 2022-03-24T17:57:19Z

Thanks for testing @wainersm. I plan to update the PR further based on feedback tomorrow...

jodh-intel · 2022-04-01T10:32:06Z

@wainersm - thanks for reviewing. I've updated the branch based on your feedback and found a cleaner way of passing the installation variables to kata-deploy.

@wainersm, @fidencio - Could you re-review and try re-testing this branch?

jodh-intel · 2022-04-11T10:41:50Z

@wainersm and @fidencio - do you have any cycles to help re-test the kata-deploy changes? If this is going to be a problem, I'll splint this into 2 PRs: my originally focussed changes and a separate one for the kata-deploy specific bits @fidencio asked for.

fidencio · 2022-04-11T19:03:59Z

/test_kata_deploy

jodh-intel · 2022-05-05T12:11:10Z

@fidencio, @wainersm - It would be geat to get this landed, so please could you tal?

fidencio · 2022-05-12T06:51:08Z

@fidencio, @wainersm - It would be geat to get this landed, so please could you tal?

You have my LGTM, considering the tests are green.

jodh-intel · 2022-05-12T15:31:09Z

Hmm, it seems that snap is failing because it's very / overly opinionated:

container.go:177: in snap "kata-containers": "usr/bin/containerd-shim-kata-v2" should be world-readable and executable, and isn't: -r-xr-x---
container.go:177: in snap "kata-containers": "usr/bin/kata-collect-data.sh" should be world-readable and executable, and isn't: -r-xr-x---
container.go:177: in snap "kata-containers": "usr/bin/kata-runtime" should be world-readable and executable, and isn't: -r-xr-x---
error: cannot pack "/home/runner/work/kata-containers/kata-containers/prime": snap is unusable due to bad permissions

I've tweaked the binary perms to set them for "other" (but not for config files) to see if that makes snap any happier...

c3d · 2022-05-12T16:10:13Z

I ran with my local CI script, and I got

ERROR: Found unexpected /usr/bin/qemu-system-x86_64 present

I don't think this is related to this commit. Need to run again on main.

c3d · 2022-05-12T21:06:06Z

I ran with my local CI script, and I got
ERROR: Found unexpected /usr/bin/qemu-system-x86_64 present
I don't think this is related to this commit. Need to run again on main.

OK; seen only with Alma Linux (twice). Works with Fedora. Let's ignore that for the moment.

amshinde · 2022-05-12T22:52:28Z

src/agent/Makefile

@@ -45,12 +45,11 @@ ifneq ($(EXTRA_RUSTFEATURES),)
    override EXTRA_RUSTFEATURES := --features "$(EXTRA_RUSTFEATURES)"
 endif

+##VAR DESTDIR=<path> is a directory prepended to each installed target file


Would it make better sense to add this comment in the utils.mk file rather than here?
Maybe you can specify here that DESTDIR can be set in utils.mk.

@amshinde - fixed.

amshinde · 2022-05-12T23:20:53Z

utils.mk

+INSTALL_OWNER     ?= root
+
+# Group for installed files
+INSTALL_GROUP     ?= adm


@why not just keep the group as root? Whats the benefit of having admin here?

amshinde · 2022-05-12T23:33:12Z

utils.mk

+INSTALL_GROUP     ?= adm
+
+# Permissions for installed executable files
+INSTALL_EXE_PERMS ?= 0750


Just to make it clear, there are two scenarios for rootless - rootless hypervisor and rootless runtime itself when the upper layers such as containerd/podman itself call kata as a non-priveleged user.
I am afraid that this PR will break the later case, as we restrict kata-runtime to be called by restricted user group.
We do not have control over what user calls kata-runtime in that case.

My suggestion would be to have kata-runtime as world-executable and the kata conf file as world-readable. I believe any sensitive information should not be part of config files itself. I know this exposes any sensitive information present in the kata-configuration file, but I don't think they should have been there in any place. We could have another file to hold any sensitive data and that could be restricted to be read on by admin/root group.

cc @jodh-intel

jodh-intel · 2022-05-17T10:03:11Z

Let's spin the wheels again...

/test

jodh-intel · 2022-05-19T09:43:25Z

The CI is green so what do we think about the approach folks?

pmores · 2022-05-25T08:19:15Z

utils.mk

+
+# Callers should check if this variable is set to avoid including this
+# file multiple times.
+INCLUDED_UTILS_MK = 1


I assume there's no way to move this check inside utils.mk, is that correct? (I'm not aware of any, just making sure we've considered it.)

Yes, I considered that. We could do it, nominally we'd end up with utils.mk looking like this:

ifeq ($(INCLUDED_UTILS_MK),) # << Entire contents of "utils.mk" goes here >> INCLUDED_UTILS_MK = 1 endif

That would be simpler from the callers perspective (just call include utils.mk everywhere you want), but awkward to update utils.mk itself.

However, I've just found an alternative which keeps the simpler caller behaviour and avoids the potentially confusing / easy to break utils.mk as show above. But there is a catch: we'd need two files, not one:

Rename the actual utils.mk to something like private_utils.mk.

Create utils.mk as:
ifeq ($(INCLUDED_UTILS_MK),) $(eval include private_utils.mk) endif

Change all calls to:
include .../utils.mk

That's a bit more complex, but we could obviously add comments to the utils.mk and private_utils.mk to explain what's happening.

What do folk think?

That's interesting! In my view, given the multiple sites where utils.mk is included, I think a solution that doesn't require callers to think of putting the include statement within guards would be considerably more robust. From this perspective, both of the solutions you mention would seem acceptable to me (maybe I'm overlooking something but even wrapping the whole contents of the file in guards doesn't seem too bad to me).

fidencio

lgtm, thanks @jodh-intel!

jodh-intel · 2022-05-31T10:39:55Z

@amshinde - please can you comment on #3905 (comment).

Remove erroneous tabs and additional blank lines in makefiles. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

Correct the `osbuilder` instructions explaining how to display a variable at build time. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

Set the default value for `DESTDIR` variable in `utils.mk`, allowing it to be over-ridden on the command-line. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

Add a guard around the inclusion of `utils.mk` to avoid including it multiple times. Also, ensure that file is included _after_ the first rule (since it defines rules itself). Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

Move the `make(1)` code used to install files to `utils.mk`, removing all duplication in the process. Summary of permission and ownership changes: | File type | Old metadata | New metadata | |--------------|--------------------------|-------------------------| | binaries | `-rwxr-xr-x 1 root root` | `-r-xr-xr-x 1 root adm` | | config files | `-rw-r--r-- 1 root root` | `-rw-r----- 1 root adm` | > **Notes:** > > - This change tightens up the permissions so that all files are now > installed explicitly as user `root` and group `adm`. > > - The "other" permissions for binaries disallow writes (as previously). > > - But the "other" permissions for configuration files are now zero > meaning you need elevated privileges to run and view the installed > files. This change is needed to ensure that any secrets stored > within the configuration file are not readable by a non-privileged user > (in the "other" group). > > - Ideally, binary files set the "other" permissions to zero for > consistency with the configuration file permissions. However, > unfortunately that isn't possible currently since Kata is packaged > in various formats including "snap" which is hard-coded to disallow > binary files with no "other" permissions. > > - This change also corrects a few "buglets" where some of the installation > targets were specified as a file. This isn't necessary as it's simpler > to allow `install(1)` to derive that from the specified source. Fixes: kata-containers#3904. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

jodh-intel · 2022-06-08T08:38:14Z

Branch updated to resolve the conflict.

@pmores - I've also gone with the guard in utils.mk (aka no extra makefile) and simplified the code in the callers to remove all the checks as that check is now handled by utils.mk itself.

/test

pmores

@jodh-intel Great, thanks! I'm approving this PR with the caveat that I don't think I can really properly review the final commit of the series.

jodh-intel · 2022-06-21T08:03:17Z

Ping @amshinde.

/retest-arm

jodh-intel · 2022-07-04T13:11:29Z

/retest-arm

fidencio · 2023-07-04T13:04:15Z

@Jodh, we're closing this PR as it's been opened and inactive for more than one year.
If you think this is a mistake and the PR should be re-opened, please, feel free to do so after rebasing it atop of current main.

katacontainersbot added the size/large Task of significant size label Mar 17, 2022

liubin reviewed Mar 17, 2022

View reviewed changes

jodh-intel added security Potential or actual security issue area/security labels Mar 21, 2022

jodh-intel force-pushed the build-improve-installs branch 2 times, most recently from 5e9136e to db06697 Compare March 21, 2022 16:58

jodh-intel force-pushed the build-improve-installs branch from db06697 to 1e20c2a Compare March 21, 2022 18:03

wainersm reviewed Mar 21, 2022

View reviewed changes

jodh-intel force-pushed the build-improve-installs branch 2 times, most recently from 304fa64 to 0abb1da Compare April 1, 2022 10:29

jodh-intel force-pushed the build-improve-installs branch 2 times, most recently from 4cb2352 to ace1acf Compare May 12, 2022 15:07

amshinde reviewed May 12, 2022

View reviewed changes

jodh-intel force-pushed the build-improve-installs branch from ace1acf to af5ade3 Compare May 16, 2022 10:40

pmores reviewed May 25, 2022

View reviewed changes

fidencio approved these changes May 30, 2022

View reviewed changes

jodh-intel mentioned this pull request Jun 6, 2022

runtime-rs: makefile for dragonball #4380

Merged

jodh-intel added 3 commits June 8, 2022 09:02

build: Whitespace fixes

712914b

Remove erroneous tabs and additional blank lines in makefiles. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

osbuilder: Fix comment typo

e92036d

Correct the `osbuilder` instructions explaining how to display a variable at build time. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

build: Set DESTDIR centrally

2e34bdb

Set the default value for `DESTDIR` variable in `utils.mk`, allowing it to be over-ridden on the command-line. Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

jodh-intel force-pushed the build-improve-installs branch from af5ade3 to a29f17b Compare June 8, 2022 08:05

jodh-intel added 2 commits June 8, 2022 09:27

build: Improve file inclusion

23630d0

Add a guard around the inclusion of `utils.mk` to avoid including it multiple times. Also, ensure that file is included _after_ the first rule (since it defines rules itself). Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

jodh-intel force-pushed the build-improve-installs branch from a29f17b to 67c0e1a Compare June 8, 2022 08:36

pmores approved these changes Jun 8, 2022

View reviewed changes

jodh-intel mentioned this pull request Jun 13, 2022

Move secrets out of configuration file #4448

Open

jodh-intel mentioned this pull request Oct 12, 2022

kata-ctl: Improve install rule #5403

Closed

fidencio closed this Jul 4, 2023

		@@ -34,6 +34,10 @@ workdir="${WORKDIR:-$PWD}"

		destdir="${workdir}/kata-static"

		KATA_INSTALL_OWNER="${KATA_INSTALL_OWNER:-root}"

Build improve installs #3905

Build improve installs #3905

Conversation

jodh-intel commented Mar 17, 2022 • edited

jodh-intel commented Mar 17, 2022

jodh-intel commented Mar 17, 2022

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

liubin Mar 17, 2022 • edited

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

fidencio commented Mar 17, 2022

fidencio commented Mar 17, 2022

fidencio commented Mar 18, 2022

jodh-intel commented Mar 21, 2022

fidencio commented Mar 21, 2022

wainersm commented Mar 21, 2022

jodh-intel commented Mar 21, 2022

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

wainersm commented Mar 21, 2022

wainersm commented Mar 22, 2022

wainersm commented Mar 22, 2022

jodh-intel commented Mar 24, 2022

jodh-intel commented Apr 1, 2022

jodh-intel commented Apr 11, 2022

fidencio commented Apr 11, 2022

jodh-intel commented May 5, 2022

fidencio commented May 12, 2022

jodh-intel commented May 12, 2022

c3d commented May 12, 2022

c3d commented May 12, 2022

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

jodh-intel commented May 17, 2022

jodh-intel commented May 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fidencio left a comment

Choose a reason for hiding this comment

jodh-intel commented May 31, 2022

jodh-intel commented Jun 8, 2022

pmores left a comment

Choose a reason for hiding this comment

jodh-intel commented Jun 21, 2022

jodh-intel commented Jul 4, 2022

fidencio commented Jul 4, 2023

jodh-intel commented Mar 17, 2022 •

edited

liubin Mar 17, 2022 •

edited