[quagga-dev 10545] Re: RFC-6506(Supporting Authentication Trailer for OSPFv3) implementation in quagga-0.99.21 version
infrastation at yandex.ru
Wed May 29 09:25:30 BST 2013
09.05.2013, 09:46, "Lokesh Pareta" <lokesh.pareta at tcs.com>:
> Hi All,
> Tata Consultancy Services (TCS) wants to contribute to Quagga development by providing the implementation code for RFC-6506, developed and tested on quagga-0.99.21 version.
Below is a cursory review of the patch, which the author of this work may use to improve it further.
RFC6506 belongs to a series of works on routing protocols authentication using cryptographic hash algorithms. My personal experience in implementing such authentication in Quagga code base stands for:
* implementation of RIPv2 authentication (http://tools.ietf.org/html/rfc4822), annotated in detail in http://marc.info/?l=quagga-dev&m=135244834410653
* design and implementation of Babel authentication (http://tools.ietf.org/html/draft-ovsienko-babel-hmac-authentication)
The commits standing for these two works are mapped here: https://github.com/Quagga-RE/quagga-RE/wiki/hashes
It is possible to see that these prior works establish and use a common internal API to cryptographic hash algorithms implemented in a library, libgcrypt in this case. This approach provides agility without the cost of duplicating and maintaining the source code of each hash algorithm in use. Agility matters within the scope of this work, because RFC6506 sets SHA-256 as the mandatory-to-implement, but not the only possible hash algorithm. The contents of sha256.c and sha256.h together with much of the code in ospf6_check_sha256_digest() and ospf6_make_sha256_digest(), which implement the construct specified in RFC6506 (and derived from RFC4822), can be replaced with a call to hash_key_compress_rfc4822() and hash_make_hmac().
Respectively, it makes sense to review the CLI syntax of the RFC6506 patch to reflect SHA-256 as just a particular case of the cryptographic authentication (see CLI syntax of the two implementations above for an example). As a related point, in the patch authentication is configured not only per interface but also per area. RFC6506 defines configuration and operation of the authentication mechanism per interface and it seems reasonable to omit the per area extension until the basic per interface code is completed.
There is a few issues with the code between the CLI and the hash algorithms:
* The sequence number check in ospf6_check_sha256_digest() is taken wrong and will reject valid packets.
* ospf6_make_sha256_digest() would default to an empty string key when authentication is configured without any keys.
* Many of the changes to ospf6_packet_examin() don't belong there, the purpose of that function is to validate packet framing, but not the authenticity. This should be better seen looking at the counterpart xxx_examin() functions and respective authentication layer of ospfd and (especially) ripd.
* The set of authentication key management functions added to ospf6_interface module doesn't look as good as the existing functions of lib/keychain module.
* In the new ospf6_crypt_key structure the key size is limited to the digest length.
Stylistic issues with the patch include:
* ospf6_auth_type() code should be cleaned.
* Some of the code uses hardcoded numbers instead of the symbolic constants that come within the same changeset.
* The changes to Makefile.in shouldn't be in the patch since this file is auto-generated from Makefile.am.
* A notable share of the new code lacks proper indentation and introduces trailing whitespace and empty lines.
More information about the Quagga-dev