Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ParseInt Error with role duration seconds key #1192

Closed
3 tasks done
Zeouterlimits opened this issue Mar 24, 2021 · 27 comments · Fixed by #1568
Closed
3 tasks done

ParseInt Error with role duration seconds key #1192

Zeouterlimits opened this issue Mar 24, 2021 · 27 comments · Fixed by #1568
Labels
bug This issue is a bug. needs-reproduction This issue needs reproduction.

Comments

@Zeouterlimits
Copy link

Zeouterlimits commented Mar 24, 2021

Confirm by changing [ ] to [x] below to ensure that it's a bug:

Describe the bug
Unable to use configured roles due to parsing error of duration_seconds

When instiating an aws client (s3 in my case), the aws go v2 sdk is trying to parse my local roles duration_seconds (28800) and throwing a parse int error exception:
error merging role duration seconds key, strconv.ParseInt: parsing "炀": invalid syntax

Checking out the SDK locally and running the tests reproduces this issue:

> cd config
> go build .
> go test .
2021/03/25 11:01:53 http: TLS handshake error from 127.0.0.1:49463: remote error: tls: bad certificate
--- FAIL: TestResolveLogger (0.00s)
    resolve_test.go:206: expect no error, got error merging role duration seconds key, strconv.ParseInt: parsing "炀": invalid syntax
FAIL
FAIL    github.com/aws/aws-sdk-go-v2/config     0.805s
FAIL
> go version
go version go1.15.2 darwin/amd64

Version of AWS SDK for Go?
aws-sdk-go-v2 1.3.0

Version of Go (go version)?
1.15.2
Update: Occurs on go1.15.10 too

To Reproduce (observed behavior)
Have a configured local aws profile with duration_seconds e.g:

[staging]
role_arn = arn:aws:iam::WHATEVER
mfa_serial = arn:aws:iam::WHATEVER
source_profile = aSourceProfile
duration_seconds = 28800

Run the sdk unit tests

Expected behavior
No error occurs, duration_seconds correctly applied to client config

Additional context

@Zeouterlimits Zeouterlimits added the bug This issue is a bug. label Mar 24, 2021
Zeouterlimits pushed a commit to Zeouterlimits/aws-sdk-go-v2 that referenced this issue Mar 25, 2021
@jasdel jasdel added documentation This is a problem with documentation. feature-request A feature should be added or improved. wontfix We have determined that we will not resolve the issue. and removed feature-request A feature should be added or improved. documentation This is a problem with documentation. labels Mar 29, 2021
@jasdel
Copy link
Contributor

jasdel commented Mar 29, 2021

Thanks for reporting this issue @Zeouterlimits I added comment in the PR, about compatibility of this change with other SDKs. I don't think the trailing s notation is supported by any of the other SDKs, as this field is documented as integer seconds.

Because of this we couldn't take this change, because it changes the behavior of this field, and would not be compatible with the other SDKs.

@jasdel jasdel added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Mar 29, 2021
@Zeouterlimits
Copy link
Author

Thanks for the feedback, @jasdel, but I am somewhat confused, my roles don't have a trailing 's', its duration seconds is
duration_seconds = 28800.

The result of running it through the SharedConfig loading path / LoadDefaultConfig is what creates a string that has a trailing s.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Mar 30, 2021
@jasdel
Copy link
Contributor

jasdel commented Mar 31, 2021

Thanks for clarifying that @Zeouterlimits I miss read the PR and reported issue. I've re-opened the PR. Could you help clarify where the is coming from in the error that you reported. I'm not seeing anything in the config file you linked that suggest it would have some value like that.

Also any additional background for using Raw would also be very helpful.

@Zeouterlimits
Copy link
Author

Zeouterlimits commented Apr 1, 2021

Ah no worries, ty for taking another look :)

28800 = decimal value of the html entity for this particular ideograph - 炀
https://graphemica.com/%E7%82%80

I used raw as that is what shared config is expecting / is retrieved in loading these shared local configs.
The existing path of treating it as an int immediately to be parsed is what leads to this encoding error.

@jasdel jasdel removed the wontfix We have determined that we will not resolve the issue. label Apr 7, 2021
@kichik
Copy link

kichik commented Jan 11, 2022

@jasdel this is still an issue. Any plans to merge the PR?
@Zeouterlimits have you found a different work around? I'm stuck on this now too.

@jasdel
Copy link
Contributor

jasdel commented Jan 21, 2022

Took a look at this PR and issue but have not been able to reproduce the issue. I created PR #1568 that adds a test case to the SDK for the issue outlined in this issue associated PR.

Is it possible that the shared config file being passed to the SDK is not UTF-8 encoded? The SDK requires the file to be encoded in this file. Please let us know if you're still able to reproduce this issue, and is so please share the shared config file that is able to reproduce the problem.

@jasdel jasdel added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jan 21, 2022
@kichik
Copy link

kichik commented Jan 21, 2022

Thanks @jasdel. I am still able to reproduce this. The file is technically UTF-8 as it has no special characters but has no BOM. It's just the file that AWS CLI created. If I add BOM, I get:

2022/01/21 10:45:18 failed to load shared config file, C:\Users\...\.aws\credentials, unsupported expression {expr {1 STRING 0 [65279]} true []}

@jasdel
Copy link
Contributor

jasdel commented Jan 21, 2022

Which version of the AWS CLI are you using to create the config file? Are you able to reproduce this with a newly created shared config file?

Does the SDK parse the file correctly if the BOM is not present?

Also I'll confirm if the shared config file is supposed to be limited to ASCII characters, or is supposed to support UTF-8. confirmed UTF-8 is supposed to be supported.

@jasdel jasdel added the needs-reproduction This issue needs reproduction. label Jan 21, 2022
@kichik
Copy link

kichik commented Jan 21, 2022

The SDK doesn't parse it correctly if BOM is not present and fails completely if BOM is present. In fact, I get the same error even with this simple file:

[default]
duration_seconds = 10800

@jasdel
Copy link
Contributor

jasdel commented Jan 21, 2022

Thanks for the update, could you link that file to the GitHub issue, or run the config file through a hex dump, and paste the output to the issue?

For example a utility like hexdump -C <config_file>, or the following Go program will provide a similar output.

package main

import (
	"encoding/hex"
	"fmt"
	"io"
	"log"
	"os"
)

// Usage: go run main.go <filename>
func main() {
	b, err := io.ReadAll(os.Args[1])
	if err != nil {
		log.Fatalf("failed to read file, %v", err)
	}

	fmt.Println(hex.Dump(b))
}

e.g. I would expect to see something like the following for a file without any extra encodings in it.

00000000  5b 64 65 66 61 75 6c 74  5d 0a 64 75 72 61 74 69  |[default].durati|
00000010  6f 6e 5f 73 65 63 6f 6e  64 73 20 3d 20 31 30 38  |on_seconds = 108|
00000020  30 30 0a                                          |00.|

But something in the file(s) you're using is triggering some unexpected behavior in the SDK's ini file handling.

@kichik
Copy link

kichik commented Jan 21, 2022

It's just a normal text file. I swear :)

$ hexdump -C .aws/credentials
00000000  5b 64 65 66 61 75 6c 74  5d 0d 0a 64 75 72 61 74  |[default]..durat|
00000010  69 6f 6e 5f 73 65 63 6f  6e 64 73 20 3d 20 31 30  |ion_seconds = 10|
00000020  38 30 30 0d 0a                                    |800..|
00000025

@jasdel
Copy link
Contributor

jasdel commented Jan 21, 2022

The only difference between the two dumps looks to be 0d 0a (\r\n) vs 0a (\n) in my sample. Are you using windows by chance? Its possible the ini parser doesn't correctly support \r\n newline encoding.

@kichik
Copy link

kichik commented Jan 21, 2022

I am using Windows. If this happens because Go's INI parser doesn't handle \r\n, that would be pretty funny. But even saving the file with just \n doesn't fix it.

@jasdel
Copy link
Contributor

jasdel commented Jan 21, 2022

Could you include a hex dump of the file saved with just \n encoding?

The SDK uses a custom ini file parser due to additional rules and behaviors the SDKs need to support. the config files are not 100% ini format, but very similar.

@kichik
Copy link

kichik commented Jan 21, 2022

$ hexdump -C .aws/credentials
00000000  5b 64 65 66 61 75 6c 74  5d 0a 64 75 72 61 74 69  |[default].durati|
00000010  6f 6e 5f 73 65 63 6f 6e  64 73 20 3d 20 31 30 38  |on_seconds = 108|
00000020  30 30 0a                                          |00.|
00000023

And this is the relevant part of my go.sum so we can be sure we're looking at the same code:

github.com/aws/aws-sdk-go-v2 v1.12.0 h1:z5bijqy+eXLK/QqF6eQcwCN2qw1k+m9OUDicqCZygu0=
github.com/aws/aws-sdk-go-v2 v1.12.0/go.mod h1:tWhQI5N5SiMawto3uMAQJU5OUN/1ivhDDHq7HTsJvZ0=
github.com/aws/aws-sdk-go-v2/config v1.12.0 h1:WOhIzj5HdixjlvQ4SLYAOk6OUUsuu88RwcsTzexa9cg=
github.com/aws/aws-sdk-go-v2/config v1.12.0/go.mod h1:GQONFVSDdG6RRho1C730SGNyDhS1kSTnxpOE76ptBqo=
github.com/aws/aws-sdk-go-v2/credentials v1.7.0 h1:KFuKwPs7i5SE5a0LxqAxz75qxSjr2HnHnhu0UPGlvpM=
github.com/aws/aws-sdk-go-v2/credentials v1.7.0/go.mod h1:Kmq64kahHJtXfmnEwnvRKeNjLBqkdP++Itln9BmQerE=
github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.9.0 h1:fPq3oloONbHaA0O8KX/KYUQk7pG9JjKBwYQvQsQDK84=
github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.9.0/go.mod h1:19SxQ+9zANyJCnNaoF3ovl8bFil4TaqCYEDdqNGKM+A=
github.com/aws/aws-sdk-go-v2/internal/configsources v1.1.3 h1:YPNiEXnuWdkpNOwBFHhcLwkSmewwQRcPFO9dHmxU0qg=
github.com/aws/aws-sdk-go-v2/internal/configsources v1.1.3/go.mod h1:L72JSFj9OwHwyukeuKFFyTj6uFWE4AjB0IQp97bd9Lc=
github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.1.0 h1:ArRd27pSm66f7cCBDPS77wvxiS4IRjFatpzVBD7Aojc=
github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.1.0/go.mod h1:KdVvdk4gb7iatuHZgIkIqvJlWHBtjCJLUtD/uO/FkWw=
github.com/aws/aws-sdk-go-v2/internal/ini v1.3.3 h1:fmGqMNlFTHr9Y48qmYYv2qIo+TAsST3qZa2d1HcwBeo=
github.com/aws/aws-sdk-go-v2/internal/ini v1.3.3/go.mod h1:N4dv+zawriMFZBO/6UKg3zt+XO6xWOQo1neAA0lFbo4=
github.com/aws/aws-sdk-go-v2/service/ec2 v1.27.0 h1:PXuF+b/U5j6e7y9MzGOt/OMCs4sJstlLzbvei3bDQSw=
github.com/aws/aws-sdk-go-v2/service/ec2 v1.27.0/go.mod h1:U3n3r74WpCXS0r46HSWw9dpsuseCurxOUN8eoiE3ev8=
github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.6.0 h1:rwE0kWa5qm0yEoNPwC3zhrt1tFVXTmkWRlUxLayAwyc=
github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.6.0/go.mod h1:wTgFkG6t7jS/6Y0SILXwfspV3IXowb6ngsAlSajW0Kc=
github.com/aws/aws-sdk-go-v2/service/ssm v1.19.0 h1:Vh5uiTlIdh5+H7gktS10P6SDhRk7SlToRiesZfjEH18=
github.com/aws/aws-sdk-go-v2/service/ssm v1.19.0/go.mod h1:Axs5mEKca5yIkSNmD3H4CEeXZuGLlDvhNxffsFoWVoY=
github.com/aws/aws-sdk-go-v2/service/sso v1.8.0 h1:X77LUt6Djy3Z02r6tW7Z+4FNr6GCnEG54EXfskc19M4=
github.com/aws/aws-sdk-go-v2/service/sso v1.8.0/go.mod h1:AB6v3BedyhVRIbPQbJnUsBmtup2pFiikpp5n3YyB6Ac=
github.com/aws/aws-sdk-go-v2/service/sts v1.13.0 h1:n8+dZMOvwkGtmhub8B2wYvRHut45/NB7DeNhNcUnBpg=
github.com/aws/aws-sdk-go-v2/service/sts v1.13.0/go.mod h1:jQto17aC9pJ6xRa1g29uXZhbcS6qNT3PSnKfPShq4sY=
github.com/aws/smithy-go v1.9.1 h1:5vetTooLk4hPWV8q6ym6+lXKAT1Urnm49YkrRKo2J8o=
github.com/aws/smithy-go v1.9.1/go.mod h1:SObp3lf9smib00L/v3U2eAKG8FyQ7iLrJnQiAmR5n+E=

@jasdel
Copy link
Contributor

jasdel commented Jan 21, 2022

Does the following script fail for you? This specifies a mock file created that will be created in the local directory and uses the SDK's LoadSharedConfigProfile to read it. This is the same tool that LoadDefaultConfig uses under the hood.

I've not been able to reproduce this issue with \n or \r\n.

What version of Go are you using by the way?

package main

import (
	"context"
	"fmt"
	"log"
	"os"

	"github.com/aws/aws-sdk-go-v2/config"
)

func main() {
	v := []byte("[default]\r\nduration_seconds = 10800\r\n")
	os.WriteFile("mock_config_test", v, 0644)

	cfg, err := config.LoadSharedConfigProfile(context.Background(), "default", func(o *config.LoadSharedConfigOptions) {
		o.CredentialsFiles = []string{}
		o.ConfigFiles = []string{"mock_config_test"}
	})
	if err != nil {
		log.Fatalf("failed to load shared config, %v", err)
	}

	if cfg.RoleDurationSeconds == nil {
		log.Fatalf("duration_seconds, not read from config file, %#v", cfg)
	}
	fmt.Println(*cfg.RoleDurationSeconds)
}

@jasdel
Copy link
Contributor

jasdel commented Jan 21, 2022

github.com/aws/aws-sdk-go-v2/internal/ini is the important one and its a patch version behind, but that's fine, only difference between v1.3.3 and v1.3.4 (latest) is transient dependencies which don't impact the ini module.

github.com/aws/aws-sdk-go-v2/config is one minor version behind. I don't see that impacting this, but its worth updating to v1.13.0

@kichik
Copy link

kichik commented Jan 21, 2022

That script doesn't fail. I'm using go 1.17. github.com/aws/aws-sdk-go-v2/internal/ini is on 1.3.3. Updating config didn't help.

@jasdel
Copy link
Contributor

jasdel commented Jan 21, 2022

Guessing this is going to fail, but if the above script is modified to read your file does it fail?

package main

import (
	"context"
	"fmt"
	"log"
	"os"

	"github.com/aws/aws-sdk-go-v2/config"
)

func main() {
	cfg, err := config.LoadSharedConfigProfile(context.Background(), "default", func(o *config.LoadSharedConfigOptions) {
		o.CredentialsFiles = []string{}
		o.ConfigFiles = []string{os.Args[1]}
	})
	if err != nil {
		log.Fatalf("failed to load shared config, %v", err)
	}

	if cfg.RoleDurationSeconds == nil {
		log.Fatalf("duration_seconds, not read from config file, %#v", cfg)
	}
	fmt.Println(*cfg.RoleDurationSeconds)
}
go run main <config_file>

3h0m0s

@kichik
Copy link

kichik commented Jan 21, 2022

That one worked with the reduced credentials file. But it failed a different way with the full file which still suggests a parsing error. It said it couldn't find the source profile.

2022/01/21 14:08:33 failed to load shared config, failed to load assume role arn:aws:iam::123456:role/Admin, of profile default-mfa, failed to get shared config profile, default-mfa

I would guess this is not related to the actual parsing issue though. Sounds like the code just doesn't have everything setup right for this use case. The important part is the duration_seconds number was read correctly.

@jasdel
Copy link
Contributor

jasdel commented Jan 21, 2022

Thanks, that's helpful!

That one worked with the reduced credentials file. But it failed a different way with the full file which still suggests a parsing error. It said it couldn't find the source profile.

Is your configuration spread across a ~/.aws/config and ~/.aws/credentials file? If so that that's potentially related. The script explicitly only targeted a single file. If you add the other file into the list of SharedConfigFiles or SharedCredentialsFiles the other file should be merged in and parsed as well.

Could you try a similar script that uses LoadDefaultConfig instead of the SharedConfig loader directly. This script, similar to the previous ones, explicitly only loads the files specified to help narrow down the issue. Then try adding your other config or credentials file, which ever was not used the first time.

package main

import (
	"context"
	"log"
	"os"

	"github.com/aws/aws-sdk-go-v2/config"
)

func main() {
	_, err := config.LoadDefaultConfig(context.Background(), func(o *config.LoadOptions) error {
		o.SharedConfigProfile = "default"
		o.SharedConfigFiles = []string{os.Args[1]}
		o.SharedCredentialsFiles = []string{}
		return nil
	})
	if err != nil {
		log.Fatalf("failed to load config, %v", err)
	}
}

@kichik
Copy link

kichik commented Jan 21, 2022

Yes, my configuration is split between config and credentials. That's what AWS CLI generates.

The latest script doesn't fail on the duration number, but does fail finding the source profile again. Both those profiles are in credentials, BTW. I tried making sure it's in config too, but that didn't help.

If put config in ShareConfigFiles and credentials in SharedCredentialsFiles, as seems expected, I am back to the number parsing issue. If I reverse them (and still include both), it gives me the profile not found issue. So seems like the profile finding issue is just because those two were flipped.

If I don't include SharedConfigFiles, it works. It just works. And config doesn't have the number in it or anything.

So this works:

package main

import (
	"context"
	"github.com/aws/aws-sdk-go-v2/credentials/stscreds"
	"log"
	"os"

	"github.com/aws/aws-sdk-go-v2/config"
)

func main() {
	_, err := config.LoadDefaultConfig(context.Background(),
		config.WithAssumeRoleCredentialOptions(func(o *stscreds.AssumeRoleOptions) {
			o.TokenProvider = stscreds.StdinTokenProvider
		}),
		func(o *config.LoadOptions) error {
			o.SharedConfigProfile = "default"
			//o.SharedConfigFiles = []string{os.Args[1]}
                        o.SharedConfigFiles = []string{}
			o.SharedCredentialsFiles = []string{os.Args[2]}
			return nil
		},
	)
	if err != nil {
		log.Fatalf("failed to load config, %v", err)
	}
}

With:

go run test-aws.go %USERPROFILE%\.aws\config %USERPROFILE%\.aws\credentials

But as soon as I put config back into SharedConfigFiles, it fails again.

@kichik
Copy link

kichik commented Jan 21, 2022

My original code also just works if I physically move config out of the way.

@kichik
Copy link

kichik commented Jan 21, 2022

The problem seems to be here:

srcValue := srcSection.Int(key)
v, err := ini.NewIntValue(srcValue)

srcSection.Int(key) returns int64, but ini.NewIntValue(srcValue) is trying to parse that number as a rune. I don't think line 656 is needed at all.

@jasdel
Copy link
Contributor

jasdel commented Jan 21, 2022

Good find. i was able to reproduce this via unit test.

Looks like the actual root issue is that NewIntValue is trying to convert the integer to a rune which is wrong. It should first convert the integer to a string, then convert the string to a rune slice.

The value needs to be a ini.Value so it can be merged into the destination via UpdateValue

I'll get a fixed and included in PR #1568

jasdel added a commit to jasdel/aws-sdk-go-v2 that referenced this issue Jan 21, 2022
Fixes the SDK's handling of `duration_sections` in the shared
credentials file or specified in multiple shared config and shared
credentials files under the same profile.

Fixes aws#1192
@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jan 22, 2022
@kichik
Copy link

kichik commented Jan 22, 2022

Thanks @jasdel !!!

jasdel added a commit that referenced this issue Jan 24, 2022
…1568)

Fixes the SDK's handling of `duration_sections` in the shared
credentials file or specified in multiple shared config and shared
credentials files under the same profile.

Fixes #1192
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

jrichard8 pushed a commit to jrichard8/aws-sdk-go-v2 that referenced this issue Feb 14, 2022
…ws#1568)

Fixes the SDK's handling of `duration_sections` in the shared
credentials file or specified in multiple shared config and shared
credentials files under the same profile.

Fixes aws#1192
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. needs-reproduction This issue needs reproduction.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants