Subject: Re: [Gen-art] draft-baeuerle-netnews-cancel-lock-06 and RFCs 5536 & 5322

From: Paul Kyzivat <[email-masked]>
Date: Mon, 18 Sep 2017 17:14:53 -0400

I'm on tap to do the genart telechat review on 
draft-baeuerle-netnews-cancel-lock-06. I see that nothing has been done 
about the ABNF that was my primary concern in my prior wglc review of -05:

https://mailarchive.ietf.org/arch/msg/gen-art/wXKa5tadbo_xQbbUUCk9RZyB0w8

I'm told by the authors that Alexey asked that it be submitted this way, 
but the ABNF *is* wrong, and I can't see how a new draft can be allowed 
to be published with knowingly wrong ABNF. OTOH, the problem didn't 
originate with this draft. Rather the problem originated with RFC5536, 
and this draft has simply built on that mistake. Unfortunately the root 
of the problem is with RFC5322, since its ABNF is structured in a way 
that makes it really hard to define extensions such as those in RFC5536 
and this draft. So the cleanest solution I can see is to revise both 
5322 and 5536, and then this draft can build on those changes. That is a 
rather involved fix but I don't see any other reasonable way.

This prompted me to submit an erratum (#5116) on RFC5536 regarding this 
issue. But errata aren't often acted on quickly when they require 
revising RFCs.

The question is: in the meantime, what to do about this draft? By 
default I guess I will simply restate the issue in my genart telechat 
review and leave it for the IESG discuss it.

Anybody have a better idea?

	Thanks,
	Paul

On 7/14/17 12:58 PM, Paul Kyzivat wrote:
> Michael,
> 
> Please see further comments inline.
> 
> (Sorry - the indenting is getting deep, but I think we are pretty much 
> done.)
> 
> On 7/11/17 2:39 PM, Michael Bäuerle wrote:
>> Paul Kyzivat wrote:
>>> On 7/7/17 11:47 AM, Michael Bäuerle wrote:
>>>> Paul Kyzivat wrote:
>>>>>
>>>>> [...]
>>>>> General Comments:
>>>>>
>>>>> I have not attempted to validate the security properties of this
>>>>> document - leaving that to a security review.
>>>>>
>>>>> I have also not attempted to verify the operational suitability of 
>>>>> this
>>>>> mechanism because I don't have the experience needed to do so.
>>>>>
>>>>> Issues:
>>>>>
>>>>> Major: 1
>>>>> Minor: 3
>>>>> Nits:  0
>>>>>
>>>>> (1) MAJOR:
>>>>>
>>>>> [Problem with ABNF syntax of header fields]
>>>>
>>>> Hi Paul,
>>>>
>>>> Julien Élie has proposed a new syntax definition.
>>>
>>> I was copied on his mail proposing a change to RFC5536, and that is
>>> tentative. *This* document would of course require a similar change. Or
>>> else it could extend that change, via something like
>>>
>>>     news-fields =/ cancel-lock / cancel-key
>>>
>>> But that can only be done if there is actually a draft that revises 5536
>>> that can then be referenced. Bottom line is that it seems there needs to
>>> be some coordination of that fix with this draft.
>>
>> It's clear that the Cancel-Lock draft needs to be changed. My comment
>> was about the potential erratum for RFC 5536 and the question:
>> Is it (in general) possible to reference a modified ABNF definition in
>> an erratum?
> 
> I don't know of any way to do so. Perhaps somebody else can provide a 
> more definitive answer. You might try asking on rfc-interest.
> 
>> If this is possible, then the current:
>>
>>     fields =/ *( cancel-lock / cancel-key )
>>
>> can be changed to:
>>
>>     news-fields =/ cancel-lock / cancel-key
>>
>> and should then reference the new ABNF from the erratum.
>> If not, what about a less formal definition like this:
>>
>>     The Cancel-Lock and Cancel-Key header fields MUST be treated as
>>     though it were news header fields as defined in Section 3 of
>>     [RFC5536].
> 
> I'm not sure about that. It might fly, or not. Again, I think you need 
> to ask somebody more authoritative.
> 
>> Rewriting RFC 5322 and RFC 5536 should be avoided if possible.
> 
> I understand why you might not want to take on responsibility for that. 
> But it is the right thing to do. Question is whether it needs to be done 
> now.
> 
>>>>> (2) Minor:
>>>>>
>>>>> In Section 3.5, step 1 says to hash the key using the algorithm 
>>>>> from its
>>>>> scheme. But IIUC the scheme describes the algorithm that has already
>>>>> been used when constructing the Cancel-Key header.
>>>>
>>>> No. There are two different operations that use a hash function.
>>>>
>>>> The recommended algorithm described in Section 4 calculates:
>>>> |
>>>> | K = HMAC(uid+mid, sec)
>>>>
>>>> K is then used for <c-key-string> in the Cancel-Key header field only
>>>> with Base64 as transfer encoding (no further hash operation is used):
>>>>
>>>>      c-key-string = Base64(K)
>>>>
>>>> HMAC can be based on a different hash function than the one in <scheme>
>>>> (because a serving agent doesn't need to know how K was calculated for
>>>> the check algorithm defined in Section 3.5).
>>>>
>>>> A value of e.g. "sha256" for <scheme> in a Cancel-Key header field
>>>> doesn't mean that SHA256 was used to generate this field, but that the
>>>> corresponding Cancel-Lock header field (in the original article) 
>>>> contains
>>>> the value of <c-key-string> hashed with SHA256 (and finally Base64
>>>> encoded).
>>>>
>>>> The hash function defined by <scheme> is used to generate 
>>>> <c-lock-string>
>>>> elements for the Cancel-Lock header field:
>>>>
>>>>      c-lock-string = Base64(hash(c-key-string))
>>>>                             ^^^^
>>>> This hash function is the one specified with <scheme> (in both matching
>>>> elements <c-lock> and <c-key> in the Cancel-Lock and Cancel-Key header
>>>> fields of the two articles involved).
>>>>> IIUC the serving
>>>>> agent need not hash the provided key. Rather it only uses the 
>>>>> scheme to
>>>>> select the locks to compare the hash against.
>>>>
>>>> <scheme> is used for both purposes.
>>>>
>>>> Step 1 is defined in Section 3.5 as:
>>>> |
>>>> | 1. The <c-key-string> part of the <c-key> element is hashed using
>>>> |    the algorithm defined by its <scheme> part.
>>>>
>>>> This is the operation for <c-lock-string> listed above.
>>>> The hash operation is required here because if the elements from the
>>>> Cancel-Key header field would be directly comparable, everybody could
>>>> forge them (copy from the public original article).
>>>> The hijack/abuse attack against <c-key-string> elements on their way
>>>> through the network (as discussed for the review from David Mandelberg)
>>>> is not the same. The relevant difference is who can *create* valid
>>>> <c-key-string> elements.
>>>>
>>>> In Step 2 <scheme> is used again to skip <c-lock-string> elements that
>>>> were created using different hash functions. The compare operation from
>>>> Step 2 (with first operand from Step 1) is:
>>>>
>>>>      Base64(hash(c-key-string)) == c-lock-string
>>>>
>>>> The Base64 encode part is not explicitly noted in Step 1.
>>>> Base64 decoding the second operand instead would work too:
>>>>
>>>>      hash(c-key-string) == Base64_decode(c-lock-string)
>>>>
>>>> To be unambiguous and consistent with the definition of <c-lock-string>
>>>> from Section 2.1, the words for Step 1 in Section 3.5 maybe should
>>>> explicitly cover the Base64 encode operation. Suggestion:
>>>>
>>>>      1. The <c-key-string> part of the <c-key> element is hashed using
>>>>         the algorithm defined by its <scheme> part and then Base64
>>>>         encoded.
>>>
>>> Hmm. That wasn't so clear to me, though on rereading I can understand it
>>> that way.
>>>
>>> But in that case, what is the point of passing the scheme around at all?
>>
>> The <scheme> part of <c-key> elements serve the following purposes:
>> - Backward compatibility
>>    Existing implementations use it this way since many years
>> - Define the hash algorithm to apply on <c-key-string> elements
>> - Select corresponding <c-lock-string> elements to compare against
>>
>>> IIUC doing so doesn't add any additional security or privacy. You could
>>> simply pass the hashed value in cancel-key. Then the verifying agent
>>> wouldn't do any hashing.
>>
>> In this case the creator of the Cancel-Key header field would not prove
>> that he really knows K.
>>
>>> This would mean that the cancel-key would be
>>> checked against locks that used a different hash, but that shouldn't
>>> hurt anything.
>>
>> Yes, it is unlikely that comparing against <c-lock-string> elements,
>> that used a different hash algorithm, will hurt.
>>
>> But changing the general syntax of the header fields would break
>> backward compatibility to existing implementations. This should be
>> avoided with highest priority.
> 
> I studied the draft further. I finally get that it is simpler than I was 
> expecting it to be. So really it is just that the lock contains 
> hash(password) while the key contains the password. (The key is just an 
> algorithmically generated password.) I couldn't believe you would really 
> be passing the password in the clear, since that would expose it to 
> on-path attack. But I guess your assumption is that this is really a 
> one-time password, and that if it is correct then it will immediately be 
> invalidated, and if it is wrong then it doesn't matter.
> 
> So I'll withdraw my concerns on this section, except that maybe it could 
> be clearer. Specifically, section 3 could be more explicit about what 
> each distinct actor (poster, posting agent, moderator or injecting 
> agent) does.
> 
>>> [...]
>>> I didn't see a response from you on (4).
>>
>> Sorry for the delay, I was busy.
>>
>> | (4) Minor:
>> |
>> | In Section 8.1/8.2 the syntax/semantics of the Owner/Change
>> | controller is unspecified. But IANA is charged with 
>> allowing/rejecting change
>> | requests only from the "same" owner. How is IANA expected to verify
>> | this?
>>
>> If it is an experimental algorithm, I see no need for strong 
>> authentication
>> of external contributors.
>>
>> For the algorithms on the standards track, the IESG is defined as owner
>> (may authenticate itself to IANA with a signature).
>>
>> | What if a change is required but the original owner is no longer
>> | available?
>> | E.g., the owner might be specified by email address. Later the mail
>> | server for that email address may no longer exist.
>>
>> In this case the part "[...] where the owner of the registration [...]
>> has moved out of contact [...]" can be applied.
> 
> I won't belabor this point because it will clearly be an infrequent event.
> 
>      Thanks,
>      Paul
> 
> _______________________________________________
> Gen-art mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/gen-art
>

---

From: Brian E Carpenter <[email-masked]>
Date: Tue, 19 Sep 2017 16:53:10 +1200

> review and leave it for the IESG discuss it.

That's about all you can do. Of course if the IESG approves it anyway
you can appeal that decision. I'm not aware of any case where a Gen-ART
reviewer has needed to do that, but there's a first time for everything,
and the plenaries are boring if there are no appeals to rehash ;-)

Regards
   Brian

---

From: Alexey Melnikov <[email-masked]>
Date: Tue, 19 Sep 2017 10:06:14 +0100

Paul,

> Anybody have a better idea?

It is on my to do list *before* the telechat. I contacted Pete Resnick
and he suggested a slightly better fix to your erratum. This is just
taking time on my part.

Best Regards,
Alexey

---

From: Alexey Melnikov <[email-masked]>
Date: Tue, 19 Sep 2017 10:42:45 +0100

Hi Paul,

> I'm told by the authors that Alexey asked that it be submitted this way,

I asked the editor to submit a new version addressing all other
concerns. I wanted to see how the rest of the draft looks.
 
> and this draft has simply built on that mistake.

I was looking at your comment and it was very hard to figure out how big
of a deal it was. You are right that ABNF by itself will cause the
ordering issue that you reported. However, section 3.6 of RFC 5322 says
(ignoring for the moment trace header fields, that have some ordering):

   It is important to note that the header fields are not guaranteed to
   be in a particular order.  They may appear in any order, and they
   have been known to be reordered occasionally when transported over
   the Internet.  However, for the purposes of this specification,
   header fields SHOULD NOT be reordered when a message is transported
   or transformed.  More importantly, the trace header fields and resent
   header fields MUST NOT be reordered, and SHOULD be kept in blocks
   prepended to the message.  See sections 3.6.6 and 3.6.7 for more
   information.

So I think this already clear that header fields other than trace or
resent can appear in any order. This is effectively provides an extra
level of flexibility not covered by the ABNF from RFC 5322. As far as I
remember, none of the new header fields defined for Netnews articles are
trace or resent header fields.

> Unfortunately the root 

I think I agree with you that RFC 5322 could have introduced a few extra
ABNF productions to make extensibility easier.

However in short term, I think we should go with the easiest fix. Pete
suggested:

Section 3 of RFC 5536:

Original Text:

    fields          =/ *( approved /
                          archive /
                          control /
                          distribution /
                          expires /
                          followup-to /
                          injection-date /
                          injection-info /
                          lines /
                          newsgroups /
                          organization /
                          path /
                          summary /
                          supersedes /
                          user-agent /
                          xref )

Corrected Text:

    optional-field  =/ *( approved /
                          archive /
                          control /
                          distribution /
                          expires /
                          followup-to /
                          injection-date /
                          injection-info /
                          lines /
                          newsgroups /
                          organization /
                          path /
                          summary /
                          supersedes /
                          user-agent /
                          xref )

The name optional-field is a bit unfortunate, but it was always intended
for extensibility. So I think doing this change and adding a note saying
that some of Netnews header fields are mandatory (despite
"optional-field" production name) would be the best way forward.

> issue. But errata aren't often acted on quickly when they require 

Thank you for the erratum. Unless there is a better proposal, I will
edit resolution based on Pete's feedback and I will approve it.

> review and leave it for the IESG discuss it.

As per my comments above, I think we should acknowledge that there is a
subtle issue with ABNF in RFC 5536. I think the best way forward is to
approve some fix to your erratum and let
draft-baeuerle-netnews-cancel-lock be reviewed by IESG. At this moment
it is not clear to me that work on RFC 5536 will happen any time soon,
so I don't think we should hold this draft hostage.

Best Regards,
Alexey

---

From: Alexey Melnikov <[email-masked]>
Date: Tue, 19 Sep 2017 13:13:41 +0100

>     optional-field  =/ *( approved /

Just to clarify: optional-field is defined in RFC 5322 and is
specifically designed for extensibility.

We can also drop "*" in front of "(approved", as this field is already
allowed multiple times in an RFC 5322 message.

---

From: Paul Kyzivat <[email-masked]>
Date: Tue, 19 Sep 2017 10:00:38 -0400

Hi Alexey,

> trace or resent header fields.

I guess this can be read as providing some wiggle room, though in the 
context of the existing 5322 text this statement seems *consistent* with 
the ABNF, and so might not be read as providing extra latitude.

>      optional-field  =/ *( approved /

I see one issue with the above. <optional-field> appears *twice* in the 
definition of <fields> in 5322. I don't understand what the intent was 
there - whether it was a mistake or was trying to express something that 
I am missing. This really needs some further discussion. (E.g., should 
the valid values for <optional-field> as used with trace be distinct 
from those in its later appearance? This needs to be thrashed out with 
mail experts before this fix is finalized. I don't know what forum is 
appropriate for that.

Ignoring that, I agree this change to 5536 would achieve the goal 
without requiring a change in 5322, which is progress. However I think a 
tweak to the above would be be a bit cleaner:

     optional-field  =/    approved /
                           archive /
                           control /
                           distribution /
                           expires /
                           followup-to /
                           injection-date /
                           injection-info /
                           lines /
                           newsgroups /
                           organization /
                           path /
                           summary /
                           supersedes /
                           user-agent /
                           xref

This is definitely a better fix than I was suggesting. (Thank you Pete!)

> "optional-field" production name) would be the best way forward.

Even as it is currently written the *intent* of the current ABNF in 5536 
is that all of the netnews header fields be optional. And syntactically 
it allows them to be repeated multiple times but specifically forbids 
that in text. Section 3.1 uses text to specify that certain of those 
fields are actually mandatory. (Writing ABNF that allows fields to be in 
any order while making some at-most-once and some entirely optional is 
not practical.) So I don't think any new text is needed for this.

> so I don't think we should hold this draft hostage.

I don't see any reason to hold it hostage if there is a path forward 
that gets everything fixed. The work-around above can be used 
independently in 5536 and draft-baeuerle-netnews-cancel-lock, so they 
can proceed independently.

I will update my erratum to include the fix above, and write my genart 
telechat review to recommend the same fix.

	Thanks,
	Paul

---

From: Paul Kyzivat <[email-masked]>
Date: Tue, 19 Sep 2017 10:03:02 -0400

> allowed multiple times in an RFC 5322 message.

Oops. I replied to your earlier mail before reading this one thoroughly. 
We arrived at the same conclusion.

	Thanks,
	Paul

---

From: Alexey Melnikov <[email-masked]>
Date: Tue, 19 Sep 2017 15:23:54 +0100

Hi Paul,

On Tue, Sep 19, 2017, at 03:00 PM, Paul Kyzivat wrote:
> I am missing.

I believe it was entirely intentional. The first instance allows to add
new trace header fields (which should be kept together in groups), the
second allows adding other types of header fields.

> This really needs some further discussion. (E.g., should 
> from those in its later appearance?

Yes. It would have been better to have 2 separate productions, like
trace-optional-field and other-optional-field, but what Pete did seems
to be Ok.

> This needs to be thrashed out with 

I am not sure. Pete?

> This is definitely a better fix than I was suggesting. (Thank you Pete!)

Good.

> not practical.) So I don't think any new text is needed for this.

I doesn't hurt to point out, that some header fields are required to be
in Netnews articles. Maybe a pointer to the section where extra
restrictions are specified.

> telechat review to recommend the same fix.

If you can't do that, I can edit the erratum.

Thank you for raising the issue and for the followup discussion.

Best Regards,
Alexey

---

From: Pete Resnick <[email-masked]>
Date: Tue, 19 Sep 2017 13:41:35 -0500

> second allows adding other types of header fields.

Correct, that was the intention. In 5322, optional-field is a catchall 
for any new header field, so you need one for new trace fields and one 
for other fields. Otherwise, there's no way to put a new field between 
two trace fields. This was a fix in 5322 from 2822.

> trace-optional-field and other-optional-field, but what Pete did seems

Yes, that might have been nice, but putting extensibility syntax 
throughout the grammar starts to get ugly. (Imagine 
resent-optional-field, originator-optional-field, etc.) I think just one 
is fine.

>> mail experts before this fix is finalized. I don't know what forum is

Probably ietf-822, but (a) I personally haven't read the list in a very 
long time, and (b) I don't think there's anything terribly controversial 
about the change.

>> This is definitely a better fix than I was suggesting. (Thank you 

You're very welcome. I am equally fine with Alexey and Julien's version:

      optional-field  = <see RFC 5322 Section 3.6.8>

      news-fields     =     approved /
                            archive /
                            control /
                            distribution /
                            expires /
                            followup-to /
                            injection-date /
                            injection-info /
                            lines /
                            newsgroups /
                            organization /
                            path /
                            summary /
                            supersedes /
                            user-agent /
                            xref

      optional-field  /=    newsfields

pr
-- 
Pete Resnick <http://www.qualcomm.com/~presnick/>
Qualcomm Technologies, Inc. - +1 (858)651-4478

---

From: Julien ÉLIE <[email-masked]>
Date: Tue, 19 Sep 2017 21:19:04 +0200

Hi all,

 > optional-field  /=    news-fields

This change looks great (with typo fixed for "news-fields" instead of 
"newsfields").

Please note that the first line about <optional-field> updates Section 
1.4 of RFC 5536 (the line defining <fields> is changed to the line 
defining <optional-field>).

The other lines update Section 3 of RFC 5536.  The introduction of 
Section 3 should also refer to Section 3.6.8 of RFC 5322 instead of 
Section 3.6.

-- 
Julien ÉLIE

« Vanitas uanitatum et omnia uanitas. » (Ecclésiaste)

---

From: Julien ÉLIE <[email-masked]>
Date: Tue, 19 Sep 2017 21:30:21 +0200

Hi Alexey,

> so I don't think we should hold this draft hostage.

No work on RFC 5536 is scheduled in a near future.
The next revision will probably be when we deal with Netnews 
internationalization (UTF-8 newsgroup names, UTF-8 distributions...). 
It may be in a separate RFC or in a rewrite of RFC 5536 (in case RFC 
5322 is also rewritten).
Also, the USEFOR WG began a BCP for Netnews but the WG concluded before 
the task finished.  That is the next consequent work to do.
   https://datatracker.ietf.org/doc/draft-ietf-usefor-useage/

-- 
Julien ÉLIE

« De mourir, ça ne me fait rien. Mais ça me fait peine de quitter la
   vie. » (Marcel Pagnol)

---

From: Paul Kyzivat <[email-masked]>
Date: Tue, 19 Sep 2017 16:02:12 -0400

>     trace-optional-field and other-optional-field, but what Pete did seems

This probably wasn't *too* bad as long as <optional-field> was simply 
defined as:

    optional-field  =   field-name ":" unstructured CRLF

Even then, ideally there should have been some words to indicate that 
the <field-name>s used as extensions for trace be disjoint from those 
used as non-trace extension headers.

It gets more ugly when we start to extends <optional-field> 
syntactically via =/. Now, when the syntax is evaluated the new header 
fields we are defining show up as explicitly syntactically valid in both 
places. :-(

> resent-optional-field, originator-optional-field, etc.) I think just one 

Since there is an IANA registry, another possibility would be to not 
attempt to revise the syntax from 5322 at all. Instead, simply rely on 
the the extensibility already there via <optional-field> and register 
all the new header fields in the IANA registry. (Use ABNF to define the 
syntax of the individual new headers but not their linkage into the 
message.)

Then use text to specify conditions about where they may appear in a 
message.

	Thanks,
	Paul

> |optional-field = <see RFC 5322 Section 3.6.8> news-fields = approved / 
> archive / control / distribution / expires / followup-to / 
> injection-date / injection-info / lines / newsgroups / organization / 
> path / summary / supersedes / user-agent / xref optional-field /= 
> newsfields |
> 
> pr
> -- 
> Pete Resnick http://www.qualcomm.com/~presnick/ 
> <http://www.qualcomm.com/%7Epresnick/>

---

From: Julien ÉLIE <[email-masked]>
Date: Tue, 19 Sep 2017 22:17:50 +0200

Hi Paul,

> fields we are defining show up as explicitly syntactically valid in both 

In the erratum for RFC 5536, a sentence may be added to say that news 
header fields defined by this document are not trace header fields, in 
the terminology of RFC 5322.

-- 
Julien ÉLIE

« You know it's love when all you want is that person to be happy;
   even if you're not part of their happiness. »

---

From: Alexey Melnikov <[email-masked]>
Date: Wed, 20 Sep 2017 10:57:29 +0100

Hi Paul,

> On 19 Sep 2017, at 21:02, Paul Kyzivat <[email-masked]> wrote:
> 
>> Correct, that was the intention. In 5322, optional-field is a catchall for any new header field, so you need one for new trace fields and one for other fields. Otherwise, there's no way to put a new field between two trace fields. This was a fix in 5322 from 2822.
> This probably wasn't *too* bad as long as <optional-field> was simply defined as:
> 
> Even then, ideally there should have been some words to indicate that the <field-name>s used as extensions for trace be disjoint from those used as non-trace extension headers.
> 
> It gets more ugly when we start to extends <optional-field> syntactically via =/. Now, when the syntax is evaluated the new header fields we are defining show up as explicitly syntactically valid in both places. :-(
> 
>> Yes, that might have been nice, but putting extensibility syntax throughout the grammar starts to get ugly. (Imagine resent-optional-field, originator-optional-field, etc.) I think just one is fine.
> 
> Since there is an IANA registry, another possibility would be to not attempt to revise the syntax from 5322 at all.

On my memory most of new header fields are defined without extending "optional-field" production. So people are already doing what you propose.

> Instead, simply rely on the the extensibility already there via <optional-field> and register all the new header fields in the IANA registry. (Use ABNF to define the syntax of the individual new headers but not their linkage into the message.)
> 
> Then use text to specify conditions about where they may appear in a message.

Right.

>> Probably ietf-822, but (a) I personally haven't read the list in a very long time, and (b) I don't think there's anything terribly controversial about the change.
>> |optional-field = <see RFC 5322 Section 3.6.8> news-fields = approved / archive / control / distribution / expires / followup-to / injection-date / injection-info / lines / newsgroups / organization / path / summary / supersedes / user-agent / xref optional-field /= newsfields |
>> pr
>> -- 
>> Pete Resnick http://www.qualcomm.com/~presnick/ <http://www.qualcomm.com/%7Epresnick/>

---

From: Paul Kyzivat <[email-masked]>
Date: Wed, 20 Sep 2017 11:22:37 -0400

>> Then use text to specify conditions about where they may appear in a message.

OK. Barring further comments to the contrary I will revise my review 
(and the erratum) to recommend this approach.

	Thanks,
	Paul