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

apparmor: Fix wrong determination whether crun is confined #1409

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

idleroamer
Copy link
Contributor

makes #1408 and #1406 unnecessary
Closes: #1385

Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM

I don't have much idea about apparmor but this strstr could find the unconfined value not related to the current scope, since now this will return true even if unconfined is found anywere in string. Is this expected ?

@@ -941,7 +941,7 @@ is_current_process_confined (libcrun_error_t *err)
if (UNLIKELY (bytes_read < 0))
return crun_make_error (err, errno, "error reading file `%s`", attr_path);

return (strncmp (buf, "unconfined", bytes_read) != 0 && buf[0] != '\0');
return (strncmp (buf, "unconfined", 10) != 0);
Copy link
Member

Choose a reason for hiding this comment

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

it can use unitialized data.

We need to enforce bytes_read >= 10 and we could even use memcmp since we know the size, something like:

#define UNCONFINED "unconfined"
#define UNCONFINED_LEN (sizeof (UNCONFINED) - 1)
return bytes_read >= UNCONFINED_LEN && memcmp (buf, UNCONFINED, UNCONFINED_LEN));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds much better :)
Thanks

@giuseppe
Copy link
Member

giuseppe commented Feb 2, 2024

@hswong3i what do you think about this 3rd option? :-)

@hswong3i
Copy link
Contributor

hswong3i commented Feb 2, 2024

@hswong3i what do you think about this 3rd option? :-)

I could replace the patch to my OBS package and put my production server to death for a try 🤪

@idleroamer idleroamer force-pushed the nnp-confined branch 2 times, most recently from 98deadd to 278a23d Compare February 2, 2024 11:43
Closes: containers#1385
Signed-off-by: 😎Mostafa Emami <mustafaemami@gmail.com>
@giuseppe
Copy link
Member

giuseppe commented Feb 2, 2024

@hswong3i please let us know if this solves your problem :-)

hswong3i added a commit to alvistack/containers-crun that referenced this pull request Feb 2, 2024
    git clean -xdf
    git submodule sync --recursive
    git submodule update --recursive
    git submodule foreach --recursive git clean -xdf
    tar zcvf ../crun_1.14.orig.tar.gz --exclude=.git .
    debuild -uc -us
    cp crun.spec ../crun_1.14-1.spec
    cp ../crun*1.14*.{gz,xz,spec,dsc} /osc/home\:alvistack/containers-crun-1.14/
    rm -rf ../crun*1.14*.*

See containers#1405
See containers#1409

Signed-off-by: Wong Hoi Sing Edison <hswong3i@pantarei-design.com>
@hswong3i
Copy link
Contributor

hswong3i commented Feb 2, 2024

@hswong3i please let us know if this solves your problem :-)

Yes it works (alvistack@96cf21b and https://build.opensuse.org/package/show/home:alvistack/containers-crun-1.14):

root@ansible21:~# kubectl -n ingress-nginx get all
NAME                                            READY   STATUS    RESTARTS   AGE
pod/ingress-nginx-controller-79df5b7cd7-2fgc4   1/1     Running   0          88s
pod/nginx-ingress-controller-proxy-9hzr9        2/2     Running   0          65s
pod/nginx-ingress-controller-proxy-cd9st        2/2     Running   0          60s
pod/nginx-ingress-controller-proxy-sswl5        2/2     Running   0          61s

NAME                                         TYPE        CLUSTER-IP      EXTERNAL-IP   PORT(S)          AGE
service/ingress-nginx-controller             ClusterIP   10.233.27.0     <none>        80/TCP,443/TCP   136m
service/ingress-nginx-controller-admission   ClusterIP   10.233.23.233   <none>        443/TCP          136m

NAME                                            DESIRED   CURRENT   READY   UP-TO-DATE   AVAILABLE   NODE SELECTOR   AGE
daemonset.apps/nginx-ingress-controller-proxy   3         3         3       3            3           <none>          136m

NAME                                       READY   UP-TO-DATE   AVAILABLE   AGE
deployment.apps/ingress-nginx-controller   1/1     1            1           136m

NAME                                                  DESIRED   CURRENT   READY   AGE
replicaset.apps/ingress-nginx-controller-79df5b7cd7   1         1         1       136m

NAME                                       COMPLETIONS   DURATION   AGE
job.batch/ingress-nginx-admission-create   1/1           7s         136m
job.batch/ingress-nginx-admission-patch    1/1           8s         136m

@giuseppe
Copy link
Member

giuseppe commented Feb 2, 2024

wonderful, let's merge it!

@giuseppe giuseppe merged commit 0ac616b into containers:main Feb 2, 2024
36 checks passed
hswong3i added a commit to alvistack/containers-crun that referenced this pull request Feb 6, 2024
    git clean -xdf
    git submodule sync --recursive
    git submodule update --recursive
    git submodule foreach --recursive git clean -xdf
    tar zcvf ../crun_1.14.orig.tar.gz --exclude=.git .
    debuild -uc -us
    cp crun.spec ../crun_1.14-1.spec
    cp ../crun*1.14*.{gz,xz,spec,dsc} /osc/home\:alvistack/containers-crun-1.14/
    rm -rf ../crun*1.14*.*

See containers#1405
See containers#1409

Signed-off-by: Wong Hoi Sing Edison <hswong3i@pantarei-design.com>
hswong3i added a commit to alvistack/containers-crun that referenced this pull request Feb 9, 2024
    git clean -xdf
    git submodule sync --recursive
    git submodule update --recursive
    git submodule foreach --recursive git clean -xdf
    tar zcvf ../crun_1.14.orig.tar.gz --exclude=.git .
    debuild -uc -us
    cp crun.spec ../crun_1.14-1.spec
    cp ../crun*1.14*.{gz,xz,spec,dsc} /osc/home\:alvistack/containers-crun-1.14/
    rm -rf ../crun*1.14*.*

See containers#1405
See containers#1409

Signed-off-by: Wong Hoi Sing Edison <hswong3i@pantarei-design.com>
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.

crun doesn't use apparmor stacking if confined and nnp set
4 participants