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

tebeco commented 4 years ago

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 commented 4 years ago

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.

poke commented 4 years ago

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’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.

ghost commented 4 years ago

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 commented 4 years ago

At a minimum we should log a warning.

tebeco commented 4 years ago

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"

ghost commented 3 years ago

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.

ghost commented 2 years ago

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

Tratcher commented 2 years ago

Hit this again. We should reconsider making this scenario throw, we're not binding to what the user said they wanted.

tebeco commented 2 years ago

yep, i guess anybody would expect to throw on that

i remember when opening this issue that i wondered if i could PR that during net5. IIRC the code seemed rather specific than just a int.TryParse and i wasn't sure about the implications

Tratcher commented 2 years ago

Yeah, I think we'd add an else clause there that threw a more specific exception.

dotnet / aspnetcore