Re: [PATCH v20] GSSAPI encryption support

Stephen Frost <sfrost@snowman.net> writes:

> One of the things that I really didn't care for in this patch was the
> use of the string buffers, without any real checks (except for "oh,
> you tried to allocated over 1G"...) to make sure that the other side
> of the connection wasn't feeding us ridiculous packets, and with the
> resizing of the buffers, etc, that really shouldn't be necessary.
> After chatting with Robbie about these concerns while reading through
> the code, we agreed that we should be able to use fixed buffer sizes
> and use the quite helpful gss_wrap_size_limit() to figure out how much
> data we can encrypt without going over our fixed buffer size.  As
> Robbie didn't have time to implement those changes this past week, I
> did so, and added a bunch more comments and such too, and have now
> gone through and done more testing.  Robbie has said that he should
> have time this upcoming week to review the changes that I made, and
> I'm planning to go back and review other parts of the patch more
> closely now as well.

In general this looks good - there are a couple minor comments inline,
but it's fine.

I wanted to note a couple things about this approach.  It now uses one
more buffer than before (in contrast to the previous approach, which
reused a buffer for received data that was encrypted and decrypted).
Since these are static fixed size buffers, this increases the total
steady-state memory usage by 16k as opposed to re-using the buffer.
This may be fine; I don't know how tight RAM is here.

> Note that there's an issue with exporting the context to get the
> encryption algorithm used that I've asked Robbie to look into, so
> that's no longer done and instead we just print that the connection is
> encrypted, if it is.  If we can't figure out a way to make that work
> then obviously I'll pull out that code, and if we can get it to work
> then I'll update it to be done through libpq properly, as I had
> suggested earlier.  That's really more of a nice to have in any case
> though, so I may just exclude it for now anyway if it ends up adding
> any complications.

Correct.  Unfortunately I'd overlooked that the lucid interface won't
meet our needs (destroys the context).  So the two options here are:
SASL SSF (and I'll separately push more mechs to add support for that),
or nothing at all.

If you want a patch for that I can make one, but I think there was code
already... just needed a ./configure check program for whether the OID
is defined.

> +ssize_t
> +be_gssapi_write(Port *port, void *ptr, size_t len)
> +{
> +	size_t		bytes_to_encrypt = len;
> +	size_t		bytes_encrypted = 0;
> +
> +	/*
> +	 * Loop through encrypting data and sending it out until
> +	 * secure_raw_write() complains (which would likely mean that the socket
> +	 * is non-blocking and the requested send() would block, or there was some
> +	 * kind of actual error) and then return.
> +	 */
> +	while (bytes_to_encrypt || PqGSSSendPointer)
> +	{

I guess it's not a view everyone will share, but I think this block is
too long.  Maybe a helper function around secure_raw_write() would help?
(The check-and-send part at the start.)

I have similar nits about the other functions that don't fit on my
(86-line high) screen, though I guess a lot of this is due to project
style using a lot of vertical space.

> +	if (GSS_ERROR(major))
> +		pg_GSS_error(FATAL, gettext_noop("GSSAPI context error"),

I'd prefer this to be a different error message than the init/accept
errors - maybe something like "GSSAPI size check error"?

> +		if (GSS_ERROR(major))
> +			pg_GSS_error(libpq_gettext("GSSAPI context error"), conn,

Same here.

Again, these are nits, and I think I'm okay with the changes.

Thanks,
--Robbie