diff --git a/lib/aws/awsconfigfile/awsconfigfile.go b/lib/aws/awsconfigfile/awsconfigfile.go index feefdc410787d..1ee5637641bfe 100644 --- a/lib/aws/awsconfigfile/awsconfigfile.go +++ b/lib/aws/awsconfigfile/awsconfigfile.go @@ -17,7 +17,6 @@ package awsconfigfile import ( - "cmp" "os" "path/filepath" "strings" @@ -26,6 +25,10 @@ import ( ini "gopkg.in/ini.v1" ) +const ( + ownershipComment = "Do not edit. Section managed by Teleport." +) + // AWSConfigFilePath returns the path to the AWS configuration file. func AWSConfigFilePath() (string, error) { // See https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-envvars.html @@ -43,19 +46,19 @@ func AWSConfigFilePath() (string, error) { // SetDefaultProfileCredentialProcess sets the credential_process for the default profile. // File is created if it does not exist. -func SetDefaultProfileCredentialProcess(configFilePath, sectionComment, credentialProcess string) error { +func SetDefaultProfileCredentialProcess(configFilePath, credentialProcess string) error { const sectionName = "default" - return trace.Wrap(addCredentialProcessToSection(configFilePath, sectionName, sectionComment, credentialProcess)) + return trace.Wrap(addCredentialProcessToSection(configFilePath, sectionName, credentialProcess)) } // UpsertProfileCredentialProcess sets the credential_process for the profile with name profileName. // File is created if it does not exist. -func UpsertProfileCredentialProcess(configFilePath, profileName, sectionComment, credentialProcess string) error { +func UpsertProfileCredentialProcess(configFilePath, profileName, credentialProcess string) error { sectionName := "profile " + profileName - return trace.Wrap(addCredentialProcessToSection(configFilePath, sectionName, sectionComment, credentialProcess)) + return trace.Wrap(addCredentialProcessToSection(configFilePath, sectionName, credentialProcess)) } -func addCredentialProcessToSection(configFilePath, sectionName, sectionComment, credentialProcessCommand string) error { +func addCredentialProcessToSection(configFilePath, sectionName, credentialProcessCommand string) error { iniFile, err := ini.LoadSources(ini.LoadOptions{ AllowNestedValues: true, // Allow AWS-like nested values. Docs: http://docs.aws.amazon.com/cli/latest/topic/config-vars.html#nested-values Loose: true, // Allow non-existing files. ini.SaveTo will create the file if it does not exist. @@ -64,16 +67,24 @@ func addCredentialProcessToSection(configFilePath, sectionName, sectionComment, return trace.Wrap(err) } - if !iniFile.HasSection(sectionName) { - iniFile.NewSection(sectionName) - } + var section *ini.Section - section := iniFile.Section(sectionName) - if cmp.Or(section.Comment, sectionComment) != sectionComment { - return trace.BadParameter("%s: section %q is not managed by teleport, remove the section and try again", configFilePath, sectionName) + switch { + case iniFile.HasSection(sectionName): + section = iniFile.Section(sectionName) + + if !strings.Contains(section.Comment, ownershipComment) { + return trace.BadParameter("%s: section %q is not managed by Teleport, remove the section and try again", configFilePath, sectionName) + } + + default: + section, err = iniFile.NewSection(sectionName) + if err != nil { + return trace.Wrap(err) + } } - section.Comment = sectionComment + section.Comment = ownershipComment _, err = section.NewKey("credential_process", credentialProcessCommand) if err != nil { return trace.Wrap(err) @@ -91,33 +102,46 @@ func addCredentialProcessToSection(configFilePath, sectionName, sectionComment, return trace.Wrap(iniFile.SaveTo(configFilePath)) } -// RemoveCredentialProcessByComment removes the credential_process key on sections that have a specific section comment. -func RemoveCredentialProcessByComment(configFilePath, sectionComment string) error { - if !strings.HasPrefix(sectionComment, "; ") { - sectionComment = "; " + sectionComment - } +// RemoveTeleportManagedProfile removes the credential_process key on sections that have a specific section comment. +func RemoveTeleportManagedProfile(configFilePath, profile string) error { + sectionName := "profile " + profile - compareExactlyFn := func(comment string) bool { - return sectionComment == comment + iniFile, err := ini.LoadSources(ini.LoadOptions{ + AllowNestedValues: true, // Allow AWS-like nested values. Docs: http://docs.aws.amazon.com/cli/latest/topic/config-vars.html#nested-values + Loose: false, // If file does not exist, then there's nothing to be removed. + }, configFilePath) + if err != nil { + if os.IsNotExist(err) { + // Ignore non-existing file. + return nil + } + + return trace.Wrap(err) } - return removeCredentialProcess(configFilePath, compareExactlyFn) -} + configFileChanged := false + if iniFile.HasSection(sectionName) { + section := iniFile.Section(sectionName) -// RemoveCredentialProcessByCommentPrefix removes the credential_process key on sections that have a specific section comment prefix. -func RemoveCredentialProcessByCommentPrefix(configFilePath, sectionComment string) error { - if !strings.HasPrefix(sectionComment, "; ") { - sectionComment = "; " + sectionComment - } + if !strings.Contains(section.Comment, ownershipComment) { + return trace.BadParameter("%s: section %q is not managed by Teleport, remove the section manually and try again", configFilePath, sectionName) + } - comparePrefixFn := func(comment string) bool { - return strings.HasPrefix(comment, sectionComment) + if strings.Contains(section.Comment, ownershipComment) { + iniFile.DeleteSection(section.Name()) + configFileChanged = true + } + } + // No need to save the file if no sections were changed. + if !configFileChanged { + return nil } - return removeCredentialProcess(configFilePath, comparePrefixFn) + + return trace.Wrap(iniFile.SaveTo(configFilePath)) } -// RemoveCredentialProcessByComment removes the credential_process key on sections that have a specific section comment. -func removeCredentialProcess(configFilePath string, matchCommentFn func(string) bool) error { +// RemoveAllTeleportManagedProfiles removes all the profiles managed by Teleport. +func RemoveAllTeleportManagedProfiles(configFilePath string) error { iniFile, err := ini.LoadSources(ini.LoadOptions{ AllowNestedValues: true, // Allow AWS-like nested values. Docs: http://docs.aws.amazon.com/cli/latest/topic/config-vars.html#nested-values Loose: false, // If file does not exist, then there's nothing to be removed. @@ -133,21 +157,10 @@ func removeCredentialProcess(configFilePath string, matchCommentFn func(string) sectionChanged := false for _, section := range iniFile.Sections() { - if !matchCommentFn(section.Comment) { - continue - } - - if !section.HasKey("credential_process") { - continue - } - - section.DeleteKey("credential_process") - if len(section.Keys()) > 0 { - return trace.BadParameter("%s: section %q contains other keys, remove the section and try again", configFilePath, section.Name()) + if strings.Contains(section.Comment, ownershipComment) { + iniFile.DeleteSection(section.Name()) + sectionChanged = true } - iniFile.DeleteSection(section.Name()) - - sectionChanged = true } // No need to save the file if no sections were changed. diff --git a/lib/aws/awsconfigfile/awsconfigfile_test.go b/lib/aws/awsconfigfile/awsconfigfile_test.go index 38eed5e247577..3a2150a579f84 100644 --- a/lib/aws/awsconfigfile/awsconfigfile_test.go +++ b/lib/aws/awsconfigfile/awsconfigfile_test.go @@ -52,7 +52,6 @@ func TestAddCredentialProcessToSection(t *testing.T) { for _, tc := range []struct { name string sectionName string - sectionComment string existingContents *string errCheck require.ErrorAssertionFunc expected string @@ -60,10 +59,9 @@ func TestAddCredentialProcessToSection(t *testing.T) { { name: "adds section", sectionName: "profile my-aws-iam-ra-profile", - sectionComment: "This section is managed by Teleport. Do not edit.", existingContents: nil, // no config file errCheck: require.NoError, - expected: `; This section is managed by Teleport. Do not edit. + expected: `; Do not edit. Section managed by Teleport. [profile my-aws-iam-ra-profile] credential_process = credential_process `, @@ -71,10 +69,9 @@ credential_process = credential_process { name: "no config file", sectionName: "default", - sectionComment: "This section is managed by Teleport. Do not edit.", existingContents: nil, // no config file errCheck: require.NoError, - expected: `; This section is managed by Teleport. Do not edit. + expected: `; Do not edit. Section managed by Teleport. [default] credential_process = credential_process `, @@ -82,10 +79,9 @@ credential_process = credential_process { name: "empty config file", sectionName: "default", - sectionComment: "This section is managed by Teleport. Do not edit.", existingContents: strPtr(""), errCheck: require.NoError, - expected: `; This section is managed by Teleport. Do not edit. + expected: `; Do not edit. Section managed by Teleport. [default] credential_process = credential_process `, @@ -93,38 +89,37 @@ credential_process = credential_process { name: "no default profile", sectionName: "default", - sectionComment: "This section is managed by Teleport. Do not edit.", existingContents: strPtr("[profile foo]"), errCheck: require.NoError, expected: `[profile foo] -; This section is managed by Teleport. Do not edit. +; Do not edit. Section managed by Teleport. [default] credential_process = credential_process `, }, { - name: "replaces default credential process", - sectionName: "default", - sectionComment: "This section is managed by Teleport. Do not edit.", - existingContents: strPtr(`[default] + name: "replaces default credential process", + sectionName: "default", + existingContents: strPtr(`; Do not edit. Section managed by Teleport. +[default] credential_process = another process`), errCheck: require.NoError, - expected: `; This section is managed by Teleport. Do not edit. + expected: `; Do not edit. Section managed by Teleport. [default] credential_process = credential_process `, }, { - name: "comments are kept", - sectionName: "default", - sectionComment: "This section is managed by Teleport. Do not edit.", - existingContents: strPtr(`[default] + name: "comments are kept", + sectionName: "default", + existingContents: strPtr(`; Do not edit. Section managed by Teleport. +[default] ; this is a comment # yet another comment credential_process = another process`), errCheck: require.NoError, - expected: `; This section is managed by Teleport. Do not edit. + expected: `; Do not edit. Section managed by Teleport. [default] ; this is a comment # yet another comment @@ -132,9 +127,8 @@ credential_process = credential_process `, }, { - name: "error when default profile exists and has other fields", - sectionName: "default", - sectionComment: "This section is managed by Teleport. Do not edit.", + name: "error when default profile exists and has other fields", + sectionName: "default", existingContents: strPtr(`[default] credential_process = another process another_field = another_value`), @@ -147,15 +141,84 @@ another_field = another_value`), errCheck: require.Error, }, { - name: "error when profile does not have the expected comment", - sectionName: "default", - sectionComment: "This section is managed by Teleport. Do not edit.", + name: "error when profile does not have the expected comment", + sectionName: "default", existingContents: strPtr(`; Another Comment [default] credential_process = another process another_field = another_value`), errCheck: require.Error, }, + { + name: "re-apply the login, should not add another section", + sectionName: "profile Upper-and-lower-CASE", + existingContents: strPtr(`[sectionA] +some_setting = value + +; Do not edit. Section managed by Teleport. +[profile Upper-and-lower-CASE] +credential_process=credential_process +`), + errCheck: require.NoError, + expected: `[sectionA] +some_setting = value + +; Do not edit. Section managed by Teleport. +[profile Upper-and-lower-CASE] +credential_process = credential_process +`, + }, + { + name: "refuses to change the profile when a profile with the same name already exists but has no comment", + sectionName: "profile My-Profile", + existingContents: strPtr(`[sectionA] +some_setting = value + +[profile My-Profile] +credential_process=credential_process +`), + errCheck: require.Error, + }, + { + // This is not exactly a test but serves documentation purposes on the limitation of the library we use. + // It's not possible to keep the exact formatting of the existing file because it doesn't support it. + // Instead, it will reformat the file before saving it. + // The library supports turning off pretty printing but that would just reformat the entire file using no spaces, and no alignment, + // even if the original file had it. + name: "document reformatting behavior", + sectionName: "profile Upper-and-lower-CASE", + existingContents: strPtr(`[sectionA] +with_spaces = value +without_spaces=value`), + errCheck: require.NoError, + expected: `[sectionA] +with_spaces = value +without_spaces = value + +; Do not edit. Section managed by Teleport. +[profile Upper-and-lower-CASE] +credential_process = credential_process +`, + }, + { + name: "upserting an existing profile which used the previous version of the comment", + sectionName: "profile Upper-and-lower-CASE", + existingContents: strPtr(`[sectionA] +some_setting = value + +; Do not edit. Section managed by Teleport. Generated for accessing Upper-and-lower-CASE +[profile Upper-and-lower-CASE] +credential_process=credential_process +`), + errCheck: require.NoError, + expected: `[sectionA] +some_setting = value + +; Do not edit. Section managed by Teleport. +[profile Upper-and-lower-CASE] +credential_process = credential_process +`, + }, } { t.Run(tc.name, func(t *testing.T) { configFilePath := filepath.Join(t.TempDir(), "config") @@ -164,7 +227,7 @@ another_field = another_value`), require.NoError(t, err) } - err := addCredentialProcessToSection(configFilePath, tc.sectionName, tc.sectionComment, "credential_process") + err := addCredentialProcessToSection(configFilePath, tc.sectionName, "credential_process") tc.errCheck(t, err) if tc.expected != "" { @@ -178,14 +241,13 @@ another_field = another_value`), t.Run("creates directory if it does not exist", func(t *testing.T) { tmpDir := t.TempDir() configFilePath := filepath.Join(tmpDir, "dir", "config") - sectionComment := "This section is managed by Teleport. Do not edit. Profile for app: my-app" - err := SetDefaultProfileCredentialProcess(configFilePath, sectionComment, "credential_process") + err := SetDefaultProfileCredentialProcess(configFilePath, "credential_process") require.NoError(t, err) require.DirExists(t, filepath.Join(tmpDir, "dir")) bs, err := os.ReadFile(configFilePath) require.NoError(t, err) - require.Equal(t, `; This section is managed by Teleport. Do not edit. Profile for app: my-app + require.Equal(t, `; Do not edit. Section managed by Teleport. [default] credential_process = credential_process `, string(bs)) @@ -194,69 +256,91 @@ credential_process = credential_process t.Run("sets a named profile", func(t *testing.T) { tmpDir := t.TempDir() configFilePath := filepath.Join(tmpDir, "dir", "config") - sectionComment := "This section is managed by Teleport. Do not edit. Profile for app: my-app" - err := UpsertProfileCredentialProcess(configFilePath, "my-profile", sectionComment, "credential_process") + err := UpsertProfileCredentialProcess(configFilePath, "my-profile", "credential_process") require.NoError(t, err) require.DirExists(t, filepath.Join(tmpDir, "dir")) bs, err := os.ReadFile(configFilePath) require.NoError(t, err) - require.Equal(t, `; This section is managed by Teleport. Do not edit. Profile for app: my-app + require.Equal(t, `; Do not edit. Section managed by Teleport. [profile my-profile] credential_process = credential_process `, string(bs)) }) } -func TestRemoveCredentialProcessByComment(t *testing.T) { +func TestRemoveTeleportManagedProfile(t *testing.T) { for _, tc := range []struct { name string - commentSection string existingContents *string + profile string errCheck require.ErrorAssertionFunc expected string }{ { name: "no config file", - commentSection: "a comment", existingContents: nil, // no config file errCheck: require.NoError, expected: "", }, { name: "empty config file", - commentSection: "a comment", existingContents: strPtr(""), errCheck: require.NoError, expected: "", }, { name: "no section with expected comment", - commentSection: "a comment", existingContents: strPtr("; another comment\n[profile foo]\ncredential_process = process"), errCheck: require.NoError, expected: "; another comment\n[profile foo]\ncredential_process = process", }, { name: "matching comment but no profile using credential_process", - commentSection: "; a comment", existingContents: strPtr("; a comment\n[profile foo]\nanother_key = value"), errCheck: require.NoError, expected: "; a comment\n[profile foo]\nanother_key = value", }, { name: "removes the entire profile when the only key is the credential process", - commentSection: "a comment", existingContents: strPtr("; a comment\n[profile foo]\ncredential_process = process"), errCheck: require.NoError, expected: "", }, { - name: "an error is returned if more keys exist", - commentSection: "a comment", - existingContents: strPtr("; a comment\n[profile foo]\ncredential_process = process\nanother_key = value"), - errCheck: require.Error, - expected: "; a comment\n[profile foo]\ncredential_process = process\nanother_key = value", + name: "an error is returned if comment doesn't match", + profile: "foo", + existingContents: strPtr(`; a comment +[profile foo] +credential_process = process +`), + errCheck: require.Error, + expected: `; a comment +[profile foo] +credential_process = process +`, + }, + { + name: "no error even if it has more keys", + profile: "foo", + existingContents: strPtr(`; Do not edit. Section managed by Teleport. +[profile foo] +credential_process = process +another_key = value +`), + errCheck: require.NoError, + expected: ``, + }, + { + name: "no error even if it is using the previous comment version", + profile: "foo", + existingContents: strPtr(`; Do not edit. Section managed by Teleport. Generated for accessing MyApp +[profile foo] +credential_process = process +another_key = value +`), + errCheck: require.NoError, + expected: ``, }, } { t.Run(tc.name, func(t *testing.T) { @@ -265,7 +349,7 @@ func TestRemoveCredentialProcessByComment(t *testing.T) { err := os.WriteFile(configFilePath, []byte(*tc.existingContents), 0600) require.NoError(t, err) } - err := RemoveCredentialProcessByComment(configFilePath, tc.commentSection) + err := RemoveTeleportManagedProfile(configFilePath, tc.profile) tc.errCheck(t, err) if tc.expected != "" { @@ -277,17 +361,16 @@ func TestRemoveCredentialProcessByComment(t *testing.T) { } } -func TestRemoveCredentialProcessByCommentPrefix(t *testing.T) { +func TestRemoveAllTeleportManagedProfiles(t *testing.T) { for _, tc := range []struct { - name string - commentSectionPrefix string - existingContents *string - errCheck require.ErrorAssertionFunc - expected string + name string + profile string + existingContents *string + errCheck require.ErrorAssertionFunc + expected string }{ { - name: "multiple sections are removed", - commentSectionPrefix: "Do not remove. Generated by Teleport for app", + name: "multiple sections are removed", existingContents: strPtr(`; Do not remove. Generated by ACME Tool [profile foo1] credential_process = process @@ -295,11 +378,11 @@ credential_process = process [default] aws_region = us-east-1 -; Do not remove. Generated by Teleport for app XYZ +; Do not edit. Section managed by Teleport. [profile foo2] credential_process = process -; Do not remove. Generated by Teleport for app ABC +; Do not edit. Section managed by Teleport. [profile foo3] credential_process = process `), @@ -313,8 +396,7 @@ aws_region = us-east-1 `, }, { - name: "does not remove any section when comments are missing", - commentSectionPrefix: "Do not remove. Generated by Teleport for app", + name: "does not remove any section when comments are missing", existingContents: strPtr(`[profile foo1] credential_process = process @@ -342,7 +424,7 @@ credential_process = process err := os.WriteFile(configFilePath, []byte(*tc.existingContents), 0600) require.NoError(t, err) } - err := RemoveCredentialProcessByCommentPrefix(configFilePath, tc.commentSectionPrefix) + err := RemoveAllTeleportManagedProfiles(configFilePath) tc.errCheck(t, err) if tc.expected != "" { @@ -360,21 +442,19 @@ func TestUpdateRemoveCycle(t *testing.T) { err := os.WriteFile(configFilePath, []byte(initialContents), 0600) require.NoError(t, err) - sectionComment := "This section is managed by Teleport. Do not edit." - - err = UpsertProfileCredentialProcess(configFilePath, "my-profile", sectionComment, "my-process") + err = UpsertProfileCredentialProcess(configFilePath, "my-profile", "my-process") require.NoError(t, err) - err = UpsertProfileCredentialProcess(configFilePath, "my-profile2", sectionComment+" xyz", "my-process") + err = UpsertProfileCredentialProcess(configFilePath, "my-profile2", "my-process") require.NoError(t, err) - err = UpsertProfileCredentialProcess(configFilePath, "my-profile3", sectionComment+" xyz", "my-process") + err = UpsertProfileCredentialProcess(configFilePath, "my-profile3", "my-process") require.NoError(t, err) - err = RemoveCredentialProcessByComment(configFilePath, sectionComment) + err = RemoveTeleportManagedProfile(configFilePath, "my-profile") require.NoError(t, err) - err = RemoveCredentialProcessByCommentPrefix(configFilePath, sectionComment) + err = RemoveAllTeleportManagedProfiles(configFilePath) require.NoError(t, err) bs, err := os.ReadFile(configFilePath) diff --git a/tool/tsh/common/app_aws_test.go b/tool/tsh/common/app_aws_test.go index f825a58a9303f..db146e272df11 100644 --- a/tool/tsh/common/app_aws_test.go +++ b/tool/tsh/common/app_aws_test.go @@ -254,7 +254,7 @@ func TestAWSRolesAnywhereBasedAccess(t *testing.T) { awsConfigContents, err := os.ReadFile(awsConfigFile) require.NoError(t, err) - expectedProfileConfig := `; Do not edit. Section managed by Teleport. Generated for accessing aws-profile + expectedProfileConfig := `; Do not edit. Section managed by Teleport. [profile aws-profile] credential_process=tsh apps config --format aws-credential-process aws-profile ` diff --git a/tool/tsh/common/app_external_files.go b/tool/tsh/common/app_external_files.go index df3f81b851f39..8c3d7ded04a4e 100644 --- a/tool/tsh/common/app_external_files.go +++ b/tool/tsh/common/app_external_files.go @@ -61,12 +61,6 @@ func removeExternalFilesForAllApps() error { return nil } -// This value is used to identify the section in the AWS config file that is managed by Teleport. -// If it changes, the removal of the section during logout will not work correctly. -func awsConfigCommentForProfile(appName string) string { - return fmt.Sprintf("Do not edit. Section managed by Teleport. Generated for accessing %s", appName) -} - // addAWSProfileToConfig adds a new profile to the AWS config file (ie. ~/.aws/config). // This profile will invoke the `tsh apps config --format aws-credential-process ` command // which returns the credentials for accessing AWS. @@ -77,10 +71,9 @@ func addAWSProfileToConfig(appName string) error { } credentialProcessCommand := fmt.Sprintf("tsh apps config --format aws-credential-process %s", appName) - sectionComment := awsConfigCommentForProfile(appName) profileName := appName - if err := awsconfigfile.UpsertProfileCredentialProcess(awsConfigFileLocation, profileName, sectionComment, credentialProcessCommand); err != nil { + if err := awsconfigfile.UpsertProfileCredentialProcess(awsConfigFileLocation, profileName, credentialProcessCommand); err != nil { return trace.Wrap(err) } @@ -93,9 +86,7 @@ func removeAWSProfileFromConfig(appName string) error { return trace.Wrap(err) } - sectionComment := awsConfigCommentForProfile(appName) - - return trace.Wrap(awsconfigfile.RemoveCredentialProcessByComment(awsConfigFileLocation, sectionComment)) + return trace.Wrap(awsconfigfile.RemoveTeleportManagedProfile(awsConfigFileLocation, appName)) } func removeAllAWSProfilesFromConfig() error { @@ -104,7 +95,5 @@ func removeAllAWSProfilesFromConfig() error { return trace.Wrap(err) } - sectionComment := awsConfigCommentForProfile("") - - return trace.Wrap(awsconfigfile.RemoveCredentialProcessByComment(awsConfigFileLocation, sectionComment)) + return trace.Wrap(awsconfigfile.RemoveAllTeleportManagedProfiles(awsConfigFileLocation)) }