Subject: [Gen-art] Gen-art review of draft-josefsson-rfc3548bis-02

From: Elwyn Davies <[email-masked]>
Date: Mon, 10 Apr 2006 10:45:47 +0100

I was selected as General Area Review Team reviewer for this specification
(for background on Gen-ART, please see
http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html).

Document: draft-josefsson-rfc3548bis-02
Intended Status: Proposed Standard (Individual submission via AD)
Shepherding AD: Ted Hardie
Review Trigger: IETF Last Call (ends 1 May 2006)

Summary:
This is almost ready for PS.  I suspect (but don't know on account of 
not being an IPR lawyer) that, in trying to do the 'right thing', the 
author may have created an impossible (or maybe just excessively 
complicated) copyright position regarding the code in s11.  The document 
now contains three different statements that are intended to apply to 
the code: BCP78, the GNU copyright statements embedded in the code and 
s15. Help!

Aside from this there are a couple of minor issues (below) and a number of

Minor issues:
s11/s15: potential IPR (statement) issue: Is there any interaction  
between the overall BCP78 copyright position, the GNU copyright 
positions mentioned in the code sections and the author's statements in 
s15?  BCP78 seems to indicate that s11 could be freely copied and 
wouldn't need any copyright statements.

s3.4, last para:
> In this document, we document and *name* some currently used alphabets.
Names are not explicitly given to all the alphabets.

s11: Would be worth noting that it is assumed that the encoded input has 
been stripped of embedded line feeds (if the specification needs them) 
before applying the decoding functions.

s3.3, 2nd para:
> Furthermore, such specifications may
> consider the pad character, "=", as not part of the base alphabet
> until the end of the string.  If more than the allowed number of pad
> characters are found at the end of the string, e.g., a base 64 string
> terminated with "===", the excess pad characters could be ignored.
I wasn't absolutely sure what was intended here.  I think this could be 
improved (If this is not the sense that is intended than it shpuld be 
'differently' improved):
Furthermore, such specifications MAY consider the pad character, "=", as 
not part of the base alphabet when found at the end of a string of 
encoded data.  In this case if more than the allowed number of pad 
characters are found at the end of the string, e.g., a base 64 string 
terminated with "===", the excess pad characters MAY be ignored.

Editorial:

General: Consistent Capitalization of Section Titles is required.

s3.3: Consistent use of 'non-alphabet' rather then 'non alphabet' - 4 
instances in the body.

General: Several instances of '(non-)alphabet' s/b '(non-)alphabetic':
including Abstract (1st instance), s1 (both instances), s3.3 (title, 
2nd, 3rd and 4th instances in para 1 and 3rd instance in para 2), s11.1 
(comment before function base64_decode), s12 (1st instance in para 2.

s1: Expand MIME acronym
s3: Expand PEM acronym.

s3.2: Useful to point to sections which tell how much padding (ss4 and 6).

s3.3, para 2, 1st sentence: s/reject the encoding/reject the encoded data/
s3.3, 2nd para:
Need to expand the CRLF acronym.('adjacent carriage return/line feed 
(CRLF) characters')

s3.4, bullet 1: s/are easily interchanged, as well/are easily confused, 
as well as/, s/is not present/are not present/

s3.4, bullet 2: s/place/mandate/

s4, s6, : s/a single digit in the xxx alphabet/s single character in the 
xxx alphabet/

s4, s6 and s7, para 1 in each case: s/due to/is derived from/

s4, para 2: I think 'requires case sensitivity' is not the right 
phrase.  I take it it should be something like 'allows the use of both 
upper- and lowercase letters' or 'preserves case sensitive encodings'.

s5: s/understrike/underrscore/

_______________________________________________
Gen-art mailing list
[email-masked]
https://www1.ietf.org/mailman/listinfo/gen-art

---

From: Simon Josefsson <[email-masked]>
Date: Tue, 18 Apr 2006 18:05:37 +0200

Hello Elwyn, and thanks for your careful review!

I have incorporated most of your suggestions (see below) and updated
the document.  The new document, with yours (and others) fixes, is
available from:

http://josefsson.org/base-encoding/draft-josefsson-rfc3548bis.txt

The differences compared to the version in last call can be found at:

http://josefsson.org/base-encoding/draft-josefsson-rfc3548bis-from--02.diff.html

I would appreciate if you could double-check that I merged all your
suggestions correctly, but I understand if you don't have time to do
this.

Elwyn Davies <[email-masked]> writes:

> complicated) copyright position regarding the code in s11.  The
> document now contains three different statements that are intended to
> apply to the code: BCP78, the GNU copyright statements embedded in the
> code and s15. Help!

I think you misread section 15.  It doesn't apply to section 11.

The situation is intended to be like this (and from what I can tell,
is what the document says): LGPL applies to the code and only the
code.  Section 15 (my own copying condition) cover what I wrote.  The
IETF copying conditions cover the entire document, including the text
of the document that I copied from earlier RFCs.

> positions mentioned in the code sections and the author's statements
> in s15?  BCP78 seems to indicate that s11 could be freely copied and

The idea of an additional copyright statement is that when
redistributing the excerpted code, which is encouraged to happen, it
should have a copyright notice.  The ISOC copyright notice does not
apply to my code, hence my own copyright notice.

> Names are not explicitly given to all the alphabets.

How so?  The names are the names in section title, for each alphabet.

> s11: Would be worth noting that it is assumed that the encoded input
> has been stripped of embedded line feeds (if the specification needs
> them) before applying the decoding functions.

Isn't this covered by the 'Note...' in the function description?

   /* Decode base64 encoded input array IN of length INLEN to
      output array OUT that can hold *OUTLEN bytes.  Return
      true if decoding was successful, i.e. if the input was
      valid base64 data, false otherwise.  If *OUTLEN is too
      small, as many bytes as possible will be written to OUT.
      On return, *OUTLEN holds the length of decoded bytes in
      OUT.  Note that as soon as any non-alphabet characters
      are encountered, decoding is stopped and false is
      returned. */
   bool
   base64_decode (const char *restrict in, size_t inlen,
                  char *restrict out, size_t *outlen)

Perhaps you could clarify what additional text you are looking for.

> I wasn't absolutely sure what was intended here.  I think this could
> be improved (If this is not the sense that is intended than it shpuld
> be 'differently' improved):
> Furthermore, such specifications MAY consider the pad character, "=",
> as not part of the base alphabet when found at the end of a string of

I agree it is not clear.  I think your text is incorrect.

The idea here is to say that '=' EARLIER in an encoded string (e.g.,
in the string "foo=bar=baz===") MAY be ignored, and that ALSO for a
terminating '===', you would also have to ignore the excess pad
characters.

I've added this:

  Furthermore, such specifications MAY ignore the pad character, "=",
  treating it as non-alphabet data, if it is present before the end of
  the encoded data.  If more than the allowed number of pad characters
  are found at the end of the string, e.g., a base 64 string
  terminated with "===", the excess pad characters MAY also be
  ignored.

What do you think?

> General: Consistent Capitalization of Section Titles is required.

Fixed.

> 2nd, 3rd and 4th instances in para 1 and 3rd instance in para 2),
> s11.1 (comment before function base64_decode), s12 (1st instance in
> para 2.

I've changed them to "non-alpha*".

> s3: Expand PEM acronym.

Done.

> s3.2: Useful to point to sections which tell how much padding (ss4 and 6).

I've added:

<t>The base64 and base32 alphabets use padding, as described below in
section 4 and 6, but the base16 alphabet does not need it, see section
8.</t>

> s3.4, bullet 1: s/are easily interchanged, as well/are easily
> confused, as well as/, s/is not present/are not present/
>
> s4, s6, : s/a single digit in the xxx alphabet/s single character in
> the xxx alphabet/
>

Done!

> upper- and lowercase letters' or 'preserves case sensitive encodings'.

I chose the former.

> s5: s/understrike/underrscore/

I changed it to 'underline', since RFC 20 use that term.

Thanks,
Simon

---

From: Elwyn Davies <[email-masked]>
Date: Tue, 18 Apr 2006 17:48:28 +0100

I looked over the diff .. couple of notes below.

Glad you trapped the test vector errors!

> I think you misread section 15.  It doesn't apply to section 11.
True.. I know what you are trying to achieve but I have no idea if the 
legalese works. :-(
> of the document that I copied from earlier RFCs
>   
>   

I'll leave that one to the IP lawyers.  I really have no idea.
> How so?  The names are the names in section title, for each alphabet.
I was expecting a short, pithy name for each alphabet that would allow 
them to be distinguished.  I didn't feel that the whole of the section 
title constituted a 'name', whereas some of them have (nearly) got 
names: "base32", etc.  I think an explicit name like this would help 
future protocol designers to refer very precisely to the alphabet in use.
> Perhaps you could clarify what additional text you are looking for.
Sort of, but since you mention that some protocols need to split the 
text into lines earlier, I thought it would be worth making it clear 
that this code is intended to be applied after the line separators have 
been removed.

The rest seems fine to me.

/Elwyn

---

From: Simon Josefsson <[email-masked]>
Date: Wed, 19 Apr 2006 00:28:39 +0200

> Glad you trapped the test vector errors!

Yup.  Just checking if anyone was paying attention... :)

> True.. I know what you are trying to achieve but I have no idea if the

It works for me.  I would be happy to address specific concerns.

> I'll leave that one to the IP lawyers.  I really have no idea.

I understand.

> future protocol designers to refer very precisely to the alphabet in

Oh, I see.  I agree.  I have added sentences like:

This encoding may be referred to as "base64".
This encoding may be referred to as "base64url".
This encoding may be referred to as "base32".
This encoding may be referred to as "base32hex"
...

See the updated URLs above.

Is this OK?

> that this code is intended to be applied after the line separators
> have been removed.

I've added the following to the comment:

   This means that, when applicable, you must de-fold any CRLF that is
   part of the data stream before calling this function.

Thanks,
Simon

---

From: Elwyn Davies <[email-masked]>
Date: Wed, 19 Apr 2006 15:23:10 +0100

Hi.

Think we are almost done....

Simon Josefsson wrote:

<<snip>>

As regards the IPR stuff:  We need a 'reading' from the IP lawyers. 
'Nuff said.
> See the updated URLs above.
Fine by me.
>    part of the data stream before calling this function.
Since it won't necessarily be CRLF, I would suggest s/CRLF/line terminators/

/Elwyn

---

From: Simon Josefsson <[email-masked]>
Date: Wed, 19 Apr 2006 16:33:32 +0200

> Think we are almost done....

Yup.  Thanks for following up on the review.

> Since it won't necessarily be CRLF, I would suggest s/CRLF/line terminators/

Applied, and I also changed "de-fold" into "remove".

Thanks,
Simon

---

From: "Spencer Dawkins" <[email-masked]>
Date: Wed, 19 Apr 2006 18:13:37 -0500

Hi, Simon,

ABSOLUTELY no one asked my opinion about this, but

> See the updated URLs above.

I really appreciate spec authors who do stuff like this to improve the 
usability of our specs. Don't you wish everyone did?

Thanks!

Spencer