Julien Laganier wrote: Hi,
Folks,Vijay Gurbani has reviewed the BTNS C API and kindly agreed to let me forward it to the mailing list, please see it below.Many thanks to Vijay!
thanks Vijay and apologies for really late response. Sasu handled this round of iteration and has addressed most of your comments. There are still many details to be filled. The updated draft is in here: http://www.ietf.org/internet-drafts/draft-ietf-btns-c-api-04.txt Nico and Michael seem to have updated abstract APIs draft.
Subject: Review of draft-ietf-btns-c-api-03 From: "Vijay K. Gurbani" <vkg at alcatel-lucent.com> Date: Sat, 12 Apr 2008 17:36:16 -0500 To:mcr at sandelman.ottawa.on.ca, Nicolas.Williams at sun.com, miika at iki.fi, sasu.tarkoma at hiit.fiTo:mcr at sandelman.ottawa.on.ca, Nicolas.Williams at sun.com, miika at iki.fi, sasu.tarkoma at hiit.fiCC:lha at stacken.kth.se, julien.ietf at laposte.net, Tim Polk <tim.polk at nist.gov>, chris.newman at sun.com, lisa at osafoundation.org, Eric Burger <eburger at bea.com>, APPS-REVIEW at ietf.orgDr. Richardson et al.: I was asked by the Applications area to review draft-ietf-btns-c-api-03. Here is a review of it, following a similar one I did last week for draft-ietf-btns-abstract-api-01. draft-ietf-btns-c-api-03 ------------------------------- As a follow-up draft to draft-ietf-btns-abstract-api, the reasons why this document is important are much the same as why the latter one was important. I do note that this document appears to be more fleshed out than its abstract-api counterpart, whereas I would have expected the opposite. For instance, the Security Considerations section of the abstract-api I-D was empty, and the one for c-api is filled out. Ostensibly, the Security Considerations section for the abstract-api should be the one that could be filled out and "inherited", if you will, by a language-specific binding document that fulfills the abstract-api one.
We can move the security section to the other draft if it is ok for Nico and Michael? We can move also some parts of the intro.
Furthermore, and I think this is important, the c-api document should do more to align itself with the abstract-api document since the latter document asserts the shape of the information in the c-api document. For instance, S2.3.2 of c-api contains the attributes of a pToken. Clearly, these are related to S7 of abstract-api (Properties of pToken objects). Likewise, S2.2.2 of c-api is similarly related to S8 of abstract-api. Currently, there is a lack of such close cross referencing between these documents. I suspect that the c-api is the first language binding document; thus, any time spent now in such cross-referencing will pay for itself later when c-api is used as a template for a Java-api or c++-api document.
Added the few references, but there is clearly still more room for improvement. You should find rest of your focused comments in the draft.
Here are more focused comments:
- S1, end of first paragraph: (1) may be a good idea to provide
a reference for HIP (also expand the acronym). SIP (RFC3261)
may be another protocol that could use this API; S26.3.1 of
rfc3261 does state that "Proxy servers, redirect servers,
registrars, and UAs MAY also implement IPSec or other lower-
layer security protocols." So conceivably, a user agent
can create a connection to its proxy and use the IPSec APIs to
figure out whether that connection is integrity and confidentiality
protected. In certain networks (like IMS), IPSec is used
fairly heavily between the UA and the proxy.
- S1, third paragraph:
(1) s/we defined APIs/we define APIs/
(2) s/document fulfisl/document fulfills/
(3) s/requirements presented in/requirements in/
(4) s/and present C-bindings/and presents C-bindings/
- S1, paragraph under Figure 1: s/'The third/The third
- IMHO, it would be good to move S2.1 to *after* the
attributes for pToken and iToken are defined.
That way, the reader can make more sense of these APIs. As
the draft currently stands, the reader does not know
anything about the attributes when he or she
reaches S2.1, so the discussion there appears out of place,
without a good context.
- S2.1, second paragraph: s/using malloc/from heap.
- S2.1, second paragraph, last sentence: "When attr_val is NULL ...
of the attr_val." I do not understand the need for this.
If attr_val is NULL, why would it have a non-zero attr_len?
Maybe I am missing somethng?
- S2.1, last paragraph: You may want to mention that attr_val
must not be NULL, and neither should attr_len.
- S2.2.1, text under Figure 3: what does the size have to do
with a c'tor and d'tor? I would suggest a re-write as follows:
Operating environments that support the IPSec API will
provide appropriate constructor and destructor for the
iToken objects. Because applications will often not be
aware of the byte-representation of the iToken object, nor
will it know which attributes to initialize upon construction,
applications MUST only use the provided constructor to
create an iToken object, and when no longer needed, use the
provided destructor to destroy it. Figure 4 shows the
APIs:
- S2.2.2, Figure 5: s/LEAFOFFAITH/LEAPOFFAITH
Also, you may want to tie these attributes to S8 in abstract-api
document.
- S2.2.2, text under Figure 5: consider organizing the text in
this paragraph to have hanging indents and such. It will
make it much more readable.
- S2.2.2, I do not understand the contents of the paragraph
"The attributes for the second ... should be used."
- S2.3.1, Figure 7:
s/ipsec_create_pToken()/ipsec_create_pToken(void)/
The reason is that a func() is different than a func(void) under
ANSI C.
- S2.3.1, last paragraph of the section: what would cause
ipsec_free_pToken() to return non-zero? About the only thing I
can think of is if the parameter passed in is null. I believe
we can in fact get by with having ipsec_free_pToken() return a
void, but I will leave it to your good judgement.
- S2.3.2: same comments as S2.3.1 about tying the attributes
of pToken to abstract-api draft, and having hanging indents
for readability.
- S2.3.4, the text in first paragraph: I am curious why the
decision to only support the IPSec APIs on sendmsg() and
recvmsg()? I am sure you had a technical reason, but I
cannot think of any. It may be a good idea to list the reason
as a note in the draft.
- S2.3.5: the text under Figure 11 mentions "ipsec_cmp_policy()".
Did you mean "ipsec_cmp_pToken()" instead?
- S2.3.5: in the text under Figure 11, you may want to mention
that ipsec_cmp_pToken() returns non-zero if p1 != p2.
- S2.3.6, Figure 12: should the type of "p" not be ipsec_pToken
only? "p" is the source of the copy, and as such only a
pointer to it is required, not a **. Plus, qualifying it with
a const will probably not hurt.
Hope that helps!
Thank you.
- vijay
------------------------------------------------------------------------
_______________________________________________
Note Well: Messages sent to this mailing list are the opinions of the senders and do not imply endorsement by the IETF.