Skip to content

OCPBUGS-38759: client: re-use a single file for building the session instead of randomly named files #140

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

OCPBUGS-38759: client: re-use a single file for building the session instead of randomly named files #140

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 20 additions & 7 deletions pkg/client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ import (
"bytes"
"context"
"fmt"
"io/ioutil"
"os"
"path"
"strings"
"sync"
"time"
Expand All @@ -45,6 +45,7 @@ import (
configv1 "github.com/openshift/api/config/v1"
machineapiapierrors "github.com/openshift/machine-api-operator/pkg/controller/machine"
apimachineryerrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/util/rand"
)

//go:generate go run ../../vendor/github.com/golang/mock/mockgen -source=./client.go -destination=./mock/client_generated.go -package=mock
Expand All @@ -68,6 +69,11 @@ const (
awsRegionsCacheExpirationDuration = time.Minute * 30
)

var (
sharedCredentialsFileMutex sync.Mutex
sharedCredentialsFileName = path.Join(os.TempDir(), "aws-shared-credentials"+rand.String(16))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can rand.String output characters which are invalid in a path?

Copy link
Author

Choose a reason for hiding this comment

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

It only uses bcdfghjklmnpqrstvwxz2456789

	// We omit vowels from the set of available characters to reduce the chances
	// of "bad words" being formed.
	alphanums = "bcdfghjklmnpqrstvwxz2456789"

So no :-)

)

// AwsClientBuilderFuncType is function type for building aws client
type AwsClientBuilderFuncType func(client client.Client, secretName, namespace, region string, configManagedClient client.Client, regionCache RegionCache) (Client, error)

Expand Down Expand Up @@ -369,10 +375,18 @@ func newAWSSession(ctrlRuntimeClient client.Client, secretName, namespace, regio
}
return nil, err
}
sharedCredentialsFileMutex.Lock()
defer sharedCredentialsFileMutex.Unlock()
sharedCredsFile, err := sharedCredentialsFileFromSecret(&secret)
if err != nil {
return nil, fmt.Errorf("failed to create shared credentials file from Secret: %v", err)
}

// Ensure the file gets deleted in any case.
defer func() {
os.Remove(sharedCredsFile)
}()

sessionOptions.SharedConfigState = session.SharedConfigEnable
sessionOptions.SharedConfigFiles = []string{sharedCredsFile}
}
Expand All @@ -392,11 +406,6 @@ func newAWSSession(ctrlRuntimeClient client.Client, secretName, namespace, regio
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 s, nil
Expand Down Expand Up @@ -471,12 +480,16 @@ func sharedCredentialsFileFromSecret(secret *corev1.Secret) (string, error) {
return "", fmt.Errorf("invalid secret for aws credentials")
}

f, err := ioutil.TempFile("", "aws-shared-credentials")
// Re-using the same file every time to prevent leakage of memory to slab.
// Related issue: https://issues.redhat.com/browse/RHEL-119532
f, err := os.Create(sharedCredentialsFileName)
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 still writing the file every time, even if it now has the same filename.

If this is enough to stem the bleeding then fine, I guess, but we should put a big comment here explaining why we're not using ioutil.TempFile() and why we shouldn't put it back, preferrably with a link to a RHEL bug.

However, ideally we would return the same file if the contents are the same. How about we give the file some name which is unique to the secret's contents and don't create it if it already exists? A couple of ideas:

  • aws-shared-credentials-[hash of secret contents]
  • aws-shared-credentials-[secret.name]-[secret.uid]-[secret.generation]

The latter seems pretty simple to write. A downside is that over time we accumulate every version of the credentials we have ever seen in the temp directory, but I suspect that would never be an issue in practice and the only actor who can exploit it has no incentive to DoS themselves.

Copy link
Author

Choose a reason for hiding this comment

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

This is still writing the file every time, even if it now has the same filename.

If this is enough to stem the bleeding then fine, I guess, but we should put a big comment here explaining why we're not using ioutil.TempFile() and why we shouldn't put it back, preferrably with a link to a RHEL bug.

Going to add a comment 👍 agree.

However, ideally we would return the same file if the contents are the same. How about we give the file some name which is unique to the secret's contents and don't create it if it already exists? A couple of ideas:

  • aws-shared-credentials-[hash of secret contents]
  • aws-shared-credentials-[secret.name]-[secret.uid]-[secret.generation]

The latter seems pretty simple to write. A downside is that over time we accumulate every version of the credentials we have ever seen in the temp directory, but I suspect that would never be an issue in practice and the only actor who can exploit it has no incentive to DoS themselves.

I'd prefer to not cache the credentials on disk. If we want to do some caching instead, we might want to cache the session object in memory and not need the whole write/read/delete cycle at all. But I think this goes too far for this PR.

if err != nil {
return "", fmt.Errorf("failed to create file for shared credentials: %v", err)
}
defer f.Close()
if _, err := f.Write(data); err != nil {
// Delete the file in case of having an error. Otherwise the calling function needs to handle deletion.
defer func() { os.Remove(f.Name()) }()
return "", fmt.Errorf("failed to write credentials to %s: %v", f.Name(), err)
}
return f.Name(), nil
Expand Down