Project

General

Profile

Actions

Task #3886

closed

Remove util::Digest class template

Added by Junxiao Shi about 8 years ago. Updated over 7 years ago.

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

100%

Estimated time:

Description

ndn::util::Digest has been superseded by ndn::security::transform::DigestFilter.


Related issues 2 (0 open2 closed)

Blocks ndn-cxx - Task #3946: Get rid of cryptopp dependencyClosedDavide Pesavento

Actions
Blocked by ndn-cxx - Task #3924: Reimplement util::Sha256 without cryptoppClosedDavide Pesavento

Actions
Actions #1

Updated by Davide Pesavento almost 8 years ago

  • Assignee deleted (Davide Pesavento)
Actions #2

Updated by Alex Afanasyev almost 8 years ago

I disagree with deprecation of this class. DigestFilter is not a replacement of it, but the underlying mechanism that should be used for digest calculations.

I agree that ndn-cxx's util/ folder became a kitchen sink for various unrelated things, but unless we create a convenient framework for a better place for those, I would like to keep Digest (Sha256) class.

Actions #3

Updated by Alex Afanasyev almost 8 years ago

  • Status changed from New to Rejected
Actions #4

Updated by Davide Pesavento almost 8 years ago

DigestFilter is not a replacement of it, but the underlying mechanism that should be used for digest calculations.

So you agree that util::Digest should be reimplemented using transform::DigestFilter then? Main motivation is getting rid of cryptopp dependency.

Actions #5

Updated by Alex Afanasyev almost 8 years ago

Yes, it definitely should be re-implemented without cryptopp dependency. The interface will need to change a bit (right now, the template parameter is cryptopp-type), but that should be fine given only two known projects are using this type and probably using Sha256 typedef.

Actions #6

Updated by Davide Pesavento almost 8 years ago

Apparently we have a THIRD way of calculating a SHA256 digest in ndn-cxx: the computeSha256Digest() function in util/crypto.hpp (basically the only function in that file). Are you going to tell me we need all three of them?

Actions #7

Updated by Alex Afanasyev almost 8 years ago

It is not a third way. It is a helper to calculate digest, which was inteoduced long time ago. If you're suggesting yhag writing 2-4 lines of code using "transformation" API is the way of calculatig digest in all cases, I will strongly disagree. The transformation API was/is simply a way to avoid cryptopp dependency and rigidity of openssl. It is a more or less internal business of the library. What is so special in having a number of helper APIs that don't make assumptions about specific APIs and shortcut code writing?

Actions #8

Updated by Davide Pesavento almost 8 years ago

I understand your point about the transform API, and I could even agree: transform is a medium-level API that requires a couple of lines of code and the usage of streams or buffers. You want something higher level, easier to use for simple cases, and more compact to write. Fine. We have computeSha256Digest for that, it's stateless and trivial to use. I'm not suggesting to remove it.

But then what do we need the util::Digest class for? You said transform::DigestFilter is not a replacement for it, but I don't understand why. From an API perspective they're very similar and require more or less the same number of lines of code...

Actions #9

Updated by Junxiao Shi almost 8 years ago

  • Status changed from Rejected to New
  • Assignee set to Davide Pesavento

20170206 call agrees that ndn::util::Digest can be deleted (not deprecated) because it relies on CryptoPP.
Deprecation notice is NOT needed because a breaking change notice is already included in v0.5.1 release notes.

ndn::util::Sha256 should be re-implemented with DigestFilter with its public API unchanged, because it's used by ChronoSync.

Actions #10

Updated by Junxiao Shi almost 8 years ago

  • Blocks Task #3946: Get rid of cryptopp dependency added
Actions #11

Updated by Davide Pesavento over 7 years ago

  • Subject changed from Deprecate Digest class template to Remove util::Digest class template
Actions #12

Updated by Davide Pesavento over 7 years ago

  • Blocked by Task #3924: Reimplement util::Sha256 without cryptopp added
Actions #13

Updated by Davide Pesavento over 7 years ago

  • Status changed from New to In Progress
  • Estimated time deleted (1.50 h)
Actions #14

Updated by Davide Pesavento over 7 years ago

  • Status changed from In Progress to Code review
  • % Done changed from 0 to 100

https://gerrit.named-data.net/4018

Now, should util/digest.hpp be renamed and/or moved somewhere else? For example:

  • util/sha256.hpp
  • security/sha256.hpp
  • security/crypto-hash-helpers.hpp (inspired by signing-helpers.hpp; the more general name allows easier additions in the future)

Note that we also have a security/digest-sha256.hpp which can cause some confusion...

Actions #15

Updated by Davide Pesavento over 7 years ago

  • Status changed from Code review to Closed
Actions #16

Updated by Davide Pesavento over 7 years ago

Davide Pesavento wrote:

Now, should util/digest.hpp be renamed and/or moved somewhere else? For example:

  • util/sha256.hpp
  • security/sha256.hpp
  • security/crypto-hash-helpers.hpp (inspired by signing-helpers.hpp; the more general name allows easier additions in the future)

Note that we also have a security/digest-sha256.hpp which can cause some confusion...

Actually, I'm still waiting for an answer to the above.

Actions #17

Updated by Junxiao Shi over 7 years ago

If your goal is compatibility, as evidenced by refusing to deprecate duplicate API or improve Doxygen, don't rename anything.

Actions #18

Updated by Davide Pesavento over 7 years ago

I didn't mean it as a breaking change. We move the header and leave a deprecated "forwarding header" in its place, like it was done for mgmt/nfd/* headers.

Actions

Also available in: Atom PDF