BIND 10 #782: Implement cryptographic API using Botan

[BIND 10 Development]

#782: Implement cryptographic API using Botan
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  hanfeng
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20110517
                   Priority:  major  |            Resolution:
                  Component:         |             Sensitive:  0
  Unclassified                       |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  3.0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:  tsig   |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => hanfeng


Comment:

 Hello

 Replying to [comment:11 hanfeng]:
 > > The use of macros is generally avoided inside the Bind 10 code. And as
 the standard guarantees that size of uint8_t is 1, you could directly use
 sizeof instead of the macro, which is IMO cleaner.
 > For macro #define C_ARRAY_LEN(array) (sizeof(array)/sizeof(array[0]))
 which used to calculate the
 > c array length, I don't know what't wrong with it. Macro does have some
 problem, but it can save a lot
 > of code without decrease the readability, and in test code i think it's
 ok. Since by default all the
 > secret is uint_8 array, I will just use sizeof.

 Well, the problem with this specific macro is, while returns the same
 numbers in this case, it had different meaning. While sizeof is number of
 bytes, the macro was number of elements, which is different. So even when
 the code was correct functionally-wise, it confused the people who tried
 to read it.

 And macros have some problems, but I believe it's mostly a style issue in
 our code.

 > >  * The test function is rather long and it tests 3 different
 algorithms. If one of them failed, we wouldn't know which one of them it
 is. May I suggest splitting the test into 3?
 > I have split it into three and do some macro thing to make the code as
 short as possible.

 I see. You really seem to like macros. I don't think they are so bad to
 stop from merging, but they don't look nice.

 > >  * This is not comment directly to your code, but the UNKNOWN_HASH
 constant is the next number, which gets increased every time a new
 algorithm is added. But let's assume there was an older ...
 > If you are talking about the so/dll lib compatibility, it always has
 some kind of problem if you link wrong library, except all the classes we
 exposed all use the "impl" idom. And Actually I don't know the reason why
 we need specify the number for the algorithm since they are different from
 the number in DNS protocol. And the lib user shouldn't assume any
 specified integer associated with one algorithm.

 Well, the problem I see is, as the number is in header file, it is
 compiled into the code. So while the source code doesn't assume any
 specific integer (I don't know why the specific integers are written in
 the source code anyway, but it seems to be in our style guide as well),
 the compiled binary code does use the specific number. And if the wrong
 version is used, no incompatibility happens, no crash, it would just not
 understand each other and kept running, which would be quite hard to
 debug. And as the fix to this problem is really simple, I don't see reason
 not to put it there. I don't say it solves everything, I just say the
 ratio between effort to avoid the problem and problems that could happen
 in case it happened is definitely on the side of including the solution
 before it happens.

 Anyway, I noticed one more thing. The backend hashing can do the
 algorithms now, but we can't create the keys for them. In dns/tsigkey,
 there's need for the names of the keys and recognition of the names. There
 are some constants in the python bindings for them as well. And maybe the
 TSIG RR has some constants? Could you have a look at them as well, please?
 Or is there another ticket for it?

 Thanks

-- 
Ticket URL: <http://bind10.isc.org/ticket/782#comment:13>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development