Usernames and passwords in connection strings aren't escaped

[DamianEdwards]

While investigating #2247 I used a parameter to create a stable password for a RabbitMQ resource, only to find that the password I'd generated contained a character that broke the connection string. RabbitMQ connection strings are actually URIs, so if the username or password contains restricted characters (e.g. @) then the RabbitMQ client fails due to the URI being invalid.

I updated ReferenceExpression to support custom value escaping and a convenience method for getting a URI escaped version of an existing ReferenceExpression and that solves the issue for runtime, but this issue will occur during manifest processing too. Resolving that is a much bigger challenge that will need some thought.

[eerhardt]

I think in order to be able to solve this generically, and in all the places we need to fix it, we will need to:

1. Have a set of "well known" escape strategies/algorithms.
2. Encode which escape strategy to use inside the ReferenceExpression, so it gets written to the manifest.

RabbitMQ uses URIs, but PostgreSQL uses DbConnectionString. To properly escape any string in a DbConnectionString, we would need to add single quotes ' around the value, and then escape any real single quotes in the value by doubling them: ''.

So at a minimum we would need 2 strategies: URI escaping and single-quote escaping.

As you pointed out above, we would need to put this in the manifest and have any deployment tools (like azd) understand these strategies.

cc @davidfowl @mitchdenny @ellismg @vhvb1989

[DamianEdwards]

So we'd represent this in the manifest something like this?
"amqp://{urlencode(resources.rabbitusr.value)}:{urlencode(resources.rabbitpw.value)}@{resources.rabbitmq.bindings.tcp.host}:{resources.rabbitmq.bindings.tcp.port}"

[eerhardt]

So we'd represent this in the manifest something like this? "amqp://{urlencode(resources.rabbitusr.value)}:{urlencode(resources.rabbitpw.value)}@{resources.rabbitmq.bindings.tcp.host}:{resources.rabbitmq.bindings.tcp.port}"

Yep - something like that is what I was thinking.

[DamianEdwards]

@davidfowl we thinking punt this until post-GA now? Or do we think this will be common enough when considering values in URIs and connection strings that we need to mitigate it earlier?

[davidfowl]

yes I think we can do something specifically for URLs and connection strings in azd without putting functions in the manifest (work needs to happen in azd anyways). I’m thinking for now, they can detect a url and do this magic encoding.

[DamianEdwards]

So you're thinking we have AZD recognize URIs and encode themselves for GA?

[eerhardt]

we have AZD recognize URIs and encode themselves for GA?

And connection strings. (at least that's what I understand from the above)

[davidfowl]

So you're thinking we have AZD recognize URIs and encode themselves for GA?

Yes, that's how we address this for GA.

And connection strings. (at least that's what I understand from the above)

Yes

[DamianEdwards]

OK I've put in preview.6 and opened Azure/azure-dev#3598

[oskardudycz]

@DamianEdwards I think that Postgres containers have a similar issue.

[mitchdenny]

Now that we are past GA ... how do we want to handle this?

[joperezr]

@DamianEdwards is this something we want to fix for 8.1?

[DamianEdwards]

@joperezr if we have a ready-to-go plan to fix it at both runtime and deployment time (i.e. in AZD and Aspir8) otherwise I'd move it out.

[John-Leitch]

@joperezr, I'm willing to make these changes to Aspir8. I recently made significant changes to adjacent functionality (reference parsing/evaluation), and expect this to be easy. I'd also be more than happy to add function evaluation if that's the direction things go.