builder: no delete logs OSLogin ssh key if not set #162
Conversation
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
In the SDK prior to version 0.3.2, the public key for the SSH communicator was only set if it was specified in the configuration. This changed however, and now when a private key file is specified in a packer template, the corresponding public key is set during the preparation steps. This changes the login for the instance creation, as with the update, the public key should only be uploaded to the instance metadata if it was generated, not if it was configured from a file. Therefore, this commit adds a predicate to the creation logic for the metadata of the instance so that it doesn't upload this key to the instance metadata at creation.
During the import_os_login_ssh_key step, we rely on both a state value and an extra condition to determine whether or not we should clean-up some keys from os-login. This is redundant, as the state key/value is only set when OSLogin is enabled in the configs, and some other conditions are true. Because of the redundancy, we can remove the extra check and only rely on the value being present in the state in order to continue clearing up the data from os-login.
As for the instance metadata, we relied on the sshPublicKey existing in order to determine whether or not to upload it to os-login. This is not true anymore in the case someone specifies a private key file in their template since v0.3.2 of the SDK, as the public key is derived from the private key specified in the template, so we cannot rely on this not to upload the key. So, in addition to the other checks, we check that the private key file was not specified in the configurations, and if it is, we skip the step.
lbajolet-hashicorp
force-pushed
the
check_ssh_pubkey_at_cleanup_oslogin
branch
from
c71d645 to
a3cd9a0
Compare
lbajolet-hashicorp
force-pushed
the
check_ssh_pubkey_at_cleanup_oslogin
branch
from
a3cd9a0 to
957770e
Compare
nywilken
reviewed
May 12, 2023
| // generateSSHPrivateKey generates a PEM encoded ssh private key file | ||
| // | ||
| // The file's deletion is the responsibility of the caller. | ||
| func generateSSHPrivateKey() (string, error) { |
There was a problem hiding this comment.
Nice test
I think we should also update this unit test so that public key is not set to nil. It should still pass
There was a problem hiding this comment.
Good call! I've updated it with a generated private key file, and it indeed still passes
nywilken
approved these changes
May 12, 2023
nywilken
approved these changes
May 12, 2023
The private key test for OS Login used to rely on a private key being defined, and not the public key. Since this is not a workflow that will happen in normal usage of the plugin, we update the test with a generated private key so we can use it for that test.
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
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.
Because of SDK updates to the
SSHKeyGenstep, we automatically set theSSHPublicKeyin the communicator when aSSHPrivateKeyFileis set in the template.This invalidates some conditions in the instance creation logic, and with the OSLogin key upload step, as both relied on the value of
SSHPublicKeyto make a decision.To prevent this, we only upload the SSH public key when no private key file is specified in the template, thereby making the plugin behave as it had prior to updating the SDK to newer versions that v0.3.2.
Closes #161