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

Problem: http server doesn't support HTTPS #405

Open
yrashk opened this issue Dec 20, 2023 · 9 comments
Open

Problem: http server doesn't support HTTPS #405

yrashk opened this issue Dec 20, 2023 · 9 comments
Labels
bounty Bounty $250 Bountry: $250

Comments

@yrashk
Copy link
Contributor

yrashk commented Dec 20, 2023

This is not because H2O (the HTTP server we use) doesn't (it does); it's because we never added this configuration knob.

However, this is an important step for running the entire stack without additional tools.

Proposed solution: add a way to configure certificates

A grand vision here was (and still is) to implement omni_pki, a comprehensive extension for managing keys and certificates (generate CSR, store keys, certificates, etc.). It would be essentially a wrapper around some of OpenSSL's functionality. But perhaps this can wait a bit. It is, however, helpful to know the original intention so that at least we know where this can go and be advised by it.

The grand vision notwithstanding, we need to figure out a short path to this configurability. Some options:

  1. The omni_httpd.certificates (name TBD) table would list actual certificates, and how we match them to listeners can be very interesting: chances are, we can match a certificate by SNI (and maybe listener ID?), making it a super-easy setup. My reading of h2o suggests that's quite possible.
  2. Similar to above, but the resolution of the certificate would done through a stored function. This will likely be slower as it involves communication with the database thread. Might not make sense given we want to make it performant.
  3. If the grand vision is the one that makes the most sense, let's talk [TODO: write down thoughts on that].

NB: Before implementing this bounty, please provide a rough plan for review to ensure your effort will be most fruitful.

Not in scope for this bounty:

  • ACME/LetsEncrypt

Other comments:

Eventually, much of omni_httpd functionality will be extracted into omni_workers, but this will still be useful as the HTTP-specific block will remain.


Bounty size: $250

Read more about bounties

@bilalshaikh292
Copy link

Heyy @yrashk is the issue still open ? I would like to contribute ?

@yrashk
Copy link
Contributor Author

yrashk commented Dec 23, 2023

@bilalshaikh292 it's open!

@skushagra9
Copy link

Please assign this issue to me ,I would love to contribute

@UkuLoskit
Copy link
Contributor

hey, @yrashk, is there a reasonably easy way to get this extension running in a nix environment with the extensions? when I set up the nix environment, it seemed like some of the extensions like httpd were not included and I noticed that there was a TODO regarding this in the nix files.

The only way I could get a local setup running for now was using Docker by commenting out the Rust part which was pretty slow. Even with these changes the Docker build is quite slow if I want to change some C code (since Docker caching does not work).

@yrashk
Copy link
Contributor Author

yrashk commented Jan 9, 2024

hey, @yrashk, is there a reasonably easy way to get this extension running in a nix environment with the extensions? when I set up the nix environment, it seemed like some of the extensions like httpd were not included and I noticed that there was a TODO regarding this in the nix files.

The only way I could get a local setup running for now was using Docker by commenting out the Rust part which was pretty slow. Even with these changes the Docker build is quite slow if I want to change some C code (since Docker caching does not work).

The Nix bits will likely be deprecated soon as they are not actively maintained.

Building Omnigres as is, without a container, is fairly easy: https://github.com/omnigres/omnigres?tab=readme-ov-file#building--using-extensions

With this process, testing changes takes few seconds.

The Docker images are not meant for the development of Omnigres itself.

@UkuLoskit
Copy link
Contributor

thanks! finally figured it out that I had create the extension after that executing the make target :)

@yrashk
Copy link
Contributor Author

yrashk commented Jan 10, 2024

Indeed, this should be spelled out better. There are subtle reasons why it's not done by default (although we may reconsider them)

@UkuLoskit
Copy link
Contributor

UkuLoskit commented Jan 15, 2024

hey, @yrashk, been making some minor progress with this...
I created a table for certificates that would store the certificate and private key in PEM format, additional to the primary key (id) there would be a listener_id column.

I was attaching the OpenSSL context here to accept_ctx.ssl_ctx

and using this I was able to serve omni_httpd over SSL.

I'm wondering if this is the correct approach?
Also, it seems that the query results are read at later point than the query is executed. could they be read earlier? For attaching the SSL context I would need to read the query results at the point.

One option would be to add another join to the handlers query which would perform a left join with new certificates table on listener_id.

@yrashk
Copy link
Contributor Author

yrashk commented Jan 16, 2024

@UkuLoskit, broadly seems right overall and with respect to the join.

As for reading the results at a later point, I don't think anything is stopping us from reading them twice. The results are effectively in an array anyway (

SPITupleTable *tuptable = SPI_tuptable;
for (int i = 0; i < tuptable->numvals; i++) {
HeapTuple tuple = tuptable->vals[i];
), so perhaps we can read index-matched listener SSL key (if found) when we are setting up the listeners right there as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bounty Bounty $250 Bountry: $250
Projects
None yet
Development

No branches or pull requests

4 participants