On Thu, Feb 11, 2016 at 6:06 AM, Robbie Harwood <rharwood@redhat.com> wrote:
> For your consideration, here is a new version of GSSAPI encryption
> support. For those who prefer, it's also available on my github:
> https://github.com/frozencemetery/postgres/commit/c92275b6605d7929cda5551de47a4c60aab7179e
Yeah! Glad to see you back.
> Some thoughts:
>
> - The overall design is different this time - GSS encryption sits in
> parallel construction to SSL encryption rather than at the protocol
> level - so a strict diff probably isn't useful.
>
> - The GSSAPI authentication code has been moved without modification.
> In doing so, the temptation to modify it (flags, error checking, that
> big comment at the top about things from Athena, etc.) is very large.
> I do not know whether these changes are best suited to another patch
> in this series or should be reviewed separately. I am also hesitant
> to add things beyond the core before I am told this is the right
> approach.
I would recommend a different patch if code needs to be moved around.
The move may make sense taken as an independent piece of the
integration.
> - There's no fallback here. I wrote fallback support for versions 1-3,
> and the same design could apply here without too much trouble, but I
> am hesitant to port it over before the encryption design is approved.
> I strongly suspect you will not want to merge this without fallback
> support, and that makes sense to me.
>
> - The client and server code look a lot like each other. This
> resemblance is not exact, and my understanding is that server and
> client need to compile independently, so I do not know of a way to
> rectify this. Suggestions are welcome.
At quick glance, I like the direction this is taking. You moved all
the communication protocol at a lower level where SSL and secure reads
are located, so this results in a neat integration.
+ * Portions Copyright (c) 2015-2016, Red Hat, Inc.
+ * Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
I think that this part may be a problem... Not sure the feeling of the
others regarding additional copyright notices.
It would be good to add that to the next CF, I will be happy to get a
look at it.
--
Michael