BIND 10 #781: Define cryptographic API
BIND 10 Development
do-not-reply at isc.org
Wed Apr 20 15:30:18 UTC 2011
#781: Define cryptographic API
-------------------------------------+-------------------------------------
Reporter: | Owner: jinmei
stephen | Status: reviewing
Type: task | Milestone:
Priority: | Sprint-20110503
blocker | Resolution:
Component: | Sensitive: 0
Unclassified | Sub-Project: DNS
Keywords: | Estimated Difficulty: 6.0
Defect Severity: N/A | Total Hours: 0
Feature Depending on Ticket: |
Add Hours to Ticket: 0 |
Internal?: 0 |
-------------------------------------+-------------------------------------
Changes (by jelte):
* owner: jelte => jinmei
* subproject: => DNS
* severity: => N/A
Comment:
Replying to [comment:19 jinmei]:
> >
> <snip discussion on usage>
>
> But anyway, we cannot just be so sure how it would actually be used
> yet, so I don't necessarily request this interface to be removed at
> this stage.
>
ok, let's keep that in mind for when we finalize this
>
> Frankly, libb10crypto sounds a bit awkward:-) but I must confess I'm
> not good at naming things. I just come up with a generic name
> "libcryptolink", but since at least b10crypto is much less confusing
> I'm okay with it, too if you like it better.
>
Yeah... cryptolink is fine actually. For consistency, I've renamed not
only the library file, but also the directory, namespace, main filename
and main class name. So you may need to do a bit of rebuilding :p
Oh i've also moved HMAC stuff into crypto_hmac.[cc|h].
> Okay, we may want to convert string/name to the crypto definition at
> the time of TSIGKey construction, but that's another issue.
>
Yes, we could change TSIGKey to not have a Name member, but a
HashAlgorithm, and make the getter perform a switch and return the static
Name for the specific algorithms.
> <snip discussion on initialization and raii>
Okay, so here's the other Big change; We now provide a singleton factory
object, that is automatically initialized on the first call to
CryptoLink::getCryptoLink() (which is the only call that will give you
access to anything), but can be initialized manually beforehand (which
isn't necessary right now but will be later if we want to pass
initialization arguments). This object is the only entry point for all the
classes in the cryptolink namespace. Right now that is only HMAC. So it
contains a createHMAC object (which returns a raw pointer, because HMAC
itself is noncopyable, i've added documentation that suggests to place it
into a scoped_ptr).
Another 'protection' mechanism is that HMAC itself is not directly
constructable; it's constructor is private. Only CryptoLink can create
them now (i've made it a friend). And the intention is to do this for all
classes we make that wrap backend library objects (i.e. all should be
nonconstructible, and probably noncopyable as well, and have a factory
function in CryptoLink if they must be created by the caller).
> > > - HMACImpl constructor: this is not exception safe (hmac_ could
leak).
> >
> > added delete
>
> Ack, but to be super exception safe, I'd rather (e.g.) use
> boost::scoped_ptr. I don't know the details of the Botan internals,
> but, for example, if it internally performs (throwing version of) new
> and doesn't convert the standard exceptions, the revised code is still
> not exception safe. Using RAII like scoped_ptr will ensure the
> safeness regardless of the Botan internals. (And if we use scoped_ptr
> we don't need an explicit destructor)
>
fair enough, done.
> > > - HMACImpl::update/sign/verify: can we assume the underlying Boton
> > > methods don't throw?
> >
> > only invalidkeylength, afaict, should we add 'general'
> > Botan::Exception catches?
>
> It might depend on the internal details of Botan, but as long as we
> want to prevent any Botan specific exceptions from being propagated,
> I think it would be safer to catch larger exceptions. That way we can
> ensure that even if the Botan implementation changes in its future
> versions.
>
I've added one more excpetion class, LibraryError, which is to be thrown
if anything 'unexpected' happens; in this case whenever we see
Botan::Exception being raised, and I've added a catch for Botan::Exception
around all calls to botan.
> >
> > That would introduce a build-order-dependency conflict again :)
>
> Yeah I know. So for now it probably doesn't make sense to use
> matchWireData. In future I think we should extract non DNS specific
> test utilities into a shared place so that it can be used by other
> modules.
>
+1
>
> Now some more code level comments on the revised one:
>
> '''crypto.cc'''
> - HMACImpl::verify: is the "len > getOutputLength()" case tested?
> (just checking)
they are now
> - HMAC::HashAlgorithm I'd add doxygen description for the enums (even
> though the meaning is pretty clear from the symbol). also, I'd like
> to see clarification of what "UNKNOWN" means (e.g. whether it's just
> a placeholder and always trigger some error, or if it can be used
> in some valid scenario)
>
ack. There is, added comments.
> '''crypto/tests'''
> - in checkData(), expected/actual parameters to ASSERT_EQ() still seem
> to be reversed (maybe it's helpful to name them "expected_data",
> "actual_data" instead of "data1"/"data2")
good point, done :)
> - doHMACTestArray() uses a variable length array, which is
> (unfortunately) not very portable. considering this is a test,
> maybe we can just allocate a memory dynamically, noting it's not
> exception safe as a comment (we could also use
> std::vector/boost::scoped_array)
While i think tests should be clean, I happen not to care too much about
exception safety (is there is any unexpected exception, there is something
to be fixed in the test or the code anyway :)), so that should be ok here.
> - about secret/expected data (I should have brought this up in the
> first review, sorry for the raising a new topic): the arrays can be
> const uint8_t[]. as for std::string data, shouldn't it actually
> better be vector<uint8_t>? In either case, the initialization using
> the for loop could be simplified:
> {{{
> const std::string data3(50, 0xdd);
> // std::string data3;
> // for (int i = 0; i < 50; ++i) {
> // data3.push_back(0xdd);
> //}
> }}}
> (as a bonus we can also make the variable const this way)
wow i did not even know that existed. done.
>
> '''tsigkey test'''
>
> - I'd use a different name than "TSIGTest" (for TSIGKeyFromToString)
> because I'd like to use it for TSIG sign/verify (I hope it's fair to
> say "TSIGTest" is more suitable for these kinds of tests:-). google
> test doesn't seem to allow name conflict at this level.
oh sure, made it TSIGStringTest
>
> '''tsigkey.cc'''
> - TSIGKey from string constructor: (I should have pointed this out in
> the first review) you may want to make it an 'explicit' constructor.
ok
> - TSIGKey from string constructor: this seems to be incorrect:
> {{{
> } else {
> pos2 = str.size() - pos;
> }
> }}}
> You can realize the bug by making the len(key name) <= len(secret):
> {{{
> EXPECT_EQ("example.:MSG6Ng==:hmac-md5.sig-alg.reg.int.",
> TSIGKey("example.:MSG6Ng==").toText());
> }}}
> We could fix it in a straightforward way, but I actually wonder
> whether we could achieve what we want using a higher level
> interface, rather than via lower level of "position + length" APIs,
> which are generally error prone and difficult to understand.
> So, for example, how about the attached diff? This one is not that
> concise compared to the original code, but the logic should be more
> intuitive and much safer.
that patch was from before you did the const changes, so it didn't apply
:) But it's merged.
> - And, in either case, it would be good if we extract the alg_name
> check used in the two constructors.
right, done
A lot has changed again, sorry for that :)
(but i did run cppcheck this time ;))
--
Ticket URL: <http://bind10.isc.org/ticket/781#comment:22>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list