Skip to content

Add stricter SSH key parsing#889

Merged
OddBloke merged 3 commits into
canonical:masterfrom
TheRealFalcon:stricter-ssh
May 5, 2021
Merged

Add stricter SSH key parsing#889
OddBloke merged 3 commits into
canonical:masterfrom
TheRealFalcon:stricter-ssh

Conversation

@TheRealFalcon
Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon commented May 4, 2021 •
edited
Loading

Proposed Commit Message

Add \r\n check for SSH keys in Azure

See https://bugs.launchpad.net/cloud-init/+bug/1910835

Additional Context

Test Steps

test_lp1910835.py should no longer fail.

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

@TheRealFalcon
Add stricter SSH key parsing
6a84d7a 
Due to the possibility of CRLFs breaking SSH keys, be slightly stricter
in the parsing. We now split only on spaces rather than any whitespace,
ensure the key is base64 encoded, and ensure no more than 3 fields
are present in the key line.
@TheRealFalcon TheRealFalcon requested a review from OddBloke May 4, 2021 17:40
OddBloke
OddBloke reviewed May 4, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@OddBloke OddBloke left a comment

Choose a reason for hiding this comment

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

I've identified one issue in the parse_ssh_key modifications; inline. That makes me wonder more broadly, though, if we're asking for trouble in making this parser less permissive: it's not only used for Azure's use case. I wonder if we should introduce a separate callable, or a strict parameter or something that will allow us to do the checking we want for Azure, without risking regressing other behaviour?

Comment thread cloudinit/ssh_util.py Outdated
@TheRealFalcon TheRealFalcon force-pushed the stricter-ssh branch 2 times, most recently from a85f109 to ed3e2f9 Compare May 4, 2021 20:27
@TheRealFalcon TheRealFalcon requested a review from OddBloke May 4, 2021 20:44
@OddBloke
Copy link
Copy Markdown
Collaborator

OddBloke commented May 4, 2021

Yeah, I think this is the right approach: as we've discussed, doing any more stringent validation risks excluding keys that work today that don't meet our expectations.

I've also confirmed this fixed the integration test that was failing, so add some unit testing coverage and we're good to go, I think.

@TheRealFalcon
Copy link
Copy Markdown
Contributor Author

TheRealFalcon commented May 4, 2021

@OddBloke updated

OddBloke
OddBloke approved these changes May 5, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@OddBloke OddBloke left a comment

Choose a reason for hiding this comment

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

Thanks!

@OddBloke OddBloke merged commit f17f78f into canonical:master May 5, 2021
@TheRealFalcon TheRealFalcon deleted the stricter-ssh branch August 19, 2021 20:40
@ubuntu-server-builder ubuntu-server-builder mentioned this pull request May 12, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@OddBloke OddBloke OddBloke approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@TheRealFalcon @OddBloke