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

Extract AWS credentials from secret #292

Merged
merged 8 commits into from
Sep 17, 2019
Merged

Conversation

hakanmemisoglu
Copy link
Contributor

@hakanmemisoglu hakanmemisoglu commented Sep 16, 2019

Change Overview

Extract AWS credentials from AWS typed secret.

  • Validate AWS credential secret.
  • Extract required awsAccessKeyID and awsSecretAccessKey fields from the secret.
  • Extract optional awsSessionToken from the field.

Pull request type

Please check the type of change your PR introduces:

  • Work in Progress
  • Refactoring (no functional changes, no api changes)
  • Trivial/Minor
  • Bugfix
  • Feature
  • Documentation

Test Plan

  • Manual
  • Unit test
  • E2E

Copy link
Contributor

@tdmanv tdmanv left a comment

Choose a reason for hiding this comment

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

Looks good!

pkg/secrets/aws.go Outdated Show resolved Hide resolved
pkg/secrets/aws_test.go Outdated Show resolved Hide resolved
AWSSecretType string = "secrets.kanister.io/aws"

// AWSAccessKeyID is the key for AWS access key ID.
AWSAccessKeyID string = "awsAccessKeyID"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use the same values that the AWS SDK and CLI use? That's what mot people would expect these (environment) variables to be named, and thus to be the corresponding keys in the secret

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tdmanv suggested we are using camel case with secret data fields in Kanister.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I said lowercase. The field names I've seen elsewhere in our code are:

    access_key_id: XXX
    secret_access_key: XXX

Copy link
Contributor

Choose a reason for hiding this comment

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

@tdmanv lower case is "surprising", what's the rationale for it?
With the given key names, it would not be possible to directly export all key-value pairs in a secret as environment variables.


const (
// AWSSecretType represents the secret type for AWS credentials.
AWSSecretType string = "secrets.kanister.io/aws"
Copy link
Contributor

Choose a reason for hiding this comment

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

[minor] do these need to be exported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed right now. However I think some Kanister package sure can use this value to validate their own input.

@hakanmemisoglu hakanmemisoglu merged commit 22afbde into master Sep 17, 2019
@julio-lopez julio-lopez deleted the aws-typed-secret branch September 17, 2019 06:01
if _, ok := secret.Data[AWSSessionToken]; ok {
count++
}
if len(secret.Data) > count {
Copy link
Contributor

Choose a reason for hiding this comment

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

This check seems too restrictive in the sense that it should be possible to easily extend the usage of these secrets by adding other fields without having to go and explicitly modify code that uses these secrets. In particular, this is not forward compatible. That is, in the future we decide to include other fields, suppose the role or some other metadata instead of the token, (a) it would require validation changes to allow the new definition and (b) older versions of the code would break when encountering the new types of secrets.

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.

None yet

3 participants