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

Set a default Server header for HTTP responses #396

Merged
merged 1 commit into from May 24, 2022
Merged

Set a default Server header for HTTP responses #396

merged 1 commit into from May 24, 2022

Conversation

baseplate-admin
Copy link
Contributor

@baseplate-admin baseplate-admin commented Dec 28, 2021

Fixes : #395

@baseplate-admin
Copy link
Contributor Author

baseplate-admin commented Dec 28, 2021

hi, @adamchainz please take a look

This to my knowledge works perfectly

Copy link
Sponsor Member

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have done a basic review. Please also add a note in CHANGELOG.txt

daphne/http_protocol.py Outdated Show resolved Hide resolved
daphne/http_protocol.py Outdated Show resolved Hide resolved
daphne/http_protocol.py Outdated Show resolved Hide resolved
daphne/http_protocol.py Outdated Show resolved Hide resolved
@baseplate-admin
Copy link
Contributor Author

baseplate-admin commented Dec 28, 2021

In CHANGELOG.txt, Should i create 3.0.3 or write under 3.0.2?

@adamchainz
Copy link
Sponsor Member

No heading, as per: 2b6f153

The release version number is determined at release time.

Version 3.0.2 is already released.

In general, you can check git history on the changelog file to see how a project handles things like this!

Copy link
Sponsor Member

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also like to see a test for the header on HTTP responses.

@baseplate-admin
Copy link
Contributor Author

baseplate-admin commented Dec 28, 2021

hi @adamchainz, The tests should pass now.

Capture

Thank you so much for guiding a newbie like me and Cheers 🍻
Hopefully i can make more contribution to the Django project

Also a silly question. Does Daphne have a official logo or is there any plan to add one?

daphne/http_protocol.py Outdated Show resolved Hide resolved
daphne/http_protocol.py Outdated Show resolved Hide resolved
tests/test_http_response.py Outdated Show resolved Hide resolved
tests/test_http_response.py Outdated Show resolved Hide resolved
CHANGELOG.txt Outdated Show resolved Hide resolved
daphne/http_protocol.py Show resolved Hide resolved
tests/test_http_response.py Outdated Show resolved Hide resolved
@baseplate-admin
Copy link
Contributor Author

baseplate-admin commented Dec 28, 2021

Everything done 😃 Can we merge this now ?

Also @adamchainz, Thank you very much for helping me out here.

@adamchainz
Copy link
Sponsor Member

I will let @carltongibson make the final review.

@adamchainz adamchainz changed the title Update http_protocol.py Set Server header for HTTP responses Dec 28, 2021
@baseplate-admin
Copy link
Contributor Author

Sure. 😃

@baseplate-admin baseplate-admin changed the title Set Server header for HTTP responses Set a default Server header for HTTP responses Dec 29, 2021
Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check being dropped here was added deliberately in #231. So (minor as it is) it's a breaking change and I want to be clear if we're going to proceed.

Andrew's reasoning in #231 was that Server is not a required (HTTP protocol) header. Autobahn certainly uses it for the WebSocket connection handshake but is that required, I'm not sure. 🤔

I share @adamchainz concern about setting Server headers at all really. ... If we do proceed here, do we have a story for folks not wanting to set that, other than filter it later on? — Why is the response not simply Add a Server header if you want it?

🤔

@baseplate-admin

This comment has been minimized.

@adamchainz
Copy link
Sponsor Member

Please check the Issue. Every other web server has a default header that users can override.

Carlton's point is that he doesn't find "other servers do it" a good enough reason.

I can see both sides. Sure, we already send it for websockets, so why not for HTTP responses? And it does help the openness of the web, slightly.

Then again, it makes driveby vulnerability scans much easier. We'd rather have a little protection through obscurity if a Daphne/Twisted/whatever exploit is discovered.

Perhaps the better approach is actually the opposite: drop Server from websocket connections?

The check being dropped here was added deliberately in #231. So (minor as it is) it's a breaking change and I want to be clear if we're going to proceed.

(I don't think this makes it a breaking change - afaict the check was added as a weird way of using "Daphne" a default that meant "don't send a server header".)

@baseplate-admin
Copy link
Contributor Author

baseplate-admin commented Dec 29, 2021

Carlton's point is that he doesn't find "other servers do it" a good enough reason.

Fair enough.

Perhaps the better approach is actually the opposite: drop Server from websocket connections?

If thats the goal (having no server header) just run

daphne -b 0.0.0.0 -p 8000 core.asgi:application --server-name=

This completely removes the server header addition as per #231.

While i think that this is not a good implementation. As there should be another option eg: --no-server-header for developers to remove the server header, feel free to disagree.

Then again, it makes driveby vulnerability scans much easier.

I agree with you on this.

We'd rather have a little protection through obscurity if a Daphne/Twisted/whatever exploit is discovered.

Let users developers decide if they wanna implement it. Currently we can mimic any server name in the world. If developers want security through obscurity, they can achieve it via :

daphne -b 0.0.0.0 -p 8000 core.asgi:application --server-name=gunicorn

(which will make daphne indistinguishable from gunicorn)

Or if they want to take it to anther level and doesnt wanna show their backend language they can do :

daphne -b 0.0.0.0 -p 8000 core.asgi:application --server-name=php

to falsify wappalyzer php and gunicorn checks.

Some images to back my claims:

Gunicorn
PHP

@baseplate-admin
Copy link
Contributor Author

baseplate-admin commented Feb 11, 2022

Umm, @carltongibson sorry to ping you.

But please tell me, is there anything i need to do for this Pull Request to get merged?

@carltongibson
Copy link
Member

@baseplate-admin Sorry for the delay — I've had something of a month™.

Yes, let's try and get this moving.

Reading your last comment, did you prefer the --no-server-name flag or --server-name= to disable? (I don't really mind… happy to go with either really)

Thanks!

@baseplate-admin
Copy link
Contributor Author

baseplate-admin commented Feb 11, 2022

Sorry for the delay — I've had something of a month™.

No problem :D
Sorry if I disturbed you in a busy day.

Reading your last comment, did you prefer the --no-server-name flag or --server-name= to disable? (I don't really mind… happy to go with either really)

I prefer --no-server-name.

For now if you set --server-name= when you call the main function.

Eg :

daphne -b 0.0.0.0 -p 8000 { asgi_app } --server-name=

It will remove the server header altogether. ( For this no extra work needed here | Current Daphne release can do this fine. )

Heres a picture proving this point :

1

In my opinion it should be better if we passed something like --no-server-name ( for production servers that are running Daphne without reverse-proxy and that want to disable this default server setting ) it would be better.

Because it makes the developer aware that they wanna disable a feature, instead of silently disabling it and also because almost 0 production use Daphne directly instead of relying on a reverse proxy. ( For this some work is needed )

Eg :

daphne -b 0.0.0.0 -p 8000 { asgi_app } --no-server-name

Which in return should produce the same result as :

daphne -b 0.0.0.0 -p 8000 { asgi_app } --server-name=

Now I have a small question. ( Sorry if this sounds stupid )

Edit :

@carltongibson
Copy link
Member

I think it's all one issue. If you're happy to add the extra flag, please do so here.

@baseplate-admin
Copy link
Contributor Author

baseplate-admin commented Feb 11, 2022

Hey @carltongibson, I have made the changes. Please take a look.

Changes that I have made :

  • Added some tests that references Configurable servername #231 changes.

  • Added --no-server-name.

  • Added tests that references --no-server-name

  • Added 2 changes on CHANGELOG.txt

  • Daphnedaphne in here because of this and other ASGI/WSGI servers follow the same naming system. ( Check #395 for images )

Some Proof of my work ( I ran all of them on this repo ):

Default Daphne ( No Flags ) :


Console output :

PS C:\Programming\Blog-main> pipenv run daphne core.asgi:application
2022-02-12 00:41:10,622 INFO     Starting server at tcp:port=8000:interface=127.0.0.1
2022-02-12 00:41:10,622 INFO     HTTP/2 support not enabled (install the http2 and tls Twisted extras)
2022-02-12 00:41:10,623 INFO     Configuring endpoint tcp:port=8000:interface=127.0.0.1
2022-02-12 00:41:10,624 INFO     Listening on TCP address 127.0.0.1:8000
127.0.0.1:61351 - - [12/Feb/2022:00:41:16] "GET /blog/" 200 3458
127.0.0.1:61351 - - [12/Feb/2022:00:41:16] "GET /static/core/css/root.css" 200 144
127.0.0.1:61355 - - [12/Feb/2022:00:41:16] "GET /static/core/css/main.css" 200 181
127.0.0.1:61356 - - [12/Feb/2022:00:41:16] "GET /static/blog/css/blog_list.css" 200 552
127.0.0.1:61357 - - [12/Feb/2022:00:41:16] "GET /static/core/css/navbar/navbar.css" 200 544
127.0.0.1:61358 - - [12/Feb/2022:00:41:16] "GET /static/core/css/footer/footer.css" 200 272
127.0.0.1:61359 - - [12/Feb/2022:00:41:16] "GET /static/blog/js/shapeDividerShadowDom.js" 200 529
127.0.0.1:61351 - - [12/Feb/2022:00:41:16] "GET /static/core/images/logo.png" 200 11449
127.0.0.1:61351 - - [12/Feb/2022:00:41:16] "GET /static/core/vendor/particles.js/particlesjs-config.json" 200 577
127.0.0.1:61356 - - [12/Feb/2022:00:41:16] "GET /static/core/vendor/particles.js/particlesjs-config.json" 200 577

A picture :

default

--no-server-name :


Console output :

PS C:\Programming\Blog-main> pipenv run daphne core.asgi:application --no-server-name
2022-02-12 00:45:59,029 INFO     Starting server at tcp:port=8000:interface=127.0.0.1
2022-02-12 00:45:59,030 INFO     HTTP/2 support not enabled (install the http2 and tls Twisted extras)
2022-02-12 00:45:59,030 INFO     Configuring endpoint tcp:port=8000:interface=127.0.0.1
2022-02-12 00:45:59,031 INFO     Listening on TCP address 127.0.0.1:8000
127.0.0.1:62208 - - [12/Feb/2022:00:46:00] "GET /blog/" 200 3458
127.0.0.1:62208 - - [12/Feb/2022:00:46:00] "GET /static/core/css/root.css" 200 144
127.0.0.1:62213 - - [12/Feb/2022:00:46:00] "GET /static/core/css/main.css" 200 181
127.0.0.1:62298 - - [12/Feb/2022:00:46:00] "GET /static/blog/css/blog_list.css" 200 552
127.0.0.1:62299 - - [12/Feb/2022:00:46:00] "GET /static/core/css/navbar/navbar.css" 200 544
127.0.0.1:62300 - - [12/Feb/2022:00:46:00] "GET /static/core/css/footer/footer.css" 200 272
127.0.0.1:62301 - - [12/Feb/2022:00:46:00] "GET /static/blog/js/shapeDividerShadowDom.js" 200 529
127.0.0.1:62208 - - [12/Feb/2022:00:46:00] "GET /static/core/images/logo.png" 200 11449
127.0.0.1:62302 - - [12/Feb/2022:00:46:00] "GET /static/blog/js/shapeDividerShadowDom.js" 200 529
127.0.0.1:62208 - - [12/Feb/2022:00:46:00] "GET /static/core/vendor/particles.js/particlesjs-config.json" 200 577
127.0.0.1:62298 - - [12/Feb/2022:00:46:00] "GET /static/core/vendor/particles.js/particlesjs-config.json" 200 577

A picture :

--no-server-name

--server-name=php :


Console output :

PS C:\Programming\Blog-main> pipenv run daphne core.asgi:application --server-name=php
2022-02-12 00:47:24,991 INFO     Starting server at tcp:port=8000:interface=127.0.0.1
2022-02-12 00:47:24,992 INFO     HTTP/2 support not enabled (install the http2 and tls Twisted extras)
2022-02-12 00:47:24,993 INFO     Configuring endpoint tcp:port=8000:interface=127.0.0.1
2022-02-12 00:47:24,994 INFO     Listening on TCP address 127.0.0.1:8000
127.0.0.1:62500 - - [12/Feb/2022:00:47:26] "GET /blog/" 200 3458
127.0.0.1:62500 - - [12/Feb/2022:00:47:26] "GET /static/core/css/root.css" 200 144
127.0.0.1:62506 - - [12/Feb/2022:00:47:26] "GET /static/core/css/main.css" 200 181
127.0.0.1:62507 - - [12/Feb/2022:00:47:26] "GET /static/blog/css/blog_list.css" 200 552
127.0.0.1:62508 - - [12/Feb/2022:00:47:26] "GET /static/core/css/navbar/navbar.css" 200 544
127.0.0.1:62509 - - [12/Feb/2022:00:47:26] "GET /static/core/css/footer/footer.css" 200 272
127.0.0.1:62510 - - [12/Feb/2022:00:47:26] "GET /static/blog/js/shapeDividerShadowDom.js" 200 529
127.0.0.1:62500 - - [12/Feb/2022:00:47:26] "GET /static/core/images/logo.png" 200 11449
127.0.0.1:62500 - - [12/Feb/2022:00:47:26] "GET /static/core/vendor/particles.js/particlesjs-config.json" 200 577
127.0.0.1:62500 - - [12/Feb/2022:00:47:26] "GET /static/core/vendor/particles.js/particlesjs-config.json" 200 577

A picture :

server=php

The test Result :

Picture :

Capture

Console Output :
PS C:\Programming\daphne> pipenv run test
===================================================================================================== test session starts =====================================================================================================
platform win32 -- Python 3.10.2, pytest-7.0.1, pluggy-1.0.0
rootdir: C:\Programming\daphne, configfile: setup.cfg, testpaths: tests
plugins: hypothesis-6.36.1, asyncio-0.18.1
asyncio: mode=legacy
collected 59 items

tests\test_cli.py ..............                                                                                                                                                                                         [ 23%]
tests\test_http_request.py ..............                                                                                                                                                                                [ 47%]
tests\test_http_response.py .........                                                                                                                                                                                    [ 62%] 
tests\test_utils.py ............                                                                                                                                                                                         [ 83%] 
tests\test_websocket.py ..........                                                                                                                                                                                       [100%] 

====================================================================================================== warnings summary ======================================================================================================= 
..\..\Users\.....\.virtualenvs\daphne-qhnhUZrd\lib\site-packages\pytest_asyncio\plugin.py:191
  C:\Users\.....\.virtualenvs\daphne-qhnhUZrd\lib\site-packages\pytest_asyncio\plugin.py:191: DeprecationWarning: The 'asyncio_mode' default value will change to 'strict' in future, please explicitly use 'asyncio_mode=strict' or 'asyncio_mode=auto' in pytest configuration file.
    config.issue_config_time_warning(LEGACY_MODE, stacklevel=2)

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
========================================================================================== 59 passed, 1 warning in 66.77s (0:01:06) =========================================================================================== 

Please tell me if i missed anything.

@baseplate-admin baseplate-admin changed the title Set a default Server header for HTTP responses Set a default Server header for HTTP responses Feb 12, 2022
@TanimSk
Copy link

TanimSk commented Feb 12, 2022

@baseplate-admin setting a default Server header for HTTP responses is a good approach
nice work!

@baseplate-admin
Copy link
Contributor Author

Just giving this pull request a little bump.

@carltongibson
Copy link
Member

Thanks @baseplate-admin. It's on the list.

My aim is to swing back to channels &co once Django 4.1a1 is out of the door.

Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @baseplate-admin — I rebased and pushed small edits.

Welcome aboard! ⛵

@carltongibson carltongibson merged commit 54745d0 into django:main May 24, 2022
@baseplate-admin
Copy link
Contributor Author

Thank you @carltongibson

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Daphne sets server='daphne' header for websocket connections, but not for http-requests
5 participants