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

added AWS enpoint handling #3416

Merged
merged 4 commits into from
Nov 6, 2017
Merged

added AWS enpoint handling #3416

merged 4 commits into from
Nov 6, 2017

Conversation

ror6ax
Copy link
Contributor

@ror6ax ror6ax commented Oct 4, 2017

In AWS, IAM endpoint(API you actually talk to get users) is determined by the region.

There is a number of AWS clones out there, like Eucalyptus. IAM works in very similar way, but endpoints are not standardized. The only way to make clones work with Vault is to provide optional endpoint parameter.

@ror6ax ror6ax force-pushed the endpoint_patch branch 2 times, most recently from fc7aa28 to 9064cfb Compare October 4, 2017 10:12
endpoint := data.Get("endpoint").(string)

if endpoint == "" {
endpoint = "none"
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't use "none" as a placeholder value here. I'd just leave it as the empty string (which is the zero value for strings in golang) and compare against the empty string in client.go

client := iam.New(session.New(awsConfig))

if *awsConfig.Endpoint != "none" {
Copy link
Contributor

Choose a reason for hiding this comment

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

See note about not using "none" as a placeholder, use ""

@@ -73,6 +81,9 @@ func clientSTS(s logical.Storage) (*sts.STS, error) {
return nil, err
}
client := sts.New(session.New(awsConfig))
if *awsConfig.Endpoint != "none" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same -- use ""

@@ -73,6 +81,9 @@ func clientSTS(s logical.Storage) (*sts.STS, error) {
return nil, err
}
client := sts.New(session.New(awsConfig))
if *awsConfig.Endpoint != "none" {
client = sts.New(session.New(awsConfig.WithEndpoint(*awsConfig.Endpoint)))
Copy link
Contributor

Choose a reason for hiding this comment

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

You're configuring STS and IAM clients with the same endpoints, and I doubt that will work. They likely need different endpoints.

@@ -23,6 +23,10 @@ func pathConfigRoot() *framework.Path {
Type: framework.TypeString,
Description: "Region for API calls.",
},
"endpoint": &framework.FieldSchema{
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably want separate iam_endpoint and sts_endpoint here.

@ror6ax ror6ax force-pushed the endpoint_patch branch 2 times, most recently from 53f1df5 to 96c6073 Compare October 4, 2017 19:50
@ror6ax
Copy link
Contributor Author

ror6ax commented Oct 4, 2017

Not really sure why 2 endpoint fields don't get recognized, seems I've added both. @joelthompson - can you please have a look for what I missed? Thanks a ton.

Copy link
Contributor

@joelthompson joelthompson left a comment

Choose a reason for hiding this comment

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

Looking pretty good @ror6ax! Just the small bit about dealing with the annoying AWS SDK details....

@@ -23,6 +23,14 @@ func pathConfigRoot() *framework.Path {
Type: framework.TypeString,
Description: "Region for API calls.",
},
"iamendpoint": &framework.FieldSchema{
Copy link
Contributor

Choose a reason for hiding this comment

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

The auth backend already has iam_endpoint and sts_endpoint; it'd be good to underscore-separate this here.

Type: framework.TypeString,
Description: "Endpoint to custom IAM server URL",
},
"stsendpoint": &framework.FieldSchema{
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

@@ -51,6 +53,8 @@ func getRootConfig(s logical.Storage) (*aws.Config, error) {
return &aws.Config{
Credentials: creds,
Region: aws.String(credsConfig.Region),
IAMEndpoint: aws.String(credsConfig.IAMEndpoint),
STSEndpoint: aws.String(credsConfig.STSEndpoint),
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure the issue is because you're using the upstream aws.Config object and it only has an Endpoint member. Consider passing a clientType parameter to getRootConfig and if clientType is iam then you set Endpoint to credsConfig.IAMEndpoint and if it's sts then you set Endpoint to credsConfig.STSEndpoint. Largely the same code lives in the auth backend:

func (b *backend) getRawClientConfig(s logical.Storage, region, clientType string) (*aws.Config, error) {
credsConfig := &awsutil.CredentialsConfig{
Region: region,
}
// Read the configured secret key and access key
config, err := b.nonLockedClientConfigEntry(s)
if err != nil {
return nil, err
}
endpoint := aws.String("")
if config != nil {
// Override the default endpoint with the configured endpoint.
switch {
case clientType == "ec2" && config.Endpoint != "":
endpoint = aws.String(config.Endpoint)
case clientType == "iam" && config.IAMEndpoint != "":
endpoint = aws.String(config.IAMEndpoint)
case clientType == "sts" && config.STSEndpoint != "":
endpoint = aws.String(config.STSEndpoint)
}
credsConfig.AccessKey = config.AccessKey
credsConfig.SecretKey = config.SecretKey
}
credsConfig.HTTPClient = cleanhttp.DefaultClient()
creds, err := credsConfig.GenerateCredentialChain()
if err != nil {
return nil, err
}
if creds == nil {
return nil, fmt.Errorf("could not compile valid credential providers from static config, environment, shared, or instance metadata")
}
// Create a config that can be used to make the API calls.
return &aws.Config{
Credentials: creds,
Region: aws.String(region),
HTTPClient: cleanhttp.DefaultClient(),
Endpoint: endpoint,
}, nil
}

client := iam.New(session.New(awsConfig))

if *awsConfig.IAMEndpoint != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Then this becomes *awsConfig.Endpoint

@@ -73,6 +83,9 @@ func clientSTS(s logical.Storage) (*sts.STS, error) {
return nil, err
}
client := sts.New(session.New(awsConfig))
if *awsConfig.STSEndpoint != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

@ror6ax ror6ax force-pushed the endpoint_patch branch 3 times, most recently from 94b37c1 to 537e8b7 Compare October 6, 2017 10:40
@ror6ax
Copy link
Contributor Author

ror6ax commented Oct 9, 2017

@joelthompson Is there anything else you want me to add here?

Copy link
Contributor

@joelthompson joelthompson left a comment

Choose a reason for hiding this comment

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

Sorry @ror6ax -- been a bit busy the past couple of days. Looking pretty good, just a couple of quick pieces of feedback.


//Endpoint to custom STS server URL
STSEndpoint string

Copy link
Contributor

Choose a reason for hiding this comment

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

And then you can just remove these.

@@ -29,6 +30,16 @@ func getRootConfig(s logical.Storage) (*aws.Config, error) {
credsConfig.AccessKey = config.AccessKey
credsConfig.SecretKey = config.SecretKey
credsConfig.Region = config.Region
credsConfig.IAMEndpoint = config.IAMEndpoint
credsConfig.STSEndpoint = config.STSEndpoint
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only place credsConfig.IAMEndpoint and credsConfig.STSEndpoint are referenced. I don't think you need them in the CredentialsConfig struct at all because, below, you're putting the endpoint directly in the aws.Config object.


if clientType == "sts" && config.STSEndpoint != "" {
endpoint = *aws.String(config.STSEndpoint)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be more go-idiomatic to do something like:

switch {
case clientType == "iam" && config.IAMEndpoint != "":
	endpoint = *aws.String(config.IAMEndpoint)
case clientType == "sts" && config.STSEndpoint != "":
	endpoint = *aws.String(config.STSEndpoint)
}

if *awsConfig.Endpoint != "" {
client = iam.New(session.New(awsConfig.WithEndpoint(*awsConfig.Endpoint)))
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this block is necessary anymore, because the endpoint is already in the awsConfig object that gets passed in when creating the IAM client.

if err != nil {
return nil, err
}
client := sts.New(session.New(awsConfig))
if *awsConfig.Endpoint != "" {
client = sts.New(session.New(awsConfig.WithEndpoint(*awsConfig.Endpoint)))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

@ror6ax
Copy link
Contributor Author

ror6ax commented Oct 23, 2017

@joelthompson - thanks for those remarks, now fixed.

Copy link
Contributor

@joelthompson joelthompson left a comment

Choose a reason for hiding this comment

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

@ror6ax
Copy link
Contributor Author

ror6ax commented Oct 24, 2017

@joelthompson Done. Did not add it to Sample Payload as it can be confusing in use with region.

@ror6ax
Copy link
Contributor Author

ror6ax commented Nov 2, 2017

Hi, @joelthompson sorry to bother, but will this make it into 0.8.4? Thanks!

@ror6ax
Copy link
Contributor Author

ror6ax commented Nov 6, 2017

Bumping this up.

@jefferai
Copy link
Member

jefferai commented Nov 6, 2017

LGTM, thanks!

@jefferai jefferai merged commit 81e18ae into hashicorp:master Nov 6, 2017
@jefferai jefferai added this to the 0.8.4 milestone Nov 6, 2017
chrishoffman pushed a commit that referenced this pull request Nov 7, 2017
* oss/master: (30 commits)
  Handle 'not supplied' case for field type TypeNameString (#3546)
  Fix deprecated cassandra backend tests (#3543)
  changelog++
  auth/aws: Make disallow_reauthentication and allow_instance_migration mutually exclusive (#3291)
  changelog++
  More Mount Conflict Detection (#2919)
  Fix swallowed errors in TestRollbackManager_Join() (#3327)
  changelog++
  added AWS enpoint handling (#3416)
  Seal wrap all root tokens and their leases (#3540)
  Return group memberships of entity during read (#3526)
  Add note on support for using rec keys on /sys/rekey (#3517)
  Add third party tools list to website (#3488)
  Minor client refactoring (#3539)
  changelog++
  Add PKCS8 marshaling to PKI (#3518)
  Update SSH list roles docs (#3536)
  Update gocql dep
  changelog++
  Return role info for each role on pathRoleList (#3532)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants