fix: user data with a blank line was sent unencoded, and only four public key types were recognised - #63
Open
tas50 wants to merge 2 commits into
Open
fix: user data with a blank line was sent unencoded, and only four public key types were recognised#63tas50 wants to merge 2 commits into
tas50 wants to merge 2 commits into
Conversation
The check for user data that is already base64 was anchored with ^ and $, which in Ruby anchor to a line rather than to the string, and every branch of the pattern was optional. A single blank line therefore satisfied the whole pattern, and cloud-config and shell scripts are full of blank lines, so the ordinary case was handed to CloudStack unencoded. Anchor with \A and \z, require at least one group so the empty string is not treated as pre-encoded, and encode strictly so the result is one line rather than wrapped at 60 characters inside an API query parameter. Signed-off-by: Tim Smith <tim@mondoo.com>
The warning for a .pem that is not a private key compared the file's first token against a list of four public key types. That list named ssh-dsa, which is not a key type at all, and left out ssh-dss, ecdsa-sha2-nistp384, ecdsa-sha2-nistp521 and the security key types, so those went through silently and turned into an unreadable authentication failure later. Ask whether the file is a PEM instead. Every private key format an SSH transport accepts opens with -----BEGIN, so anything else -- an unusual public key type, a PuTTY .ppk, an empty file -- is now reported. Signed-off-by: Tim Smith <tim@mondoo.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in 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.
Two bugs I hit while going over the driver, each with a regression test that fails on
main.User data containing a blank line was sent to CloudStack unencoded
ServerOptionstries to avoid double-encoding user data that someone alreadybase64-encoded themselves. The check was:
In Ruby
^and$anchor to a line, not to the string, and every branch ofthat pattern is optional — so the pattern matches an empty line. Any user data
with a blank line anywhere in it therefore looked "already base64" and went out
raw:
Blank lines between sections are completely ordinary in cloud-config and in
shell scripts, so this is the common case, not an edge one. The two existing
specs both happened to use user data with no blank line, which is why it
survived.
Anchored with
\A/\zand made at least one group mandatory so the emptystring is not treated as pre-encoded either.
While in there:
Base64.encode64wraps at 60 characters, so anything over 45bytes of user data — again, any real cloud-config — was going out with line
breaks embedded in an API query parameter. Switched to
strict_encode64, whichproduces a single line.
A short word made only of base64 characters (
packages) is genuinely ambiguousand is still passed through. There is no way to tell that apart from data
someone encoded, and I have noted it in the comment rather than guessing.
Only four public key types were recognised as "not a private key"
Credentialswarns when the.pemit found is a public key, which is an easymistake to make and otherwise surfaces as an unreadable authentication failure
much later. It did that by comparing the file's first token against:
ssh-dsais not a key type — the real one isssh-dss. And the list has noecdsa-sha2-nistp384, noecdsa-sha2-nistp521, and none of the security keytypes. Those all went through silently.
Turned the test around: every private key format an SSH transport accepts —
PKCS#1, PKCS#8, OpenSSH's own — is PEM and opens with
-----BEGIN, so askwhether the file is a PEM. An unusual public key type, a PuTTY
.ppkand anempty file are all now reported. It also stops reading the whole key file into
memory to look at one token.
Testing
The seven new examples all fail on
main.Conflicts
Deliberately kept clear of #62 — the
server_options.rbchanges here are theBASE64_PATTERNconstant and theuserdatamethod, neither of which thatbranch touches, and
credentials.rbis not in it at all.