Support passing in root keys on repo initialization #801
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -81,6 +81,97 @@ func setupServer() *httptest.Server { | |
| return httptest.NewServer(setupServerHandler(storage.NewMemStorage())) | ||
| } | ||
|
|
||
| // Initializes a repo with existing key | ||
| func TestInitWithRootKey(t *testing.T) { | ||
| // -- setup -- | ||
| setUp(t) | ||
|
|
||
| tempDir := tempDirWithConfig(t, "{}") | ||
| defer os.RemoveAll(tempDir) | ||
|
|
||
| server := setupServer() | ||
| defer server.Close() | ||
|
|
||
| tempFile, err := ioutil.TempFile("", "targetfile") | ||
| require.NoError(t, err) | ||
| tempFile.Close() | ||
| defer os.Remove(tempFile.Name()) | ||
|
|
||
| // -- tests -- | ||
|
|
||
| // create encrypted root key | ||
| privKey, err := trustmanager.GenerateECDSAKey(rand.Reader) | ||
| require.NoError(t, err) | ||
| encryptedPEMPrivKey, err := trustmanager.EncryptPrivateKey(privKey, data.CanonicalRootRole, testPassphrase) | ||
| require.NoError(t, err) | ||
| encryptedPEMKeyFilename := filepath.Join(tempDir, "encrypted_key.key") | ||
| err = ioutil.WriteFile(encryptedPEMKeyFilename, encryptedPEMPrivKey, 0644) | ||
| require.NoError(t, err) | ||
|
|
||
| // init repo | ||
| _, err = runCommand(t, tempDir, "-s", server.URL, "init", "gun", "--rootkey", encryptedPEMKeyFilename) | ||
| require.NoError(t, err) | ||
|
|
||
| // publish repo | ||
| _, err = runCommand(t, tempDir, "-s", server.URL, "publish", "gun") | ||
| require.NoError(t, err) | ||
|
There was a problem hiding this comment. It would be awesome to add a check to this test that this initialized repo used the specified key from the flag (especially if we had more rootkeys in the Unfortunately I can't think of a way to verify which root key was used without manually unmarshalling metadata (unless we wanted to bring in a There was a problem hiding this comment. Apologies for the trouble - would it also be possible to add some or all of the following negative test cases? Similar test cases may already exist for importing a root key.
There was a problem hiding this comment. No problem; we already have the key id here so we can just There was a problem hiding this comment. Sorry @cyli GitHub caching caused me to miss your comment. The integration tests seem to only test the "happy path" - not saying that's all they should test, just noting that it's in keeping with the current style. IIRC the cases you listed are tested at the api levels, because the addition in this PR uses all the same code paths for key loading. I will double check to make sure. There was a problem hiding this comment. @ecordell You are definitely right in that the utility functions you use for loading the key should all have be tested already. I agree with you that it's pointless to re-test this functionality here; I mainly wanted to test that particular loading checks (like ensuring that we only allow importing encrypted keys - we did not have to include this check, after all) are called and that their errors are propagated (for instance that we don't just fall back on generating a random root key if importing a key fails), and wanted some way to trigger said failures. For instance, for case 3, it's to test that the root key is required to be encrypted - you could for instance completely skip that check and allow importing an unencrypted root key (which may or may not be a valid choice). Also, for case 4, cryptoservice's There was a problem hiding this comment. Thanks @cyli. I updated with integration tests for the cases you enumerated - deciding whether or not to change the behavior in those cases is, as you point out, up for discussion, but at least we can track those changes now with the tests. |
||
|
|
||
| // check that the root key used for init is the one listed as root key | ||
| output, err := runCommand(t, tempDir, "key", "list") | ||
| require.NoError(t, err) | ||
| require.True(t, strings.Contains(output, data.PublicKeyFromPrivate(privKey).ID())) | ||
|
|
||
| // check error if file doesn't exist | ||
| _, err = runCommand(t, tempDir, "-s", server.URL, "init", "gun2", "--rootkey", "bad_file") | ||
| require.Error(t, err, "Init with nonexistent key file should error") | ||
|
|
||
| // check error if file is invalid format | ||
| badKeyFilename := filepath.Join(tempDir, "bad_key.key") | ||
| nonPEMKey := []byte("thisisnotapemkey") | ||
| err = ioutil.WriteFile(badKeyFilename, nonPEMKey, 0644) | ||
| require.NoError(t, err) | ||
|
|
||
| _, err = runCommand(t, tempDir, "-s", server.URL, "init", "gun2", "--rootkey", badKeyFilename) | ||
| require.Error(t, err, "Init with non-PEM key should error") | ||
|
|
||
| // check error if unencrypted PEM used | ||
| unencryptedPrivKey, err := trustmanager.GenerateECDSAKey(rand.Reader) | ||
| require.NoError(t, err) | ||
| unencryptedPEMPrivKey, err := trustmanager.KeyToPEM(unencryptedPrivKey, data.CanonicalRootRole) | ||
| require.NoError(t, err) | ||
| unencryptedPEMKeyFilename := filepath.Join(tempDir, "unencrypted_key.key") | ||
| err = ioutil.WriteFile(unencryptedPEMKeyFilename, unencryptedPEMPrivKey, 0644) | ||
| require.NoError(t, err) | ||
|
|
||
| _, err = runCommand(t, tempDir, "-s", server.URL, "init", "gun2", "--rootkey", unencryptedPEMKeyFilename) | ||
| require.Error(t, err, "Init with unencrypted PEM key should error") | ||
|
|
||
| // check error if invalid password used | ||
| // instead of using a new retriever, we create a new key with a different pass | ||
| badPassPrivKey, err := trustmanager.GenerateECDSAKey(rand.Reader) | ||
| require.NoError(t, err) | ||
| badPassPEMPrivKey, err := trustmanager.EncryptPrivateKey(badPassPrivKey, data.CanonicalRootRole, "bad_pass") | ||
| require.NoError(t, err) | ||
| badPassPEMKeyFilename := filepath.Join(tempDir, "badpass_key.key") | ||
| err = ioutil.WriteFile(badPassPEMKeyFilename, badPassPEMPrivKey, 0644) | ||
| require.NoError(t, err) | ||
|
|
||
| _, err = runCommand(t, tempDir, "-s", server.URL, "init", "gun2", "--rootkey", badPassPEMKeyFilename) | ||
| require.Error(t, err, "Init with wrong password should error") | ||
|
|
||
| // check error if wrong role specified | ||
| snapshotPrivKey, err := trustmanager.GenerateECDSAKey(rand.Reader) | ||
| require.NoError(t, err) | ||
| snapshotPEMPrivKey, err := trustmanager.KeyToPEM(snapshotPrivKey, data.CanonicalSnapshotRole) | ||
| require.NoError(t, err) | ||
| snapshotPEMKeyFilename := filepath.Join(tempDir, "snapshot_key.key") | ||
| err = ioutil.WriteFile(snapshotPEMKeyFilename, snapshotPEMPrivKey, 0644) | ||
| require.NoError(t, err) | ||
|
|
||
| _, err = runCommand(t, tempDir, "-s", server.URL, "init", "gun2", "--rootkey", snapshotPEMKeyFilename) | ||
| require.Error(t, err, "Init with wrong role should error") | ||
| } | ||
|
|
||
| // Initializes a repo, adds a target, publishes the target, lists the target, | ||
| // verifies the target, and then removes the target. | ||
| func TestClientTUFInteraction(t *testing.T) { | ||
|
|
||
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.
In all likelihood
privKeyswill have the same length asrootKeyIDsso we should initialize appropriately:privKeys := make([]data.PrivateKey, 0, len(rootKeyIDs))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'm not so sure - I think you should be able to initialize with one private key and multiple public keys, so that these two don't match up.
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.
That's not implemented in this PR though. Although @dnwake's PR for
--rootcertmay make my comment moot.