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

Add keyring option #3260

Merged
merged 3 commits into from Jan 31, 2020
Merged

Add keyring option #3260

merged 3 commits into from Jan 31, 2020

Conversation

blenk92
Copy link
Contributor

@blenk92 blenk92 commented Jan 29, 2020

lxc set's up a new session keyring for every container by default. If executed on an SELinux enabled system, by default, the keyring inherits the label of the creating process. If executed with the
currently available SELinux policy, this means that the keyring is labeled with the lxc_t type. Applications inside the container, however, might expect that the keyring is labeled with a certain context (and will fail to access the keyring if it's not explicitly allowed in the global policy). This patch introduces the config option lxc.selinux.keyring_context which enables to specify the label of the newly created keyring. That is, the keyring can be labeled with the label expected by the started application.

@lxc-jenkins
Copy link

This pull request didn't trigger Jenkins as its author isn't in the whitelist.

An organization member must perform one of the following:

  • To have this branch tested by Jenkins, use the "ok to test" command.
  • To have a one time test done, use the "test this please" command.

Those commands are simple Github comments of the format: "jenkins: COMMAND"

@blenk92 blenk92 requested review from brauner and Blub January 29, 2020 17:31
Copy link
Member

@brauner brauner left a comment

Choose a reason for hiding this comment

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

So this all looks sane to me.
One question I have is whether you expect there to be additional keyring related options. If so then maybe:

lxc.selinux.keyring.context

is the better idea. If not maybe

lxc.selinux.context_keyring

might make more sense:

lxc.selinux.context
lxc.selinux.context_keyring

or even

lxc.selinux.context
lxc.selinux.context.keyring

?
@stgraber, thoughts?

@brauner
Copy link
Member

brauner commented Jan 29, 2020

lxc.selinux.context
lxc.selinux.context.keyring

I'm slightly leaning towards this one...

@blenk92
Copy link
Contributor Author

blenk92 commented Jan 30, 2020

So this all looks sane to me.
One question I have is whether you expect there to be additional keyring related options.

Thanks for the super fast review ;-)

To be honest, I truly am not an expert for that keyring stuff. I just learned about that when debugging my issue :-D

One thing that comes to my mind though would be an (probably non-SELinux related) option to completely disable the creation of a new session keyring. To my understanding, if the container is started as a systemd service (for instance), the following happens:

  1. by default systemd creates a new session keyring for every service
  2. lxc creates a new session keyring
  3. if, for instance, another init process is started within the container, yet another session keyring might be created.

Although this probably doesn't increase performance a lot, in such scenarios the creation of the session keyring by systemd (1.) and lxc (2.) could be spared (afaik systemd offers that option).

Other than that, the keyctl system call (http://man7.org/linux/man-pages/man2/keyctl.2.html) offers a lot of options, and there might be some use for some of them. And I guess if one wants to share keys via the session keyring with a container, the disable option might also be useful. But as I said, I just learned about that stuff...

@brauner
Copy link
Member

brauner commented Jan 30, 2020

Hm, yeah. Maybe a lxc.keyring.* namespace might be a good idea. lxc.keyring.session which can be set to 1 to create a new session keyring (default) and 0 to not create a new session keyring?

@blenk92
Copy link
Contributor Author

blenk92 commented Jan 30, 2020

Hm, yeah. Maybe a lxc.keyring.* namespace might be a good idea. lxc.keyring.session which can be set to 1 to create a new session keyring (default) and 0 to not create a new session keyring?

And for the context lxc.keyring.context ? Sounds good to me. I can add this if you want...

@brauner
Copy link
Member

brauner commented Jan 30, 2020 via email

@blenk92
Copy link
Contributor Author

blenk92 commented Jan 30, 2020

t I think lxc.keyring.selinux.context feels more natural?

Okay, works for me. I'll update my PR then to support both, disabling the session keyring creation (lxc.keyring.session) and labeling the keyring (lxc.keyring.selinux.context)

@blenk92 blenk92 force-pushed the add-keyring-option branch 2 times, most recently from 6c61b6f to d28784e Compare January 31, 2020 09:52
@blenk92
Copy link
Contributor Author

blenk92 commented Jan 31, 2020

last update was due to spaces vs. tabs for indentaion :-D
From my side the patches would be ready to be reviewed again :-D

@brauner
Copy link
Member

brauner commented Jan 31, 2020

last update was due to spaces vs. tabs for indentaion :-D
From my side the patches would be ready to be reviewed again :-D

I'm doing a review now but I've just spoken to @stgraber and we think that lxc.selinux.keyring.context makes more sense because by default the keyring will have the same context as specified in lxc.selinux.context. :D Sorry to make you rework again. Hopefully this is not much work...

@blenk92
Copy link
Contributor Author

blenk92 commented Jan 31, 2020

last update was due to spaces vs. tabs for indentaion :-D
From my side the patches would be ready to be reviewed again :-D

I'm doing a review now but I've just spoken to @stgraber and we think that lxc.selinux.keyring.context makes more sense because by default the keyring will have the same context as specified in lxc.selinux.context. :D Sorry to make you rework again. Hopefully this is not much work...

no worries, shouldn't be too much work :-D

Actually, I don't think this is what happens. If you look at:

        /* Setup the container, ip, names, utsname, ... */
        ret = lxc_setup(handler);
        if (ret < 0) { 
                ERROR("Failed to setup container \"%s\"", handler->name);
                goto out_warn_father;
        }

        /* Set the label to change to when we exec(2) the container's init. */
        ret = lsm_process_label_set(NULL, handler->conf, true);
        if (ret < 0) 
                goto out_warn_father;

The new session keyring is vreated in lxc_setup(). But the selinux context will earliest be set after lxc_setup(). In addition, the on_exec parameter of lsm_process_label_set() is set to true, which imho set the selinux label on the next exec..(). so by default the label of the keyring will be the label of lxc.

@blenk92
Copy link
Contributor Author

blenk92 commented Jan 31, 2020

I mean, what we can do is set the lxc.selinux.context label if lxc.selinux.keyring.context is not set...

@brauner
Copy link
Member

brauner commented Jan 31, 2020

I mean, what we can do is set the lxc.selinux.context label if lxc.selinux.keyring.context is not set...

Sounds good.
Sorry, the key should probably be lxc.selinux.context.keyring. :) Just got the ordering wrong :)

@blenk92
Copy link
Contributor Author

blenk92 commented Jan 31, 2020

I mean, what we can do is set the lxc.selinux.context label if lxc.selinux.keyring.context is not set...

Sounds good.
Sorry, the key should probably be lxc.selinux.context.keyring. :) Just got the ordering wrong :)

okay, I added that and fixed the naming of the config option :-D

@brauner
Copy link
Member

brauner commented Jan 31, 2020

jenkins: test this please

@brauner
Copy link
Member

brauner commented Jan 31, 2020

Compiler seems unhappy :)

target type [-Werror]
  return setkeycreatecon_raw(label);
  ^
In file included from lsm/selinux.c:7:0:
/usr/include/selinux/selinux.h:83:12: note: expected 'security_context_t' but argument is of type 'const char *'
 extern int setkeycreatecon_raw(const security_context_t context);
            ^
cc1: all warnings being treated as errors

@blenk92
Copy link
Contributor Author

blenk92 commented Jan 31, 2020

Compiler seems unhappy :)

target type [-Werror]
  return setkeycreatecon_raw(label);
  ^
In file included from lsm/selinux.c:7:0:
/usr/include/selinux/selinux.h:83:12: note: expected 'security_context_t' but argument is of type 'const char *'
 extern int setkeycreatecon_raw(const security_context_t context);
            ^
cc1: all warnings being treated as errors

Ah its because typedef char *security_context_t;. Didn't happen locally though :-D

Maximilian Blenk added 3 commits January 31, 2020 14:33
lxc set's up a new session keyring for every container by default.
If executed on an SELinux enabled system, by default, the keyring
inherits the label of the creating process. If executed with the
currently available SELinux policy, this means that the keyring
is labeled with the lxc_t type. Applications inside the container,
however, might expect that the keyring is labeled with a certain
context (and will fail to access the keyring if it's not explicitly
allowed in the global policy). This patch introduces the config
option lxc.selinux.context.keyring which enables to specify the
label of the newly created keyring. That is, the keyring can be
labeled with the label expected by the started application.

Signed-off-by: Maximilian Blenk <Maximilian.Blenk@bmw.de>
lxc set's up a new session keyring for every container by default.
There might be valid use-cases where this is not wanted / needed
(e.g. systemd by default creates a new session keyring anyway).

Signed-off-by: Maximilian Blenk <Maximilian.Blenk@bmw.de>
Signed-off-by: Maximilian Blenk <Maximilian.Blenk@bmw.de>
@brauner
Copy link
Member

brauner commented Jan 31, 2020

jenkins: test this please

@blenk92
Copy link
Contributor Author

blenk92 commented Jan 31, 2020

jenkins: test this please

FATAL: command execution failed
java.io.EOFException
	at java.io.ObjectInputStream$PeekInputStream.readFully(ObjectInputStream.java:2736)
	at java.io.ObjectInputStream$BlockDataInputStream.readShort(ObjectInputStream.java:3211)
	at java.io.ObjectInputStream.readStreamHeader(ObjectInputStream.java:896)
	at java.io.ObjectInputStream.<init>(ObjectInputStream.java:358)
	at hudson.remoting.ObjectInputStreamEx.<init>(ObjectInputStreamEx.java:49)
	at hudson.remoting.Command.readFrom(Command.java:142)
	at hudson.remoting.Command.readFrom(Command.java:128)
	at hudson.remoting.AbstractSynchronousByteArrayCommandTransport.read(AbstractSynchronousByteArrayCommandTransport.java:35)
	at hudson.remoting.SynchronousCommandTransport$ReaderThread.run(SynchronousCommandTransport.java:63)
Caused: java.io.IOException: Unexpected termination of the channel
	at hudson.remoting.SynchronousCommandTransport$ReaderThread.run(SynchronousCommandTransport.java:77)
Caused: java.io.IOException: Backing channel 'stgraber-buildd05' is disconnected.
	at hudson.remoting.RemoteInvocationHandler.channelOrFail(RemoteInvocationHandler.java:216)
	at hudson.remoting.RemoteInvocationHandler.invoke(RemoteInvocationHandler.java:285)
	at com.sun.proxy.$Proxy83.isAlive(Unknown Source)
	at hudson.Launcher$RemoteLauncher$ProcImpl.isAlive(Launcher.java:1147)
	at hudson.Launcher$RemoteLauncher$ProcImpl.join(Launcher.java:1139)
	at hudson.tasks.CommandInterpreter.join(CommandInterpreter.java:155)
	at hudson.tasks.CommandInterpreter.perform(CommandInterpreter.java:109)
	at hudson.tasks.CommandInterpreter.perform(CommandInterpreter.java:66)
	at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
	at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:741)
	at hudson.model.Build$BuildExecution.build(Build.java:206)
	at hudson.model.Build$BuildExecution.doRun(Build.java:163)
	at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:504)
	at hudson.model.Run.execute(Run.java:1853)
	at hudson.matrix.MatrixRun.run(MatrixRun.java:153)
	at hudson.model.ResourceController.execute(ResourceController.java:97)
	at hudson.model.Executor.run(Executor.java:428)
FATAL: Unable to delete script file /tmp/jenkins15065238728591833241.sh
java.io.EOFException
	at java.io.ObjectInputStream$PeekInputStream.readFully(ObjectInputStream.java:2736)
	at java.io.ObjectInputStream$BlockDataInputStream.readShort(ObjectInputStream.java:3211)
	at java.io.ObjectInputStream.readStreamHeader(ObjectInputStream.java:896)
	at java.io.ObjectInputStream.<init>(ObjectInputStream.java:358)
	at hudson.remoting.ObjectInputStreamEx.<init>(ObjectInputStreamEx.java:49)
	at hudson.remoting.Command.readFrom(Command.java:142)
	at hudson.remoting.Command.readFrom(Command.java:128)
	at hudson.remoting.AbstractSynchronousByteArrayCommandTransport.read(AbstractSynchronousByteArrayCommandTransport.java:35)
	at hudson.remoting.SynchronousCommandTransport$ReaderThread.run(SynchronousCommandTransport.java:63)
Caused: java.io.IOException: Unexpected termination of the channel
	at hudson.remoting.SynchronousCommandTransport$ReaderThread.run(SynchronousCommandTransport.java:77)
Caused: hudson.remoting.ChannelClosedException: Channel "hudson.remoting.Channel@204e1cd0:stgraber-buildd05": Remote call on stgraber-buildd05 failed. The channel is closing down or has closed down
	at hudson.remoting.Channel.call(Channel.java:944)
	at hudson.FilePath.act(FilePath.java:1069)
	at hudson.FilePath.act(FilePath.java:1058)
	at hudson.FilePath.delete(FilePath.java:1539)
	at hudson.tasks.CommandInterpreter.perform(CommandInterpreter.java:123)
	at hudson.tasks.CommandInterpreter.perform(CommandInterpreter.java:66)
	at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
	at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:741)
	at hudson.model.Build$BuildExecution.build(Build.java:206)
	at hudson.model.Build$BuildExecution.doRun(Build.java:163)
	at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:504)
	at hudson.model.Run.execute(Run.java:1853)
	at hudson.matrix.MatrixRun.run(MatrixRun.java:153)
	at hudson.model.ResourceController.execute(ResourceController.java:97)
	at hudson.model.Executor.run(Executor.java:428)
Build step 'Execute shell' marked build as failure

i guess this is not related to my changes?

@brauner
Copy link
Member

brauner commented Jan 31, 2020

jenkins: test this please

FATAL: command execution failed
java.io.EOFException
	at java.io.ObjectInputStream$PeekInputStream.readFully(ObjectInputStream.java:2736)
	at java.io.ObjectInputStream$BlockDataInputStream.readShort(ObjectInputStream.java:3211)
	at java.io.ObjectInputStream.readStreamHeader(ObjectInputStream.java:896)
	at java.io.ObjectInputStream.<init>(ObjectInputStream.java:358)
	at hudson.remoting.ObjectInputStreamEx.<init>(ObjectInputStreamEx.java:49)
	at hudson.remoting.Command.readFrom(Command.java:142)
	at hudson.remoting.Command.readFrom(Command.java:128)
	at hudson.remoting.AbstractSynchronousByteArrayCommandTransport.read(AbstractSynchronousByteArrayCommandTransport.java:35)
	at hudson.remoting.SynchronousCommandTransport$ReaderThread.run(SynchronousCommandTransport.java:63)
Caused: java.io.IOException: Unexpected termination of the channel
	at hudson.remoting.SynchronousCommandTransport$ReaderThread.run(SynchronousCommandTransport.java:77)
Caused: java.io.IOException: Backing channel 'stgraber-buildd05' is disconnected.
	at hudson.remoting.RemoteInvocationHandler.channelOrFail(RemoteInvocationHandler.java:216)
	at hudson.remoting.RemoteInvocationHandler.invoke(RemoteInvocationHandler.java:285)
	at com.sun.proxy.$Proxy83.isAlive(Unknown Source)
	at hudson.Launcher$RemoteLauncher$ProcImpl.isAlive(Launcher.java:1147)
	at hudson.Launcher$RemoteLauncher$ProcImpl.join(Launcher.java:1139)
	at hudson.tasks.CommandInterpreter.join(CommandInterpreter.java:155)
	at hudson.tasks.CommandInterpreter.perform(CommandInterpreter.java:109)
	at hudson.tasks.CommandInterpreter.perform(CommandInterpreter.java:66)
	at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
	at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:741)
	at hudson.model.Build$BuildExecution.build(Build.java:206)
	at hudson.model.Build$BuildExecution.doRun(Build.java:163)
	at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:504)
	at hudson.model.Run.execute(Run.java:1853)
	at hudson.matrix.MatrixRun.run(MatrixRun.java:153)
	at hudson.model.ResourceController.execute(ResourceController.java:97)
	at hudson.model.Executor.run(Executor.java:428)
FATAL: Unable to delete script file /tmp/jenkins15065238728591833241.sh
java.io.EOFException
	at java.io.ObjectInputStream$PeekInputStream.readFully(ObjectInputStream.java:2736)
	at java.io.ObjectInputStream$BlockDataInputStream.readShort(ObjectInputStream.java:3211)
	at java.io.ObjectInputStream.readStreamHeader(ObjectInputStream.java:896)
	at java.io.ObjectInputStream.<init>(ObjectInputStream.java:358)
	at hudson.remoting.ObjectInputStreamEx.<init>(ObjectInputStreamEx.java:49)
	at hudson.remoting.Command.readFrom(Command.java:142)
	at hudson.remoting.Command.readFrom(Command.java:128)
	at hudson.remoting.AbstractSynchronousByteArrayCommandTransport.read(AbstractSynchronousByteArrayCommandTransport.java:35)
	at hudson.remoting.SynchronousCommandTransport$ReaderThread.run(SynchronousCommandTransport.java:63)
Caused: java.io.IOException: Unexpected termination of the channel
	at hudson.remoting.SynchronousCommandTransport$ReaderThread.run(SynchronousCommandTransport.java:77)
Caused: hudson.remoting.ChannelClosedException: Channel "hudson.remoting.Channel@204e1cd0:stgraber-buildd05": Remote call on stgraber-buildd05 failed. The channel is closing down or has closed down
	at hudson.remoting.Channel.call(Channel.java:944)
	at hudson.FilePath.act(FilePath.java:1069)
	at hudson.FilePath.act(FilePath.java:1058)
	at hudson.FilePath.delete(FilePath.java:1539)
	at hudson.tasks.CommandInterpreter.perform(CommandInterpreter.java:123)
	at hudson.tasks.CommandInterpreter.perform(CommandInterpreter.java:66)
	at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
	at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:741)
	at hudson.model.Build$BuildExecution.build(Build.java:206)
	at hudson.model.Build$BuildExecution.doRun(Build.java:163)
	at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:504)
	at hudson.model.Run.execute(Run.java:1853)
	at hudson.matrix.MatrixRun.run(MatrixRun.java:153)
	at hudson.model.ResourceController.execute(ResourceController.java:97)
	at hudson.model.Executor.run(Executor.java:428)
Build step 'Execute shell' marked build as failure

i guess this is not related with my changes?

Nah, that's not really relevant :) Just means the builder went offline.

@brauner
Copy link
Member

brauner commented Jan 31, 2020

Thanks! :)

@brauner brauner merged commit a8b9feb into lxc:master Jan 31, 2020
jpuhlman pushed a commit to MontaVista-OpenSourceTechnology/meta-virtualization that referenced this pull request Feb 14, 2020
Source: meta-virtualization
MR: 00000
Type: Integration
Disposition: Merged from meta-virtualization
ChangeID: b8c810c
Description:

The added patches allow to set the SELinux context for the session
keyring that is created by lxc. In addition it is possible to disable
the creation of a new session keyring completely.

Upstream PR: lxc/lxc#3260 (merged)

If lxc is executed on a SELinux enabled system, these options can be
used to assign the expected label to the session keyring.

Signed-off-by: Maximilian Blenk <maximilian.blenk@bmw.de>
Signed-off-by: Bruce Ashfield <bruce.ashfield@gmail.com>
Signed-off-by: Jeremy Puhlman <jpuhlman@mvista.com>
armcc pushed a commit to lgirdk/meta-virtualization that referenced this pull request Feb 14, 2020
The added patches allow to set the SELinux context for the session
keyring that is created by lxc. In addition it is possible to disable
the creation of a new session keyring completely.

Upstream PR: lxc/lxc#3260 (merged)

If lxc is executed on a SELinux enabled system, these options can be
used to assign the expected label to the session keyring.

Signed-off-by: Maximilian Blenk <maximilian.blenk@bmw.de>
Signed-off-by: Bruce Ashfield <bruce.ashfield@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants