adding param of options to config of ssl_context

[aswanidutt87]

This is related to issue #806 , we are required to resolve a vulnerability of DoS by adding the option of ssl.OP_NO_RENEGOTIATION to the ssl_context.
Currently there is no provision of adding any options to the config of ssl_context.
we have added the options list of ssl.Options type to the main.py and there by to the config.py.
Added unit tests - coverage - total as of this change - 97.69%, report attached, checks, lint ran fine.

[aswanidutt87]

uvicorn_test-coverage.docx

[euri10]

This is related to issue #806 , we are required to resolve a vulnerability of DoS by adding the option of ssl.OP_NO_RENEGOTIATION to the ssl_context. Currently there is no provision of adding any options to the config of ssl_context. we have added the options list of ssl.Options type to the main.py and there by to the config.py. Added unit tests - coverage - total as of this change - 97.69%, report attached, checks, lint ran fine.

what vulnerability

[Kludex]

This is related to issue #806 , we are required to resolve a vulnerability of DoS by adding the option of ssl.OP_NO_RENEGOTIATION to the ssl_context. Currently there is no provision of adding any options to the config of ssl_context. we have added the options list of ssl.Options type to the main.py and there by to the config.py. Added unit tests - coverage - total as of this change - 97.69%, report attached, checks, lint ran fine.

what vulnerability

Check your mailbox. 🙏

I've forwarded your email @aswanidutt87 , jfyk.

[Kludex]

Suggested change

	if config.ssl_options is not None:
	assert ssl.OP_NO_RENEGOTIATION in config.ssl_options
	assert ssl.OP_NO_TLSv1_2 in config.ssl_options
	assert ssl.OP_NO_RENEGOTIATION in config.ssl_options
	assert ssl.OP_NO_TLSv1_2 in config.ssl_options

[aswanidutt87]

if I dont check for None on the Optional[ssl.Options], getting mypy errors like "unsupported right operand type for in optional" as by default they are Optional list of ssl.Options type

[Kludex]

Shouldn't we check the config.ssl created instead?

Suggested change

	config = Config(
	app=asgi_app,
	)
	config.load()
	assert not config.ssl_options
	config = Config(app=asgi_app)
	config.load()
	assert not config.ssl_options

[aswanidutt87]

agreed, removing this test as checking for config.ssl is True assertion test is already there- coverage is good.

[Kludex]

This should probably be inside the create_ssl_context function.

[aswanidutt87]

agreed moved to create_ssl_context.

[djds]

This is slightly out of date now in terms of some of the API calls, but I think it solves a similar issue: https://github.com/OpenCTI-Platform/client-python/blob/14cbac14afe76b3003baa5fff56c72af9eb17935/pycti/connector/opencti_connector_helper.py#L77

[Kludex]

Those are integers, right? Can we point out the options available here as well?

Suggested change

	help="Options to set on ssl context",
	help="Options to set on ssl context.",

[aswanidutt87]

added the file and Enum class to look for

[aswanidutt87]

@Kludex , please have a look at the changes made per your review comments, we need this change to resolve a Vulnerability

[Kludex]

I'll take time to review this. I need to study our options, because I know there's an issue open to add the SSL context via uvicorn.run.

we need this change to resolve a Vulnerability

I think it was explained on Gitter why this is not a vulnerability on our side, but it was also agreed this PR makes sense.

[Kludex]

@aswanidutt87 What do you think about doing what #806 suggests instead? i.e. passing the SSL context via uvicorn programmatically.

[aswanidutt87]

@aswanidutt87 What do you think about doing what #806 suggests instead? i.e. passing the SSL context via uvicorn programmatically.

@Kludex there is no option of adding ssl context to uvicorn as of now, both passing SSL context or SSL options works here, I would leave it to you which one you prefer to include. if we go with the route of adding SSL context then the usage of uvicorn should be as follows
uvicorn.run(
"web:app",
host="0.0.0.0",
port=int(port),
reload=True,
ssl_context=SSL_Context
)
if we go with adding just the ssl_options, run looks like this. and this is what we are doing currently in our case as interim fix.
uvicorn.run(
"web:app",
host="0.0.0.0",
port=int(port),
reload=True,
ssl_keyfile=tls_key,
ssl_certfile=tls_cert,
ssl_version=int(ssl.PROTOCOL_TLS),
ssl_ciphers=allowed_ciphers,
ssl_options=list_options
)

just to make it to work for every scenario in future adding a ssl_context looks more useful to be honest. we can use config.py to create our own ssl_context and we can pass it into uvicorn.run

[Kludex]

PR welcome for what #806 suggests. 🙏

I'll be closing this, as the future solution that solves #806 will support this case as well.

[aswanidutt87]

@Kludex, As we have tried the route with your suggestion couldnt get the custom ssl_context to pass into the server, we want to request you to kindly consider this adding of options to existing context.
another reason to support this is that we have already added everything that we can to context already with previous PRs, its just that the options are missing, if we can add this options to the default context getting created we should be all set.

[Kludex]

@Kludex, As we have tried the route with your suggestion couldnt get the custom ssl_context to pass into the server, we want to request you to kindly consider this adding of options to existing context.

That's not how I see it. I've replied to your last message here, and you didn't reply.

Please, do not send me emails regarding issues or PRs. I read all comments on this repository.