/snapd

Add an interface for tpm #1502

Merged
merged 19 commits into from Aug 12, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
Reviewed
Projects
None yet
Milestone
No milestone
6 participants
@jessesung @zyga @snappy-m-o @morphis @jdstrand @mvo5
@jessesung
Contributor

jessesung commented Jul 7, 2016

Add an interface for tpm 1.2

Not sure if tpm 2.0 would share the same interface, so right now just name it as tpm12.

Signed-off-by: Wen-chien Jesse Sung jesse.sung@canonical.com
@jessesung
Add an interface for tpm 1.2 
Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
07c1c09
interfaces/builtin/tpm12.go
+capability dac_override,
+
+/dev/tpm0 rw,
+/etc/tcsd.conf r,
@zyga

zyga Jul 7, 2016

Contributor

Is this file present in the core snap?

interfaces/builtin/tpm12.go
+
+/dev/tpm0 rw,
+/etc/tcsd.conf r,
+/usr/sbin/tcsd mr,
@zyga

zyga Jul 7, 2016

Contributor

Is this executable present in the core snap?

interfaces/builtin/tpm12.go
+/dev/tpm0 rw,
+/etc/tcsd.conf r,
+/usr/sbin/tcsd mr,
+/var/lib/tpm/system.data rw,
@zyga

zyga Jul 7, 2016

Contributor

This shouldn't be used like that. Please make tpm tools respect $SNAP and $SNAP_DATA

interfaces/builtin/tpm12.go
+#include <abstractions/apache2-common>
+#include <abstractions/base>
+
+@{HOME}/.trousers/user.data rw,
@zyga

zyga Jul 7, 2016

Contributor

$HOME will be snap-specific so this rule is irrelevant.

@zyga zyga added the Reviewed label Jul 7, 2016

@zyga
Contributor

zyga commented Jul 7, 2016 •
Edited 1 time

Hey

I think this is not the way to go. Did you test this interface with trousers?

Remember that at runtime $HOME won't be the real home and most of the filesystem will be the read-only core snap. You have to use $SNAP_DATA to store data related to the snap but not specific to any user.
interfaces/builtin/tpm12.go
+# Description: for tcsd
+# Usage: common
+
+#include <abstractions/apache2-common>
@zyga

zyga Jul 7, 2016

Contributor

Hmm, why?

interfaces/builtin/tpm12.go
+
+#include <abstractions/apache2-common>
+#include <abstractions/base>
+#include <abstractions/postfix-common>
@zyga

zyga Jul 7, 2016

Contributor

Why?

@jessesung
Clean up AppArmor profiles for both slot and plug 
Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
9b86c7d
@snappy-m-o
Contributor

snappy-m-o commented Jul 12, 2016

Can one of the admins verify this patch?
@snappy-m-o
Contributor

snappy-m-o commented Jul 12, 2016

Can one of the admins verify this patch?
@jessesung
Contributor

jessesung commented Jul 12, 2016

Thanks for the comments.

Removed rules that should be utilized via $SNAP and $SNAP_DATA in tpm snap.

The <abstractions/apache2-common> and <abstractions/postfix-common> are generated by aa-genprof, and the permission actually required by tpm snap is bind (slot) and connect to (plug) interface lo. Both of these includes are removed.

For the network permissions, I'm not sure if this should be the way to go, to include network permissions directly in tpm interface, or just let tpm snap uses network and network-bind instead and remove the <abstractions/nameservice>.
interfaces/builtin/all.go
@@ -34,6 +34,7 @@ var allInterfaces = []interfaces.Interface{
&NetworkManagerInterface{},
&PppInterface{},
&SerialPortInterface{},
+ &Tpm12Interface{},
@morphis

morphis Jul 13, 2016

Contributor

Why do we name it Tpm12? Shouldn't this be just TPM so its not specific to any software or its version?

interfaces/builtin/tpm12.go
+func (iface *Tpm12Interface) ConnectedPlugSnippet(plug *interfaces.Plug, slot *interfaces.Slot, securitySystem interfaces.SecuritySystem) ([]byte, error) {
+ switch securitySystem {
+ case interfaces.SecurityAppArmor:
+ old := []byte("###SLOT_SECURITY_TAGS###")
@morphis

morphis Jul 13, 2016

Contributor

I don't see this uses anywhere so drop it.

interfaces/builtin/tpm12.go
+`)
+
+var tpm12PermanentSlotSecComp = []byte(`
+# Description: for tcsd
@morphis

morphis Jul 13, 2016

Contributor

This shouldn't menation tcsd but describe that those are needed to talk to the system TPM chip over /dev/tpm0

@jessesung
Rename the interface and remove plug settings 
Let network and network-bind do their jobs. This interface
serves processes would like to talk to /dev/tpm0 only.

Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
b6838aa
interfaces/builtin/tpm.go
+# Description: for those who need to talk to the system TPM chip over /dev/tpm0
+# Usage: common
+
+/dev/tpm0 rw,
@morphis

morphis Jul 18, 2016

Contributor

Can we ever only have /dev/tpm0 or can we also have multiple? Maybe not really common so lets start with tpm0.

interfaces/builtin/tpm.go
+ case interfaces.SecurityAppArmor:
+ return tpmPermanentSlotAppArmor, nil
+ case interfaces.SecuritySecComp, interfaces.SecurityDBus, interfaces.SecurityUDev:
+ return nil, nil
@morphis

morphis Jul 18, 2016

Contributor

Is there no further syscall required other than what the common part of the security profile already allows?

@jessesung

jessesung Jul 19, 2016 •
Edited 1 time

Contributor

No, syscalls required are in network and network-bind and tpm snap will use these interfaces.

@jdstrand

jdstrand Jul 25, 2016

Contributor

Based on your comment, I think you are saying that access to /dev/tpm0 would use standard syscalls and anything beyond that is simply because the software opening it has additional functionality that is correctly covered by network and network-bind.

What Simon and I want to make sure is that if the tpm snap legitimately needs network (ie, it reaches out over the network) and network-bind (it opens a network port), then, sure, don't worry about extra syscalls, but if you are using those interfaces when the tpm snap doesn't reach out over the network or bind to a port, then the needed syscalls should be here too.

interfaces/builtin/tpm.go
+/dev/tpm0 rw,
+`)
+
+type TpmInterface struct { }
@morphis

morphis Jul 21, 2016

Contributor

Also we can simply this class a lot of the allow the common interface class to specify slot snippets too. @zyga what do you think?

interfaces/builtin/tpm.go
+
+var tpmPermanentSlotAppArmor = []byte(`
+# Description: for those who need to talk to the system TPM chip over /dev/tpm0
+# Usage: common
@morphis

morphis Jul 21, 2016

Contributor

This should be reserved

@morphis
Contributor

morphis commented Jul 21, 2016

@jessesung You need to make the CI still happy. Also please add a description to doc/interfaces.md and atleast a manual integration test to https://github.com/snapcore/snapd/blob/master/integration-tests/manual-tests.md

jessesung added some commits Jul 22, 2016

@jessesung
Fix source formatting 
Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
a888c7b
@jessesung
Add copyright header 
Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
7611ba4
@jessesung
Change usage to reserved instead of common 
Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
0a4d199
@jessesung
Add an entry for tpm in docs/interfaces.md 
Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
80e45a9
interfaces/builtin/tpm.go
+# Description: for those who need to talk to the system TPM chip over /dev/tpm0
+# Usage: reserved
+
+/dev/tpm0 rw,
@jdstrand

jdstrand Jul 25, 2016

Contributor

I suspect you'll also need:
/sys/class/tpm/tpm0/** r,

but also to resolve /sys/class/tpm/tpm0/device and /sys/class/tpm/tpm0/subsystem to add the resolved paths.

@jessesung

jessesung Jul 27, 2016

Contributor

Trousers works after having the permission to read/write /dev/tpm0 in my tests. I think we can start from this simple rule and add more rules later if we find something doesn't work.

interfaces/builtin/tpm.go
+
+func (iface *TpmInterface) PermanentPlugSnippet(plug *interfaces.Plug, securitySystem interfaces.SecuritySystem) ([]byte, error) {
+ switch securitySystem {
+ case interfaces.SecurityDBus, interfaces.SecurityAppArmor, interfaces.SecuritySecComp, interfaces.SecurityUDev:
@jdstrand

jdstrand Jul 25, 2016

Contributor

You're missing interfaces.SecurityMount here and elsewhere.

@jdstrand
Contributor

jdstrand commented Jul 25, 2016 •
Edited 1 time

In addition to my other comments, this also needs tests. Furthermore, a cursory check shows that a tpm 2 device shows up as /dev/tpm0 so I think calling this 'tpm' is the right choice (even if the description in this PR says otherwise (the code is right though)).
@jessesung
Add missing interfaces.SecurityMount 
Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
8280e8f

@jessesung jessesung changed the title from Add an interface for tpm 1.2 to Add an interface for tpm Jul 27, 2016

jessesung added some commits Aug 10, 2016

@jessesung
Use plug instead of slot in tpm interface 
Originally tscd would be the slot and tpm-tools the plug.
Since tcsd and tpm-tools talk through lo which can be covered by
network and network-bind, this interface is for tcsd only.

This commit makes ubuntu-core provide tpm slot, and tcsd will
be the plug.

Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
3585ee5
@jessesung
Add an entry for tpm interface in mainual-tests 
Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
dbe0410
@jessesung
Tests for tpm interface 
Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
7b8a330
@jessesung
Merge branch 'master' into tpm12 
Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
febbced
@jessesung
Fix compilation error 
Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
fd2c1bb
@jessesung
Remove unused import 
Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
62d24d5
docs/interfaces.md
+
+### tpm
+
+Can access the first tpm device.
@morphis

morphis Aug 11, 2016

Contributor

What does 'first' mean here?

@jessesung

jessesung Aug 11, 2016

Contributor

tpm0 in /dev since that's the only device apparmor profile allows now.

integration-tests/manual-tests.md
+# Test tpm interface with tpm-tools
+
+1. Install tpm snap from store.
+2. Use tpm.version to read some basic information from tpm device.
@morphis

morphis Aug 11, 2016

Contributor

Can you add the exact commands and example output?

@morphis
Contributor

morphis commented Aug 11, 2016

Some small things in line, otherwise LGTM.
@jessesung
Update documentation 
Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
fa20197
@morphis
Contributor

morphis commented Aug 11, 2016

LGTM
interfaces/builtin/tpm.go
+/dev/tpm0 rw,
+`
+
+const tpmConnectedPlugSecComp = `
@morphis

morphis Aug 11, 2016

Contributor

This can be dropped as its empty.

@jessesung
Remove unused connectedPlugSecComp 
Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
22f4453
docs/interfaces.md
+Can access the tpm device /dev/tpm0.
+
+Usage: reserved
+Auto-Connect: no
@jdstrand

jdstrand Aug 11, 2016

Contributor

Can you adjust this for the new format? FYI I notice that system-trace isn't following the proper format (feel free to fix it in passing if you want).

interfaces/builtin/tpm_test.go
+ c.Assert(snippet, IsNil)
+ snippet, err = s.iface.ConnectedPlugSnippet(s.plug, s.slot, interfaces.SecurityUDev)
+ c.Assert(err, IsNil)
+ c.Assert(snippet, IsNil)
@jdstrand

jdstrand Aug 11, 2016 •
Edited 1 time

Contributor

interfaces.SecuritySecComp and interfaces.SecurityMount are also unused. with s.iface.ConnectedPlugSnippet

interfaces/builtin/tpm_test.go
+ // connected plugs have a nil security snippet for seccomp
+ snippet, err = s.iface.ConnectedPlugSnippet(s.plug, s.slot, interfaces.SecuritySecComp)
+ c.Assert(err, IsNil)
+ c.Assert(snippet, IsNil)
@jdstrand

jdstrand Aug 11, 2016

Contributor

You can drop the seccomp one here based on my comment to add it to TestUnusedSecuritySystems

@jessesung
Fix formats in docs/interfaces.md 
Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
11a3f77
@jdstrand
Contributor

jdstrand commented Aug 11, 2016

Once you make the testsuite and documentation fixes, LGTM.
@jessesung
Fix testsuite 
interfaces.SecuritySecComp and interfaces.SecurityMount of
s.iface.ConnectedPlugSnippet are unused. Move them to proper place.

Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
56b7f7c
@jdstrand
Contributor

jdstrand commented Aug 11, 2016

Thanks for making these changes (and the system-trace doc change). LGTM so long as the tests pass and/or a member of the snappy core team says the test failure is ok.

+1
@jessesung
Fix test for SecuritySecComp of ConnectedPlugSnippet 
SecuritySecComp is not used in the interface, but it won't be nil
because of commonInterface.

Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
b1d0446
@morphis
Contributor

morphis commented Aug 12, 2016

LGTM.
docs/interfaces.md
@@ -357,11 +357,16 @@ Can use kernel tracing facilities. This is restricted because it gives
privileged access to all processes on the system and should only be used with
trusted apps.
-Usage: reserved
@mvo5

mvo5 Aug 12, 2016

Collaborator

This change looks unreleated to the branch. Copy/paste error?

@mvo5

mvo5 Aug 12, 2016

Collaborator

Oh, nevermind. I see that Usage: reserved is removed everywhere. So +1 for this change.

@jessesung

jessesung Aug 12, 2016

Contributor

#1502 (comment)

System-trace happens to be in old format too. Fix it in the same commit with tpm.

interfaces/builtin/tpm.go
+# Description: for those who need to talk to the system TPM chip over /dev/tpm0
+# Usage: reserved
+
+/dev/tpm0 rw,
@mvo5

mvo5 Aug 12, 2016

Collaborator

Silly(?) question - can there ever be more than a single tpm? i.e. /dev/tpm{0,1,2} or something?

@jessesung

jessesung Aug 12, 2016

Contributor

I'm not sure... But since devices we have right now have only tpm0, I'd suggest we start from here.

@mvo5
Collaborator

mvo5 commented Aug 12, 2016

👍

@mvo5 mvo5 merged commit d09b97f into snapcore:master Aug 12, 2016
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@snappy-m-o
Contributor

snappy-m-o commented Aug 12, 2016

Can one of the admins verify this patch?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment