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

use shared credentials file for session creation #374

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 1 addition & 4 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,6 @@ github.com/asaskevich/govalidator v0.0.0-20180720115003-f9ffefc3facf/go.mod h1:l
github.com/asaskevich/govalidator v0.0.0-20190424111038-f61b66f89f4a/go.mod h1:lB+ZfQJz7igIIfQNfa7Ml4HSf2uFQQRzpGGRXenZAgY=
github.com/aws/aws-sdk-go v1.15.66 h1:c2ScQzjFUoD1pK+6GQzX8yMXwppIjP5Oy8ljMRr2cbo=
github.com/aws/aws-sdk-go v1.15.66/go.mod h1:E3/ieXAlvM0XWO57iftYVDLLvQ824smPP3ATZkfNZeM=
github.com/aws/aws-sdk-go v1.21.10 h1:lTRdgyxraKbnNhx7kWeoW/Uow1TKnSNDpQGTtEXJQgk=
github.com/aws/aws-sdk-go v1.21.10/go.mod h1:KmX6BPdI08NWTb3/sm4ZGu5ShLoqVDhKgpiN924inxo=
github.com/aws/aws-sdk-go v1.35.20 h1:Hs7x9Czh+MMPnZLQqHhsuZKeNFA3Vuf7pdy2r5QlVb0=
github.com/aws/aws-sdk-go v1.35.20/go.mod h1:tlPOdRjfxPBpNIwqDj61rmsnA85v9jc0Ps9+muhnW+k=
github.com/beorn7/perks v0.0.0-20180321164747-3a771d992973/go.mod h1:Dwedo/Wpr24TaqPxmxbtue+5NUziq4I4S80YR8gNf3Q=
Expand Down Expand Up @@ -319,10 +317,9 @@ github.com/inconshreveable/mousetrap v1.0.0 h1:Z8tu5sraLXCXIcARxBp/8cbvlwVa7Z1NH
github.com/inconshreveable/mousetrap v1.0.0/go.mod h1:PxqpIevigyE2G7u3NXJIT2ANytuPF1OarO4DADm73n8=
github.com/jmespath/go-jmespath v0.0.0-20160202185014-0b12d6b521d8 h1:12VvqtR6Aowv3l/EQUlocDHW2Cp4G9WJVH7uyH8QFJE=
github.com/jmespath/go-jmespath v0.0.0-20160202185014-0b12d6b521d8/go.mod h1:Nht3zPeWKUH0NzdCt2Blrr5ys8VGpn0CEB0cQHVjt7k=
github.com/jmespath/go-jmespath v0.0.0-20180206201540-c2b33e8439af h1:pmfjZENx5imkbgOkpRUYLnmbU7UEFbjtDA2hxJ1ichM=
github.com/jmespath/go-jmespath v0.0.0-20180206201540-c2b33e8439af/go.mod h1:Nht3zPeWKUH0NzdCt2Blrr5ys8VGpn0CEB0cQHVjt7k=
github.com/jmespath/go-jmespath v0.4.0 h1:BEgLn5cpjn8UN1mAw4NjwDrS35OdebyEtFe+9YPoQUg=
github.com/jmespath/go-jmespath v0.4.0/go.mod h1:T8mJZnbsbmF+m6zOOFylbeCJqk5+pHWvzYPziyZiYoo=
github.com/jmespath/go-jmespath/internal/testify v1.5.1 h1:shLQSRRSCCPj3f2gpwzGwWFoC7ycTf1rcQZHOlsJ6N8=
github.com/jmespath/go-jmespath/internal/testify v1.5.1/go.mod h1:L3OGu8Wl2/fWfCI6z80xFu9LTZmf1ZRjMHUOPmWr69U=
github.com/jonboulle/clockwork v0.1.0/go.mod h1:Ii8DK3G1RaLaWxj9trq07+26W01tbo22gdxWY5EU2bo=
github.com/json-iterator/go v0.0.0-20180612202835-f2b4162afba3/go.mod h1:+SdeFBvtyEkXs7REEP0seUULqWtbJapLOCVDaaPEHmU=
Expand Down
72 changes: 57 additions & 15 deletions pkg/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,11 @@ limitations under the License.
package client

import (
"bytes"
"context"
"fmt"
"io/ioutil"
"os"

corev1 "k8s.io/api/core/v1"
"sigs.k8s.io/cluster-api-provider-aws/pkg/version"
Expand Down Expand Up @@ -145,7 +149,11 @@ func (c *awsClient) ELBv2RegisterTargets(input *elbv2.RegisterTargetsInput) (*el
// secret if defined (i.e. in the root cluster),
// otherwise the IAM profile of the master where the actuator will run. (target clusters)
func NewClient(ctrlRuntimeClient client.Client, secretName, namespace, region string) (Client, error) {
awsConfig := &aws.Config{Region: aws.String(region)}
sessionOptions := session.Options{
Config: aws.Config{
Region: aws.String(region),
},
}

if secretName != "" {
var secret corev1.Secret
Expand All @@ -155,31 +163,30 @@ func NewClient(ctrlRuntimeClient client.Client, secretName, namespace, region st
}
return nil, err
}
accessKeyID, ok := secret.Data[AwsCredsSecretIDKey]
if !ok {
return nil, machineapiapierrors.InvalidMachineConfiguration("AWS credentials secret %v did not contain key %v",
secretName, AwsCredsSecretIDKey)
}
secretAccessKey, ok := secret.Data[AwsCredsSecretAccessKey]
if !ok {
return nil, machineapiapierrors.InvalidMachineConfiguration("AWS credentials secret %v did not contain key %v",
secretName, AwsCredsSecretAccessKey)
sharedCredsFile, err := sharedCredentialsFileFromSecret(&secret)
if err != nil {

Choose a reason for hiding this comment

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

Looks to me like if the temp file can be created but not written to this function returns early and the temp file is never deleted.

return nil, fmt.Errorf("failed to create shared credentials file from Secret: %v", err)
}

awsConfig.Credentials = credentials.NewStaticCredentials(
string(accessKeyID), string(secretAccessKey), "")
sessionOptions.SharedConfigState = session.SharedConfigEnable
sessionOptions.SharedConfigFiles = []string{sharedCredsFile}
}

// Resolve custom endpoints
if err := resolveEndpoints(awsConfig, ctrlRuntimeClient, region); err != nil {
if err := resolveEndpoints(&sessionOptions.Config, ctrlRuntimeClient, region); err != nil {
return nil, err
}

// Otherwise default to relying on the IAM role of the masters where the actuator is running:
s, err := session.NewSession(awsConfig)
s, err := session.NewSessionWithOptions(sessionOptions)
if err != nil {
return nil, err
}

// Remove any temporary shared credentials files after session creation so they don't accumulate
if len(sessionOptions.SharedConfigFiles) > 0 {
os.Remove(sessionOptions.SharedConfigFiles[0])
}

s.Handlers.Build.PushBackNamed(addProviderVersionToUserAgent)

return &awsClient{
Expand Down Expand Up @@ -266,3 +273,38 @@ func buildCustomEndpointsMap(customEndpoints []configv1.AWSServiceEndpoint) map[

return customEndpointsMap
}

// sharedCredentialsFileFromSecret returns a location (path) to the shared credentials
// file that was created using the provided secret
func sharedCredentialsFileFromSecret(secret *corev1.Secret) (string, error) {
var data []byte
switch {
case len(secret.Data["credentials"]) > 0:
data = secret.Data["credentials"]

Choose a reason for hiding this comment

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

Just for my understanding really, this credentials file, will it have a reference to the token that is being projected into the pod or does that come from somewhere else?

Copy link
Author

@sjenning sjenning Nov 12, 2020

Choose a reason for hiding this comment

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

It will have the path to the token file in it. Will have to be in sync with the projected token location in openshift/machine-api-operator#743.

Currently, this is just to make it possible to use STS cred, but phase 0 will only allow CCO in Manual mode and cluster admin will have to create the cred secrets manually.

case len(secret.Data["aws_access_key_id"]) > 0 && len(secret.Data["aws_secret_access_key"]) > 0:
data = newConfigForStaticCreds(
string(secret.Data["aws_access_key_id"]),
string(secret.Data["aws_secret_access_key"]),
)
default:
return "", fmt.Errorf("invalid secret for aws credentials")
}

f, err := ioutil.TempFile("", "aws-shared-credentials")
sjenning marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return "", fmt.Errorf("failed to create file for shared credentials: %v", err)
}
defer f.Close()

Choose a reason for hiding this comment

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

In the example code, there's an explicit call to close to handle an error: https://golang.org/pkg/io/ioutil/#example_TempFile

if _, err := f.Write(data); err != nil {
return "", fmt.Errorf("failed to write credentials to %s: %v", f.Name(), err)
}
return f.Name(), nil

Choose a reason for hiding this comment

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

We should return the whole file here (or better, pass in the file object that we opened outside this function?) as it has a method to remove the file for us.

Choose a reason for hiding this comment

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

Ignore this, it's doing defer os.Remove(tmpfile.Name())

Choose a reason for hiding this comment

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

Or the method could receive an io.WriteCloser so the caller could be the one to handle the filesystem stuff.

}

func newConfigForStaticCreds(accessKey string, accessSecret string) []byte {
buf := &bytes.Buffer{}
fmt.Fprint(buf, "[default]\n")
fmt.Fprintf(buf, "aws_access_key_id = %s\n", accessKey)
fmt.Fprintf(buf, "aws_secret_access_key = %s\n", accessSecret)
return buf.Bytes()
}