[asterisk-dev] [Code Review] 4420: testsuite: Add nominal and off-nominal SRTP negotiation tests for key lifetime/MKI

[Matt Jordan]

> On Feb. 18, 2015, 2:58 p.m., Mark Michelson wrote:
> > /asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/sipp/inject_attrib_single_nominal.csv, lines 7-8
> > <https://reviewboard.asterisk.org/r/4420/diff/1/?file=71424#file71424line7>
> >
> >     I have no idea what difference this makes on the tests, but why do these two lines not end in semicolons, when all of the others do?

It doesn't make a difference since the ';' is a delimiter and nothing uses the second field. I'll make it consistent.


> On Feb. 18, 2015, 2:58 p.m., Mark Michelson wrote:
> > /asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/test-config.yaml, lines 10-13
> > <https://reviewboard.asterisk.org/r/4420/diff/1/?file=71429#file71429line10>
> >
> >     I think that the lifetime checks should include the bottom and top of the allowed range.

Bottom of the allowed range for sure. The top is a little hard, since there is no max lifetime.


> On Feb. 18, 2015, 2:58 p.m., Mark Michelson wrote:
> > /asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/test-config.yaml, line 24
> > <https://reviewboard.asterisk.org/r/4420/diff/1/?file=71429#file71429line24>
> >
> >     I think it would be worthwhile to have some double-crypto specific off-nominal tests. Namely to check for duplicated items between lines of crypto that should be unique (such as crypto tags).

We don't guarantee uniqueness of the tags, but that's mostly because our handling of crypto keys/tags is rudimentary.

When we have two crypto lifetime attributes, we accept the first attribute and punt on the rest. We don't actually track or remember more than a single crypto atatribute. Thus, if we have duplicate valid tags, we'd never detect it in the first place.


> On Feb. 18, 2015, 2:58 p.m., Mark Michelson wrote:
> > /asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/test-config.yaml, lines 36-37
> > <https://reviewboard.asterisk.org/r/4420/diff/1/?file=71429#file71429line36>
> >
> >     I think your short lifetimes should be one-off from the permitted minimum to verify we have no off-by-one errors in the code.
> >     
> >     Also, I think you should have cases for declining long lifetimes, preferably as close to one-off from the maximum allowance as possible.

We don't have a maximum allowed lifetime.

I can update the small lifetime checks to reflect off by one errors. That would be 2^20 (which is < 1800000) and 1799999.


- Matt


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4420/#review14489
-----------------------------------------------------------


On Feb. 13, 2015, 9:26 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4420/
> -----------------------------------------------------------
> 
> (Updated Feb. 13, 2015, 9:26 p.m.)
> 
> 
> Review request for Asterisk Developers and Olle E Johansson.
> 
> 
> Bugs: ASTERISK-17721, ASTERISK-17899 and ASTERISK-22748
>     https://issues.asterisk.org/jira/browse/ASTERISK-17721
>     https://issues.asterisk.org/jira/browse/ASTERISK-17899
>     https://issues.asterisk.org/jira/browse/ASTERISK-22748
> 
> 
> Repository: testsuite
> 
> 
> Description
> -------
> 
> This patch adds nominal and off-nominal tests for SDP negotiation of SDES-SRTP crypto attributes, for both chan_sip and chan_pjsip.
> 
> Specifically:
> 
> * Moved the tests/channels/SIP/sip_srtp test to tests/channels/SIP/sip_srtp/srtp_call. This change was done merely to allow for additional SRTP tests for chan_sip, and is not part of this review. Note that due to a quirk of Review Board, the moved files don't show up in the review.
> 
> * Added tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer. This covers both nominal and off-nominal SDP offers with SDES-SRTP for chan_sip. Note that the scenarios use injection files to vary the crypto attributes.
> 
> * Updated tests/channels/PJSIP/srtp_negotiation.
>   - All but two off-nominal scenarios were moved into a single off nominal scenario, decline.xml. This uses the same injection file for off nominal parameter testing as the chan_sip variant.
>   - Updated accept_nominal.xml to use an injection file to test multiple nominal values.
>   - Updated accept_multiple_attrib_first_bad.xml to not use a crypto attribute with lifetime as a 'bad' attribute.
> 
> 
> Diffs
> -----
> 
>   /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/test-config.yaml 6415 
>   /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/decline_with_lifetime.xml 6415 
>   /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/decline_unknown_suite.xml 6415 
>   /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/decline_no_tag.xml 6415 
>   /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/decline_no_suite.xml 6415 
>   /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/decline_no_key.xml 6415 
>   /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/decline.xml PRE-CREATION 
>   /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/decline.csv PRE-CREATION 
>   /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/accept_nominal.xml 6415 
>   /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/accept_multiple_attrib_first_bad.xml 6415 
>   /asterisk/trunk/tests/channels/pjsip/srtp_negotiation/sipp/accept.csv PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/tests.yaml 6415 
>   /asterisk/trunk/tests/channels/SIP/sip_srtp/test-config.yaml 6415 
>   /asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/test-config.yaml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/sipp/srtp-invite-single-crypto.xml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/sipp/srtp-invite-off-nominal-crypto.xml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/sipp/srtp-invite-double-crypto.xml PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/sipp/inject_attrib_single_off_nominal.csv PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/sipp/inject_attrib_single_nominal.csv PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/sipp/inject_attrib_double_nominal.csv PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/configs/ast1/sip.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/configs/ast1/extensions.conf PRE-CREATION 
>   /asterisk/trunk/tests/channels/SIP/sip_srtp/run-test 6415 
>   /asterisk/trunk/tests/channels/SIP/sip_srtp/configs/ast2/sip.conf 6415 
>   /asterisk/trunk/tests/channels/SIP/sip_srtp/configs/ast2/extensions.conf 6415 
>   /asterisk/trunk/tests/channels/SIP/sip_srtp/configs/ast1/sip.conf 6415 
>   /asterisk/trunk/tests/channels/SIP/sip_srtp/configs/ast1/extensions.conf 6415 
> 
> Diff: https://reviewboard.asterisk.org/r/4420/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150220/3e2ad123/attachment-0001.html>