HTTP && HTTPS Transport Review

[Robbie Harwood]

On 08/12/2013 12:36 PM, Greg Hudson wrote:
> On 08/08/2013 05:41 PM, Robbie Harwood wrote:
>> I have written code to add support for HTTP && HTTPS transport of
>> Kerberos traffic to the KDC which I would like to submit for review.  It
>> can be found on my github[0].
> 
> Thanks for this.  Please take a look at:
> 
>     http://k5wiki.kerberos.org/wiki/Contributing_code
> 
> We will need a few things in particular:
> 
> * Please create a project page with a design writeup.  It is difficult
> for us to review code without an accompanying design document, as we
> have to intuit the design from the code changes.  If you don't have a
> wiki account, you'll need to talk to Tom Yu (tlyu on irc) about
> registering one; we unfortunately had to disable open registration
> because of spammers.

Alright, I was hoping this would be small enough to not need a design
page.  One has been created here:

    http://k5wiki.kerberos.org/wiki/Projects/HTTP_Transport

> * Please make sure to review the C style checker output before asking
> for a round of review.  Sometimes it is appropriate to ignore output,
> but currently the checker finds a few no-brainer things like overly long
> lines in the first commit.  Skimming the code, it looks like you did a
> reasonably good job of matching our style, but see:
> http://k5wiki.kerberos.org/wiki/Coding_style/Practices#Local_variables

Should be fixed now.  Sorry about that.

> Without looking at the code in too much detail, I have some design comments:
> 
> * What Kerberos-over-HTTP protocol does this implement?  We've discussed
> a couple of different pieces of prior art here, and it isn't obvious
> whether you've tried to match either of them.  This is one of the things
> I'd expect to find in a design writeup.

Should be present in the project page.

> * The first commit (~6) adds support for "tcp://hostname:port" and
> "udp://hostname:port".  I don't believe we had discussed this, and I'm
> not fond of the idea.  "tcp:" and "udp:" are not real URI schemes, and
> our parsing rules for hostname:port combinations do not follow the
> common URI syntax.  I am still fine with having http: and https: URLs in
> that field, overloaded with the hostname:port syntax; it's easy to tell
> the difference by recognizing the "http://" and "https://" prefixes.

Okay, added as blocker.  Does the scheme in the project page look good
to you?  If so, I'll go ahead and switch to that instead.

> * The next commit (~5) adds an enumeration type which contains platform
> socket type values as well as protocol designators we define ourselves.
>  This idea is inherently non-portable, and we'll need to find an
> alternative design.  Most likely, we'll want an enumeration containing
> our own protocol designators (UDP/TCP, UDP, TCP, HTTP, and HTTPS) and we
> can map that onto platform socket types when required.

That makes sense.  I will implement that.  Added as blocker.

> * The third commit (~4) appears to contain a lot of cut-and-paste from
> service_tcp_fd to service_http_fd.  We'll need more careful design.

Understood.  Added as blocker.

I'll post again when I believe I have resolved these issues in the code.
 Thanks for your feedback.

--Robbie
IRC: freenode/rharwood