-
Notifications
You must be signed in to change notification settings - Fork 103
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 support for collecitng multiple user keys on via secure enclave #1644
Merged
James-Pickett
merged 15 commits into
kolide:main
from
James-Pickett:james/secure-enclave-key-init
Mar 8, 2024
Merged
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
bb7805d
add support for collecitng multiple user keys on via secure enclave
James-Pickett d41f914
update root dir for secure enclave signer test
James-Pickett b1cf533
updates secure enclave signer to fetch own data from store
James-Pickett 5adb031
Merge branch 'main' into james/secure-enclave-key-init
James-Pickett 78a0cd4
remove unneeded code
James-Pickett 4ee293d
remove unneeded mock
James-Pickett f1ebb6f
add tracing, create key in new
James-Pickett 6ba5fcb
Merge branch 'main' into james/secure-enclave-key-init
James-Pickett f730fb2
lint
James-Pickett 2a6e787
add bool for setting up hardware keys
James-Pickett 9a0a3e9
skip hardware key setup in tests
James-Pickett bc81209
more hw key set up skips
James-Pickett 5f22b6f
more hardware key skipping in tests
James-Pickett 721794d
use available contexts for logging
James-Pickett 022537e
dont use hardware keys for signing on darwin
James-Pickett File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
//go:build darwin | ||
// +build darwin | ||
|
||
package main | ||
|
||
import ( | ||
"errors" | ||
"fmt" | ||
"os" | ||
|
||
"github.com/kolide/krypto/pkg/echelper" | ||
"github.com/kolide/krypto/pkg/secureenclave" | ||
"github.com/kolide/launcher/ee/secureenclavesigner" | ||
) | ||
|
||
// runSecureEnclave performs either a create-key operation using the secure enclave. | ||
// It's available as a separate command because launcher runs as root by default and since it's | ||
// not in a user security context, it can't use the secure enclave directly. However, this command | ||
// can be run in the user context using launchctl. | ||
func runSecureEnclave(args []string) error { | ||
// currently we are just creating key, but plan to add sign command in future | ||
if len(args) < 1 { | ||
return errors.New("not enough arguments, expect create_key") | ||
} | ||
|
||
switch args[0] { | ||
case secureenclavesigner.CreateKeyCmd: | ||
return createSecureEnclaveKey() | ||
|
||
default: | ||
return fmt.Errorf("unknown command %s", args[0]) | ||
} | ||
} | ||
|
||
func createSecureEnclaveKey() error { | ||
secureEnclavePubKey, err := secureenclave.CreateKey() | ||
if err != nil { | ||
return fmt.Errorf("creating secure enclave key: %w", err) | ||
} | ||
|
||
secureEnclavePubDer, err := echelper.PublicEcdsaToB64Der(secureEnclavePubKey) | ||
if err != nil { | ||
return fmt.Errorf("marshalling public key to der: %w", err) | ||
} | ||
|
||
os.Stdout.Write(secureEnclavePubDer) | ||
return nil | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
//go:build !darwin | ||
// +build !darwin | ||
|
||
package main | ||
|
||
import "errors" | ||
|
||
func runSecureEnclave(args []string) error { | ||
return errors.New("not implemented on non darwin platforms") | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,148 @@ | ||
//go:build darwin | ||
// +build darwin | ||
|
||
package main | ||
|
||
import ( | ||
"bytes" | ||
"context" | ||
"fmt" | ||
"os" | ||
"os/exec" | ||
"path/filepath" | ||
"testing" | ||
"time" | ||
|
||
"github.com/kolide/krypto/pkg/echelper" | ||
"github.com/kolide/launcher/ee/secureenclavesigner" | ||
"github.com/stretchr/testify/require" | ||
) | ||
|
||
const ( | ||
testWrappedEnvVarKey = "SECURE_ENCLAVE_TEST_WRAPPED" | ||
macOsAppResourceDir = "../../ee/secureenclavesigner/test_app_resources" | ||
) | ||
|
||
// TestSecureEnclaveTestRunner creates a MacOS app with the binary of this packages tests, then signs the app with entitlements and runs the tests. | ||
// This is done because in order to access secure enclave to run tests, we need MacOS entitlements. | ||
// #nosec G306 -- Need readable files | ||
func TestSecureEnclaveTestRunner(t *testing.T) { | ||
directionless marked this conversation as resolved.
Show resolved
Hide resolved
|
||
t.Parallel() | ||
|
||
if os.Getenv("CI") != "" { | ||
t.Skipf("\nskipping because %s env var was not empty, this is being run in a CI environment without access to secure enclave", testWrappedEnvVarKey) | ||
} | ||
|
||
if os.Getenv(testWrappedEnvVarKey) != "" { | ||
t.Skipf("\nskipping because %s env var was not empty, this is the execution of the codesigned app with entitlements", testWrappedEnvVarKey) | ||
} | ||
|
||
t.Log("\nexecuting wrapped tests with codesigned app and entitlements") | ||
|
||
// set up app bundle | ||
rootDir := t.TempDir() | ||
appRoot := filepath.Join(rootDir, "launcher_test.app") | ||
|
||
// make required dirs launcher_test.app/Contents/MacOS and add files | ||
require.NoError(t, os.MkdirAll(filepath.Join(appRoot, "Contents", "MacOS"), 0700)) | ||
copyFile(t, filepath.Join(macOsAppResourceDir, "Info.plist"), filepath.Join(appRoot, "Contents", "Info.plist")) | ||
copyFile(t, filepath.Join(macOsAppResourceDir, "embedded.provisionprofile"), filepath.Join(appRoot, "Contents", "embedded.provisionprofile")) | ||
|
||
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
defer cancel() | ||
|
||
// build an executable containing the tests into the app bundle | ||
executablePath := filepath.Join(appRoot, "Contents", "MacOS", "launcher_test") | ||
out, err := exec.CommandContext( //nolint:forbidigo // Only used in test, don't want as standard allowedcmd | ||
ctx, | ||
"go", | ||
"test", | ||
"-c", | ||
"--cover", | ||
"--race", | ||
"./", | ||
"-o", | ||
executablePath, | ||
).CombinedOutput() | ||
|
||
require.NoError(t, ctx.Err()) | ||
require.NoError(t, err, string(out)) | ||
|
||
// sign app bundle | ||
signApp(t, appRoot) | ||
|
||
// run app bundle executable | ||
cmd := exec.CommandContext(ctx, executablePath, "-test.v") //nolint:forbidigo // Only used in test, don't want as standard allowedcmd | ||
cmd.Env = append(os.Environ(), fmt.Sprintf("%s=%s", testWrappedEnvVarKey, "true")) | ||
out, err = cmd.CombinedOutput() | ||
require.NoError(t, ctx.Err()) | ||
require.NoError(t, err, string(out)) | ||
|
||
// ensure the test ran successfully | ||
require.Contains(t, string(out), "PASS: TestSecureEnclaveCmd") | ||
require.NotContains(t, string(out), "FAIL") | ||
} | ||
|
||
func TestSecureEnclaveCmd(t *testing.T) { //nolint:paralleltest | ||
if os.Getenv(testWrappedEnvVarKey) == "" { | ||
t.Skipf("\nskipping because %s env var was empty, test not being run from codesigned app with entitlements", testWrappedEnvVarKey) | ||
} | ||
|
||
t.Log("\nrunning wrapped tests with codesigned app and entitlements") | ||
|
||
oldStdout := os.Stdout | ||
defer func() { | ||
os.Stdout = oldStdout | ||
}() | ||
|
||
// create a pipe to capture stdout | ||
pipeReader, pipeWriter, err := os.Pipe() | ||
require.NoError(t, err) | ||
|
||
os.Stdout = pipeWriter | ||
|
||
require.NoError(t, runSecureEnclave([]string{secureenclavesigner.CreateKeyCmd})) | ||
require.NoError(t, pipeWriter.Close()) | ||
|
||
var buf bytes.Buffer | ||
_, err = buf.ReadFrom(pipeReader) | ||
require.NoError(t, err) | ||
|
||
// convert response to public key | ||
createKeyResponse := buf.Bytes() | ||
secureEnclavePubKey, err := echelper.PublicB64DerToEcdsaKey(createKeyResponse) | ||
require.NoError(t, err) | ||
require.NotNil(t, secureEnclavePubKey, "should be able to get public key") | ||
} | ||
|
||
// #nosec G306 -- Need readable files | ||
func copyFile(t *testing.T, source, destination string) { | ||
bytes, err := os.ReadFile(source) | ||
require.NoError(t, err) | ||
require.NoError(t, os.WriteFile(destination, bytes, 0700)) | ||
} | ||
|
||
// #nosec G204 -- This triggers due to using env var in cmd, making exception for test | ||
func signApp(t *testing.T, appRootDir string) { | ||
codeSignId := os.Getenv("MACOS_CODESIGN_IDENTITY") | ||
require.NotEmpty(t, codeSignId, "need MACOS_CODESIGN_IDENTITY env var to sign app, such as [Mac Developer: Jane Doe (ABCD123456)]") | ||
|
||
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
defer cancel() | ||
|
||
cmd := exec.CommandContext( //nolint:forbidigo // Only used in test, don't want as standard allowedcmd | ||
ctx, | ||
"codesign", | ||
"--deep", | ||
"--force", | ||
"--options", "runtime", | ||
"--entitlements", filepath.Join(macOsAppResourceDir, "entitlements"), | ||
"--sign", codeSignId, | ||
"--timestamp", | ||
appRootDir, | ||
) | ||
|
||
out, err := cmd.CombinedOutput() | ||
require.NoError(t, ctx.Err()) | ||
require.NoError(t, err, string(out)) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot decide, but we could try to examine the parent process to see if launcher is in there. Though the parent is just going to be sudo, so this may not be feasible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Early on we considered trying to do this, but decided not to since it was easy to spoof ppid. If we're feeling different now and we do go this route, we could even exec codesign and verify that the calling binary is signed as we expect.