Project

General

Profile

Task #2306

Adjust KeyChain exception handling

Added by Yingdi Yu over 4 years ago. Updated almost 3 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Security
Target version:
Start date:
12/16/2014
Due date:
% Done:

100%

Estimated time:

Description

KeyChain and related classes may throw some exceptions.
Some exceptions, however, could be avoided.

For example, when getting a public key which does not exist, current implementation throws an exception.
However, since the return value is a shared pointer to the public key, it is more reasonable to return a empty pointer in this case.

Another example is getting default settings in PIB.
Current implementation throws exceptions if the requested default setting is not available.
However, since neither key name nor certificate name could be empty, an empty name as a return value can be a good indicator that these two types of default settings are not available.
For default identity, since now PIB has at least one identity "/localhost/identity/digest-sha256", when default identity is not set, PIB can automatically set "/localhost/identity/digest-sha256" as the default identity.

The third example is signing.
Current implementation may throw exception when the requested signing identity or key or certificate is not available.
With the "/localhost/identity/digest-sha256", if the situation above happens, DigestSha256 will be used to "sign" packets.
The purpose is that signing should never fail, even if a weak signature is used.
Therefore, an application should check if its keychain is well configured during the initialization stage.


Related issues

Blocked by ndn-cxx - Feature #2451: New Abstraction for Identity Key CertificateClosed2015-01-29

Blocks ndn-cxx - Task #2926: Refactor KeyChainClosed

History

#1 Updated by Yingdi Yu over 4 years ago

When retrieving a public key of certificate from PIB, if the requested object does not exist, return a nullptr rather than throwing an exception.

When retrieving the default setting from PIB (e.g., default key name or default certificate name), return an empty name rather than throwing an exception.

When checking the existence of a key or a certificate in PIB, if the search name is malformed, return false rather than throwing an exception.

When signing a packet, if the required key or certificate does not exist, sign packets using sha-256 rather than throw an exception.

#2 Updated by Junxiao Shi over 4 years ago

When retrieving a public key of certificate from PIB, if the requested object does not exist, return a nullptr rather than throwing an exception.

This is reasonable.

When retrieving the default setting from PIB (e.g., default key name or default certificate name), return an empty name rather than throwing an exception.

This is ambiguous because ndn:/ is a valid identity Name.

A better approach is changing return type to shared_ptr<Name> or std::pair<bool, Name>, and explicitly indicate the non-existence of a setting by returning nullptr or {false, ignored}.

When checking the existence of a key or a certificate in PIB, if the search name is malformed, return false rather than throwing an exception.

This looks okay.

When signing a packet, if the required key or certificate does not exist, sign packets using sha-256 rather than throw an exception.

This is dangerous. The caller expects the packet to be signed with a strong signature when identity isn't set to #1705 special value, but the library fails to deliver this result.

#3 Updated by Yingdi Yu over 4 years ago

For the second change, since key name and certificate name can never be empty, therefore, it's safe to use an empty name as the non-existing result.

For the fourth change, I can revert it back to have throw exception, if the explicitly specified key or certificate does not exist.

#4 Updated by Junxiao Shi over 4 years ago

For the second change, since key name and certificate name can never be empty, therefore, it's safe to use an empty name as the non-existing result.

I'm specifically talking about KeyChain::getDefaultIdentity(). ndn:/ is a valid identity.


In any case, this is a potentially breaking change because it changes the semantics of these methods. Once the design is finalized, send a notice to nfd-dev and ndn-lib, and give 7 days for dependent projects to make necessary changes.

#5 Updated by Yingdi Yu over 4 years ago

For that particular one, I did not change the behavior (note that I only mentioned key and certificate, not identity in this case), if there is no default identity, error will still be thrown.

#6 Updated by Yingdi Yu over 4 years ago

I would like to send the notification a little bit later (to include the changes on validation callback), so that people do not have to change their code twice.

#7 Updated by Junxiao Shi over 4 years ago

This is a breaking change that needs a 5-day notice. Given that there are more related breaking changes, I suggest creating a separate branch for all these breaking changes.

PIB API is also in need of some refactoring:

  • [out] vectors shall be moved to return value, because C++11 supports move semantics on vectors, so returning a vector is no longer inefficient.
  • Most "public virtual" methods should be avoided. Instead, declare a public non-virtual method that invokes a private virtual method, to separate external API from internal structure. See http://www.spatial.com/blog/public-virtual-methods-bad-idea
  • Declarations shall be reordered:

    public:
      constructors
      methods that don't fit in any category below
    public: // identity
      doesIdentityExist
      getDefaultIdentity
      addIdentity
      removeIdentity
      setDefaultIdentity
    public: // key
      ..
    public: // certificate
      ..
    

#8 Updated by Yingdi Yu over 4 years ago

Given there could a lot of API changes (not only this handling and those Junixiao mentioned, but also some other changes I would like to make), I agree it would be better to open a new branch for this API change. I will break the gerrit commit http://gerrit.named-data.net/#/c/1536/ into two, the one for sha-256 identity will be left in master branch, the rest (for exception handling) will be moved to the API branch.

#9 Updated by Yingdi Yu about 4 years ago

  • Blocked by Feature #2451: New Abstraction for Identity Key Certificate added

#10 Updated by Yingdi Yu about 4 years ago

#11 Updated by Alex Afanasyev over 3 years ago

  • Target version set to v0.5

#12 Updated by Yingdi Yu almost 3 years ago

  • Status changed from Code review to Closed

Also available in: Atom PDF