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.

Sign up for GitHub

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

S3 SignatureV4 Without Config File Credentials #735

Closed
wants to merge 2 commits into from
Closed

S3 SignatureV4 Without Config File Credentials #735

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0541427
S3 SignatureV4 Without Config File Credentials
Jan 14, 2019
7b1c1d8
Pinned k8s.io/client-go to a Version That Works With Prometheus 2.3.2
Jan 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,3 +91,7 @@ ignored = ["github.com/improbable-eng/thanos/benchmark/*"]
[[constraint]]
version = "v0.11.0"
name = "github.com/mozillazg/go-cos"

[[constraint]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this is needed?

version = "v4.0.0"
name = "k8s.io/client-go"
13 changes: 7 additions & 6 deletions pkg/objstore/s3/s3.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,12 +90,13 @@ func NewBucketWithConfig(logger log.Logger, config Config, component string) (*B
if err := validate(config); err != nil {
return nil, err
}
if config.AccessKey != "" {
signature := credentials.SignatureV4
if config.SignatureV2 {
signature = credentials.SignatureV2
}

signature := credentials.SignatureV4
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is not necessary, the signature is used by static credentials only. And by default, the following credential providers of minio provided will use SignatureV4.

  • EnvAWS
  • FileAWSCredentials
  • IAM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe in theory, but unfortunately not in practice. Using v0.2.1 with the following bucket.yaml and environment variables (AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY), in AWS:

bucket.yaml

type: S3
config:
  bucket: my-secret-bucket-name
  endpoint: s3.us-west-2.amazonaws.com
  insecure: false
  signature_version2: false
  encrypt_sse: false

fails with the following error:

level=info ts=2019-01-15T19:38:03.422651711Z caller=flags.go:75 msg="gossip is disabled"
level=info ts=2019-01-15T19:38:03.422720714Z caller=factory.go:39 msg="loading bucket configuration"
store command failed: bucket store initial sync: sync block: iter: Access Denied

With this patch it works correctly.

Copy link
Member

@bwplotka bwplotka Jan 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm.. I think I know what is happening (:

So @jojohappy is right here, nothing really changed by moving signature outside of if config.AccessKey body.

I think this patch works for you because recently we fixed S3 auth order: #732 This was after 0.2.1 was released (: I think you accidenly assumed your patch work, but master most likely works for you as well (:

if config.SignatureV2 {
signature = credentials.SignatureV2
}

if config.AccessKey != "" {
chain = []credentials.Provider{&credentials.Static{
Value: credentials.Value{
AccessKeyID: config.AccessKey,
Expand All @@ -106,7 +107,7 @@ func NewBucketWithConfig(logger log.Logger, config Config, component string) (*B
} else {
chain = []credentials.Provider{
&credentials.EnvAWS{},
&credentials.FileAWSCredentials{},
&credentials.FileAWSCredentials{},
&credentials.IAM{
Client: &http.Client{
Transport: http.DefaultTransport,
Expand Down