This reverts commit 981ebc4f12b88fbf52ed0352428a0612dd2c2568.
This broke `-o' to print the server certificate; without explicit
handshake there will be no tls_read(3) in this short code path.
---
irc.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/irc.c b/irc.c
index dc40201..61d74bb 100644
--- a/irc.c
+++ b/irc.c
@@ -162,6 +162,11 @@ int ircConnect(const char *bindHost, const char *host, const char *port) {
error = tls_connect_socket(client, sock, host);
if (error) errx(EX_PROTOCOL, "tls_connect: %s", tls_error(client));
+ do {
+ error = tls_handshake(client);
+ } while (error == TLS_WANT_POLLIN || error == TLS_WANT_POLLOUT);
+ if (error) errx(EX_PROTOCOL, "tls_handshake: %s", tls_error(client));
+
return sock;
}
--
2.32.0
No need to keep them at runtime; do so unconditionally for the sake of
simplicity.
Declare TLS config globally so ircConnect() can clear it and declare
both client and config statically as they are not used outside the irc.c
module.
---
irc.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/irc.c b/irc.c
index 61d74bb..c308e46 100644
--- a/irc.c
+++ b/irc.c
@@ -43,12 +43,13 @@
#include "chat.h"
-struct tls *client;
+static struct tls *client;
+static struct tls_config *config;
void ircConfig(
bool insecure, const char *trust, const char *cert, const char *priv
) {
- struct tls_config *config = tls_config_new();
+ config = tls_config_new();
if (!config) errx(EX_SOFTWARE, "tls_config_new");
int error;
@@ -167,6 +168,7 @@ int ircConnect(const char *bindHost, const char *host, const char *port) {
} while (error == TLS_WANT_POLLIN || error == TLS_WANT_POLLOUT);
if (error) errx(EX_PROTOCOL, "tls_handshake: %s", tls_error(client));
+ tls_config_clear_keys(config);
return sock;
}
--
2.32.0
ircConnect() yields a connected TCP socket after which "inet dns" is
no longer needed.
Possibly having loaded private key material, it seems a tad more
comforting to speak TLS *after* dropping any network capabilities
(except for socket read/write to the IRC host, of course).
Instead of moving the final pledge into irc.c:ircConnect() and thus
complicating the code around pledge across two C modules, simply
stub out an mnemonic ircHandshake() and call that explicitly.
This restores behaviour gained with
981ebc4 "Remove explicit tls_handshake(3) from ircConnect" which
was reverted for other reasons.
---
chat.c | 3 +++
chat.h | 1 +
irc.c | 16 ++++++++++------
3 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/chat.c b/chat.c
index ab0678a..b7562f6 100644
--- a/chat.c
+++ b/chat.c
@@ -240,6 +240,7 @@ int main(int argc, char *argv[]) {
if (error) err(EX_OSERR, "pledge");
#endif
ircConnect(bind, host, port);
+ ircHandshake();
ircPrintCert();
ircClose();
return EX_OK;
@@ -322,6 +323,8 @@ int main(int argc, char *argv[]) {
if (error) err(EX_OSERR, "pledge");
#endif
+ ircHandshake();
+
#ifdef __FreeBSD__
cap_rights_t rights;
caph_stream_rights(&rights, CAPH_WRITE);
diff --git a/chat.h b/chat.h
index a4c7670..39d4f7f 100644
--- a/chat.h
+++ b/chat.h
@@ -234,6 +234,7 @@ void ircConfig(
bool insecure, const char *trust, const char *cert, const char *priv
);
int ircConnect(const char *bind, const char *host, const char *port);
+void ircHandshake(void);
void ircPrintCert(void);
void ircRecv(void);
void ircSend(const char *ptr, size_t len);
diff --git a/irc.c b/irc.c
index c308e46..8901c1a 100644
--- a/irc.c
+++ b/irc.c
@@ -107,6 +107,16 @@ void ircConfig(
tls_config_free(config);
}
+void ircHandshake(void) {
+ int error;
+ do {
+ error = tls_handshake(client);
+ } while (error == TLS_WANT_POLLIN || error == TLS_WANT_POLLOUT);
+ if (error) errx(EX_PROTOCOL, "tls_handshake: %s", tls_error(client));
+
+ tls_config_clear_keys(config);
+}
+
int ircConnect(const char *bindHost, const char *host, const char *port) {
assert(client);
@@ -163,12 +173,6 @@ int ircConnect(const char *bindHost, const char *host, const char *port) {
error = tls_connect_socket(client, sock, host);
if (error) errx(EX_PROTOCOL, "tls_connect: %s", tls_error(client));
- do {
- error = tls_handshake(client);
- } while (error == TLS_WANT_POLLIN || error == TLS_WANT_POLLOUT);
- if (error) errx(EX_PROTOCOL, "tls_handshake: %s", tls_error(client));
-
- tls_config_clear_keys(config);
return sock;
}
--
2.32.0
Here's the same patch with ircHandshake() after ircConnect() inside
irc.c as intended; no other change.
-- >8 --
From 4c3ca47c9f88623d163e8f5846a16b6c925cb7dc Mon Sep 17 00:00:00 2001
From: Klemens Nanni <klemens@posteo.de>
Date: Tue, 29 Jun 2021 15:34:03 +0200
Subject: [PATCH] Perform TLS handshake after final pledge
ircConnect() yields a connected TCP socket after which "inet dns" is
no longer needed.
Possibly having loaded private key material, it seems a tad more
comforting to speak TLS *after* dropping any network capabilities
(except for socket read/write to the IRC host, of course).
Instead of moving the final pledge into irc.c:ircConnect() and thus
complicating the code around pledge across two C modules, simply
stub out an mnemonic ircHandshake() and call that explicitly.
This restores behaviour gained with
981ebc4 "Remove explicit tls_handshake(3) from ircConnect" which
was reverted for other reasons.
---
chat.c | 3 +++
chat.h | 1 +
irc.c | 6 +++++-
3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/chat.c b/chat.c
index ab0678a..b7562f6 100644
--- a/chat.c
+++ b/chat.c
@@ -240,6 +240,7 @@ int main(int argc, char *argv[]) {
if (error) err(EX_OSERR, "pledge");
#endif
ircConnect(bind, host, port);
+ ircHandshake();
ircPrintCert();
ircClose();
return EX_OK;
@@ -322,6 +323,8 @@ int main(int argc, char *argv[]) {
if (error) err(EX_OSERR, "pledge");
#endif
+ ircHandshake();
+
#ifdef __FreeBSD__
cap_rights_t rights;
caph_stream_rights(&rights, CAPH_WRITE);
diff --git a/chat.h b/chat.h
index a4c7670..39d4f7f 100644
--- a/chat.h
+++ b/chat.h
@@ -234,6 +234,7 @@ void ircConfig(
bool insecure, const char *trust, const char *cert, const char *priv
);
int ircConnect(const char *bind, const char *host, const char *port);
+void ircHandshake(void);
void ircPrintCert(void);
void ircRecv(void);
void ircSend(const char *ptr, size_t len);
diff --git a/irc.c b/irc.c
index c308e46..6f0d496 100644
--- a/irc.c
+++ b/irc.c
@@ -163,13 +163,17 @@ int ircConnect(const char *bindHost, const char *host, const char *port) {
error = tls_connect_socket(client, sock, host);
if (error) errx(EX_PROTOCOL, "tls_connect: %s", tls_error(client));
+ return sock;
+}
+
+void ircHandshake(void) {
+ int error;
do {
error = tls_handshake(client);
} while (error == TLS_WANT_POLLIN || error == TLS_WANT_POLLOUT);
if (error) errx(EX_PROTOCOL, "tls_handshake: %s", tls_error(client));
tls_config_clear_keys(config);
- return sock;
}
void ircPrintCert(void) {
--
2.32.0
I don’t like how this adds to the sequence of irc* functions to be
called. I’ve moved the explicit tls_handshake(3) to ircPrintCert()
where it is needed, and added the tls_config_clear_keys(3) call to
ircSend() so that they’re cleared after the first write naturally
causes the handshake:
https://git.causal.agency/catgirl/commit/?id=18e8484a9814d03307f65fb5d7d4513e92f6d8ec
(This kind of feels like something libtls should do itself if the
client is the last one holding a reference to the config object,
as was the case originally, but maybe that’s a little too clever.)
On Sun, Jul 04, 2021 at 06:31:42PM -0400, june wrote:
> I don’t like how this adds to the sequence of irc* functions to be
> called. I’ve moved the explicit tls_handshake(3) to ircPrintCert()
> where it is needed, and added the tls_config_clear_keys(3) call to
> ircSend() so that they’re cleared after the first write naturally
> causes the handshake:
>
> https://git.causal.agency/catgirl/commit/?id=18e8484a9814d03307f65fb5d7d4513e92f6d8ec
Now the handshake is explicit only where it is needed, that seems good.
Deferral of zeroing keys and freeing config to ircSend() sure works, it
seems less straight forward to me.
Code got improved however, so that's good, thanks.
What bothers me, though, is that you've taken two patches of mine and
committed slightly modified versions under your name without any
attribution whatsoever.
It's nothing big but not a simple whitespace or typo fix either, so it
would have been nice to
a) ask for a second iteration of the diff after discussing it here,
b) commit as you did with as little as "idea from Klemens Nanni" or
c) comit my patch and amend as you like right afterwards
d) ...
There could be a bug none of us saw, so now git blame/log will point at
the wrong person.
The code history on its own is simply incorrect and filtering by
author/committer is now off.
Regardless of the size of the change, the original idea should be
attributed to the original author -- especially in the FOSS community.
That said: thanks for catgirl and your work! I'll be happy to keep
sending patches in the future.
> (This kind of feels like something libtls should do itself if the
> client is the last one holding a reference to the config object,
> as was the case originally, but maybe that’s a little too clever.)
I guess that would break clients which load files once, then possibly
lose filesystem access or simply don't want to reread them, but
(re)connect multiple times? Self-cleanup would be "too clever" here.
> On Jul 4, 2021, at 19:22, Klemens Nanni <klemens@posteo.de> wrote:
>
> What bothers me, though, is that you've taken two patches of mine and
> committed slightly modified versions under your name without any
> attribution whatsoever.
>
> It's nothing big but not a simple whitespace or typo fix either, so it
> would have been nice to
> a) ask for a second iteration of the diff after discussing it here,
> b) commit as you did with as little as "idea from Klemens Nanni" or
Easy to miss when I’m trying to catch up on like 10 patches accumulated
in my inbox.
> c) comit my patch and amend as you like right afterwards
Harder to do when I have 3 separate patches where I want one smaller
one for a concise history.
> d) ...
>
> There could be a bug none of us saw, so now git blame/log will point at
> the wrong person.
> The code history on its own is simply incorrect and filtering by
> author/committer is now off.
And it’s wrong if I do (c) and rewrite a patch any more than
cosmetically. Seems to go both ways.
> Regardless of the size of the change, the original idea should be
> attributed to the original author -- especially in the FOSS community.
Force-pushed commit messages since they’re still fresh:
https://git.causal.agency/catgirl/commit/?id=bf7b88a9914343808cc010487bc7461296a45499
https://git.causal.agency/catgirl/commit/?id=871df6b47e9d31a7e5c38541730ac5c4a85f6931
On Sun, Jul 04, 2021 at 08:02:41PM -0400, june wrote:
> > On Jul 4, 2021, at 19:22, Klemens Nanni <klemens@posteo.de> wrote:
> >
> > What bothers me, though, is that you've taken two patches of mine and
> > committed slightly modified versions under your name without any
> > attribution whatsoever.
> >
> > It's nothing big but not a simple whitespace or typo fix either, so it
> > would have been nice to
> > a) ask for a second iteration of the diff after discussing it here,
> > b) commit as you did with as little as "idea from Klemens Nanni" or
>
> Easy to miss when I’m trying to catch up on like 10 patches accumulated
> in my inbox.
No problem, didn't mean to imply ill intentions whatsoever, and I know
the workload/burden that comes with maintaining an open source project.
> > c) comit my patch and amend as you like right afterwards
>
> Harder to do when I have 3 separate patches where I want one smaller
> one for a concise history.
>
> > d) ...
> >
> > There could be a bug none of us saw, so now git blame/log will point at
> > the wrong person.
> > The code history on its own is simply incorrect and filtering by
> > author/committer is now off.
>
> And it’s wrong if I do (c) and rewrite a patch any more than
> cosmetically. Seems to go both ways.
We/you have done that with small patches in the past already and it is
all fine, but you are right: it goes both ways.
"amend" was the wrong word, i should've said
c) commit my patch and commit a fixup/finish/change afterwards
> > Regardless of the size of the change, the original idea should be
> > attributed to the original author -- especially in the FOSS community.
>
> Force-pushed commit messages since they’re still fresh:
Appreciated, thanks.