Skip to content

Commit

Permalink
[v14] puttyconfig: Switch to string-based Validity format and depreca…
Browse files Browse the repository at this point in the history 
…te MatchHosts (#32856)

* puttyconfig: Switch to string-based Validity format and deprecate MatchHosts

* Switch to more restrictive, reliable parsing

* Add validity string errors to docs

* Remove invalid test case

* Add test case

* Remove any spaces from user-provided input and use sanitized hostname

* Apply fixes from code review

* Tidy up errors, provide consistent detail about which field contains an error

* Disable docs lint for dots in heading

This is needed here, as there are 5 error messages which all start the same way but end differently.

* Catch a few more error cases

* Only delete old MatchHosts key after new Validity key has been written successfully

* Apply suggestions from code review

Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>

* Address Zac's comments from code review

---------

Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>
  • Loading branch information
@webvictim @zmb3
webvictim and zmb3 committed Oct 2, 2023
1 parent 59bc516 commit 77ec93a
Show file tree
Hide file tree
Showing 4 changed files with 326 additions and 14 deletions.
16 changes: 16 additions & 0 deletions docs/pages/connect-your-client/putty.mdx
View file
Expand Up @@ -239,6 +239,22 @@ itself to be able to start a session against the leaf cluster.
You do not have valid `tsh` credentials locally. Run `tsh login --proxy=<proxy hostname>` to log in first, or provide the `--proxy`
parameter when running `tsh puttyconfig`.

{/* lint ignore no-heading-punctuation remark-lint */}
### `ERROR: validity string for TeleportHostCA-<cluster_name>...`

Any errors related to the validity string are caused by invalid formatting in the PuTTY registry key which holds the list of hostnames
that should be trusted for a given host CA. Invalid formatting is generally caused by manual editing of host CA records in the
PuTTY GUI or Registry Editor - this should never be necessary and is strongly discouraged.

To fix this error, load PuTTY and go to SSH -> Host Keys -> Configure Host CAs at the left hand side. Select the matching
`TeleportHostCA-<cluster_name>` host CA record from the error and click **Load**. You can manually fix the entry under "Valid hosts this
key is trusted to certify" - the format must follow `<hostname> || <hostname> || ...` - or you can click the **Delete** button.

Note that deleting a host CA record will mean you then need to re-run `tsh puttyconfig` for every Teleport SSH service in that cluster
that you want to be able to access via PuTTY, even if the saved sessions still exist.

If this error appears during normal day-to-day operation, this is a bug and should be reported via [GitHub](https://github.com/gravitational/teleport/issues/new/choose).


## Uninstalling tsh

Expand Down
55 changes: 53 additions & 2 deletions lib/puttyhosts/puttyhosts.go
View file
Expand Up @@ -49,12 +49,12 @@ func hostnameContainsDot(hostname string) bool {
return strings.Contains(hostname, ".")
}

func hostnameisWildcard(hostname string) bool {
func hostnameIsWildcard(hostname string) bool {
return strings.HasPrefix(hostname, "*.")
}

func wildcardFromHostname(hostname string) string {
if hostnameisWildcard(hostname) {
if hostnameIsWildcard(hostname) {
return hostname
}
// prevent a panic below if the string doesn't contain a hostname. this should never happen,
Expand Down Expand Up @@ -238,3 +238,54 @@ func FormatHostCAPublicKeysForRegistry(hostCAPublicKeys map[string][]string, hos
}
return registryOutput
}

// CheckAndSplitValidityKey processes PuTTY's "Validity" string key into individual list elements
// and checks that its formatting follows the simple pattern "<hostname> || <hostname> || ..."
// PuTTY uses a custom string format to represent what hostnames a given key should be trusted for.
// See https://the.earth.li/~sgtatham/putty/0.79/htmldoc/Chapter4.html#config-ssh-cert-valid-expr for details.
func CheckAndSplitValidityKey(input string, caName string) ([]string, error) {
var output []string
docsURL := "https://goteleport.com/docs/connect-your-client/putty/#troubleshooting"

// if the input string has no content (because the Validity key has no value yet), return the empty list
if len(input) == 0 {
return output, nil
}

// split the input string on spaces
splitTokens := strings.Fields(input)

// if the total number of hostnames and tokens doesn't equal an odd number, we can return an error early as the string is invalid
if len(splitTokens)%2 != 1 {
return nil, trace.BadParameter("validity string for %v contains an even [%d] number of entries but should be odd, see %v",
caName, len(splitTokens), docsURL)
}
for index, token := range splitTokens {
// check that every odd value in the string (zero-indexed) is equal to the OR splitter "||" and return an error if not
if index%2 == 1 {
if token != "||" {
return nil, trace.BadParameter("validity string for %v contains invalid splitter token %q in field %d, see %v",
caName, token, index, docsURL)
}
} else {
// if the || delimiter is in a non-odd position, return an error
if token == "||" {
return nil, trace.BadParameter("validity string for %v contains consecutive splitter tokens with no hostname in field %d, see %v",
caName, index, docsURL)
}
// if the string contains any value which is not part of a hostname, return an error
if badIndex := strings.IndexAny(token, "()&!:|"); badIndex != -1 {
return nil, trace.BadParameter("validity string for %v contains an invalid character %q in field %v, see %v",
caName, token[badIndex], index, docsURL)
}
// check the token using the naive hostname regex and return an error if it doesn't match
if !hostnameIsWildcard(token) && !NaivelyValidateHostname(token) {
return nil, trace.BadParameter("validity string for %v appears to contain non-hostname %q in field %v, see %v",
caName, token, index, docsURL)
}
output = append(output, token)
}
}

return output, nil
}
205 changes: 205 additions & 0 deletions lib/puttyhosts/puttyhosts_test.go
View file
Expand Up @@ -264,6 +264,10 @@ func TestNaivelyValidateHostname(t *testing.T) {
hostname: "hostname",
shouldPass: true,
},
{
hostname: "test",
shouldPass: true,
},
{
hostname: "testhost-withdashes.example.com",
shouldPass: true,
Expand Down Expand Up @@ -305,3 +309,204 @@ func TestNaivelyValidateHostname(t *testing.T) {
})
}
}

func TestCheckAndSplitValidityKey(t *testing.T) {
t.Parallel()

var tests = []struct {
name string
input string
desiredOutput []string
checkErr require.ErrorAssertionFunc
}{
{
name: "Should pass with an empty input string",
input: "",
desiredOutput: []string(nil),
checkErr: require.NoError,
},
{
name: "Should pass with two wildcards",
input: "*.foo.example.com || *.bar.example.com",
desiredOutput: []string{
"*.foo.example.com",
"*.bar.example.com",
},
checkErr: require.NoError,
},
{
name: "Should pass with a single string and no delimiter",
input: "test",
desiredOutput: []string{"test"},
checkErr: require.NoError,
},
{
name: "Should pass with wildcard, single string and regular hostname",
input: "*.example.com || test || teleport.test.com",
desiredOutput: []string{
"*.example.com",
"test",
"teleport.test.com",
},
checkErr: require.NoError,
},
{
name: "Should pass with mixed usage",
input: "*.example.com || test || teleport.test.com || longstring || *.wow.com",
desiredOutput: []string{
"*.example.com",
"test",
"teleport.test.com",
"longstring",
"*.wow.com",
},
checkErr: require.NoError,
},
{
name: "Should pass with trailing space",
input: "*.example.com || lol.example.com || test.teleport.com ",
desiredOutput: []string{
"*.example.com",
"lol.example.com",
"test.teleport.com",
},
checkErr: require.NoError,
},
{
name: "Should pass with preceding space",
input: " *.example.com || lol.example.com",
desiredOutput: []string{
"*.example.com",
"lol.example.com",
},
checkErr: require.NoError,
},
{
name: "Should pass with random spacing",
input: " *.example.com || lol.example.com",
desiredOutput: []string{
"*.example.com",
"lol.example.com",
},
checkErr: require.NoError,
},
{
name: "Should pass with extra space in the middle",
input: " *.example.com || lol.example.com || test.teleport.com",
desiredOutput: []string{
"*.example.com",
"lol.example.com",
"test.teleport.com",
},
checkErr: require.NoError,
},
{
name: "Should error if colons are used",
input: "*.example.com && port:22",
checkErr: require.Error,
},
{
name: "Should fail if negation is used",
input: "*.example.com && ! *.extrasecure.example.com",
checkErr: require.Error,
},
{
name: "Should fail if parentheses are used",
input: "(*.foo.example.com || *.bar.example.com)",
checkErr: require.Error,
},
{
name: "Should fail if parentheses and port are used",
input: "(*.foo.example.com || *.bar.example.com) && port:0-1023",
checkErr: require.Error,
},
{
name: "Should fail with multiple parentheses and port",
input: "(*.foo.example.com || *.bar.example.com || *.qux.example.com) && port:0-1023",
checkErr: require.Error,
},
{
name: "Should fail with multiple parentheses, port and trailing hostname",
input: "((*.foo.example.com || *.bar.example.com || *.qux.example.com) && port:0-1023) || teleport.example.com",
checkErr: require.Error,
},
{
name: "Should fail with multiple parentheses, port and two trailing hostnames",
input: "((*.example.com || lol.example.com && port:22) && port:1024) || test.teleport.com || teleport.test.com",
checkErr: require.Error,
},
{
name: "Should fail if single pipe delimiter is used",
input: "*.example.com || lol.example.com | test.teleport.com",
checkErr: require.Error,
},
{
name: "Should fail if single ampersand delimiter is used",
input: "*.example.com & lol.example.com || test.teleport.com",
checkErr: require.Error,
},
{
name: "Should fail if boolean AND is used",
input: "*.example.com && lol.example.com || test.teleport.com",
checkErr: require.Error,
},
{
name: "Should fail with misplaced pipe delimiter",
input: "*.example.com || lol.example.com || | test.teleport.com",
checkErr: require.Error,
},
{
name: "Should fail with empty final field",
input: "*.example.com || lol.example.com || | test.teleport.com || ",
checkErr: require.Error,
},
{
name: "Should fail with double delimiter and empty field",
input: "*.example.com || lol.example.com || || test.teleport.com",
checkErr: require.Error,
},
{
name: "Should fail with triple delimiter and empty field",
input: "*.example.com || lol.example.com || || || test.teleport.com",
checkErr: require.Error,
},
{
name: "Should fail with pipe character in hostname",
input: "*.example.com || lol.example.com || test.|teleport.com",
checkErr: require.Error,
},
{
name: "Should fail with negation character around hostname",
input: "*.example.com || lol.example.com || !test.teleport.com",
checkErr: require.Error,
},
{
name: "Should fail with a non-hostname",
input: "*.example.com || lol.example.com || test.teleport.com || \"\"",
checkErr: require.Error,
},
{
name: "Should fail with a single trailing dot",
input: "*.example.com || lol.example.com || .",
checkErr: require.Error,
},
{
name: "Should error with single wildcard symbol",
input: "*",
checkErr: require.Error,
},
{
name: "Should error if multiple single wildcard symbols are present",
input: "* || *",
checkErr: require.Error,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
testResult, err := CheckAndSplitValidityKey(tt.input, "TeleportHostCA-testcluster.example.com")
tt.checkErr(t, err)
require.Equal(t, tt.desiredOutput, testResult)
})
}
}

0 comments on commit 77ec93a

Please sign in to comment.