From 77ec93ad0492f4162504951e1d0b2223a8beb450 Mon Sep 17 00:00:00 2001 From: Gus Luxton Date: Mon, 2 Oct 2023 20:02:06 -0300 Subject: [PATCH] [v14] puttyconfig: Switch to string-based Validity format and deprecate 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 * Address Zac's comments from code review --------- Co-authored-by: Zac Bergquist --- docs/pages/connect-your-client/putty.mdx | 16 ++ lib/puttyhosts/puttyhosts.go | 55 +++++- lib/puttyhosts/puttyhosts_test.go | 205 +++++++++++++++++++++++ tool/tsh/common/putty_config_windows.go | 64 +++++-- 4 files changed, 326 insertions(+), 14 deletions(-) diff --git a/docs/pages/connect-your-client/putty.mdx b/docs/pages/connect-your-client/putty.mdx index 0132d6d389ae7..f67ed99fdbe4e 100644 --- a/docs/pages/connect-your-client/putty.mdx +++ b/docs/pages/connect-your-client/putty.mdx @@ -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=` 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-...` + +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-` 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 ` || || ...` - 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 diff --git a/lib/puttyhosts/puttyhosts.go b/lib/puttyhosts/puttyhosts.go index a616e406892c0..8d4f198641db5 100644 --- a/lib/puttyhosts/puttyhosts.go +++ b/lib/puttyhosts/puttyhosts.go @@ -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, @@ -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 " || || ..." +// 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 +} diff --git a/lib/puttyhosts/puttyhosts_test.go b/lib/puttyhosts/puttyhosts_test.go index e4845818b2ac3..eaee3360403a4 100644 --- a/lib/puttyhosts/puttyhosts_test.go +++ b/lib/puttyhosts/puttyhosts_test.go @@ -264,6 +264,10 @@ func TestNaivelyValidateHostname(t *testing.T) { hostname: "hostname", shouldPass: true, }, + { + hostname: "test", + shouldPass: true, + }, { hostname: "testhost-withdashes.example.com", shouldPass: true, @@ -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) + }) + } +} diff --git a/tool/tsh/common/putty_config_windows.go b/tool/tsh/common/putty_config_windows.go index 91defaf543896..183846b200cd0 100644 --- a/tool/tsh/common/putty_config_windows.go +++ b/tool/tsh/common/putty_config_windows.go @@ -19,6 +19,7 @@ package common import ( "fmt" "net" + "strings" "syscall" "github.com/gravitational/trace" @@ -161,7 +162,8 @@ func addPuTTYSession(proxyHostname string, hostname string, port int, login stri return nil } -// addHostCAPublicKey adds a host CA to the registry with a set of space-separated hostnames +// addHostCAPublicKey adds a host CA to the registry with a set of hostnames delimited by " || " +// as per PuTTY's "Validity" syntax. func addHostCAPublicKey(registryHostCAStruct puttyhosts.HostCAPublicKeyForRegistry) error { registryKeyName := fmt.Sprintf(`%v\%v`, puttyRegistrySSHHostCAsKey, registryHostCAStruct.KeyName) @@ -171,7 +173,27 @@ func addHostCAPublicKey(registryHostCAStruct puttyhosts.HostCAPublicKeyForRegist return trace.Wrap(err) } defer registryKey.Close() - hostList, _, err := registryKey.GetStringsValue("MatchHosts") + + // get the "new" string-based Validity value if present. + validity, _, err := registryKey.GetStringValue("Validity") + if err != nil { + // ERROR_FILE_NOT_FOUND is an acceptable error, meaning that the value does not already + // exist and it must be created + if err != syscall.ERROR_FILE_NOT_FOUND { + log.Debugf("Can't get registry value %v: %T", registryKeyName, err) + return trace.Wrap(err) + } + } + + // split the Validity key out into a list of individual hostnames (hostList) + hostList, err := puttyhosts.CheckAndSplitValidityKey(validity, registryHostCAStruct.KeyName) + if err != nil { + return trace.Wrap(err) + } + + // get the "old" multistring-based MatchHosts value if present, so we can migrate it to the newer + // "Validity" format and then delete it. + matchHosts, _, err := registryKey.GetStringsValue("MatchHosts") if err != nil { // ERROR_FILE_NOT_FOUND is an acceptable error, meaning that the value does not already // exist and it must be created @@ -180,16 +202,24 @@ func addHostCAPublicKey(registryHostCAStruct puttyhosts.HostCAPublicKeyForRegist return trace.Wrap(err) } } - // initialize an empty hostlist if there isn't one stored under the registry key - if len(hostList) == 0 { - hostList = []string{} + // if matchHosts has any entries, we do a one-time migration of all the values from the "old" MatchHosts + // multistring to the new Validity string, + if len(matchHosts) > 0 { + log.Debugf("Found %v legacy MatchHosts value(s) in registry key %v, migrating to new Validity format", len(matchHosts), registryKeyName) + hostList = append(hostList, matchHosts...) } // add the new hostname to the existing hostList from the registry key (if one exists) hostList = puttyhosts.AddHostToHostList(hostList, registryHostCAStruct.Hostname) + // Reconstruct the "Validity" string using our hostList, separated by " || ". + hostListValidity := strings.Join(hostList, " || ") + // write strings to subkey - if err := registry.WriteMultiString(registryKey, "MatchHosts", hostList); err != nil { + // In beta versions of PuTTY 0.78 and the initial release of 'tsh puttyconfig', the list of valid hosts was + // represented by a REG_MULTI_SZ called "MatchHosts". Newer versions of PuTTY, WinSCP and 'tsh puttyconfig' use + // and prefer the string-formatted "Validity" instead. PuTTY will ignore "MatchHosts" when "Validity" is set. + if err := registry.WriteString(registryKey, "Validity", hostListValidity); err != nil { return trace.Wrap(err) } if err := registry.WriteString(registryKey, "PublicKey", registryHostCAStruct.PublicKey); err != nil { @@ -207,6 +237,16 @@ func addHostCAPublicKey(registryHostCAStruct puttyhosts.HostCAPublicKeyForRegist return trace.Wrap(err) } + // if matchHosts has any entries, delete the "MatchHosts" key from the registry as its entries were migrated above. + if len(matchHosts) > 0 { + log.Debugf("Deleting %v legacy MatchHosts value(s) from registry key %v", len(matchHosts), registryKeyName) + err := registryKey.DeleteValue("MatchHosts") + // failure to delete this value isn't a fatal error, so we should continue regardless + if err != nil { + log.Debugf("Failed to delete old MatchHosts value for %v: %v", registryHostCAStruct.KeyName, err) + } + } + return nil } @@ -217,10 +257,10 @@ func onPuttyConfig(cf *CLIConf) error { return trace.Wrap(err) } - // validate hostname against a naive regex to make sure it doesn't contain obviously illegal characters due - // to typos or similar. setting an "invalid" key in the registry makes it impossible to delete via the PuTTY - // UI and requires registry edits, so it's much better to error out early here. - hostname := tc.Config.Host + // remove any spaces from provided hostname and validate it against a naive regex to make sure it doesn't contain + // obviously illegal characters due to typos or similar. setting an "invalid" key in the registry makes it impossible + // to delete via the PuTTY UI and requires registry edits, so it's much better to error out early here. + hostname := strings.ReplaceAll(tc.Config.Host, " ", "") if !puttyhosts.NaivelyValidateHostname(hostname) { return trace.BadParameter("provided hostname %v does not look like a valid hostname. Make sure it doesn't contain illegal characters.", hostname) } @@ -229,7 +269,7 @@ func onPuttyConfig(cf *CLIConf) error { userHostString := hostname login := "" if tc.Config.HostLogin != "" { - login = tc.Config.HostLogin + login = strings.ReplaceAll(tc.Config.HostLogin, " ", "") userHostString = fmt.Sprintf("%v@%v", login, userHostString) } @@ -293,7 +333,7 @@ func onPuttyConfig(cf *CLIConf) error { } // add session to registry - if err := addPuTTYSession(proxyHost, tc.Config.Host, port, login, ppkFilePath, certificateFilePath, localCommandString, cf.LeafClusterName); err != nil { + if err := addPuTTYSession(proxyHost, hostname, port, login, ppkFilePath, certificateFilePath, localCommandString, cf.LeafClusterName); err != nil { log.Errorf("Failed to add PuTTY session for %v: %T\n", userHostString, err) return trace.Wrap(err) }