From: Olga Kornievskaia <olga.kornievskaia@gmail.com>
To: Chuck Lever III <chuck.lever@oracle.com>
Cc: "kernel-tls-handshake@lists.linux.dev"
<kernel-tls-handshake@lists.linux.dev>
Subject: Re: [RFC PATCH] ktls-utils: Add minor error checking of gnutls functions
Date: Wed, 17 May 2023 14:59:48 -0400 [thread overview]
Message-ID: <CAN-5tyHLz3C4DHPytwn-x2zmpYm=bRxYu_njVKSmtq67bvnFUg@mail.gmail.com> (raw)
In-Reply-To: <D032DEDF-98DE-40D2-88F9-A50E6A776895@oracle.com>
On Wed, May 17, 2023 at 2:11 PM Chuck Lever III <chuck.lever@oracle.com> wrote:
>
> Review comments:
>
> > On May 17, 2023, at 12:14 PM, Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote:
> >
> > From: Olga Kornievskaia <kolga@netapp.com>
> >
> > Add error checking to non void gnutls functions and log if we failed.
>
> It would be better if there were a few sentences that
> explained why this change is needed. For example, what
> errors are missed by the current code? Why should
> verification fail in these additional cases?
This was in the spirit of "always check the return code of the
function and handle appropriately". I don't have a failure case in
mind that I know would lead to problems. However, I speculate if for
whatever reason (something like memory allocation), we might want to
fail instead of having it fail later?
Also as you see it was RFC, as in if you think those checks are really
not needed then I defer to your judgement.
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > ---
> > src/tlshd/client.c | 20 +++++++++++++++++---
> > 1 file changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/tlshd/client.c b/src/tlshd/client.c
> > index 6a16263..5fcacfa 100644
> > --- a/src/tlshd/client.c
> > +++ b/src/tlshd/client.c
> > @@ -233,7 +233,13 @@ static int tlshd_client_x509_verify_function(gnutls_session_t session)
> > gnutls_x509_crt_t cert;
> >
> > gnutls_x509_crt_init(&cert);
> > - gnutls_x509_crt_import(cert, &peercerts[i], GNUTLS_X509_FMT_DER);
> > + ret = gnutls_x509_crt_import(cert, &peercerts[i],
> > + GNUTLS_X509_FMT_DER);
> > + if (ret != GNUTLS_E_SUCCESS) {
> > + tlshd_log_gnutls_error(ret);
>
> I am wondering here, and below, if a more actionable message
> can be emitted. I can apply this as is, but we should review
> all the error reporting to see where it can be improved.
I could add either the name of the function which failed (or verbose
description like import of cert failed or set of DNS name failed),
would that be helpful? Otherwise those functions don't have a list of
possible error conditions listed to make the error more verbose.
> > + gnutls_x509_crt_deinit(cert);
> > + return GNUTLS_E_CERTIFICATE_ERROR;
> > + }
> > parms->remote_peerid[i] =
> > tlshd_keyring_create_cert(cert, parms->peername);
> > gnutls_x509_crt_deinit(cert);
> > @@ -278,9 +284,17 @@ static void tlshd_client_x509_handshake(struct tlshd_handshake_parms *parms)
> > gnutls_transport_set_int(session, parms->sockfd);
> > gnutls_session_set_ptr(session, parms);
> >
> > - gnutls_server_name_set(session, GNUTLS_NAME_DNS,
> > + ret = gnutls_server_name_set(session, GNUTLS_NAME_DNS,
> > parms->peername, strlen(parms->peername));
> > - gnutls_credentials_set(session, GNUTLS_CRD_CERTIFICATE, xcred);
> > + if (ret != GNUTLS_E_SUCCESS) {
> > + tlshd_log_gnutls_error(ret);
> > + goto out_free_creds;
> > + }
> > + ret = gnutls_credentials_set(session, GNUTLS_CRD_CERTIFICATE, xcred);
> > + if (ret != GNUTLS_E_SUCCESS) {
> > + tlshd_log_gnutls_error(ret);
> > + goto out_free_creds;
> > + }
> > gnutls_certificate_set_verify_function(xcred,
> > tlshd_client_x509_verify_function);
> >
> > --
> > 2.39.1
> >
> >
>
> --
> Chuck Lever
>
>
prev parent reply other threads:[~2023-05-17 19:00 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-17 16:14 [RFC PATCH] ktls-utils: Add minor error checking of gnutls functions Olga Kornievskaia
2023-05-17 18:10 ` Chuck Lever III
2023-05-17 18:59 ` Olga Kornievskaia [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAN-5tyHLz3C4DHPytwn-x2zmpYm=bRxYu_njVKSmtq67bvnFUg@mail.gmail.com' \
--to=olga.kornievskaia@gmail.com \
--cc=chuck.lever@oracle.com \
--cc=kernel-tls-handshake@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).