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

Wed Feb 18 14:58:31 CST 2015

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

Comments regarding recommended test cases for the chan_sip test also apply to the PJSIP test.

/asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/sipp/inject_attrib_single_nominal.csv
<https://reviewboard.asterisk.org/r/4420/#comment25031>

    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?

/asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/sipp/srtp-invite-double-crypto.xml
<https://reviewboard.asterisk.org/r/4420/#comment25036>

    The regex being used here has some issues. 

    First the bit towards the end for [32|80] will match values you don't want it to. Square brackets defines a character class, which matches only a single character. I *think* that this would match a single 3, a single 2 or 8, or a single 0. This means that the expected valid values of 32 or 80 would be accepted, but it also means invalid values of 3, 0, 81, 39, etc. would be accepted.

    Also, .* being used afterwards means that completely invalid values like AES_CM_128_HMAC_SHA1_32HIMOM would be accepted. Asterisk should be placing a space after the crypto suite in all of its 200 OK responses, so this space should be included in the regex.

    The regex should be

    "a=crypto[12] AES_CM_128_HMAC_SHA1_(31|80) .*"

/asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/test-config.yaml
<https://reviewboard.asterisk.org/r/4420/#comment25030>

    I think that the lifetime checks should include the bottom and top of the allowed range.

/asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/test-config.yaml
<https://reviewboard.asterisk.org/r/4420/#comment25037>

    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).

/asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/test-config.yaml
<https://reviewboard.asterisk.org/r/4420/#comment25034>

    You have missing key, 0-length key, and partial key, but it is probably a good idea to have an invalidly formatted key (e.g. missing "inline", missing colon).

/asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/test-config.yaml
<https://reviewboard.asterisk.org/r/4420/#comment25035>

    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.

/asterisk/trunk/tests/channels/SIP/sip_srtp/srtp_sdp_offer_answer/test-config.yaml
<https://reviewboard.asterisk.org/r/4420/#comment25032>

    Maybe add a test case for an invalidly formatted MKI (e.g. non-integer, colon missing)?

- Mark Michelson

On Feb. 14, 2015, 3:26 a.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4420/
> -----------------------------------------------------------
> 
> (Updated Feb. 14, 2015, 3:26 a.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/20150218/aef71f36/attachment-0001.html>