Project

General

Profile

Actions

Feature #2868

closed

ValidityPeriod abstraction for SignatureInfo

Added by Yingdi Yu almost 9 years ago. Updated over 8 years ago.

Status:
Closed
Priority:
Normal
Category:
Security
Target version:
Start date:
06/09/2015
Due date:
% Done:

100%

Estimated time:

Related issues 1 (0 open1 closed)

Related to ndn-cxx - Task #2122: Replace DER encoded certificate using NDN's own TLV encodingClosed11/03/2014

Actions
Actions #1

Updated by Yingdi Yu almost 9 years ago

  • Related to Task #2122: Replace DER encoded certificate using NDN's own TLV encoding added
Actions #2

Updated by Yingdi Yu almost 9 years ago

  • Status changed from New to Code review
  • % Done changed from 0 to 100
Actions #3

Updated by Alex Afanasyev almost 9 years ago

  • Assignee changed from Yingdi Yu to Alex Afanasyev
  • Target version set to v0.4
Actions #4

Updated by Alex Afanasyev almost 9 years ago

  • Status changed from Code review to In Progress
  • % Done changed from 100 to 60
Actions #5

Updated by Junxiao Shi almost 9 years ago

Design issue:

In commit:adfbf23006d269c379f9f4eff1c0d1c4d72ee7ab, ndn::security::ValidityPeriod is a subclass of Block.
This is harmful, because:

  • TLV-VALUE of ValidityPeriod has a certain internal structure.
  • By public-inheriting from Block, the caller is able to modify the TLV-VALUE using Block APIs, without going through ValidityPeriod APIs.

The result is, a ValidityPeriod instance is not guaranteed to represent a valid ValidityPeriod abstraction.

A preceding example of having an abstraction type inheriting from Block is name::Component.
However, there's a fundamental difference in that case:

  • TLV-VALUE of NameComponent is unstructured.
  • Any modification to the NameComponent's TLV-VALUE using Block APIs still results in a valid NameComponent.
Actions #6

Updated by Alex Afanasyev over 8 years ago

Inheriting from Block allows simplified and generalized implementation of SignatureInfo block without requiring any additional memory overhead.

If somebody wants to do anything bad with the abstraction, it can do it anyways. We are developing for normal use cases, not something that someone can do to make something bad.


In the implementation I specifically added invalid value handling in the only method that interprets the value (getPeriod). Even if somebody screws up with the content of the value, there is no unintended memory access and only exception will be generated.

Actions #7

Updated by Junxiao Shi over 8 years ago

  • Tracker changed from Task to Feature
  • Subject changed from Add ValidityPeriod abstraction for SignatureInfo to ValidityPeriod abstraction for SignatureInfo

Reply to note-6:

Not inheriting from Block, but making ValidityPeriod fulfill WireEncodable and WireDecodable with a cached Block private field, can also achieve the goal of simplified SignatureInfo implementation.

The benefit is that a ValidityPeriod element is guaranteed to contain valid TLV-VALUE.

If ValidityPeriod is designed to be default-contructable, correctness can be guaranteed by making wireEncode throw an exception if TLV-VALUE is not yet valid.
This is better than to generate an invalid packet, transmit over the network, and get dropped on the remote end when getPeriod throws.

This correctness benefit far outweighs the harm of a tiny memory overhead (it's a constant overhead because copying Block is a cheap operation, see #2200 note-47).

Actions #8

Updated by Junxiao Shi over 8 years ago

Design problem in commit:8673bad855a5b0c4a240063806ad068515b3218d :

ValidityPeriod public API is using time_point<system_clock, nanoseconds> to represent NotBefore and NotAfter fields.
This type indicates a precision of nanoseconds.

However, ValidityPeriod is only capable to storing NotBefore and NotAfter fields at seconds precision.

The Chrono library (std::chrono or boost::chrono) is all about being explicit about type (duration vs time_point) and precision (nanoseconds vs seconds, and others).

It's semantically wrong to indicate a precision of nanoseconds while the real precision is seconds.

The public API shall use time_point<system_clock, seconds> type to indicate a correct precision.

Alternatively, use time_t type which has the same clock and precision, so caller doesn't have to do time_point_casts.

Actions #9

Updated by Alex Afanasyev over 8 years ago

Junxiao, please stop making unreasonable suggestions. I agreed with many others, but will not agree with this one.

There is no problem with the implementation. Whatever ValidityPeriod internally uses it is internal business for the ValidityPeriod. Public interface is in terms of "standard" TimePoint that is used throughout the application.

Actions #10

Updated by Junxiao Shi over 8 years ago

Reply to note-9:

note-8 is not an implementation problem, but a semantics problem in the API design.

Chrono library is all about being explicit about type and precision.

In API design, it's important to avoid surprises.

It would be surprising when the getter returns a different value than what's passed to the setter due to loss of precision.

This can be avoided by using time_point<system_clock, seconds> or time_t to indicate the correct precision.

Actions #11

Updated by Davide Pesavento over 8 years ago

I think it's misleading to have an API that handles nanoseconds while the underlying data type has a precision of one second. It's like having getFoo/setFoo() methods where a double is being passed, but the corresponding member field is a float. So I tend to agree with Junxiao here.

Actions #12

Updated by Alex Afanasyev over 8 years ago

I am not changing the code. We are talking about some trivial not-even-correctness things, which would result in significant increase in complexity when using ValidityPeriod abstraction---everybody will have to add unnecessary boost::time_point_cast<time::seconds>(...) in order to actually use the constructor.

Actions #13

Updated by Junxiao Shi over 8 years ago

Reply to note-12:

This is a correctness issue.
Using the wrong precision is semantically wrong.

An API that leads to a surprise (see note-10) is bad.

If you want to avoid time_point_cast, design the API to use time_t type, which has the same meaning as time_point<system_clock, seconds>.

Actions #14

Updated by Alex Afanasyev over 8 years ago

  • Status changed from In Progress to Closed
  • % Done changed from 60 to 100
Actions

Also available in: Atom PDF