HTTP && HTTPS Transport Review
rharwood at redhat.com
Mon Aug 12 17:27:24 EDT 2013
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.
> Thanks for this. Please take a look at:
> 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:
> * 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:
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.
More information about the krbdev