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

feat: support JSON Credentials as repo secret #903

Closed

Conversation

cbentkowski
Copy link

Fixes #900

Added new functionality to GetCredsType in utils.go to parse JSON Username/Password in this format:
{ "username": "someusername", "password": "somepassword" }

Added new function, GetJSONSecret to parse and return the JSON Username/Password above to utils.go.

Updated parseCreds in main.go to look for SECRET_JSON and call the GetJSONSecret and return the creds.

Corrected spelling of unknown in error return of parseCreds in main.go.

Tests pass in utils_test.go.

@cbentkowski
Copy link
Author

I apologize if I named the pull request/branch incorrectly. This is my very first one to a public repository.

@cbentkowski cbentkowski changed the title Feature/add json credentials feat: Add support for JSON Credentials Oct 26, 2024
@mrgrain mrgrain changed the title feat: Add support for JSON Credentials feat: support JSON Credentials as repo secret Oct 29, 2024
Copy link
Contributor

@mrgrain mrgrain left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you.

One thing though. I'm not 100% sure I understand what exactly this new feature is covering. Is it adding support for stringified JSON in the creds property? Or for JSON in the SecretsManager Secret? Or for both?

From the code, I think this PR currently only adds supports for stringified JSON in creds, but I might be wrong. However I also think the linked issue is talking about JSON support inside the SecretsManager Secret. Could you check that? I reckon this PR would be a huge step to that, but we would need a little bit more work before we can close the issue.

And a couple minor notes:

  • Once the above is resolved, please update the docs here to describe the newly supported format(s).
  • Let's add a test case for GetJSON. I know the other types don't have tests, but they are also much simpler.

@cbentkowski
Copy link
Author

Based on #900, I was going just for stringified JSON in the creds property. I can add a comment on #900 to see if they are wanting JSON support inside SecretsManager and if so I can add that. I'll also work on the test for GetJSON and update the docs later today.

@mrgrain
Copy link
Contributor

mrgrain commented Oct 29, 2024

Based on #900, I was going just for stringified JSON in the creds property. I can add a comment on #900 to see if they are wanting JSON support inside SecretsManager and if so I can add that. I'll also work on the test for GetJSON and update the docs later today.

That's perfect, thank you!

@mrgrain
Copy link
Contributor

mrgrain commented Oct 30, 2024

Ok, similar to the ARN credentials that it already accepts, but the return from the ARN will be JSON formatted text. I've got a JSON parser built, but let me work on detecting JSON string formatted credentials from the ARN and see what I can come up with.

#900 (comment)

With that, we can either

Either is fine with me. LMK what you prefer.

@cbentkowski
Copy link
Author

I actually have the code to detect a JSON string being returned from the GetSecretValue. It's the unit tests I'm having an issue with. Trying to mock up the secret client is a pain.

@mrgrain
Copy link
Contributor

mrgrain commented Oct 30, 2024

I actually have the code to detect a JSON string being returned from the GetSecretValue. It's the unit tests I'm having an issue with. Trying to mock up the secret client is a pain.

That's awesome!

I understand the challenges with mocking SDK calls. No idea myself how this works in Golang. Alternatively, wrap all the logic that deals with the response from GetSecretValue in a helper function and test that.

GetSecret would look like this (pseudo code) and you only test ParseGetSecretResponse:

func GetSecret(secretId string) (secret string, err error) {
  resp, err = GetSecretValueHelper(secretId) // pretty much what the current method does
  secret, err = ParseGetSecretResponse(resp)
  return secret
}

auto-merge was automatically disabled October 30, 2024 10:15

Head branch was pushed to by a user without write access

@cbentkowski
Copy link
Author

I went ahead and pushed the code I have while I continue to work on unit tests. I'll have to come back to it later this afternoon/evening.

@mrgrain
Copy link
Contributor

mrgrain commented Nov 7, 2024

@cbentkowski Thanks for your work so far. Anything I can help with to get this wrapped up?

@cbentkowski
Copy link
Author

@mrgrain Sorry, work has gotten busy. I'll try to find some time this afternoon/evening to wrap this up.

@Hi-Fi
Copy link
Contributor

Hi-Fi commented Nov 9, 2024

Maybe this could be typed also on construct side.

I had similar thing included to other PR (1eebc0c#diff-c54113cf61ec99691748a3890bfbeb00e10efb3f0a76f03a0fd9ec49072e410aR98-R110), but removed it already from there as it's easily creating breaking change.

It doesn't have any lambda side handling, as Lambda already handles secrets when ARN is given.

As one biggest advantage of typescript is typing of data, so plain JSOn sounds just very dirty solution.

@Bbuff101
Copy link

@mrgrain Sorry, work has gotten busy. I'll try to find some time this afternoon/evening to wrap this up.

@cbentkowski , anything I can help with for this PR, or any questions that might be lingering that I can help answer?

@cbentkowski
Copy link
Author

I'm sorry. I've had a lot of things come up with work and personal life and unfortunately I don't currently have time to continue working on this.

@mrgrain
Copy link
Contributor

mrgrain commented Nov 22, 2024

I'm sorry. I've had a lot of things come up with work and personal life and unfortunately I don't currently have time to continue working on this.

All good. Thanks for letting us know.

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.

Support Non-PlainText Secret Format for Container Repository Login
4 participants