Merge pull request #22 from google/fix

Fixing fscrypt build system

15 files changed, 137 insertions, 91 deletions

diff --git a/Makefile b/Makefile
index 56b3646..5adec6e 100644
--- a/Makefile
+++ b/Makefile
@@ -17,66 +17,106 @@
 
 NAME = fscrypt
 
-CFLAGS += -O2 -Wall
-CMD_DIR = github.com/google/$(NAME)/cmd/$(NAME)
-
-# The code below lets the caller of the makefile change the build flags for
-# fscrypt in a familiar manner. For example, to force the program to statically
-# link its C components, run "make fscrypt" with:
-#	make fscrypt "LDFLAGS += -static -luuid -ldl -laudit -lpthread"
-#
-# Similarly, to modify the flags passed to the C components, just modify CFLAGS
-# or LDFLAGS as you would with a C program. To modify the Go flags, either
-# modify GO_FLAGS or GO_LINK_FLAGS (as appropriate).
+INSTALL = install
+DESTDIR = /usr/local/bin
+
+CMD_PKG = github.com/google/$(NAME)/cmd/$(NAME)
+
+SRC_FILES = $(shell find . -type f -name '*.go' -o -name "*.h" -o -name "*.c")
+GO_FILES = $(shell find . -type f -name '*.go' -not -path "./vendor/*")
+C_FILES = $(shell find . -type f -name "*.h" -o -name "*.c" -not -path "./vendor/*")
+GO_PKGS = $(shell go list ./... | grep -v /vendor/)
+
+# The flags code below lets the caller of the makefile change the build flags
+# for fscrypt in a familiar manner.
+#	CFLAGS
+#		Change the flags passed to the C compiler. Default = "-O2 -Wall"
+#		For example:
+#			make fscrypt "CFLAGS = -O3 -Werror"
+#		builds the C code with high optimizations, and C warnings fail.
+#	LDFLAGS
+#		Change the flags passed to the C linker. Empty by default.
+#		For example:
+#			make fscrypt "LDFLAGS = -static -luuid -ldl -laudit -lpthread"
+#		will build a static binary.
+#	GO_FLAGS
+#		Change the flags passed to "go build". Empty by default.
+#		For example:
+#			make fscrypt "GO_FLAGS = -race"
+#		will build the Go code with race detection.
+#	GO_LINK_FLAGS
+#		Change the flags passed to the Go linker. Default = "-s -w"
+#		For example:
+#			make fscrypt GO_LINK_FLAGS=""
+#		will not strip the binary.
 
 # Set the C flags so we don't need to set C flags in each CGO file.
+CFLAGS ?= -O2 -Wall
 export CGO_CFLAGS = $(CFLAGS)
 
-# Pass the version to the command line program (pulled from tags)
+# By default, we strip the binary to reduce size.
+GO_LINK_FLAGS ?= -s -w
+# Pass the version to the command line program (pulled from tags).
 VERSION_FLAG = -X "main.version=$(shell git describe --tags)"
-# Pass the current date and time to the command line program
+# Pass the current date and time to the command line program.
 DATE_FLAG = -X "main.buildTime=$(shell date)"
-# Pass the C linking flags into Go
-GO_LINK_FLAGS += -s -w $(VERSION_FLAG) $(DATE_FLAG) -extldflags "$(LDFLAGS)"
-GOFLAGS += --ldflags '$(GO_LINK_FLAGS)'
-
-.PHONY: default all $(NAME) go update lint format install clean
+# Add the version, date, and any specified LDFLAGS to any user-specified flags.
+override GO_LINK_FLAGS += $(VERSION_FLAG) $(DATE_FLAG) -extldflags "$(LDFLAGS)"
+# Add the link flags to any user-specified flags.
+override GO_FLAGS += --ldflags '$(GO_LINK_FLAGS)'
 
+.PHONY: default all
 default: $(NAME)
-all: update go format lint $(NAME)
+all: update go format lint default
 
-$(NAME):
-	go build $(GOFLAGS) -o $(NAME) $(CMD_DIR)
+$(NAME): $(SRC_FILES)
+	go build $(GO_FLAGS) -o $(NAME) $(CMD_PKG)
 
-# Makes sure go files build and tests pass
+.PHONY: clean
+clean:
+	rm -rf $(NAME)
+
+# Make sure go files build and tests pass.
+.PHONY: go
 go:
-	govendor generate +local
-	govendor build $(GOFLAGS) +local
-	govendor test $(GOFLAGS) -p 1 +local
+	@go generate $(GO_FLAGS) $(GO_PKGS)
+	@go build $(GO_FLAGS) $(GO_PKGS)
+	@go test -p 1 $(GO_FLAGS) $(GO_PKGS)
 
+# Update the vendored dependencies.
+.PHONY: update
 update:
+	@govendor init
 	@govendor fetch +missing
 	@govendor add +external
 	@govendor remove +unused
 
-lint:
-	@golint $$(go list ./... | grep -v vendor) | grep -v "pb.go" || true
-	@govendor vet +local
-
+.PHONY: format
 format:
-	@govendor fmt +local
-	@find . -name "*.h" -o -name "*.c" -not -path "./vendor/*" | xargs clang-format -i -style=Google
-
-install:
-	go install $(GOFLAGS) $(CMD_DIR)
-
-install_all:
-	govendor install $(GOFLAGS) +local
+	@gofmt -l -s -w $(GO_FILES)
+	@clang-format -i -style=Google $(C_FILES)
 
-TARBALL = $(NAME).$(shell date --iso-8601).tar.gz
-$(TARBALL):
-	git archive --format=tar.gz --output=$(TARBALL) HEAD
-tarball: $(TARBALL)
-
-clean:
-	rm -rf $(NAME) $(TARBALL)
+# Run lint rules (skipping generated files)
+.PHONY: lint
+lint:
+	@go vet $(GO_PKGS)
+	@golint $(GO_PKGS) | grep -v "pb.go" | ./input_fail.py
+	@megacheck -unused.exported $(GO_PKGS)
+
+# Check all files
+.PHONY: check
+check: all 
+	@govendor list +missing +external +unused \
+	| ./input_fail.py "Incorrect vendored dependencies. Run \"make update\""
+	@git diff
+	@git status -s \
+	| ./input_fail.py "Files have changed unexpectedly. Run \"make all\""
+
+.PHONY: install
+install: $(NAME)
+	$(INSTALL) -d $(DESTDIR)
+	$(INSTALL) $(NAME) $(DESTDIR)
+
+.PHONY: uninstall
+uninstall:
+	rm -rf $(DESTDIR)/$(NAME)
diff --git a/actions/hashing_test.go b/actions/hashing_test.go
index e3cffb6..d249d8b 100644
--- a/actions/hashing_test.go
+++ b/actions/hashing_test.go
@@ -30,8 +30,8 @@ import (
 // estimations are somewhat close to the targets.
 func TestCostsSearch(t *testing.T) {
 	for _, target := range []time.Duration{
-		20 * time.Millisecond,
 		100 * time.Millisecond,
+		200 * time.Millisecond,
 		500 * time.Millisecond,
 	} {
 		costs, err := getHashingCosts(target)
diff --git a/actions/policy.go b/actions/policy.go
index 1291e6b..946bdd4 100644
--- a/actions/policy.go
+++ b/actions/policy.go
@@ -61,7 +61,6 @@ func PurgeAllPolicies(ctx *Context) error {
 		switch errors.Cause(err) {
 		case nil, crypto.ErrKeyringSearch:
 			// We don't care if the key has already been removed
-			break
 		default:
 			return err
 		}
diff --git a/cmd/fscrypt/errors.go b/cmd/fscrypt/errors.go
index f11ff12..e0875e9 100644
--- a/cmd/fscrypt/errors.go
+++ b/cmd/fscrypt/errors.go
@@ -42,7 +42,6 @@ const failureExitCode = 1
 
 // Various errors used for the top level user interface
 var (
-	ErrReadingStdin       = util.SystemError("read from standard input failed")
 	ErrCanceled           = errors.New("operation canceled")
 	ErrNoDesctructiveOps  = errors.New("operation would be destructive")
 	ErrMaxPassphrase      = util.SystemError("max passphrase length exceeded")
diff --git a/cmd/fscrypt/flags.go b/cmd/fscrypt/flags.go
index 16412bf..d54a3bd 100644
--- a/cmd/fscrypt/flags.go
+++ b/cmd/fscrypt/flags.go
@@ -51,7 +51,7 @@ func (b *boolFlag) GetArgName() string { return "" }
 func (b *boolFlag) GetUsage() string   { return b.Usage }
 
 func (b *boolFlag) String() string {
-	if b.Default == false {
+	if !b.Default {
 		return longDisplay(b)
 	}
 	return longDisplay(b, strconv.FormatBool(b.Default))
diff --git a/cmd/fscrypt/keys.go b/cmd/fscrypt/keys.go
index 65360a9..872ca2a 100644
--- a/cmd/fscrypt/keys.go
+++ b/cmd/fscrypt/keys.go
@@ -87,7 +87,7 @@ func (p passphraseReader) Read(buf []byte) (int, error) {
 // the passphrase into the key normally.
 func getPassphraseKey(prompt string) (*crypto.Key, error) {
 	if !quietFlag.Value {
-		fmt.Printf(prompt)
+		fmt.Print(prompt)
 	}
 
 	// Only disable echo if stdin is actually a terminal.
diff --git a/cmd/fscrypt/prompt.go b/cmd/fscrypt/prompt.go
index 52f8c47..b882c08 100644
--- a/cmd/fscrypt/prompt.go
+++ b/cmd/fscrypt/prompt.go
@@ -282,7 +282,7 @@ func promptForProtector(options []*actions.ProtectorOption) (int, error) {
 	}
 
 	if numLoadErrors > 0 {
-		fmt.Printf(wrapText("NOTE: %d of the %d protectors failed to load. "+loadHelpText, 0))
+		fmt.Print(wrapText("NOTE: %d of the %d protectors failed to load. "+loadHelpText, 0))
 	}
 
 	for {
diff --git a/cmd/fscrypt/strings.go b/cmd/fscrypt/strings.go
index b77389f..fb79c38 100644
--- a/cmd/fscrypt/strings.go
+++ b/cmd/fscrypt/strings.go
@@ -49,8 +49,6 @@ const (
 	directoryArg    = "DIRECTORY"
 	mountpointArg   = "MOUNTPOINT"
 	pathArg         = "PATH"
-	archiveArg      = "ARCHIVE_FILE"
-	recoveryCodeArg = "RECOVERY_CODE"
 	mountpointIDArg = mountpointArg + ":ID"
 )
 
diff --git a/crypto/crypto_test.go b/crypto/crypto_test.go
index 5655fef..a154fbf 100644
--- a/crypto/crypto_test.go
+++ b/crypto/crypto_test.go
@@ -48,14 +48,15 @@ func makeKey(b byte, n int) (*Key, error) {
 	return NewFixedLengthKeyFromReader(ConstReader(b), n)
 }
 
-var fakeValidDescriptor = "0123456789abcdef"
-var fakeInvalidDescriptor = "123456789abcdef"
-var fakeSalt = bytes.Repeat([]byte{'a'}, metadata.SaltLen)
-var fakePassword = []byte("password")
-
-var fakeValidPolicyKey, _ = makeKey(42, metadata.PolicyKeyLen)
-var fakeInvalidPolicyKey, _ = makeKey(42, metadata.PolicyKeyLen-1)
-var fakeWrappingKey, _ = makeKey(17, metadata.InternalKeyLen)
+var (
+	fakeValidDescriptor = "0123456789abcdef"
+	fakeSalt            = bytes.Repeat([]byte{'a'}, metadata.SaltLen)
+	fakePassword        = []byte("password")
+
+	fakeValidPolicyKey, _   = makeKey(42, metadata.PolicyKeyLen)
+	fakeInvalidPolicyKey, _ = makeKey(42, metadata.PolicyKeyLen-1)
+	fakeWrappingKey, _      = makeKey(17, metadata.InternalKeyLen)
+)
 
 // As the passpharase hashing function clears the passphrase, we need to make
 // a new passphrase key for each test
diff --git a/filesystem/filesystem_test.go b/filesystem/filesystem_test.go
index 16bf82b..2a6baab 100644
--- a/filesystem/filesystem_test.go
+++ b/filesystem/filesystem_test.go
@@ -63,7 +63,7 @@ func getFakePolicy() *metadata.PolicyData {
 		KeyDescriptor: "0123456789abcdef",
 		Options:       metadata.DefaultOptions,
 		WrappedPolicyKeys: []*metadata.WrappedPolicyKey{
-			&metadata.WrappedPolicyKey{
+			{
 				ProtectorDescriptor: "fedcba9876543210",
 				WrappedKey:          wrappedPolicyKey,
 			},
diff --git a/filesystem/mountpoint_test.go b/filesystem/mountpoint_test.go
index 5d53dc1..73904a2 100644
--- a/filesystem/mountpoint_test.go
+++ b/filesystem/mountpoint_test.go
@@ -20,37 +20,15 @@
 package filesystem
 
 import (
-	"fmt"
 	"testing"
 )
 
-func printMountInfo() {
-	fmt.Println("\nBy Mountpoint:")
-	for _, mnt := range mountsByPath {
-		fmt.Println(mnt)
-	}
-
-	fmt.Println("\nBy Device:")
-	for device, mnts := range mountsByDevice {
-		fmt.Println("\t" + device)
-		for _, mnt := range mnts {
-			fmt.Println("\t\tPath: " + mnt.Path)
-		}
-	}
-}
-
 func TestLoadMountInfo(t *testing.T) {
 	if err := UpdateMountInfo(); err != nil {
 		t.Error(err)
 	}
 }
 
-func TestPrintMountInfo(t *testing.T) {
-	// Uncomment to see the mount info in the tests
-	// printMountInfo()
-	// t.Fail()
-}
-
 // Benchmarks how long it takes to update the mountpoint data
 func BenchmarkLoadFirst(b *testing.B) {
 	for n := 0; n < b.N; n++ {
diff --git a/input_fail.py b/input_fail.py
new file mode 100755
index 0000000..4b7e802
--- /dev/null
+++ b/input_fail.py
@@ -0,0 +1,13 @@
+#!/usr/bin/env python
+
+# Exit with 1 if any input is provided. Print the input to stdout, unless an
+# argument is specified. In that case, also print the argument.
+
+import sys
+
+input_string = sys.stdin.read()
+if input_string != "":
+    sys.stdout.write(input_string)
+    if len(sys.argv) >= 2:
+        print(sys.argv[1])
+    sys.exit(1)
diff --git a/pam/pam.c b/pam/pam.c
index e32770f..4769705 100644
--- a/pam/pam.c
+++ b/pam/pam.c
@@ -79,7 +79,8 @@ static int conversation(int num_msg, const struct pam_message** msg,
   return PAM_SUCCESS;
 }
 
-const struct pam_conv conv = {conversation, NULL};
+static const struct pam_conv conv = {conversation, NULL};
+const struct pam_conv* goConv = &conv;
 
 void freeData(pam_handle_t* pamh, void* data, int error_status) { free(data); }
 
@@ -106,4 +107,4 @@ void freeSecret(pam_handle_t* pamh, char* data, int error_status) {
   memset_sec(data, 0, size);
   munlock(data, size);
   free(data);
-}
\ No newline at end of file
+}
diff --git a/pam/pam.go b/pam/pam.go
index 010d4d2..e928883 100644
--- a/pam/pam.go
+++ b/pam/pam.go
@@ -65,22 +65,32 @@ func (h *Handle) getData(name string) (unsafe.Pointer, error) {
 	return data, h.err()
 }
 
+// ClearData remotes the PAM data with the specified name.
+func (h *Handle) ClearData(name string) error {
+	return h.setData(name, unsafe.Pointer(C.CString("")), C.CleanupFunc(C.freeData))
+}
+
+// SetSecret sets a copy of the C string secret into the PAM data with the
+// specified name. This copy will be held in locked memory until this PAM data
+// is cleared.
 func (h *Handle) SetSecret(name string, secret unsafe.Pointer) error {
 	return h.setData(name, C.copyIntoSecret(secret), C.CleanupFunc(C.freeSecret))
 }
 
+// GetSecret returns a pointer to the C string PAM data with the specified name.
+// This a pointer directory to the data, so it shouldn't be modified. It should
+// have been previously set with SetSecret().
 func (h *Handle) GetSecret(name string) (unsafe.Pointer, error) {
 	return h.getData(name)
 }
 
-func (h *Handle) ClearSecret(name string) error {
-	return h.setData(name, unsafe.Pointer(C.CString("")), C.CleanupFunc(C.freeData))
-}
-
+// SetString sets a string value for the PAM data with the specified name.
 func (h *Handle) SetString(name string, s string) error {
 	return h.setData(name, unsafe.Pointer(C.CString(s)), C.CleanupFunc(C.freeData))
 }
 
+// GetString gets a string value for the PAM data with the specified name. It
+// should have been previously set with SetString().
 func (h *Handle) GetString(name string) (string, error) {
 	data, err := h.getData(name)
 	if err != nil {
@@ -89,6 +99,7 @@ func (h *Handle) GetString(name string) (string, error) {
 	return C.GoString((*C.char)(data)), nil
 }
 
+// SetSlice sets a []string value for the PAM data with the specified name.
 func (h *Handle) SetSlice(name string, slice []string) error {
 	sliceLength := uintptr(len(slice))
 	memorySize := (sliceLength + 1) * unsafe.Sizeof(uintptr(0))
@@ -103,6 +114,8 @@ func (h *Handle) SetSlice(name string, slice []string) error {
 	return h.setData(name, data, C.CleanupFunc(C.freeArray))
 }
 
+// GetSlice gets a []string value for the PAM data with the specified name. It
+// should have been previously set with SetSlice().
 func (h *Handle) GetSlice(name string) ([]string, error) {
 	data, err := h.getData(name)
 	if err != nil {
@@ -166,7 +179,11 @@ func Start(service, username string) (*Transaction, error) {
 		handle: nil,
 		status: C.PAM_SUCCESS,
 	}
-	t.status = C.pam_start(cService, cUsername, &C.conv, &t.handle)
+	t.status = C.pam_start(
+		cService,
+		cUsername,
+		C.goConv,
+		&t.handle)
 	return t, (*Handle)(t).err()
 }
 
diff --git a/pam/pam.h b/pam/pam.h
index 9f3cdb2..09afb2e 100644
--- a/pam/pam.h
+++ b/pam/pam.h
@@ -23,7 +23,7 @@
 #include <security/pam_appl.h>
 
 // Conversation that will call back into Go code when appropriate.
-const struct pam_conv conv;
+const struct pam_conv *goConv;
 
 // CleaupFuncs are used to cleanup specific PAM data.
 typedef void (*CleanupFunc)(pam_handle_t *pamh, void *data, int error_status);