Kestrel port binding default to 80 or 443 / No error/warn/info/debug when "{abc}" provided

[tebeco]

Hello 👋

Describe the bug

Invalid Http/Https binding can be defaulted without any log and go on

After few long minutes (hours?) of debugging we found out that invalid port configuration will not say anything and silent.
Why did it takes so long ?
Because we were looking for the https binding that was https://*:8443 on our POD, and it seems that another appsettings...json file still had an http://*:{place.holder} that was not replaced
So it seems that it defaulted to port 80 without saying anything at all.

The behavior was AccessDeniedException on a Socket binding attempt (because Pods should not run as root so any binding bellow 1024 is forbidden)

Questions

so I wonder about 3 things now :

• should the SocketException / AccessDenied be explicit on the full binding url that was denied ? => http://[::]:80 or http://*:80`
• should there be at least a WARNING on the fact that a string with letter and placerhoder is probably not a valid URI / Port ?
• is there any form of URI that would makes this valid: http://*:{foo.bar.kix} with the port 80 ?

To Reproduce

dotnet new webapi
dotnet publish
cd ./bin/Debug/net5.0/publish/
$env:ASPNETCORE_URLS="https://*:{abc}"
./foo.exe

Further technical details

• ASP.NET Core version : both 3.1.x and 5.0-preview6
• Include the output of dotnet --info

[poke]

This appears to be a problem with BindingAdress.Parse which gets called early in the AddressBinder. Since the binder is not able to produce a better result from the input, it will eventually fall back to just binding on the port that the BindingAdress contains:

aspnetcore/src/Servers/Kestrel/Core/src/Internal/AddressBinder.cs

Lines 135 to 139 in 4aa5e03

	else
	{
	// when address is 'http://hostname:port', 'http://*:port', or 'http://+:port'
	options = new AnyIPListenOptions(parsedAddress.Port);
	}

Unfortunately, BindingAddress.Parse does not appear to parse the input very well:

Calling BindingAddress.Parse("https://*:{abc}") will recognize *:{abc} as the host and assume the default HTTPS port 443.

[poke]

Digging a bit deeper, it seems that this part of BindingAddress.Parse is responsible:

aspnetcore/src/Http/Http/src/BindingAddress.cs

Lines 131 to 138 in 4aa5e03

	string portString = address.Substring(portDelimiterEnd, pathDelimiterStart - portDelimiterEnd);
	int portNumber;
	if (int.TryParse(portString, NumberStyles.Integer, CultureInfo.InvariantCulture, out portNumber))
	{
	hasSpecifiedPort = true;
	serverAddress.Host = address.Substring(schemeDelimiterEnd, portDelimiterStart - schemeDelimiterEnd);
	serverAddress.Port = portNumber;
	}

portString is correctly determined as "{abc}" but then there is an attempt to parse it which will obviously fail. Since there is no port, the default scheme port will be used and the :{abc} will be understood as part of the host.

To be honest, I don’t really understand why the logic was done like this. I would expect the parsing to fail when the identified portString does not end up being a number. After all, it existing means that there is a colon before, so the original string is a:b where b is not a number. I only see two options where a colon would be valid as part of the authority of an URI though:

• As part of the user info, although this would require an @ before the host name. And for the address binder, this wouldn’t really make any sense.
• Or for IPv6 addresses. Although then the part after the colon would be required to be a hexadecimal number, so curly braces definitely wouldn’t be allowed.

While supporting IPv6 addresses is obviously required in some way, I would expect the parsing to actually fail with some kind of error instead of blindly accepting the :{abc} here. Maybe we can fine tune this just a little bit so inputs like these would throw an error.

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[BrennanConroy]

At a minimum we should log a warning.

[tebeco]

yeah as you said Warning would be the minimal ;)

I honestly think this should throw (would it be considered a breaking change given the fact that it maybe should have thrown from day 0 ?)
especially since this is an explicit value being realllly wrong and not "a defaulted" value, like "nothing was supplied so we defaulted"

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.