Skip to content

Conversation

lizard-boy
Copy link

Motivation

With this PR I am trying to fix the bug mentioned on this issue : localstack#11319

Changes

Previously localstack assumed the behaviour of SecretBinary field is same for all the sdk request, but the behaviour differs. According to AWS docs:

When you retrieve a SecretBinary using the HTTP API, the Python SDK, or the AWS CLI, the value is Base64-encoded. Otherwise, it is not encoded.

So on this PR, I am adding a check to identify what entity is making the request, and localstack will decode the value if needed.

Testing

  • Added a test to fetch a secret when the SecretBinary should not be decoded
  • [Todo] Add a test to fetch a secret when SecretBinary should be decoded

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR addresses a bug in LocalStack's Secrets Manager service, fixing the decoding of SecretBinary values to align with AWS behavior.

  • Modified localstack-core/localstack/services/secretsmanager/provider.py to add decode_secret_binary_from_response function for conditional decoding
  • Updated get_secret_value method in the same file to use the new decoding function
  • Added new test case test_get_secret_value in tests/aws/services/secretsmanager/test_secretsmanager.py to verify correct encoding/decoding
  • Updated snapshot and validation files to include the new test case

4 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings

@@ -1,10 +1,11 @@
from __future__ import annotations

import base64
Copy link

Choose a reason for hiding this comment

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

style: Consider using from base64 import b64decode for a more specific import


secret_arn = response["ARN"]

secret_value_response = aws_client.secretsmanager.get_secret_value(SecretId=secret_arn)
Copy link

Choose a reason for hiding this comment

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

style: Consider using a constant or configuration value for the secret string instead of hardcoding 'footest'

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.

2 participants