Project

General

Profile

Feature #2922

Helpers to create SigningInfo

Added by Alex Afanasyev over 5 years ago. Updated about 5 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Security
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:
3.00 h

Description

Currently, it requires too much typing to write a signing command.
For example, to sign with a cert name, one has to write

keyChain.sign(packet, security::SigningInfo(security::SigningInfo::SIGNER_TYPE_CERT, certName));

We should create helper functions for SigningInfo creation.

SigningInfo
signingByIdentity(const Name& identityName);

SigningInfo
signingByKey(const Name& keyName);

SigningInfo
signingByCertificate(const Name& certName);

SigningInfo
signingWithSha256();

These free functions should be declared in ndn::security namespace, and exported to ndn namespace.

After this change, the above example can be written as:

keyChain.sign(packet, signingByCertificate(certName));

Related issues

Blocks ndn-tools - Bug #2919: poke and pingserver use deprecated signing APIClosedEric Newberry06/18/2015

Actions
Blocked by ndn-cxx - Feature #2871: KeyChain: general signing APIClosedYingdi Yu

Actions
#1

Updated by Junxiao Shi over 5 years ago

  • Tracker changed from Task to Feature
  • Subject changed from Implement helpers to create SigningInfo to Helpers to create SigningInfo

I disagree with creating helper classes. They should be free functions that returns SigningInfo type.

#2

Updated by Alex Afanasyev over 5 years ago

Free functions also could be good. My only objective is to have a simple and straightforward syntax for signing operation.

#3

Updated by Junxiao Shi over 5 years ago

  • Description updated (diff)
  • Estimated time set to 3.00 h

Function names shouldn't start with verb sign, because the intuitive meaning would be "sign the argument and return the signed object".

I made the function names start with signing. This is an intentional violation of rule 2.5.
A proper name that conforms to rule 2.5 would be makeSigningInfoByIdentity, which is again too long.

#4

Updated by Alex Afanasyev over 5 years ago

can we remove "signing" prefix? It would be more meaningful to write

keyChain.sign(packet, byCertificate(certName));
keyChain.sign(packet, byIdentity(identityName));
keyChain.sign(packet, byKey(keyName));

It would a little clutter ndn:: namespace, but more clear.

#5

Updated by Junxiao Shi over 5 years ago

Answer to note-4:

No, they look weird in ndn namespace.

I could put them into ndn::security::signing namespace, but the application needs to manually import them with using.

#6

Updated by Junxiao Shi over 5 years ago

  • Assignee set to Eric Newberry

20150619 conference call approves the design in Description.

#7

Updated by Junxiao Shi over 5 years ago

  • Blocks Bug #2919: poke and pingserver use deprecated signing API added
#8

Updated by Junxiao Shi over 5 years ago

#9

Updated by Eric Newberry over 5 years ago

  • Status changed from New to In Progress
#10

Updated by Eric Newberry over 5 years ago

How do I get the keyName from the keyId?

#11

Updated by Eric Newberry over 5 years ago

  • Status changed from In Progress to Code review
#12

Updated by Alex Afanasyev over 5 years ago

I have not noticed that before, but signingByKey should accept key name. Identity and "id" are irrelevant for the purpose of signing.

#13

Updated by Junxiao Shi over 5 years ago

  • Description updated (diff)

signingByKey should accept key name. Identity and "id" are irrelevant for the purpose of signing.

Agreed. I used to believed "key name" has no definition, until I see Key::getName method.

#14

Updated by Junxiao Shi over 5 years ago

Should KeyChain and other unit tests (modified in #2871) be updated to use signing helpers, instead of creating SigningInfo with the long syntax?

#15

Updated by Eric Newberry over 5 years ago

Junxiao Shi wrote:

Should KeyChain and other unit tests (modified in #2871) be updated to use signing helpers, instead of creating SigningInfo with the long syntax?

I would agree with this proposal, as it would simply the test commands. However, it would also require the inclusion of an extra header in the unit test (signing-helpers.hpp).

#16

Updated by Eric Newberry over 5 years ago

  • Status changed from Code review to Closed
  • % Done changed from 0 to 100
#17

Updated by Eric Newberry over 5 years ago

I forgot about Junxiao's question when I closed this issue. Could it please be changed to Resolved until all questions are sorted out?

#18

Updated by Junxiao Shi over 5 years ago

  • Status changed from Closed to Resolved

note-14 must be discussed before this issue can close.

#19

Updated by Yingdi Yu about 5 years ago

what is the harm of not doing so?

#20

Updated by Alex Afanasyev about 5 years ago

I see no reason to do any modifications, as the code is equivalent. The new code can use either, though I would suspect developers would choose to use helpers.

#21

Updated by Junxiao Shi about 5 years ago

  • Status changed from Resolved to Closed
#22

Updated by Joao Pereira about 5 years ago

Looking into these functions for another task I am not sure why do we need to supply the name. Shouldn't the interface be like this:

SigningInfo
signingBy(const Name& identity);

SigningInfo
signingBy(const KeyChain& key);

SigningInfo
signingBy(const IdentityCertificate& certificate);

Like this the functions would be more usefull, because at this point these functions only abstract the caller from the enum.

#23

Updated by Alex Afanasyev about 5 years ago

Because identity, key, and certificates all are identified by name. I think what you're suggesting would require additional lookup to keychain, with code size only increasing

#24

Updated by Joao Pereira about 5 years ago

At this point we are just using only the identity and the certificate signers and we call the certificate like this:

  security::SigningInfo signingInfo;
  if (!certificate.getName().empty()) {
    signingInfo = signingByCertificate(certificate.getName());
  }

What I was proposing was to remove this logic from every place we are trying to sign and move it to the helper.

The KeyChain at this point have no call, will it be used? How are we getting the name from the KeyChain when we want to call the signer using the signingByKey?

#25

Updated by Junxiao Shi about 5 years ago

Reply to note-24:

The quoted snippet comes from a \deprecated function to support legacy API.

New code shouldn't load Certificate in order to construct SigningInfo.

#26

Updated by Joao Pereira about 5 years ago

Yes you are right, so this code will be moved to the application right? Because the setInterestFilter will be receiving the SigningInfo object already.

Looking at what was said in note-25 I think that it make even more sense to have override helper functions instead of having one function per type.

But if you guys think that this logic should be passed to the application then we can close this discussion

#27

Updated by Junxiao Shi about 5 years ago

Reply to note-26:

Application shouldn't load Certificate either.

#28

Updated by Alex Afanasyev about 5 years ago

The input that application provides is Name (it is what user supplies either in configuration or command line parameters). Any looking up procedures will be done inside the keychain during thr signing (how to optimize the number of lookups is another good question).

Neither key or certificate can really be used directly for signing operations. To sign one needs to find a corresponding private key, for which we are using name of identity/key/certificate, just in a slightly different ways.

Also available in: Atom PDF