Skip to content

Add Public Key #59

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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Add Public Key #59

wants to merge 14 commits into from

Conversation

amao9098
Copy link
Collaborator

  • The setup should always be adding the public key even if the file exists - causes issues in external since the file already exists so new keys are not being added

@amao9098 amao9098 requested a review from MarSamMS October 24, 2024 21:42
@@ -5,7 +5,6 @@ Param(
[parameter(Mandatory=$true)] [string] $SrcIp,
[parameter(Mandatory=$true)] [string] $DestIp,
[parameter(Mandatory=$true)] [ValidateScript({Test-Path $_ -PathType Container})] [String] $OutDir = "",
[ValidateSet("Default", "Azure", "Detail", "Max", "Container")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to remove this line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this prevents adding new configs for one off tests.

Write-Host "`nTrusted admin keys already exist"
}
Write-Host "`nAdd the AuthorizedKey as a trusted admin key"
Add-Content -Force -Path "$env:ProgramData\ssh\administrators_authorized_keys" -Value "`n$authorizedKey"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if it will cause problems if we add the same key multiple times? I think after this change we may add the same key multiple times to the authorized keys file. Maybe it would be better to see if the key is present in the file before adding it.

Also, are you hitting this during manual or automated end-to-end test passes? Since we're recreating the containers for every test, you shouldn't hit this issue for the full test passes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s for the external setup. I can check if the passkey already exists for better code cleanup.

Write-Host "`nAdd the AuthorizedKey as a trusted admin key"
Add-Content -Force -Path "$env:ProgramData\ssh\administrators_authorized_keys" -Value "$authorizedKey"
Add-Content -Force -Path "$env:ProgramData\ssh\administrators_authorized_keys" -Value "`n$authorizedKey"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need the `n? I think Add-Content will add a new line by default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the `n is still here.

@@ -118,16 +118,11 @@ param(
Write-Host "`nSSHD default config files already exist"
}

if (-NOT (Test-Path "$env:ProgramData\ssh\administrators_authorized_keys"))
{
if (-NOT (Get-Content -Path "$env:ProgramData\ssh\administrators_authorized_keys" | ForEach-Object{$_ -match $authorizedKey})) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will work the way you want it to. If Get-Content returns more than one line, the result of the Get-Content | Foreach {} is going to be an array and an array is always considered "true" when interpreted as bool. So, this line will work if the authorized_keys file is empty or has one line, but will always skip adding the key if there is more than one key in the file, even if the key being checked isn't in there.

You could do something like this:
$isPresent = $false
PS C:\Users\marsam.REDMOND> Get-Content | foreach { $isPresent = $isPresent -OR $_ -match $authorizedKey }
if (-NOT $IsPresent)
{...}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modified the statement so that it just checks the string!

Copy link
Contributor

@MarSamMS MarSamMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve, but left one comment

Write-Host "`nAdd the AuthorizedKey as a trusted admin key"
Add-Content -Force -Path "$env:ProgramData\ssh\administrators_authorized_keys" -Value "$authorizedKey"
Add-Content -Force -Path "$env:ProgramData\ssh\administrators_authorized_keys" -Value "`n$authorizedKey"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the `n is still here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants