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

Azure naming conventions and input sanitization - issues with names derived from context names #2334

Open
5 tasks done
craddm opened this issue Dec 9, 2024 · 2 comments
Open
5 tasks done
Labels
bug Problem when deploying a Data Safe Haven.

Comments

@craddm
Copy link
Contributor

craddm commented Dec 9, 2024

✅ Checklist

  • I have searched open and closed issues for duplicates.
  • This is a problem observed when deploying a Data Safe Haven.
  • I can reproduce this with the latest version.
  • I have read through the documentation.
  • This isn't an open-ended question (open a discussion if it is).

💻 System information

  • Operating System: Debian Bookworm
  • Data Safe Haven version: develop

📦 Packages

List of packages
Paste list of packages here

🚫 Describe the problem

There are several places where our approach to sanitization is inconsistent.

Context names are allowed to use letters (upper and lower case), numbers, hyphens, and underscores.

Storage account names cannot use uppercase letters, underscores, or hyphens.
When creating the SHM storage account, we strip the underscores and hyphens, but leave uppercase letters alone.
Creating the storage account then fails, as the name is invalid, but our error message simply says that creating the account failed without explaining why.

We don't sanitise the name at all when creating the key vault for the account, which means that fails if there are any underscores in the context name.
Again, the error message does not point in the right direction.

The most straightforward approach is to allow only lowercase letters and numbers (possibly) for context names (i.e. only characters valid for both storage account and key vault names).

🌳 Log messages

Relevant log messages
Ensured that resource group shm-MATT_CONTEXT_DEV-rg exists in uksouth.                                                                                                                                
Ensured that managed identity shm-MATT_CONTEXT_DEV-identity-reader exists.                                                                                                                            
Failed to create storage account shmMATTCONTEXTDEV.  
Failed to create key vault shm-matt_context_dev-kv. Check if a key vault with the same name already exists in a deleted state.  

♻️ To reproduce

Use a variety of different character types in context names.

@craddm craddm added the bug Problem when deploying a Data Safe Haven. label Dec 9, 2024
@jemrobinson
Copy link
Member

Sounds like a nice place for a unit test :)

@JimMadge
Copy link
Member

JimMadge commented Dec 9, 2024

Whether or not we change what an allowed context name is, I think we should also sanitise the names being used to create Azure resources. It would be good if those functions can take arbitrary strings and handle them appropriately (or at least raise/handle the exception if they are invalid).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Problem when deploying a Data Safe Haven.
Projects
None yet
Development

No branches or pull requests

3 participants