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

redbean: add tls socket lua binding #1279

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

chamot1111
Copy link

Add TLS module for Lua using mbedTLS

  • Implements TLS socket creation and management
  • Supports connection, read, write, and close ops
  • Handles memory safely with proper cleanup
  • Provides error handling and status reporting
  • Configurable SSL verification and timeouts

@github-actions github-actions bot added the tool label Sep 4, 2024
@@ -0,0 +1,293 @@
#include "libc/intrin/kprintf.h"
Copy link
Owner

Choose a reason for hiding this comment

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

You can probably remove this line.

I'm also reasonably certain this doesn't need to be a .inc file. LuaFetch() had to be one because it's dug in like a tick with all the global variables defined in redbean.c. But I see that here you're going for a more conventional library API. For example, rather than checking the sslclientverify you're just accepting that as a parameter. That is probably the right thing to do, honestly.

So could you, if possible, make this ltls.c and update tool/net/BUILD.mk appropriately? Be sure to put the standard copyright header at the top.

return 1;
} else {
lua_pushnil(L);
if (ret == MBEDTLS_ERR_SSL_WANT_READ || ret == MBEDTLS_ERR_SSL_WANT_WRITE) {
Copy link
Owner

Choose a reason for hiding this comment

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

Do you really support non-blocking i/o?

Copy link
Author

Choose a reason for hiding this comment

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

No I don't, i remove it

return 1;
}

static const struct luaL_Reg tls_methods[] = {{"connect", tls_connect},
Copy link
Owner

Choose a reason for hiding this comment

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

Please put the elements on their own lines.

state_str = "unknown";
}

lua_pushfstring(L, "TLS connection (%p): %s", tls, state_str);
Copy link
Owner

Choose a reason for hiding this comment

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

With redbean what we like to do is have a __repr method so when an object is printed on the redbean shell, it'll print the code you'd type to create that object. Then the __tostring method will usually be the same thing as __repr. I don't think it's worth showing state here, because the state you're showing is just the internal class state and might not reflect the current status in the OS of the socket.

Copy link
Author

@chamot1111 chamot1111 Sep 9, 2024

Choose a reason for hiding this comment

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

Ok. I will use:

static int tls_tostring(lua_State *L) {
  TlsContext **tlsp = checktls(L);
  TlsContext *tls = *tlsp;

  lua_pushfstring(L, "tls.TlsClient({fd=%d})", tls->fd);
  return 1;
}

static const struct luaL_Reg tls_methods[] = {
    {"write", tls_write},
    {"read", tls_read},
    {"close", tls_close},
    {"__gc", tls_gc},
    {"__tostring", tls_tostring},
    {"__repr", tls_tostring},
    {NULL, NULL}
};

@@ -4982,6 +4982,48 @@ unix = {
X_OK = nil
}

---@class TlsContext
---@field connect fun(self: TlsContext, server_name: string, server_port: string): boolean, string?
Copy link
Owner

Choose a reason for hiding this comment

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

In terms of API design, have you considered this?

tls = require "tls"
conn = tls.TlsClient(ResolveIp("google.com"), 80) -- returns object of TlsClient class
conn.write("GET / HTTP/1.0\r\n\r\n")
print(conn.read())
conn.close()

That would be more similar and composable with other redbean APIs. For example, we like to pass IPs as uint32. You wouldn't need to maintain a state machine with this design. You could have functions or variables associated with the tls module for doing context-wide configuration, like whether or not SSL client verification should be enabled. You could also have that default to the redbean settings.

Another thing you might do that's even better is:

tls = require "tls"
unix = require "unix"

fd = assert(unix.socket(unix.AF_INET, unix.SOCK_STREAM, unix.IPPROTO_IP))
assert(unix.connect(fd, ResolveIp("google.com"), 80))
conn = assert(tls.TlsClient(fd)) -- returns object of TlsSocket class
assert(conn.write("GET / HTTP/1.0\r\n\r\n"))
response = assert(conn.read())
print(response)
assert(unix.close(fd))

I noticed you're drawing a lot of influence from mbedTLS's net_sockets.c API. I don't like that API. I don't think it's very good. If you use that abstraction, then you lose the ability to compose with redbean APIs like ResolveIp(), unix.poll(), etc. For examples of how I've made mbedTLS work with raw file descriptors, see tool/curl/curl.c and tool/build/lib/eztls.c.

Copy link
Author

Choose a reason for hiding this comment

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

This API indeed seems more natural. I will make the modifications to get as close as possible to this example. Thank you

Copy link
Author

@chamot1111 chamot1111 Sep 9, 2024

Choose a reason for hiding this comment

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

I use now:

---@class tls
local tls = {}

--- Creates a new TLS client.
---@param fd integer File descriptor of the socket
---@param verify? boolean Whether to verify the server's certificate (default: true)
---@param timeout? integer Read timeout in milliseconds (default: 0, no timeout)
---@return TlsContext|nil context
---@return string? error
function tls.TlsClient(fd, verify, timeout) end

--- Writes data to the TLS connection.
---@param context TlsContext
---@param data string
---@return integer bytes_written
---@return string? error
function tls:write(data) end

--- Reads data from the TLS connection.
---@param context TlsContext
---@param bufsiz? integer Maximum number of bytes to read (default: BUFSIZ)
---@return string? data
---@return string? error
function tls:read(bufsiz) end

@jeromew
Copy link
Contributor

jeromew commented Sep 9, 2024

I add a reference to this fullmoon request - pkulchenko/fullmoon#29 (comment) that seems related to the exposure of more mbedtls endpoints (I don't know if this should belong to the tls module or somewhere else)

@pkulchenko listed the openssl APIs that we are currently missing in order to add automatic letsencrypt DNS-01 challenge like the one existing in the caddy web server.

it would be a great new redbean feature if the exposure of more mbedtls endpoints could make it possible to have automatic ssl certification embedded.

I copy them here for reference

capturex('openssl', ('genrsa', '-out', $_, KEY_SIZE));
capturex('openssl', ('rsa', '-text', '-in', $self->{domain}{account}, '-noout', '-modulus'));
capturex('openssl', ('rsa', '-in', $self->{domain}{account}, '-pubout')));
capturex('openssl', ('dgst', '-sha256', '-binary', '-sign', $self->{domain}{account}, $stf->filename))
capturex('openssl', ('req', '-new', '-outform', 'DER', '-key', $self->{domain}{key}, '-config', $oct->filename, '-out', $self->{req}{csr}));

@chamot1111 chamot1111 requested a review from jart September 9, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants