stage0: add container hash data into TPM #1775
Conversation
mjg59
force-pushed
the
tpm_hash
branch
3 times, most recently
from
0671a81
to
08bf020
Compare
|
go-tspi deps get added in mjg59@61ce4d8 and updated (?) in mjg59@73ab334. They should be added in only one commit. |
|
Whoops, yes, my mistake. Fixed. |
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
| // +build tpm |
There was a problem hiding this comment.
Should this be specified in a build system as a tag?
There was a problem hiding this comment.
Yeah. I don't want to do it by default yet because the build root doesn't have the necessary C libraries.
There was a problem hiding this comment.
(Do we document build tags anywhere?)
There was a problem hiding this comment.
I suppose that the libs detection could be added in Miscellaneous stuff section in configure.ac. If the required libraries exist, set some variable to -tags tpm and AC_SUBST it in SUBSTITUTIONS section in configure.ac. Then add the substituted value to RKT_TAGS in variables.mk.in.
And no, we do not document build tags. Should we?
There was a problem hiding this comment.
I'll add code to configure.ac
|
Some nitpicks, otherwise looks fine and dandy. Also, just something I spotted - the email you used for commits is not associated to your github account. Not that this is important, just to let you know. |
| ) | ||
|
|
||
| // Extend extends the TPM log with the provided string. Returns any error. | ||
| func Extend(desription string) error { |
There was a problem hiding this comment.
Didn't notice it before - there is a missing c in parameter name - should be description. We don't build it for now, so it wasn't caught by CI before.
There was a problem hiding this comment.
Is there anything preventing us from adding the deps in semaphore and
building this there?
On Tue, Nov 24, 2015 at 1:43 PM, Krzesimir Nowak notifications@github.com
wrote:
In pkg/tpm/tpm.go
#1775 (comment):+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+// +build tpm
+
+package tpm
+
+import (
- "github.com/coreos/rkt/Godeps/_workspace/src/github.com/coreos/go-tspi/tpmclient"
+)
+// Extend extends the TPM log with the provided string. Returns any error.
+func Extend(desription string) error {Didn't notice it before - there is a missing c in parameter name -
should be des_c_ription. We don't build it for now, so it wasn't caught
by CI before.—
Reply to this email directly or view it on GitHub
https://github.com/coreos/rkt/pull/1775/files#r45799081.
There was a problem hiding this comment.
No idea, haven't checked what deps are required and whether they exist in ubuntu.
There was a problem hiding this comment.
Ouch! Damn, I must have messed up setting the flag when testing the build (I'd just moved the code with no functional changes). @jonboulle The dependencies are in Ubuntu - we just need libtspi-dev installed.
| @@ -18,7 +18,7 @@ REPO_PATH := $(ORG_PATH)/rkt | |||
| # [STAGE1] build settings | |||
|
|
|||
| # selinux tags for rkt and functional tests | |||
| RKT_TAGS := -tags selinux | |||
| RKT_TAGS := -tags selinux @TPM_TAGS@ | |||
There was a problem hiding this comment.
To specify multiple tags, you must put them inside quotes. So -tags 'selinux @TPM_TAGS@' should ok.
There was a problem hiding this comment.
Bother. I have this locally but managed to miss committing it.
| @@ -452,6 +457,8 @@ func Run(cfg RunConfig, dir string, dataDir string) { | |||
| log.Fatalf("error clearing FD_CLOEXEC on lock fd") | |||
| } | |||
|
|
|||
| tpmEvent := fmt.Sprintf("rkt: Rootfs: %s Manifest: %s Stage 1 args: %s", cfg.CommonConfig.RootHash, cfg.CommonConfig.ManifestData, strings.Join(args, " ")) | |||
| tpm.Extend(tpmEvent) | |||
There was a problem hiding this comment.
Error handling? What's the desired/expected behaviour if rkt is compiled with tpm but a) the system doesn't actually have it, or b) the operation otherwise fails?
There was a problem hiding this comment.
Right now we should just continue anyway. I've added a comment to that effect. Longer term we'll want the ability to enforce TPM use and so failures here should be fatal, but that's going to involve a more global security policy.
| // reason, ignore it and continue anyway. Long term we'll want policy | ||
| // that enforces TPM behaviour, but we don't have any infrastructure | ||
| // around that yet. | ||
| tpm.Extend(tpmEvent) |
There was a problem hiding this comment.
sorry, more things:
a) to explicitly flag the error is being ignored, please _ = tpm.Extend(tpmEvent)
b) after digging into the actual details - I'm a bit scared of this being in the critical path when it involves an HTTP request to an endpoint that in most cases is going to be unavailable (or running who knows what). Any ideas to mitigate that?
There was a problem hiding this comment.
Hm. This is going to be to localhost, so negative outcomes here are either going to be connection refused or something unexpected running on the port. What's the case you're concerned with?
|
@mjg59 Looks like it needs a rebase. edit: and while you're at it could you please amend the commit headers to follow our usual |
|
@mjg59 Filed issues for those things. One final question: is there any expectation that the string logged in the TPM needs to be machine-consumable? (what I'm really asking is: are we committing in any way to the particular format of the log message?). If so, I'd like. to split it out/formalise it more. If not, and 👍 |
|
We may want it to be machine readable in future, but right now it's not intended to be. I don't think there's a problem in changing that later. |
We want to be able to record the hash of the container image that we started, so return the calculated hash rather than just discarding it.
|
gofmt is failing, please fix that + commit messages and then we can ship it. |
We don't call Check() when we render an image for the first time, so return the filesystem hash from RenderTreeStore() as well.
We want to be able to share data between Prepare() and Run(), and the easiest way to do that is to pass CommonConfig by reference rather than by value.
We can store certain pieces of container configuration in the TPM event log for later audit purposes. For now let's stash the hash of the root FS, the manifest and the arguments passed to stage 1.
Autodetect the presence of the libtspi headers and build TPM support if they're present.
stage0: add container hash data into TPM
Hash container data into the TPM if tpmd is running