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

Created on 28 Jul 2020  路  5Comments  路  Source: dotnet/aspnetcore

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

image

Further technical details

  • ASP.NET Core version : both 3.1.x and 5.0-preview6
  • Include the output of dotnet --info
affected-very-few area-servers bug severity-minor

Most helpful comment

At a minimum we should log a warning.

All 5 comments

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:

https://github.com/dotnet/aspnetcore/blob/4aa5e03207a005d7310ba792c76f5339d74c3364/src/Servers/Kestrel/Core/src/Internal/AddressBinder.cs#L135-L139

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

BindingAddress.Parse producing a port 443

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

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

https://github.com/dotnet/aspnetcore/blob/4aa5e03207a005d7310ba792c76f5339d74c3364/src/Http/Http/src/BindingAddress.cs#L131-L138

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鈥檛 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鈥檛 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鈥檛 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.

At a minimum we should log a warning.

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"

Was this page helpful?
0 / 5 - 0 ratings