Horton

Thread

Commits

  1. Improve error handling of HMAC computations

  2. Fix incorrect comments in hmac.c and hmac_openssl.c

  3. Fix comment related to pg_cryptohash_error()

  1. Improve error handling of HMAC computations and SCRAM

    Michael Paquier <michael@paquier.xyz> — 2022-01-11T04:56:22Z

    Hi all,

This is a follow-up of the work done in b69aba7 for cryptohashes, but
this time for HMAC.  The main issue here is related to SCRAM, where we
have a lot of code paths that have no idea about what kind of failure
is happening when an error happens, and this exists since v10 where
SCRAM has been introduced, for some of them, frontend and backend
included.  \password is one example.

The set of errors improved here would only trigger in scenarios that
are unlikely going to happen, like an OOM or an internal OpenSSL
error.  It would be possible to create a HMAC from a MD5, which would
cause an error when compiling with OpenSSL and FIPS enabled, but the
only callers of the pg_hmac_* routines involve SHA-256 in core through
SCRAM, so I don't see much a point in backpatching any of the things
proposed here.

The attached patch creates a new routine call pg_hmac_error() that one
can use to grab details about the error that happened, in the same
fashion as what has been done for cryptohashes.  The logic is not that
complicated, but note that the fallback HMAC implementation relies
itself on cryptohashes, so there are cases where we need to look at
the error from pg_cryptohash_error() and store it in the HMAC private
context.

Thoughts?
--
Michael

  2. Re: Improve error handling of HMAC computations and SCRAM

    Sergey Shinderuk <s.shinderuk@postgrespro.ru> — 2022-01-11T07:50:50Z

    Hi,

On 11.01.2022 07:56, Michael Paquier wrote:
 > Thoughts?

A few comments after a quick glance...

+ * Returns a static string providing errors about an error that happened

"errors about an error" looks odd.


+static const char *
+SSLerrmessage(unsigned long ecode)
+{
+	if (ecode == 0)
+		return NULL;
+
+	/*
+	 * This may return NULL, but we would fall back to a default error path if
+	 * that were the case.
+	 */
+	return ERR_reason_error_string(ecode);
+}

We already have SSLerrmessage elsewhere and it's documented to never 
return NULL.  I find that confusing.

If I have two distinct pg_hmac_ctx's, are their errreason's idependent 
from one another or do they really point to the same static buffer?


Regards,

-- 
Sergey Shinderuk		https://postgrespro.com/

  3. Re: Improve error handling of HMAC computations and SCRAM

    Michael Paquier <michael@paquier.xyz> — 2022-01-11T07:57:19Z

    On Tue, Jan 11, 2022 at 10:50:50AM +0300, Sergey Shinderuk wrote:
> A few comments after a quick glance...

Thanks!

> + * Returns a static string providing errors about an error that happened
> 
> "errors about an error" looks odd.

Sure, that could be reworded.  What about "providing details about an
error"?

> We already have SSLerrmessage elsewhere and it's documented to never return
> NULL.  I find that confusing.

This name is chosen on purpose.  There could be some refactoring done
with those things.

> If I have two distinct pg_hmac_ctx's, are their errreason's idependent from
> one another or do they really point to the same static buffer?

Each errreason could be different, as each computation could fail for
a different reason.  If they fail for the same reason, they would
point to the same error context strings.
--
Michael

  4. Re: Improve error handling of HMAC computations and SCRAM

    Sergey Shinderuk <s.shinderuk@postgrespro.ru> — 2022-01-11T08:08:59Z

    On 11.01.2022 10:57, Michael Paquier wrote:
> On Tue, Jan 11, 2022 at 10:50:50AM +0300, Sergey Shinderuk wrote:
>> + * Returns a static string providing errors about an error that happened
>>
>> "errors about an error" looks odd.
> 
> Sure, that could be reworded.  What about "providing details about an
> error"?

Yeah, that's better.  I thought "providing errors about an error" was a 
typo, but now I see the same comment was committed in b69aba745.  Is it 
just me? :)

Thanks,

-- 
Sergey Shinderuk		https://postgrespro.com/

  5. Re: Improve error handling of HMAC computations and SCRAM

    Michael Paquier <michael@paquier.xyz> — 2022-01-12T03:56:17Z

    On Tue, Jan 11, 2022 at 11:08:59AM +0300, Sergey Shinderuk wrote:
> Yeah, that's better.  I thought "providing errors about an error" was a
> typo, but now I see the same comment was committed in b69aba745.  Is it just
> me? :)

It is not only you :)  I have applied a fix to fix the comments on
HEAD and REL_14_STABLE.

Attached is a rebased patch for the HMAC portions, with a couple of
fixes I noticed while going through this stuff again (mostly around
SASLprep and pg_fe_scram_build_secret), and a fix for a conflict
coming from 9cb5518.  psql's \password is wrong to assume that the
only error that can happen for scran-sha-256 is an OOM, but we'll get
there.
--
Michael

  6. Re: Improve error handling of HMAC computations and SCRAM

    Michael Paquier <michael@paquier.xyz> — 2022-01-12T11:32:27Z

    On Wed, Jan 12, 2022 at 12:56:17PM +0900, Michael Paquier wrote:
> Attached is a rebased patch for the HMAC portions, with a couple of
> fixes I noticed while going through this stuff again (mostly around
> SASLprep and pg_fe_scram_build_secret), and a fix for a conflict
> coming from 9cb5518.  psql's \password is wrong to assume that the
> only error that can happen for scran-sha-256 is an OOM, but we'll get
> there.

With an attachment, that's even better.  (Thanks, Daniel.)
--
Michael

  7. Re: Improve error handling of HMAC computations and SCRAM

    Sergey Shinderuk <s.shinderuk@postgrespro.ru> — 2022-01-12T23:01:24Z

    On 12.01.2022 14:32, Michael Paquier wrote:
> On Wed, Jan 12, 2022 at 12:56:17PM +0900, Michael Paquier wrote:
>> Attached is a rebased patch for the HMAC portions, with a couple of
>> fixes I noticed while going through this stuff again (mostly around
>> SASLprep and pg_fe_scram_build_secret), and a fix for a conflict
>> coming from 9cb5518.  psql's \password is wrong to assume that the
>> only error that can happen for scran-sha-256 is an OOM, but we'll get
>> there.
> 
> With an attachment, that's even better.  (Thanks, Daniel.)
Gave it a thorough read.  Looks good, except for errstr not set in a 
couple of places (see the diff attached).

Didn't test it.

-- 
Sergey Shinderuk		https://postgrespro.com/

  8. Re: Improve error handling of HMAC computations and SCRAM

    Michael Paquier <michael@paquier.xyz> — 2022-01-13T07:24:31Z

    On Thu, Jan 13, 2022 at 02:01:24AM +0300, Sergey Shinderuk wrote:
> Gave it a thorough read.  Looks good, except for errstr not set in a couple
> of places (see the diff attached).

Thanks for the review.  The comments about pg_hmac_ctx->data were
wrong from the beginning, coming, I guess, from one of the earlier
patch versions where this was discussed.  So I have applied that
independently.

I have also spent a good amount of time on that to close the loop and
make sure that no code paths are missing an error context, adjusted a
couple of comments to explain more the role of *errstr in all the
SCRAM routines, and finally applied it on HEAD.
--
Michael

  9. Re: Improve error handling of HMAC computations and SCRAM

    Sergey Shinderuk <s.shinderuk@postgrespro.ru> — 2022-01-13T08:26:05Z

    On 13.01.2022 10:24, Michael Paquier wrote:
> Thanks for the review.  The comments about pg_hmac_ctx->data were
> wrong from the beginning, coming, I guess, from one of the earlier
> patch versions where this was discussed.  So I have applied that
> independently.
> 
> I have also spent a good amount of time on that to close the loop and
> make sure that no code paths are missing an error context, adjusted a
> couple of comments to explain more the role of *errstr in all the
> SCRAM routines, and finally applied it on HEAD.
Thanks!

-- 
Sergey Shinderuk		https://postgrespro.com/