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

Support defining file credentials as SecretString #154

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sebhopley
Copy link

Hi, I am proposing this change to resolve issue 131.

This changes allows someone to set a SecretString (in UTF8) as the secret file content instead of a SecretBinary.

I've provided a test for this, please let me know if you need additional testing.

Seb

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@chriskilding
Copy link
Contributor

An update: I would like the dual string-or-secretbytes supplier in CredentialFactory to be used only with the Secret File type, as the other credential types don't need this behaviour. The implementation currently enables this behaviour for all credential types. Would you be able to create a separate StringOrSecretBytesSupplier inner static class within CredentialFactory that implements this behaviour, and pass it to the constructor of AwsFileCredentials?

@sebhopley
Copy link
Author

Hi, sorry for the late response, yeah I'll make that change

@sebhopley
Copy link
Author

So I went and looked at the code again, for both cases where SecretBytesSupplier is used we are hoping to use string secrets (in a similar way to the ssh key secret). Going to the conversation in the linked issue #131 I feel like the suggestion there to catch a failure to use UTF8 encoding and throw a descriptive error seems better than blanket not allowing this. Also as talked about, enforcing using the command line can be problematic (and means a user needs access keys) vs using the aws console.

Open to suggestions on what you would like to see to go forward with the change.

@chriskilding
Copy link
Contributor

By both cases, do you mean the Certificate credentials type as well? As far as I know the certificate's keystore is supplied by a PKCS#12 (.p12) file, which is exclusively a binary file format.

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.

Plugin throws exception for Secret File with SecretString
2 participants