Re: [btns] Fwd: [anonsec] Fwd: Review of draft-ietf-btns-c-api-03

[Miika Komu <[e-mail]>]

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" <[e-mail]>
> Date:
> Sat, 12 Apr 2008 17:36:16 -0500
> To:
> [e-mail], [e-mail], [e-mail], 
> [e-mail]
> 
> To:
> [e-mail], [e-mail], [e-mail], 
> [e-mail]
> CC:
> [e-mail], [e-mail], Tim Polk 
> <[e-mail]>, [e-mail], [e-mail], Eric 
> Burger <[e-mail]>, [e-mail]
> 
> 
> Dr. 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
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________