Handling of Identity Header Errors for Secure Telephone Identity Revisited (STIR)
draft-ietf-stir-identity-header-errors-handling-08

Thank you for the work on this document.

Many thanks to Martin Dürst for his ART ART review: https://mailarchive.ietf.org/arch/msg/art/Jqiy69abgnAMBuSH7oLpt1Ae62I/, and to the author for addressing his comments.

Thanks for this short, readable, and to the point document.

I did find a couple of nits. In both cases I leave it to you to decide if a change is called for, no need to reply.

1. In Section 5, I had trouble with this sentence:

"As implied and defined in [RFC8224], error codes associated with STIR targeted at authentication services that produced a specific identity header field represent a single error occurring with the verification and processing of that identity header field."

I'm willing to believe that for someone expert in STIR, SIP, et al, that sentence might be parsable into something that makes sense. It defied me, though, so I mention it in case you want to re-word it.

2. In Section 6, isn't this a comma splice and it should really be two sentences?

"As mentioned previously, the potential use of multiple Reason header fields defined in [RFC3326] is updated in [I-D.ietf-sipcore-multiple-reasons] allowing multiple Reason header fields with the same protocol value, for this specification "STIR" should be used for any STIR error defined in [RFC8224] or future specifications."

# Sec AD review of draft-ietf-stir-identity-header-errors-handling-07

CC @paulwouters

See https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions
for more information about how to handle DISCUSS and COMMENT positions.

This review uses the format specified in https://github.com/mnot/ietf-comments/
which allows automated tools to process items (eg to produce github issers)

## Comments

### "ppi" parameter

```
   The association of a Reason header field
   and error to a specific Identity header field is accomplished by
   adding a PASSporT identifier, "ppi", parameter containing the
   PASSporT string as an identifier for the identity header and
   corresponding PASSporT that generated the error to the Reason header
   field.  The "ppi" parameter for the Reason header field is
   RECOMMENDED in particular [...] 
```
It feels like the "is accomplished" really means the "ppi" parameter MUST
be used, yet further on it states the "ppi parameter" is only RECOMMENDED ?
Should this RECOMMENDED not be REQUIRED? Also the construct of
"RECOMMENDED in particular" is a little odd.

Why not:

For a SIP INVITE containing multiple Identity header fields, the
"ppi" parameter for the Reason header field is REQUIRED.


### 7 lines is a lot for a single sentence

```
   In cases where local policy dictates that a call should continue
   regardless of any verification errors that may have occured,
   including 4XX errors described in [RFC8224] Section 6.2.2, then the
   verification service MUST NOT send the 4XX as a response, but rather
   include the error response code and reason phrase in a Reason header
   field, defined in [RFC3326], in the next provisional or final
   responses sent to the authentication service.
```

This sentence does not parse (due to the ", then the"). It is also 7 lines
long so it would improve readability to split this in various sentences.

# Éric Vyncke, INT AD, comments for draft-ietf-stir-identity-header-errors-handling-07

CC @evyncke

Thank you for the work put into this document. 

Please find below one nit.

Special thanks to Ben Campbell for the shepherd's detailed write-up including the WG consensus and the justification of the intended status. 

I hope that this review helps to improve the document,

Regards,

-éric


## NITS

### Placement of ';' in the examples

Just to exhibit my lack of SIP/STIR knowledge, I was wondering about the placement of ';' in the different examples, e.g.,
```
Reason: STIR ;cause=436 ;text
Reason: STIR ;cause=438 ; text
```

I guess that this is not a SIP violation but it looks weird to the reader.

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. 

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments