Stephen Frost <sfr...@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
signature.asc
Description: PGP signature
