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

feat: add ssl support using custom key/cert files #933

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dahifi
Copy link
Contributor

@dahifi dahifi commented Apr 23, 2024

Someone on the discord asked for a way to run the app behind https, so I added a run config parameters to accept keyfile and cert paths, and pass them to uvicorn.

I was unable to run commit hooks on this, nor did I test, but putting this here to see if anyone can validate or pick it up from here.

@puppetm4st3r
Copy link

puppetm4st3r commented Apr 27, 2024

@dahifi if I add those changes to my local, I it will work? or is incomplete ? the added parameters must be provided as environment vars or running paremeters in chainlit executable ?

Best regards!

@tpatel
Copy link
Collaborator

tpatel commented Apr 30, 2024

Thanks for your PR, there is a similar one here: #905

Do you have more background as to why folks who want to create a publicly accessible Chainlit instance without having a proxy in front of it?

@puppetm4st3r
Copy link

puppetm4st3r commented Apr 30, 2024

in my case the reverse proxy + chainlit + serving wasm artifacts didnt liked it to iOS devices, so I have solved appliying this PR and removing the nginx reverse proxy for serving the ssl and works fine.... and its a clearer solution for lite infrastructure use cases.

@veeragoni
Copy link

Thanks for your PR, there is a similar one here: #905

Do you have more background as to why folks who want to create a publicly accessible Chainlit instance without having a proxy in front of it?

localhost

Copy link
Collaborator

@tpatel tpatel left a comment

Choose a reason for hiding this comment

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

Looks good, could you add the CHAINLIT_ prefix to env variables?
I've tried to do it myself but I don't have the rights on your fork.

backend/chainlit/cli/__init__.py Outdated Show resolved Hide resolved
@click.option(
"--ssl-cert",
default=None,
envvar="SSL_CERT",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
envvar="SSL_CERT",
envvar="CHAINLIT_SSL_CERT",

backend/chainlit/cli/__init__.py Outdated Show resolved Hide resolved
Comment on lines +156 to +157
os.environ["SSL_CERT"] = ssl_cert
os.environ["SSL_KEY"] = ssl_key
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
os.environ["SSL_CERT"] = ssl_cert
os.environ["SSL_KEY"] = ssl_key
os.environ["CHAINLIT_SSL_CERT"] = ssl_cert
os.environ["CHAINLIT_SSL_KEY"] = ssl_key

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dahifi can you update these so we can merge? Nice work!

dahifi and others added 2 commits May 23, 2024 13:46
Co-authored-by: Thibaut Patel <thibaut.patel@gmail.com>
Co-authored-by: Thibaut Patel <thibaut.patel@gmail.com>
@tpatel tpatel self-requested a review May 28, 2024 08:14
Copy link
Collaborator

@tpatel tpatel left a comment

Choose a reason for hiding this comment

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

There are two remaining suggestions that needs to be committed for me to merge this PR.

@yiouyou
Copy link

yiouyou commented May 28, 2024

@dahifi Same request, thanks man, good job. Waiting for the merge.

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.

None yet

6 participants