ResourceModules icon indicating copy to clipboard operation
ResourceModules copied to clipboard

[Bug Report]: Container Instance imageRegistryCredentials is not secure

Open mrsmitty opened this issue 2 years ago • 1 comments

Describe the bug

The imageRegistryCredentials can contain image registry username and password credentials. required to pull images from private registries.

The parameter is defined as an array which does not support the @secure attribute. On deployment, this leaks credentials into Azure Deployment Logs.

Appreciate changing this to object would break the ability to provide multiple credentials however, I feel this is a better position than leaking credentials into logs.

To reproduce

Populate the following parameter with the credentials of a private registry e.g. Azure Container Registry Admin Credentials:

https://github.com/Azure/ResourceModules/blob/6ef80718928c52115cdd407d9220acae1f7b370f/modules/container-instance/container-groups/main.bicep#L29

Code snippet

imageRegistryCredentials: [
  {
    password: kv.getSecret(acrPasswordSecretName)
    server: acrUrl
    username: acrUsername
  }
]


### Relevant log output

_No response_

mrsmitty avatar Jun 06 '23 03:06 mrsmitty

Given we can use @Secure for objects and strings, turning this into an object shouldn't break the ability to provide multiple credentials.

How about:

@secure()
param secureImageRegistryCredentials object
var imageRegistryCredentials = secureImageRegistryCredentials.myArray

Usage like:

secureImageRegistryCredentials: {
      myArray: [
        {
          server: 'acrUrl1',
          username: 'myUsername1',
          password: kv.getSecret(acrPasswordSecretName)
        },
        {
          server: 'acrUrl2',
          username: 'myUsername2',
          password: kv.getSecret(acrPasswordSecretName)
        }
      ]
    }

Ricky-G avatar Jul 31 '23 10:07 Ricky-G

Hey @mrsmitty & @Ricky-G, it has been a while, but the module was migrated to AVM since and the change was actually implemented using User Defined Types: https://github.com/Azure/bicep-registry-modules/blob/4116228208bd6d1f2cc21fa0fe1f82eaddd05c9f/avm/res/container-instance/container-group/main.bicep#L360-L376

@Ricky-G what you suggested is actually something we had implemented as a workaround in diverse modules. So especially before UDTs this was a good suggestion 💪

AlexanderSehr avatar Jun 14 '24 12:06 AlexanderSehr